sql-499: print ids for types in DOC ON positions by SangJunBak · Pull Request #38612 · MaterializeInc/materialize

SangJunBak · 2026-09-01T23:10:37Z

Context:
DOC ON TYPE x and DOC ON COLUMN x.c resolve a name that may denote a
type or a relation, and both spellings persist in the sink's create_sql.
A relation in these positions prints as [id AS name], but resolution
suppressed ids for types, so a type persisted as a bare qualified name.
A bare name strands the sink if the type, its schema, or its database is
renamed, and it hides the sink's dependency on the type from create_sql
reference extraction: mz_object_dependencies files the bare name under
named_relations, whose join excludes Type rows, so the edge silently
drops out of the view even though the in-memory dependency graph (which
blocks DROP TYPE) still records it.

Force ids in the two places that construct DOC ON references: the name
resolver's DOC ON folds, and sink purification, which injects DOC ON
options for every commented item the sink references. An AST migration
rewrites existing catalogs to match

Motivation

Closes sql-499

Verification

Added platform check to test the upgrade and modified a td to assert it.

linear-code · 2026-09-01T23:10:41Z

SQL-499

def- · 2026-09-01T23:55:56Z

QA LLM Review

1. HIGH -- `DOC ON` id migration can bind a type reference to a same-named non-type item

src/adapter/src/catalog/migrate.rs:1081

The migration resolves a bare DOC ON name by scanning every durable item in the schema and taking the lowest-id match, but the durable catalog deliberately lets a Type share a (schema, name) with a Sink, Secret, Connection, Func, or MetricSink. When such a pair exists and the non-type item was created first, the rewrite points the reference at the wrong object: for DOC ON TYPE the sink silently loses the type's doc comment and its dependency edge moves to the unrelated item, and for DOC ON COLUMN the sink's create_sql becomes unplannable, so environmentd panics during catalog open on every boot after the upgrade.

Details

The collision is by design, not a corner case the catalog forbids. src/catalog/src/durable/transaction.rs:212 treats (schema_id, name) as unique only when both item types conflict per CatalogItemType::conflicts_with_type, which returns false for Sink, Secret, Connection, Func, and MetricSink (src/sql/src/catalog.rs:997). plan_create_type gates on the same predicate (src/sql/src/plan/statement/ddl.rs:4699) while plan_create_sink and plan_create_connection check only resolve_item (ddl.rs:3241, ddl.rs:5852), so CREATE CONNECTION point TO KAFKA (...) followed by CREATE TYPE point AS (x int, y int) both succeed.

Name resolution disambiguates by map: resolve_item_name_name tries resolve_type (backed by schema.types) before resolve_item (backed by schema.items) at src/sql/src/names.rs:1498. That is exactly why the bare name in old create_sql still resolved to the type. tx.get_items().find(...) has no such filter, and get_items() sorts by CatalogItemId, so it returns whichever of the two happens to hold the lower id.

Failure modes after a wrong rewrite:

DOC ON TYPE — planning yields DocTarget::Type(<connection id>) (ddl.rs:3698), which matches no type in the sink's Avro schema, so the comment vanishes from the published schema. The sink's resolved_ids now name the connection instead of the type, so mz_object_dependencies reports the wrong edge and DROP TYPE point is no longer blocked.
DOC ON COLUMN — this is the shape purification injects for a commented type column, so it is the common case, not the exotic one. fold_column_name (src/sql/src/names.rs:1853) sees type_details() == None and relation_desc() == None for a connection/sink/secret and raises PlanError::ItemWithoutColumns; deserialize_item then fails and catalog open hits panic!("{e:?}: invalid persisted SQL: ...") (src/adapter/src/catalog/apply.rs:1293). The migration transaction never commits, so the next boot repeats the same rewrite and the same panic. An environment that booted fine on the previous release is stuck in a crash loop with no SQL-level way out.

