Pin and reorder the timed_belief primary key

[Flix6x]

Closes steps 1 and 2 of #2377.

What

Pins the column order of timed_belief's primary key with an explicit PrimaryKeyConstraint, and reorders it to:

(sensor_id, source_id, event_start, belief_horizon, cumulative_probability)

The set of key columns is unchanged, so uniqueness semantics are identical — this is purely a reordering. timely_beliefs passes index_elements to on_conflict_do_update as a set, so upserts are unaffected.

Why pin it

TimedBeliefDBMixin marks five columns primary_key=True and leaves the order to SQLAlchemy, which collects attributes declared on the subclass before the mixin's. So the key's shape is an accident of which columns a host happens to redeclare, and it silently changes if anyone adds a column override.

That is not hypothetical: FlexMeasures' source_id override already moves it to the front, and a schema built by create_all() therefore disagrees with a migrated one. Nothing catches that today. The new test does.

Why this order

• sensor_id first — virtually every query filters on a single sensor. A key that does not lead with it cannot serve those queries at all, which is exactly why deployments end up hand-adding a sensor-leading composite index.
• source_id second — this makes (sensor_id, source_id, event_start, belief_horizon) a prefix of the key, so such a hand-added index becomes redundant. On the dataset profiled this was the largest and most-scanned index on the table, so dropping it is the bulk of the win — and the queries that used it get faster, since they are now served by a key that no longer needs a second structure maintained alongside it on every insert.
• cumulative_probability last — it is very nearly a constant (0.5 for every deterministic belief), so it contributes no selectivity wherever it sits.
• sensor_id and source_id adjacent — both are 4-byte integers, so keeping them together avoids the alignment padding that separating them forces into every index tuple. Measured at roughly 15% of the index's size, which was the most surprising result of the profiling.

Migration

d4a7c1e93b52 drops and recreates the constraint, then drops the redundant composite index — but only after checking its definition matches, so an index that merely shares the name is left alone. The downgrade restores both, recreating the index first so the queries that relied on it are never left unserved.

This rebuilds an index but does not rewrite the table. No heap pages are touched and no other index is affected.

The migration docstring documents the online alternative for large deployments, since op.create_primary_key holds an ACCESS EXCLUSIVE lock for the whole build:

CREATE UNIQUE INDEX CONCURRENTLY timed_belief_pkey_new
    ON timed_belief (sensor_id, source_id, event_start, belief_horizon, cumulative_probability);
BEGIN;
  ALTER TABLE timed_belief DROP CONSTRAINT timed_belief_pkey;
  ALTER TABLE timed_belief ADD CONSTRAINT timed_belief_pkey
      PRIMARY KEY USING INDEX timed_belief_pkey_new;
COMMIT;
DROP INDEX CONCURRENTLY IF EXISTS idx_tb_sensor_source_event_horizon;

Only the transaction takes an exclusive lock, and it is catalog-only because the index already exists.

Verification

Ran the migration's upgrade() and downgrade() against a real PostgreSQL table shaped like the pre-migration timed_belief, including a hand-added composite index and a decoy index sharing a similar name:

• upgrade reorders the key, drops the redundant index, leaves the decoy alone, preserves all rows;
• downgrade restores the original key order and recreates the composite index, preserving all rows.

New tests (test_timed_belief_schema.py, no database needed) pin the key's order, assert sensor_id leads it, assert the two integer columns stay adjacent, and assert timely_beliefs' own indexes survive the __table_args__ override.

Full flexmeasures/data suite passes.

Step 2: dropping the redundant single-column indexes

timed_belief also carried single-column indexes on event_start and sensor_id, created because timely_beliefs declared index=True on both. Each is fully covered by a composite index the same library declares:

single-column index	covered by
(event_start)	timed_belief_search_session_idx (event_start, sensor_id, source_id) INCLUDE (belief_horizon)
(sensor_id)	timed_belief_search_session_singleevent_idx (sensor_id, event_start)

