Skip to content

fix(compare): keep a materialized view's indexes through Compare & Sync and Copy To - #3079

Merged
datlechin merged 9 commits into
mainfrom
fix/matview-indexes-compare-copy
Sep 23, 2026
Merged

datlechin merged 9 commits into
mainfrom
fix/matview-indexes-compare-copy

Conversation

@datlechin

Copy link
Copy Markdown
Member

Stacked on #3069

Summary

Compare & Sync and Copy To treated a materialized view as its definition text and nothing else. A difference in its indexes alone read as Identical, and every script that created a view (a sync create or replacement, a Copy To create or replace) left it with no indexes. On PostgreSQL that also breaks REFRESH MATERIALIZED VIEW CONCURRENTLY, which needs a unique index.

A materialized view's indexes are now part of its comparison and of every script that creates it, on the engines whose curated structure matrix says that kind of object takes indexes: PostgreSQL and PGlite today.

Root cause

The model had nowhere to put a view's indexes. CompareRunner sends only .table snapshots to StructureDiffEngine; SourceObjectDiffEngine compares normalised text; SourceObjectSyncBuilder and ObjectCopyPlanner emit the CREATE MATERIALIZED VIEW text verbatim. CompareMetadataService did read the view's indexes (the PostgreSQL bulk read has no relkind filter, and the per-object read was try? ?? []), but nothing used them.