Fix: restrict the lookup to type items, and leave the reference as a name when nothing matches instead of panicking. mz_catalog::durable::Item::item_type() already exists. Relations in DOC ON positions always carried ids, so a surviving bare name is always a type; a None result is the safe outcome for anything else.

let item = tx.get_items().find(|i| {
    i.name == item_name.as_str()
        && i.schema_id == schema.id
        && i.item_type() == CatalogItemType::Type
})?;
Some(item.id.to_string())

Making the database and schema arms return None rather than panic! closes the same boot-panic hazard on those lookups: a reference the migration cannot confidently resolve is better left alone than turned into a startup abort.

ggevay · 2026-09-02T11:39:37Z

I've kicked off the upgrade subset of Nightly: https://buildkite.com/materialize/nightly/builds/18202

ggevay

Some comments.

(Should also rebase on main to get rid of some unrelated CI flakes.)

ggevay · 2026-09-02T11:41:26Z

+            schema.unwrap_or_else(|| panic!("missing schema in doc on reference: {name:?}"));
+        let item = tx
+            .get_items()
+            .find(|i| i.name == item_name.as_str() && i.schema_id == schema.id);


Should-fix, reproduced. A type may share (schema, name) with a secret, connection, sink, or function (conflicts_with_type is false for those, and the durable uniqueness check, plan_create_sink, and plan_create_connection all allow it), and get_items() is id-sorted, so this binds to whichever same-named item is older. I upgraded a v26.40.0-rc.3 catalog holding CREATE SECRET point created before a commented CREATE TYPE point, plus an Avro sink over an MV using it, to this branch: with only the type commented, the reference became DOC ON TYPE [u11 AS ...] with u11 the secret and the sink's edge moved to the secret; with a column comment as well, the new version panics at catalog open on every boot (ItemWithoutColumns { .., item_type: Secret } from apply.rs, the rewrite never commits), so under 0dt the upgrade is stuck and without 0dt it is an outage. Add && i.item_type() == CatalogItemType::Type as the last conjunct (it parses create_sql): a bare name in a DOC ON position can only be a type, since relations always carried ids there and functions cannot resolve there.

ggevay · 2026-09-02T11:41:26Z