In the deployment profiled, neither had ever served a single index scan, while every other index on the table had scan counts in the hundreds of thousands or millions — so this was not young statistics.

SeitaBV/timely-beliefs#244, released in 4.2.0, stopped declaring them. Migration b7e5a2c40f18 (a separate file, revising d4a7c1e93b52) removes them from existing databases, and the timely-beliefs floor moves to >=4.2.0 in the same commit — deliberately, since dropping them while the ORM still declared them would recreate exactly the create_all()-vs-migrated divergence this PR set out to fix.

That migration matches indexes structurally rather than by name — single-column, non-unique, not backing a constraint, on one of the two columns — so it works whatever naming convention a deployment's indexes were created under, and is a no-op where they are already gone.

Verified against a real PostgreSQL table carrying both redundant indexes plus four deliberate decoys: a unique index on event_start, a constraint-backed unique on sensor_id, a single-column index on another column, and both composites. Exactly the two intended indexes were dropped, everything else survived, the downgrade recreated them, and a repeated upgrade was a no-op.

Dropping the sensor_id index is safe for the ON DELETE CASCADE on its foreign key: PostgreSQL only needs to find rows by sensor_id, and timed_belief_search_session_singleevent_idx leads with that column. A test pins that property.

Known trade-off

After the reorder no index leads with source_id, so deleting a data_source — whose foreign key is NO ACTION — falls back to a sequential scan. Profiled and discussed in a comment below: it costs a single ~20-second statement for an operation no CLI or API path performs, against a permanent index costing ~12% of what this PR reclaims. Recommending we accept it; easily reversed with a CREATE INDEX CONCURRENTLY if source deletion ever becomes routine.

Note on uv.lock

The lockfile shows a large diff, but the only dependency change is timely-beliefs 4.0.1 -> 4.2.0. The rest is my local uv normalising the file by adding upload-time to every entry; uv lock --upgrade-package timely-beliefs produces identical churn. Happy to drop it from the commit and let CI regenerate if you would rather keep the current format.

🤖 Generated with Claude Code

[read-the-docs-community]

Documentation build overview

📚 flexmeasures | 🛠️ Build #33890353 | 📁 Comparing 98052cd against latest (fecf13a)

  🔍 Preview build  

3 files changed

± changelog.html
± _autosummary/flexmeasures.data.models.planning.storage.html
± api/v3_0.html

[Flix6x]

One consequence I should flag rather than leave for review to find.

timed_belief.source_id has a foreign key to data_source.id with no ON DELETE clause, so it is NO ACTION: deleting a data source makes PostgreSQL check for referencing beliefs. Under the old key order that check was indexed, because source_id led the primary key. After this reorder no index leads with source_id — not the new key, not either of the timely_beliefs search indexes — so that check becomes a sequential scan of the beliefs table.

I do not think this blocks the change:

• there is no CLI or API path that deletes an individual data source; the only bulk delete is in data_gen.py, i.e. the development reset path;
• a source that does have beliefs cannot be deleted anyway (the FK rejects it) — the scan just makes the rejection slow;
• an index dedicated to source_id would cost roughly what one of the redundant single-column indexes being removed in step 2 costs, which would eat a good part of the saving for a rare administrative operation.

So my inclination is to accept it as a deliberate trade rather than add an index for it. But it is a real regression in that one path, and it should be a conscious decision rather than something noticed later — if we do start deleting data sources routinely, an index on source_id is the fix.

Worth noting the sensor side is fine: sensor_id leads the new key, so the ON DELETE CASCADE from sensor stays index-driven, and in fact gets faster.

[Flix6x]

Added step 2 in 9ce56acf, now that SeitaBV/timely-beliefs#244 is released in 4.2.0.

New migration b7e5a2c40f18 (separate file, revising d4a7c1e93b52) drops the redundant single-column indexes on event_start and sensor_id. Each was fully covered by a composite index timely_beliefs declares leading with the same column, so neither offered anything a query could use — they only occupied space on our largest table and slowed every write that had to maintain them.

The floor moves to timely-beliefs>=4.2.0 in the same commit deliberately: dropping the indexes while the ORM still declared them would recreate exactly the create_all()-vs-migrated divergence the first commit set out to fix.

The migration matches structurally rather than by name — single-column, non-unique, not backing a constraint, on one of the two columns — so it works whatever naming convention a deployment's indexes were created under, and is a no-op where they are already gone.

Verified against a real PostgreSQL table carrying both redundant indexes plus four deliberate decoys: a unique index on event_start, a constraint-backed unique on sensor_id, a single-column index on another column, and both composites. Exactly the two intended indexes were dropped, everything else survived, the downgrade recreated them, and a repeated upgrade was a no-op.

Safety of dropping the sensor_id index: the ON DELETE CASCADE from sensor only needs to find rows by sensor_id, and timed_belief_search_session_singleevent_idx leads with that column. A new test pins that, so a future reordering of those composites cannot quietly remove the coverage.

Tests: flexmeasures/data gives 249 passed against tb 4.2.0, which is the previous baseline plus the two new tests. The two test_closest_sensor failures are pre-existing on main and environmental (my sandbox lacks the cube/earthdistance extensions).

One thing to look at: uv.lock shows ~3500 changed lines, but the only dependency change is timely-beliefs 4.0.1 -> 4.2.0. The rest is my local uv (0.10.11) normalising the file by adding upload-time to every entry. uv lock --upgrade-package timely-beliefs produces the identical churn, so it is not avoidable at my end. If you would rather keep the lock in its current format, regenerate it with whichever uv produced the committed one and force-push over my version — or say the word and I will drop the lock from the commit and let CI regenerate it.

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	timely-beliefs@​4.0.1 ⏵ 4.2.0	+1

View full report

[Flix6x]

Profiled the source_id foreign-key regression I flagged above, so the trade is on record rather than a guess. Measured against a realistic dataset with the full post-reorder index set in place and a real NO ACTION foreign key — not a synthetic query.

scenario	FK check plan	check	actual DELETE
after the reorder, no source_id index	Seq Scan	~15 s	~20 s
plus a dedicated source_id index	Index Scan	0.04 ms	0.3 ms
the old source_id-leading key	Index Scan on the key	0.04 ms	2.4 ms

So the regression is large in relative terms — roughly four orders of magnitude on the delete — but its absolute cost is a single ~20-second statement.

Two details worth knowing:

• The worst case is deleting an unused source. The check is SELECT 1 … WHERE source_id = $1 LIMIT 1, so when a source has no beliefs nothing matches and the scan has to read the entire table. A source that does have beliefs stops at the first match — about 4 s before raising the expected FK violation. Counterintuitively, the source you care least about is the slow one to delete.
• It is not an outage. No exclusive lock is taken; it holds ROW SHARE and pins a transaction open for the duration, which matters more on a busy ingestion path than the wall clock suggests, but it does not block reads or writes.

Recommendation: accept it, don't add the index

The mitigation costs a permanent index — on the order of 12% of the space this PR reclaims — to speed up an operation that no CLI or API path performs. The only bulk delete of data sources is in data_gen.py, i.e. the development reset, which is already tearing everything down.

Paying that forever to turn a rare 20-second admin command into an instant one is the wrong way round.

What would change the answer: if deleting data sources ever becomes routine — a UI action, a cleanup job, pruning of versioned sources — add the index then. It is a one-line CREATE INDEX CONCURRENTLY, fully online, with no migration-ordering concerns, so the decision is cheaply reversible in both directions.

Worth noting the old key was never free here either. It happened to cover this case as a side effect of an ordering that costs us on every read and every write. This change makes that implicit subsidy explicit, which seems the right way round — but it is a deliberate trade, so it should be someone's decision rather than a discovery later.

[Copilot]

🟡 Not ready to approve

The migrations have correctness issues (downgrade restores the wrong prior PK order, and redundant-index detection isn’t schema-scoped and may target the wrong table in multi-schema databases).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Pins and standardizes the timed_belief primary key column order (and removes redundant indexes) to ensure ORM-generated schemas (create_all()) and migrated schemas stay aligned, while improving index usefulness and footprint for common query patterns.

Changes:

• Add an explicit PrimaryKeyConstraint on TimedBelief to pin PK column order to (sensor_id, source_id, event_start, belief_horizon, cumulative_probability).
• Add Alembic migrations to reorder the PK and drop redundant single-column indexes on timed_belief.
• Add schema-level tests to assert PK order and index expectations, and bump timely-beliefs to >=4.2.0.

File summaries

File	Description
pyproject.toml	Bumps timely-beliefs floor to >=4.2.0 to match ORM/index expectations.
flexmeasures/data/models/time_series.py	Pins timed_belief PK order via explicit PrimaryKeyConstraint while preserving mixin indexes.
flexmeasures/data/tests/test_timed_belief_schema.py	Adds metadata-only tests to enforce PK order and index presence/absence.
flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py	Rebuilds PK in the new order and conditionally drops a now-redundant composite index.
flexmeasures/data/migrations/versions/b7e5a2c40f18_drop_redundant_timed_belief_indexes.py	Drops redundant single-column indexes on event_start and sensor_id based on structural detection.
documentation/changelog.rst	Adds changelog entries describing the PK reorder and index drops.

Review details

Suppressed comments (1)

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:76

• The downgrade primary-key column order (OLD_ORDER) does not match the pre-upgrade order created by FlexMeasures migrations (see 04f0e2d2924a, which created (event_start, belief_horizon, cumulative_probability, sensor_id, source_id)). As written, a downgrade will not restore the prior migrated schema shape.

# The order SQLAlchemy produced before the primary key was pinned explicitly.
OLD_ORDER = [
    "source_id",
    "event_start",
    "belief_horizon",
    "cumulative_probability",
    "sensor_id",
]

• Files reviewed: 6/7 changed files
• Comments generated: 6
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

There are a couple of concrete migration robustness/conventions issues (docstring line-breaking convention and unsafe identifier quoting in a DROP INDEX statement) that should be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:33

• Module docstring breaks a line mid-phrase (ends with "inside an"), but this repo’s docstring convention is to only break lines after punctuation (see .github/instructions/docstrings.instructions.md:38). Please reflow these lines so each line ends with punctuation.

The swap is done online, so this does not need a maintenance window.
Building the replacement index is the slow part, and it runs ``CONCURRENTLY`` inside an
``autocommit_block``, so reads and writes continue throughout.

flexmeasures/data/migrations/versions/b7e5a2c40f18_drop_redundant_timed_belief_indexes.py:88

• The migration claims it quotes index identifiers safely, but wrapping the name in double quotes without escaping embedded quotes is not equivalent to PostgreSQL’s quoting and can make the migration fail on oddly-named indexes. Escape embedded double quotes before interpolating the identifier.

    for name in names:
        # Quote the identifier the same way PostgreSQL would, in case of odd naming.
        with op.get_context().autocommit_block():
            op.execute(f'DROP INDEX CONCURRENTLY IF EXISTS "{name}"')

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Round 2 addressed in e93ee02f. Both findings were valid.

Unsafe identifier quoting. The DROP INDEX statement wrapped the index name in double quotes itself, which is not equivalent to PostgreSQL's quoting — a name containing a double quote produced a syntax error rather than a valid statement. I verified that rather than taking it on trust: against an index named weird"quoted_es_idx, the old form fails with syntax error at or near "quoted_es_idx".

Fixed by letting PostgreSQL do the quoting. The lookup now returns quote_ident(schema) || '.' || quote_ident(index), so the identifier arrives ready to interpolate and is correct whether it needs quoting, contains quotes, or is mixed case. Tested against weird"quoted_es_idx and Mixed Case sid idx — both found and dropped.

Docstring line breaks. Reflowed. Copilot flagged one; I found a second by mechanically checking every added line against the convention rather than re-reading them, which is the better guard.

