fix(compare): keep a materialized view's indexes through Compare & Sync and Copy To - #3079
Merged
Merged
Conversation
…currently, and gate its structure edits by kind
…ion could not be read
…ns' into fix/matview-indexes-compare-copy
…tgreSQL's own index DDL
datlechin
changed the base branch from
fix/compare-unreadable-definitions
to
main
September 23, 2026 19:21
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
…ompare-copy # Conflicts: # TablePro/Resources/Localizable.xcstrings
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
CompareRunnersends only.tablesnapshots toStructureDiffEngine;SourceObjectDiffEnginecompares normalised text;SourceObjectSyncBuilderandObjectCopyPlanneremit theCREATE MATERIALIZED VIEWtext verbatim.CompareMetadataServicedid read the view's indexes (the PostgreSQL bulk read has norelkindfilter, and the per-object read wastry? ?? []), but nothing used them.What changed
StructureObjectEditMatrix.SourceObjectIndexes.areCarried(for:by:)is true when the matrix accepts both.addIndexand.dropIndexon 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.tablesOnlyand 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 aCREATE INDEX.CompareMetadataService.readresolves carriage per endpoint and records a carried object's index read inTableStructureRead.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.readViewDefinitionsnow takes the structure reads and hands the index read toRoutineSourceRead.indexes.SourceObjectDiffEnginetakes the target's carried kinds. When both endpoints carry the kind it diffs the index sets with the signature rule tables use (StructureDiffEngine.indexChangesgained an array overload that the snapshot version delegates to). Same text plus index changes is different withdefinitionMatches. A failed index read on either side of a pair, or on the source of a create, is acomparisonError(Skip only); a drop never needs the target's indexes. A create into a target that does not carry the kind gets a note.SourceObjectSyncBuilder.build: a create writes the view, then its indexes. An.alterwhose definition matches writes only the index changes, with noDROP MATERIALIZED VIEW, so the stored rows stay. Any other.alterdrops, creates, then writes the source's indexes. Index DDL goes throughSchemaSyncScriptBuilder.changeStatements, now internal, so it sharesSchemaChangeOrdering,SchemaStatementGeneratorand the per-change hazards. Every statement carriesobjectName = identity.displayName, so one view is one object in the Apply sheet.indexSchema, the schema the target driver writes index DDL in (plugin.currentSchemainside 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.concurrentRefreshwarning marks theDROP INDEX(index-only alter) or theDROP MATERIALIZED VIEW(replacement) when the target's index set allowsREFRESH MATERIALIZED VIEW CONCURRENTLYand the resulting set does not.ConcurrentRefreshIndexRulemirrors the predicate inPostgreSQLRelationSQL.concurrentRefreshQueryover the fields the index read carries: unique, b-tree, no predicate, no expression key.StructureGenerationInputnow carriessourceIndexes(identity stripped),definitionMatches, andchangesfor every kind, so an index changed after comparing refuses the script.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.ObjectCopyPlanner.definitionBuilddecides 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:mv_id_idx, the view's only unique index, madeREFRESH MATERIALIZED VIEW CONCURRENTLYfail: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 afterDROP+CREATEwith the same text).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, andREFRESH … CONCURRENTLYthen succeeded.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 byCREATE UNIQUE INDEX … ON "b"."mv"lefta.mvwith 0 indexes and gaveb.mv1. This is the critique's schema-binding case, confirmed.indexListquery over matviews reportsis_unique,index_type = btree,predicateandexpressionsfor each shape. Against the server: a plain unique index, a multi-column unique index,(id DESC NULLS LAST)and(id) INCLUDE (customer)all allowedREFRESH … CONCURRENTLY; a unique expression index((id + 0))and a partial unique indexWHERE id > 0both refused it;USING hashcannot be unique at all.ConcurrentRefreshIndexRuleTestspins these shapes.currentPluginKitVersionis untouched.Tests
All through
verify.sh teston the final tree: 43 suites, 373 cases executed, 373 passed. That is every suite that owns a changed type (greppedTableProTestsfor each), plus the new ones.New suites, one test class per file:
MaterializedViewIndexCompareTestsdefinitionMatchesand oneaddIndex(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 acomparisonErrorwith Skip only and never yields adeleteIndex. 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.materializedViewis carried by.postgreSQL,.viewis not,.tablesOnlycarries nothing. ThroughPluginManager: PostgreSQL and PGlite carry[.materializedView]; CockroachDB, Redshift and an unknown engine carry nothing.SourceObjectIndexCopyTestsConcurrentRefreshIndexRuleTestsAdded to existing suites:
SourceObjectSyncBuilderTests(+10: create order andobjectName; replacement recreates the indexes and carries the new drop hazard; index-only alter never drops the view and runsDROP INDEXbeforeCREATE INDEX; both concurrent-refresh warnings and their absence when a usable index remains; schema-binding refusal, including an unknownindexSchema; 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 whosegenerateAddIndexSQLisPostgreSQLIndexClauses.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).
SourceObjectDiffEngine.statusreturns.identicalwhenever the text matchestestAnIndexOnlyDifferenceIsADifference,testAnIndexOnlyDifferenceCountsItsChangesInTheResultsList?? indexes.failureremoved fromcomparisonErrortestAFailedIndexReadOnEitherSideIsNotCompared,testAViewWhoseIndexesCouldNotBeReadIsNeverCreatedtestACreateOnATargetThatTakesNoIndexesSaysTheyAreLeftOut.createreturns the definition alonetestACreatedMaterializedViewGetsTheSourcesIndexesAfterIt,testACopiedMaterializedViewGetsItsIndexesAfterItIsCreatedtestIndexesThatWouldNameAnotherSchemaThanTheDefinitionAreRefusedtestAnIndexOnlyDifferenceChangesTheIndexesInPlacetestIndexStatementsThatWouldNameAnotherSchemaAreLeftOut,testADuplicatedDatabaseWhoseIndexesWouldLandInAnotherSchemaCopiesTheViewAndSaysSotestAPredicateAnExpressionOrANonUniqueIndexDoesNotSourceObjectIndexCarriageTests.tabletestDroppingTheLastUniqueIndexWarnsThatAConcurrentRefreshStopsWorking,testAReplacementThatLosesTheUniqueIndexWarnsOnTheDropsourceIndexes, non-tablechangesordefinitionMatchesStructureChangeGuardTeststhat expect a refusalCompareMetadataReadPlanTeststestAMaterializedViewsIndexesTravelWithItsDefinitiontestAnIndexOnlyDifferenceCountsItsChangesInTheResultsListtestPartlyCopiedListsViewsAsWellAsTablesOther steps:
verify.sh buildPASS;verify.sh docsPASS;verify.sh lintover the 31 changed Swift files: the one violation left issingle_test_classonSourceObjectDiffEngineTests.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,
publicon both sides, Compare & Sync with Materialized views included):1 changes, and the Definitions pane shows the Indexes diff under the definition.CREATE UNIQUE INDEX …only, with noDROP MATERIALIZED VIEW.DROP INDEX.Critique points not taken
None. Every objection was taken:
indexSchema, the target driver's current schema, which is what PostgreSQL'sgenerateAddIndexSQLandgenerateDropIndexSQLqualify 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 hasschema: nil. Measured and tested.identity.displayName.TableDefinitionRenderer's index-line format;sourceDefinitionis untouched.PostgreSQLRelationSQL.concurrentRefreshQueryapplies 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.SourceObjectIndexes.areCarried(for:by:)maps any source-defined kind to itsTableInfo.TableTypeand 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.Deviations from the design, each deliberate:
TableStructureRead.objectIndexesas.failedrather 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 thecomparisonErrorthe design asked for.SourceObjectDiffEnginetakes the target endpoint's carried kinds rather than inferring carriage from the reads, because a create has no target read to infer it from.replacesInPlaceno longer skips theDROPfor a result that carries indexes, so an engine curated later whoseCREATE OR REPLACEdrops a view's indexes cannot lose them.Deliberately not fixed here
fetchIndexesthere covers views (OBJECT_ID(name)), andfetchViewDefinitionreturnsCREATE VIEW, so a Compare & Sync replacement of an indexed view loses its clustered index. Carrying them needs a curated SQL Server matrix row for.viewthat accepts.addIndexand.dropIndex;TablePro/Core/Compare/SourceObjectIndexes.swift:15already routes.viewthrough the matrix. Needs measuring first (SCHEMABINDING, clustered index first).PluginDatabaseDriver.fetchCommentDDLexists 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).TablePro/Core/Plugins/PluginMetadataRegistry+CuratedDefaults.swift:448and:503), and Redshift'sfetchIndexesanswers with dist and sort keys. PG-wire forks connected as PostgreSQL (AlloyDB, Citus, Greenplum) use the.postgresqlmatrix and are carried.SourceObjectDiffEngine.matchKey(TablePro/Core/Compare/SourceObjectDiffEngine.swift:166) includes the endpoint schema, so comparingaagainstblists 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.ConcurrentRefreshIndexRulecannot seeindisvalid. The index read does not carry it, so an index left invalid by a failedCREATE INDEX CONCURRENTLYcounts as usable (TablePro/Core/Compare/SourceObjectIndexes.swift:82).indimmediateis always true on a materialized view, which takes no constraints.scripts/check-postgres-matview-refresh.shchecks the plugin predicate against a server, not this app-side mirror.