+        let parts = &name.0;
+        let (db_name, schema_name, item_name) = match parts.len() {
+            3 => (&parts[0], &parts[1], &parts[2]),
+            2 => return None,


Nit: builtin types are durable too, as GidMapping rows (tx.get_system_object_mappings(), description.{schema_name, object_type, object_name} and unique_identifier.catalog_id), so the same lookup works here and would close the last gap: an old sink with an explicit DOC ON TYPE int4 keeps the bare name and gets no edge, a new one prints [sNN AS pg_catalog.int4] and does. Reachable only through an explicit DOC ON (a superuser cannot comment on a builtin type, only mz_system can), hence nit level; the doc comment's "not durable items" should go either way.

ggevay · 2026-09-02T11:41:26Z

+/// as a bare qualified name, unlike a relation in the same position, because
+/// name resolution suppressed ids for types. Resolution now prints the id
+/// (see `NameResolver::resolve_doc_on_name`); this rewrites stored statements
+/// to match, so the reference survives renames and `create_sql` reference


Today no rename strands a bare name: ALTER TYPE only parses OWNER TO, databases have no rename, and ALTER SCHEMA RENAME rewrites bare names too (CreateSqlRewriteSchema::visit_unresolved_item_name_mut). Fine as future-proofing, so please phrase it that way here and in the commit message and PR body ("would survive renames if those were ever added") rather than as a present bug.

ggevay · 2026-09-02T11:41:26Z

+
+    def initialize(self) -> Testdrive:
+        return Testdrive(dedent("""
+                > CREATE TYPE sink_comments_point AS (x integer, y integer)


Cheap end-to-end cover for the collision above: > CREATE SECRET sink_comments_point AS 'x' before this line, so the upgrade scenarios must bind the type rather than the secret (the edge assertion in validate catches a wrong binding, which is how my reproduction failed). Also keep the 2604100 guards in step with the version this actually lands in.

DOC ON TYPE x and DOC ON COLUMN x.c resolve a name that may denote a type or a relation, and both spellings persist in the sink's create_sql. A relation in these positions prints as [id AS name], but resolution suppressed ids for types, so a type persisted as a bare qualified name. A bare name strands the sink if the type, its schema, or its database is renamed, and it hides the sink's dependency on the type from create_sql reference extraction: mz_object_dependencies files the bare name under named_relations, whose join excludes Type rows, so the edge silently drops out of the view even though the in-memory dependency graph (which blocks DROP TYPE) still records it. Force ids in the two places that construct DOC ON references: the name resolver's DOC ON folds, and sink purification, which injects DOC ON options for every commented item the sink references. An AST migration in the next commit rewrites existing catalogs to match. Functions keep printing without ids, preserving the convention that function references never persist ids. No function can reach the injection today anyway: COMMENT ON FUNCTION does not resolve, so no function carries a comment.

Name resolution now prints ids for types in DOC ON positions, but existing catalogs still hold sink create_sql with bare qualified type names. Rewrite those to [id AS name] form, resolving the name against the durable catalog, so the reference survives renames and create_sql reference extraction (mz_object_dependencies) recovers the sink's edge to the type. References that already carry an id are skipped, so the rewrite is idempotent and safe to run every boot. Names without a database part denote items in ambient (system) schemas: those are not durable items and builtin names are stable, so they are left resolving by name.

Assert in kafka-avro-sinks-doc-comments.td that a sink over a relation with a commented custom type records a dependency edge on the type, and add a KafkaSinkCommentsOnType platform check so the edge also survives restarts and upgrades. In upgrade scenarios the check's initial sink is created by the old version with a bare type name in its DOC ON reference, so validating the edge on the new version exercises the AST migration end to end.

SangJunBak requested review from a team as code owners September 1, 2026 23:10

SangJunBak mentioned this pull request Sep 1, 2026

sql-499: Convert mz_object_dependencies #38252

Open

ggevay reviewed Sep 2, 2026

View reviewed changes

SangJunBak force-pushed the jun/print-comment-types-for-kafka-sinks branch from bd71692 to 7f8f84e Compare September 2, 2026 19:06

SangJunBak added 3 commits September 2, 2026 20:06

SangJunBak force-pushed the jun/print-comment-types-for-kafka-sinks branch from 7f8f84e to 83fac0f Compare September 2, 2026 20:26

Navigation Menu

Uh oh!

sql-499: print ids for types in DOC ON positions - #38612

sql-499: print ids for types in DOC ON positions#38612
SangJunBak wants to merge 3 commits into
jun/convert-mz-object-dependenciesfrom
jun/print-comment-types-for-kafka-sinks

SangJunBak commented Sep 1, 2026

Uh oh!

linear-code Bot commented Sep 1, 2026

Uh oh!

def- commented Sep 1, 2026

Uh oh!

ggevay commented Sep 2, 2026

Uh oh!

ggevay left a comment

Uh oh!

ggevay Sep 2, 2026

Uh oh!

ggevay Sep 2, 2026

Uh oh!

ggevay Sep 2, 2026

Uh oh!

ggevay Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

SangJunBak commented Sep 1, 2026

Motivation

Verification

Uh oh!

linear-code Bot commented Sep 1, 2026

Uh oh!

def- commented Sep 1, 2026

QA LLM Review

1. HIGH -- DOC ON id migration can bind a type reference to a same-named non-type item

Uh oh!

ggevay commented Sep 2, 2026

Uh oh!

ggevay left a comment

Choose a reason for hiding this comment

Uh oh!

ggevay Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

ggevay Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

ggevay Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

ggevay Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1. HIGH -- `DOC ON` id migration can bind a type reference to a same-named non-type item