Re-verified after the change: the migrations still pass the full end-to-end run (upgrade, both downgrades, no-op re-run, decoy and other-schema survival, no invalid indexes at any point, stale-index retry), and flexmeasures/data gives 249 passed with the two pre-existing test_closest_sensor failures unchanged.

[Copilot]

🟡 Not ready to approve

The main changelog entry currently violates the repository’s one-sentence changelog entry convention and should be reflowed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

documentation/changelog.rst:52

• Main changelog entries are expected to be one sentence (no period at the end). This new entry is split into multiple sentences with periods ("...redundant." / "Requires..." / "The migration...") which breaks the established format in this file.

* Speed up sensor data queries and free up disk space by reordering the ``timed_belief`` primary key to lead with ``sensor_id``, and dropping three indexes it makes redundant. Requires ``timely-beliefs`` 4.2.0. The migration runs online, so no maintenance window is needed, but building the new index can take a while on a large database [see `PR #2378 <https://www.github.com/FlexMeasures/flexmeasures/pull/2378>`_]

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Round 3 addressed in e04694471 — valid again, and documented: .github/instructions/changelog.instructions.md asks for one sentence, no trailing period, written for end users with technical details abstracted away.

The entry is now:

Speed up sensor data queries and free up disk space by reordering the timed_belief primary key to lead with sensor_id and dropping three indexes it makes redundant, in a migration that runs online and so needs no maintenance window (though it can take a while on a large database)

I dropped the timely-beliefs version from it — that is a packaging detail the migration docstring and this PR already record — and kept the part hosts actually act on: no maintenance window needed.

That is three rounds of Copilot review; all findings across them were valid, and two were substantive (the missing schema constraint, and the identifier quoting). Handing back to you from here rather than looping further.

[Copilot]

🟡 Not ready to approve

The migrations’ index-matching logic can drop unintended non-redundant indexes (e.g., superset/partial/expression indexes) due to overly broad catalog checks, which should be tightened before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:90

• The redundant-index guard uses indexdef LIKE '%(sensor_id, source_id, event_start, belief_horizon)%', which can also match indexes that start with those columns but have extra key columns or INCLUDE columns. In that case, the migration would drop a non-redundant index despite the comment claiming it only drops an exact structural match. Consider switching to a pg_index-based check that verifies: (a) exactly 4 key columns, (b) no INCLUDE columns, (c) no predicate/expression, and (d) the exact key-column name sequence.

 WHERE schemaname = current_schema()
   AND tablename = 'timed_belief'
   AND indexname = :name
   AND indexdef LIKE '%(sensor_id, source_id, event_start, belief_horizon)%'