What changed

  • Carriage is keyed on (engine, kind) through StructureObjectEditMatrix. SourceObjectIndexes.areCarried(for:by:) is true when the matrix accepts both .addIndex and .dropIndex on that kind. PostgreSQL and PGlite accept them on .materializedView (curated in feat(datagrid): show whether a materialized view can be refreshed concurrently, and gate its structure edits by kind #3063) and not on .view. Every other engine falls back to .tablesOnly and stays definition-only. A kind that is not carried is not asked for its indexes at all, so Redshift's sort and dist keys and Snowflake's clustering keys never reach a CREATE INDEX.
  • Read. CompareMetadataService.read resolves carriage per endpoint and records a carried object's index read in TableStructureRead.objectIndexes (.read / .failed), apart from the object's own failure, so a refused index read cannot be mistaken for "no indexes" and does not throw away the definition read. readViewDefinitions now takes the structure reads and hands the index read to RoutineSourceRead.indexes.
  • Diff. SourceObjectDiffEngine takes the target's carried kinds. When both endpoints carry the kind it diffs the index sets with the signature rule tables use (StructureDiffEngine.indexChanges gained an array overload that the snapshot version delegates to). Same text plus index changes is different with definitionMatches. A failed index read on either side of a pair, or on the source of a create, is a comparisonError (Skip only); a drop never needs the target's indexes. A create into a target that does not carry the kind gets a note.
  • Sync. SourceObjectSyncBuilder.build: a create writes the view, then its indexes. An .alter whose definition matches writes only the index changes, with no DROP MATERIALIZED VIEW, so the stored rows stay. Any other .alter drops, creates, then writes the source's indexes. Index DDL goes through SchemaSyncScriptBuilder.changeStatements, now internal, so it shares SchemaChangeOrdering, SchemaStatementGenerator and the per-change hazards. Every statement carries objectName = identity.displayName, so one view is one object in the Apply sheet.
  • Schema binding. The builder takes indexSchema, the schema the target driver writes index DDL in (plugin.currentSchema inside the scoped closure). When that is not the schema the definition creates the view in, the builder refuses rather than indexing another view. Copy To checks the same thing first and copies the view without its indexes, with a Partly copied note. That is what a duplicated database gets for a view outside the planning connection's current schema.
  • Hazards. A materialized view drop now says it discards the rows along with the indexes, comments and privileges; a carried replacement says the rows are computed again and the source's indexes are created on it. A new concurrentRefresh warning marks the DROP INDEX (index-only alter) or the DROP MATERIALIZED VIEW (replacement) when the target's index set allows REFRESH MATERIALIZED VIEW CONCURRENTLY and the resulting set does not. ConcurrentRefreshIndexRule mirrors the predicate in PostgreSQLRelationSQL.concurrentRefreshQuery over the fields the index read carries: unique, b-tree, no predicate, no expression key.
  • Change guard. StructureGenerationInput now carries sourceIndexes (identity stripped), definitionMatches, and changes for every kind, so an index changed after comparing refuses the script.
  • UI. The Definitions pane shows a second, display-only Indexes diff under a carried view's definition, in TableDefinitionRenderer's index-line format. sourceDefinition, which is executed verbatim, is untouched. The results list counts a view's index changes the way it counts a table's. Copy To's Partly copied list now includes definition-step notes.
  • Copy To. ObjectCopyPlanner.definitionBuild decides per view: write the indexes; leave them out with a note (the target does not carry the kind, or the index statements would name another schema); or leave the view out with the reason (the source refused its index read and the target would take them).

Measured

PostgreSQL 17.11 at 127.0.0.1:54329, throwaway schema matview_indexes_compare_copy:

  • Dropping mv_id_idx, the view's only unique index, made REFRESH MATERIALIZED VIEW CONCURRENTLY fail: cannot refresh materialized view "…" concurrently / HINT: Create a unique index with no WHERE clause on one or more columns of the materialized view. This is the state every earlier replacement left behind (the design measured 0 indexes after DROP + CREATE with the same text).
  • The replacement as now scripted (DROP MATERIALIZED VIEW, CREATE … WITH DATA, CREATE UNIQUE INDEX "mv_id_idx" ON "s"."mv" USING btree ("id"), CREATE INDEX … WHERE (amount > 10)), in one transaction, left 2 indexes, and REFRESH … CONCURRENTLY then succeeded.
  • The index-only change as now scripted (DROP INDEX "s"."mv_customer_idx", CREATE INDEX "mv_customer_amount_idx" ON "s"."mv" USING btree ("customer", "amount")) kept 101 stored rows while the base table had 102. A replacement would have computed them again.
  • CREATE MATERIALIZED VIEW "a"."mv" followed by CREATE UNIQUE INDEX … ON "b"."mv" left a.mv with 0 indexes and gave b.mv 1. This is the critique's schema-binding case, confirmed.
  • The driver's indexList query over matviews reports is_unique, index_type = btree, predicate and expressions for each shape. Against the server: a plain unique index, a multi-column unique index, (id DESC NULLS LAST) and (id) INCLUDE (customer) all allowed REFRESH … CONCURRENTLY; a unique expression index ((id + 0)) and a partial unique index WHERE id > 0 both refused it; USING hash cannot be unique at all. ConcurrentRefreshIndexRuleTests pins these shapes.
  • No PluginKit change, so currentPluginKitVersion is untouched.

Tests

All through verify.sh test on the final tree: 43 suites, 373 cases executed, 373 passed. That is every suite that owns a changed type (grepped TableProTests for each), plus the new ones.

New suites, one test class per file:

Suite Cases What it pins
MaterializedViewIndexCompareTests 13 Same text with a unique index on one side only reads different, with definitionMatches and one addIndex (it read Identical before). Equal sets are identical; a renamed but equal index is a note, not a change. A failed index read on either side is a comparisonError with Skip only and never yields a deleteIndex. A create whose source indexes failed is never offered; a drop still is. A pair where one endpoint does not carry the kind is definition-only; a create into such a target carries a note. The display lines sit apart from the executed definition. The results list counts a view's index changes.
SourceObjectIndexCarriageTests 3 The matrix gate: .materializedView is carried by .postgreSQL, .view is not, .tablesOnly carries nothing. Through PluginManager: PostgreSQL and PGlite carry [.materializedView]; CockroachDB, Redshift and an unknown engine carry nothing.
SourceObjectIndexCopyTests 5 Write where the target carries and the schemas bind; a note where the target does not carry; a note where the index statements would name another schema or none; a refusal with the reason on a failed read into a carrying target; nothing when there is nothing to carry.
ConcurrentRefreshIndexRuleTests 3 The shapes measured on PostgreSQL 17.11 above.

Added to existing suites: SourceObjectSyncBuilderTests (+10: create order and objectName; replacement recreates the indexes and carries the new drop hazard; index-only alter never drops the view and runs DROP INDEX before CREATE INDEX; both concurrent-refresh warnings and their absence when a usable index remains; schema-binding refusal, including an unknown indexSchema; no refusal when there is nothing to write; an index the driver cannot write throws; the definition-only replacement), CompareMetadataReadPlanTests (+6: carried read; a failed read kept apart from the object's failure; a view not asked; a non-carrying engine never asked; the bulk read reaches the view; a data comparison never asks), CompareSourceDefinitionReadTests (+1), StructureChangeGuardTests (+4), ObjectCopyPlannerSourceDefinitionTests (+4, including the duplicate-database schema case), ObjectCopyPlanTests (+1). The builder and copy tests use a stub whose generateAddIndexSQL is PostgreSQLIndexClauses.createStatement, so they assert the SQL the PostgreSQL driver really writes.

Every new test was shown to fail without the fix. 17 edits, each undoing one piece, were applied together and the suites rerun: 129 cases executed, 26 failed, and every edit turned at least one case red. The source was then restored and the full list rerun (373 passed).

Edit that undoes the fix Turns red
SourceObjectDiffEngine.status returns .identical whenever the text matches testAnIndexOnlyDifferenceIsADifference, testAnIndexOnlyDifferenceCountsItsChangesInTheResultsList
?? indexes.failure removed from comparisonError testAFailedIndexReadOnEitherSideIsNotCompared, testAViewWhoseIndexesCouldNotBeReadIsNeverCreated
no not-carried note on a create testACreateOnATargetThatTakesNoIndexesSaysTheyAreLeftOut
.create returns the definition alone testACreatedMaterializedViewGetsTheSourcesIndexesAfterIt, testACopiedMaterializedViewGetsItsIndexesAfterItIsCreated
the builder's schema-binding guard always passes testIndexesThatWouldNameAnotherSchemaThanTheDefinitionAreRefused
index statements stamped with the bare name testAnIndexOnlyDifferenceChangesTheIndexesInPlace
the copy decision's schema check always passes testIndexStatementsThatWouldNameAnotherSchemaAreLeftOut, testADuplicatedDatabaseWhoseIndexesWouldLandInAnotherSchemaCopiesTheViewAndSaysSo
the rule ignores expression keys testAPredicateAnExpressionOrANonUniqueIndexDoesNot
carriage ignores the matrix all three SourceObjectIndexCarriageTests
the concurrent-refresh hazard keyed on .table testDroppingTheLastUniqueIndexWarnsThatAConcurrentRefreshStopsWorking, testAReplacementThatLosesTheUniqueIndexWarnsOnTheDrop
the guard drops sourceIndexes, non-table changes or definitionMatches the three new StructureChangeGuardTests that expect a refusal
carried kinds never read their indexes four new CompareMetadataReadPlanTests
the view definition read drops the index read testAMaterializedViewsIndexesTravelWithItsDefinition
the results list counts table changes only testAnIndexOnlyDifferenceCountsItsChangesInTheResultsList
Partly copied lists table notes only testPartlyCopiedListsViewsAsWellAsTables

Other steps: verify.sh build PASS; verify.sh docs PASS; verify.sh lint over the 31 changed Swift files: the one violation left is single_test_class on SourceObjectDiffEngineTests.swift, which already holds two test classes on the base branch. No UI automation: Compare & Sync and Copy To need two live database connections, which the UI test sandbox does not provide. Every decision behind the new UI is unit-tested above.

Before / After

Screenshots to be added. States to capture (a PostgreSQL pair, public on both sides, Compare & Sync with Materialized views included):

  1. Before: a materialized view with the same definition on both sides and a unique index only on the source reads Identical. After: it reads Different, 1 changes, and the Definitions pane shows the Indexes diff under the definition.
  2. After: the Script pane for that view holds CREATE UNIQUE INDEX … only, with no DROP MATERIALIZED VIEW.
  3. After: the Apply sheet's warnings for an index-only change that drops the view's only unique index, showing Concurrent refresh on the DROP INDEX.
  4. After: the Copy To review step for a PostgreSQL materialized view copied into a target that does not carry indexes, showing the Partly copied line.

Critique points not taken

None. Every objection was taken:

  • Schema binding (material). Index DDL is bound to the schema the definition creates the view in. The builder takes indexSchema, the target driver's current schema, which is what PostgreSQL's generateAddIndexSQL and generateDropIndexSQL qualify with, and refuses when it differs. Copy To leaves the indexes out with a note instead, which covers the duplicate-database path, whose planning scope has schema: nil. Measured and tested.
  • One object in the Apply sheet. Index statements carry identity.displayName.
  • Display-only index lines. A separate Indexes diff under the definition, in TableDefinitionRenderer's index-line format; sourceDefinition is untouched.
  • Concurrent-refresh warning. Added and tested, with the predicate PostgreSQLRelationSQL.concurrentRefreshQuery applies and the feat(datagrid): show whether a materialized view can be refreshed concurrently, and gate its structure edits by kind #3063 note words, over the fields the index read carries.
  • Carriage keyed on (engine, kind). SourceObjectIndexes.areCarried(for:by:) maps any source-defined kind to its TableInfo.TableType and asks the curated matrix, so SQL Server indexed views become a curation follow-up rather than a code change. The drop hazard no longer implies only rows and indexes go: it names comments and privileges.
  • Risk list. Restated under "Deliberately not fixed here".

Deviations from the design, each deliberate:

  • A carried object's failed index read is kept in TableStructureRead.objectIndexes as .failed rather than failing the whole read, so a refused index read is not confused with a refused column read and the definition is still read. The diff engine turns it into the comparisonError the design asked for.
  • SourceObjectDiffEngine takes the target endpoint's carried kinds rather than inferring carriage from the reads, because a create has no target read to infer it from.
  • The two drop texts are one hazard with two wordings rather than a drop hazard plus a second one.
  • replacesInPlace no longer skips the DROP for a result that carries indexes, so an engine curated later whose CREATE OR REPLACE drops a view's indexes cannot lose them.

Deliberately not fixed here

  • SQL Server indexed views. fetchIndexes there covers views (OBJECT_ID(name)), and fetchViewDefinition returns CREATE VIEW, so a Compare & Sync replacement of an indexed view loses its clustered index. Carrying them needs a curated SQL Server matrix row for .view that accepts .addIndex and .dropIndex; TablePro/Core/Compare/SourceObjectIndexes.swift:15 already routes .view through the matrix. Needs measuring first (SCHEMABINDING, clustered index first).
  • Materialized view comments and privileges are not recreated by a replacement or a copy. PluginDatabaseDriver.fetchCommentDDL exists and SQL export already uses it for materialized views; grants have no read path. The drop hazard now says both go (TablePro/Core/Compare/SyncSafetyClassifier.swift:154).
  • CockroachDB and Redshift stay definition-only. Neither has a curated matrix (TablePro/Core/Plugins/PluginMetadataRegistry+CuratedDefaults.swift:448 and :503), and Redshift's fetchIndexes answers with dist and sort keys. PG-wire forks connected as PostgreSQL (AlloyDB, Citus, Greenplum) use the .postgresql matrix and are carried.
  • Views never pair across two differently named schemas. SourceObjectDiffEngine.matchKey (TablePro/Core/Compare/SourceObjectDiffEngine.swift:166) includes the endpoint schema, so comparing a against b lists every view as only in source and only in target. With this change a materialized view create across such a pair refuses its index step rather than indexing the target's own view of the same name; the pairing is a separate defect.
  • ConcurrentRefreshIndexRule cannot see indisvalid. The index read does not carry it, so an index left invalid by a failed CREATE INDEX CONCURRENTLY counts as usable (TablePro/Core/Compare/SourceObjectIndexes.swift:82). indimmediate is always true on a materialized view, which takes no constraints. scripts/check-postgres-matview-refresh.sh checks the plugin predicate against a server, not this app-side mirror.
  • A unique index created after a copied or replaced view can fail when the target's recomputed rows violate it. The failure is reported against the view; the view itself is already created.

@datlechin
datlechin deleted the branch main September 23, 2026 19:19
@datlechin datlechin closed this Sep 23, 2026
@datlechin
datlechin deleted the fix/matview-indexes-compare-copy branch September 23, 2026 19:19
@datlechin
datlechin restored the fix/matview-indexes-compare-copy branch September 23, 2026 19:20
@datlechin datlechin reopened this Sep 23, 2026
@datlechin
datlechin changed the base branch from fix/compare-unreadable-definitions to main September 23, 2026 19:21
@mintlify

mintlify Bot commented Sep 23, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated
TablePro 🟢 Ready View Preview Sep 23, 2026, 7:28 PM

💡 Tip: Enable Automations to automatically generate PRs for you.

…ompare-copy

# Conflicts:
#	TablePro/Resources/Localizable.xcstrings
@datlechin
datlechin merged commit 0d2ad17 into main Sep 23, 2026
4 checks passed
@datlechin
datlechin deleted the fix/matview-indexes-compare-copy branch September 23, 2026 19:29

This branch was successfully deployed

1 active deployment
staging - docs 0023e615 Deployed Sep 23, 2026 by mintlify[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant