[catalog] fix duplicate rows in SHOW COLUMNS after builtin type migration #37116
Conversation
mtabebe
force-pushed
the
ma/fix-duplicate-rows-in-show-sources
branch
from
3342b35 to
d0fcc06
Compare
Problem: After PR MaterializeInc#36674 converted mz_sources from a BuiltinTable to a BuiltinMaterializedView, SHOW COLUMNS FROM mz_sources returns each column twice on environments that upgraded across the change. The schema migration preserves the catalog id of mz_sources but does not drop the prior CommentObjectId::Table(id) rows, so mz_internal.mz_comments ends up with 15 'table' + 15 'materialized-view' rows for the same id. Solution: Predicate the comment join in mz_show_columns on object_type by also joining against mz_catalog.mz_objects to get the live type for each id. Testing: - New test/sqllogictest/show_columns.slt locks in per-builtin column counts for mz_sources, mz_tables, mz_secrets and exercises a user table with COMMENT ON COLUMN. - test/sqllogictest/catalog_server_explain.slt rewritten via --rewrite-results for the new EXPLAIN plan. Manually verified end-to-end: - ran environmentd on old version with mz_sources as Tableto populate the durable catalog, then restarted. - Before this fix SHOW COLUMNS FROM mz_sources returned 30 rows - after it returns 15
Problem: When a builtin's catalog item type changes across a release (e.g. mz_sources going Table -> MaterializedView in PR MaterializeInc#36674), the schema migration preserves the catalog id but does not drop comment rows written under the prior CommentObjectId variant. The builtin loader in add_new_remove_old_builtin_items_migration only drops comments under the *current* type's variant before re-inserting, so the old-type rows accumulate in mz_internal.mz_comments forever. Solution: Drop comments under all relation-style CommentObjectId variants (Table, View, MaterializedView, Source) for the same id, not just the current one. The re-bind loop runs every boot for every builtin, so this both cleans up existing stale rows on the next restart and self-heals any future Type<->Type schema migration. Testing: Manually verified end-to-end against the upgrade scenario
mtabebe
force-pushed
the
ma/fix-duplicate-rows-in-show-sources
branch
from
d0fcc06 to
1369fc7
Compare
ggevay
left a comment
ggevay
left a comment
There was a problem hiding this comment.
LGTM, just minor comments
| // for this id, not just the current one. When a builtin's type changes | ||
| // (e.g. Table -> MaterializedView) but its catalog id is preserved, the | ||
| // prior type's comment rows would otherwise linger forever. | ||
| txn.drop_comments(&BTreeSet::from_iter([ |
There was a problem hiding this comment.
Consider moving this drop above the match, so it runs for every builtin rather than only the ones whose current type is a relation. The continue for Log | Type | Func | Index | Connection happens before we get here, so a builtin that changes type from, say, Table to Log (logs are relations with columns too) would leave its stale Table comment rows behind forever, which is the same class of bug this PR fixes. Dropping the four relation variants keyed only on id for every builtin closes the whole class, and the comment-writing below can keep its current shape.
| ON columns.id = comments.id AND columns.position = comments.object_sub_id", | ||
| ON columns.id = comments.id | ||
| AND columns.position = comments.object_sub_id | ||
| AND comments.object_type = obj.type", |
There was a problem hiding this comment.
For completeness: a few other views join mz_comments without an object_type predicate and had the same exposure, e.g. the ontology column-description join in src/catalog/src/builtin/ontology.rs and the cts_obj/cts_col joins in the SHOW CREATE views further down this file. The boot-time cleanup in this PR protects them in steady state (stale rows are gone before the upgraded version serves queries), so leaving them seems fine, but it might be worth a follow-up issue so this doesn't come back one view at a time.
| # SHOW COLUMNS, regardless of whether it's a Table or MaterializedView. | ||
|
|
||
| query I | ||
| SELECT COUNT(*) FROM (SHOW COLUMNS FROM mz_sources) |
There was a problem hiding this comment.
These hardcoded counts will need updating every time someone adds a column to one of these builtins, and a bare count change won't tell that person whether they broke deduplication or just added a column. Something like SELECT COUNT(*) = COUNT(DISTINCT name) FROM (SHOW COLUMNS FROM mz_sources) asserts the no-duplicates property directly and never needs maintenance. Keeping one absolute count as an anchor seems plenty.
| # Regression: even if mz_comments holds rows for the same id under | ||
| # multiple object_types (which can happen after a builtin type change | ||
| # where the catalog id is preserved), SHOW COLUMNS must still emit | ||
| # exactly one row per column. We can't directly inject a stale row |
There was a problem hiding this comment.
The write-side scenario can actually be covered by a unit test, since Op::Comment doesn't validate that the CommentObjectId variant matches the item's actual type (that validation lives in the sequencer's planning). A test in src/adapter/src/catalog.rs following the test_catalog_revision pattern can open a debug catalog, transact a stale CommentObjectId::Table(id) comment for mz_sources, reopen on the same persist client, and assert the stale row is gone from state.comments while the legitimate materialized-view column comments survived. Note skip_migrations: true in debug opens only skips the user-item AST migrations, add_new_remove_old_builtin_items_migration still runs. Optional.
3 participants
After converting
mz_sourcesfrom a BuiltinTable to aBuiltinMaterializedView,
SHOW COLUMNS FROM mz_sourcesreturned eachcolumn twice on environments that upgraded across the change.
The cause is that the builtin schema migration preserves the catalog id but does not
drop the prior comment rows.
mz_internal.mz_commentsends up with 15 table + 15 materialized-viewrows for the same id, and mz_show_columns joined against all of them.
The same class of bug will reappear any time a builtin's
CatalogItemTypechanges across a release.
Two complementary fixes (1 commit each)
Read side: mz_show_columns, predicate the comment join on object_type
against mz_objects.type.
Write side: on each boot, drop comments under every relation-style
CommentObjectId variant for the builtin's id, not just the current one.
This cleans up the stale rows on the next restart and self-heals any future migrations.
Tests:
test/sqllogictest/show_columns.sltlocks in per-builtin columncounts for
mz_sources,mz_tables,mz_secrets, and exercises auser table with
COMMENT ON COLUMN.environmentdon the old version withmz_sourcesas a Table to populate the durable catalog, thenrestarted