"""

flexmeasures/data/migrations/versions/b7e5a2c40f18_drop_redundant_timed_belief_indexes.py:71

• FIND_REDUNDANT_INDEXES will also match and drop partial or expression indexes on event_start/sensor_id (e.g. a deployment-specific partial index), because it only checks indnatts = 1 and the leading attribute name. Since the intent is to remove the plain single-column indexes previously created by timely-beliefs (which are non-partial and non-expression), it’s safer to exclude indexes with predicates (indpred) or expressions (indexprs).

   AND n.nspname = current_schema()
   AND x.indnatts = 1
   AND NOT x.indisunique
   AND NOT x.indisprimary
   AND a.attname = ANY(:columns)

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Round 4 addressed in 7351b172, plus the lock regeneration and a merge of main.

Both findings were real. The indexdef LIKE guard would have matched an index carrying INCLUDE columns or a WHERE predicate — neither redundant with the new key — and the single-column selector would have matched a partial index on the same column. Both now match against pg_index structurally: exact key-column sequence, no INCLUDE, no predicate, no expressions.

Verified in both directions, since tightening a filter can just as easily break the intended case:

• against a table carrying an INCLUDE variant of the composite, partial indexes on event_start and sensor_id, and an expression index — all four survive, only the plain redundant ones are dropped;
• against a table shaped like production — all three intended indexes still go.

Worth noting the first version of the pg_index check failed outright on a name[]/text[] comparison. The decoy test caught it before it could reach a database, which is a fair argument for testing migrations against a real PostgreSQL rather than reading them over.

uv.lock is now a 20-line diff instead of ~3540. I swept uv releases against the committed lock: everything from 0.7.0 onward rewrites every entry to add upload-time, while 0.6.x writes the same lockfile revision this repo already uses. Regenerated with 0.6.14. The 12 graalpy wheel entries that disappear are dropped by every uv version I tested including the newest, so that is not a cost of using the older one — it happens on any re-lock.

Separately: main is merged in (cleanly), per .github/instructions/feature-branch-sync.instructions.md.

[Copilot]

🟡 Not ready to approve

The PK reorder migration uses unqualified index/table names for concurrent DDL, which can target the wrong schema under multi-schema search_path configurations and should be schema-qualified for safety.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:150

• The redundant-index drop uses an unqualified index name. If an index with the same name exists in another schema earlier on search_path, Postgres could drop the wrong index. Since the presence check is pinned to current_schema(), it’s safer to schema-qualify the index name in the DROP INDEX CONCURRENTLY statement as well.

    if present:
        with op.get_context().autocommit_block():
            op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {REDUNDANT_INDEX}")

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:131

• The concurrent index DDL here uses unqualified names (e.g. DROP INDEX ... timed_belief_pkey_new and CREATE UNIQUE INDEX ... ON timed_belief). In databases where search_path contains multiple schemas, this can target an object in a different schema than intended, especially since other parts of this migration explicitly pin catalog lookups to current_schema(). Consider schema-qualifying both the temporary index and the timed_belief table to ensure the migration operates on a single, well-defined schema.

This issue also appears on line 148 of the same file.

    with op.get_context().autocommit_block():
        op.execute(f"DROP INDEX CONCURRENTLY IF EXISTS {TEMP_INDEX}")
        op.execute(
            f"CREATE UNIQUE INDEX CONCURRENTLY {TEMP_INDEX}"
            f" ON timed_belief ({', '.join(order)})"

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Round 5 addressed in 99741dbb. The schema finding was valid.

The catalog checks were pinned to current_schema() while the surrounding DDL used unqualified names resolved through search_path — and those genuinely differ when timed_belief is not in the first schema on the path. The check would then look in one schema while the statements acted on another.

Rather than qualify statement by statement, the migrations now resolve the table's schema once via pg_table_is_visible, which picks exactly the table an unqualified reference would resolve to, and use that everywhere. The lookups and the DDL cannot disagree by construction. The resolver hands back the raw name for catalog comparisons and a quote_ident-ed form for interpolation, so a schema whose name needs escaping is PostgreSQL's problem rather than mine.

One subtlety worth recording, since it is easy to get backwards: CREATE INDEX takes an unqualified index name — the index is always created in its table's schema — while DROP INDEX takes a qualified one. My first attempt qualified both, which is a syntax error. The test caught it; reading the statements had not.

Verified against a database holding a decoy public.timed_belief with identically-named single-column indexes, and the real table in another schema reached via search_path: only the real schema's indexes were dropped, both decoys survived, the primary key was reordered on the right table, and the downgrades restored everything.

flexmeasures/data: 249 passed, with the two pre-existing test_closest_sensor failures unchanged.

[Copilot]

🟡 Not ready to approve

The new migrations have schema-quoting bugs in DDL/constraint operations that can break on schemas whose names require quoting.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

flexmeasures/data/migrations/versions/d4a7c1e93b52_reorder_timed_belief_primary_key.py:155

• op.drop_constraint expects an unquoted schema name, but _schema() returns schema as quote_ident(nspname) for string interpolation into raw SQL. Passing the quoted form here can produce an invalid identifier (e.g. Alembic will quote it again, yielding doubled quotes) when the schema name needs quoting.

    op.drop_constraint(
        "timed_belief_pkey", "timed_belief", type_="primary", schema=schema
    )

flexmeasures/data/migrations/versions/b7e5a2c40f18_drop_redundant_timed_belief_indexes.py:121

• schema is fetched as a raw namespace name and then interpolated into DDL without quoting. If the schema name requires quoting (uppercase, spaces, etc.), CREATE INDEX ... ON {schema}.timed_belief will fail even though the upgrade path handles quoting via quote_ident for drops.

    # Recreate under the naming convention this project's metadata uses.
    schema = op.get_bind().execute(sa.text(RESOLVE_SCHEMA)).scalar_one()
    for column in REDUNDANT_COLUMNS:
        with op.get_context().autocommit_block():
            op.execute(
                f"CREATE INDEX CONCURRENTLY IF NOT EXISTS timed_belief_{column}_idx"
                f" ON {schema}.timed_belief ({column})"
            )

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Round 6 addressed in 40a18343. Both findings were valid, and both were mine to get wrong.

The resolver returns two forms — a raw name for catalog comparisons, a quote_ident-ed one for interpolating into raw SQL — and I had them the wrong way round in exactly two places:

• op.drop_constraint was given the quoted form, but Alembic quotes the schema itself, so a name needing quotes would come out doubly quoted;
• the CREATE INDEX in the other migration's downgrade was given the raw form, which it interpolates directly, so a name needing quotes would fail to parse.

Worth noting why my own tests missed this. The multi-schema test I added last round used a schema named app — a name that needs no quoting, so the two forms are identical in it and the test could not possibly have distinguished them. It was too easy on itself.

Re-verified against a schema literally named My Schema: upgrade, both downgrades, the index drops and the primary key swap all behave.

flexmeasures/data: 249 passed, two pre-existing test_closest_sensor failures unchanged.

That is six rounds; every finding across them has been valid, and four were substantive (schema scoping, identifier quoting, over-broad index matching, and now the quoting forms). Handing back to you.

[Flix6x]

The review that landed after 40a18343 is a re-report of round 6 against the older commit, not a new finding — it was generated against 99741dbb, which is the commit before the fix.

Verified on the pushed branch rather than locally:

• d4a7c1e93b52 now passes schema=raw_schema to op.drop_constraint;
• b7e5a2c40f18's downgrade now takes the quoted form for its raw-SQL CREATE INDEX.

Both are exactly what the comments ask for, so no change is warranted. Re-requesting a review against the current HEAD to confirm.

[Copilot]

🟡 Not ready to approve

The added changelog entry overstates that the migration drops “three indexes” even though one of them is optional and only dropped when present/matching.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

documentation/changelog.rst:52

• This changelog entry states that the migration drops "three indexes", but one of those (the hand-added composite idx_tb_sensor_source_event_horizon) is optional and is only dropped when present and structurally matching. Consider wording this as dropping redundant indexes (up to three) to avoid over-claiming for deployments that never had the composite index (or already lacked the single-column ones).

* Speed up sensor data queries and free up disk space by reordering the ``timed_belief`` primary key to lead with ``sensor_id`` and dropping three indexes it makes redundant, in a migration that runs online and so needs no maintenance window (though it can take a while on a large database) [see `PR #2378 <https://www.github.com/FlexMeasures/flexmeasures/pull/2378>`_]

• Files reviewed: 6/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Flix6x]

Addressed in 98052cd7 — a fair catch, and the first genuinely new finding since round 6.

The entry claimed the migration drops "three indexes". Two of those come from timely-beliefs' index=True and exist in any deployment, but idx_tb_sensor_source_event_horizon is hand-added — most deployments never had it, so the count over-claims for a typical host. It now reads "dropping the indexes it makes redundant", with no number.

For the record on the previous round: that one was a re-report of round 6 against the pre-fix commit, so no change was warranted and none was made.

Tally: seven review rounds, six with valid findings (four substantive: schema scoping, identifier quoting, over-broad index matching, and the raw-vs-quoted schema forms), one stale duplicate, and this wording fix. Docs-only change, so no re-run of the suite; the last full run was 249 passed with the two pre-existing test_closest_sensor failures.