catalog: convert the sink builtin tables to materialized views (SQL-493) - #38049
Conversation
2fc1a99 to
77a62d4
Compare
| // resolves. Nothing is lost: anything that needed the old step upgrades | ||
| // from further back than this one, so this one covers it too. | ||
| MigrationStep::replacement( | ||
| "26.37.0-dev.0", |
ggevay
left a comment
There was a problem hiding this comment.
LGTM, just one minor comment
| CatalogItemType::MaterializedView, | ||
| MZ_CATALOG_SCHEMA, | ||
| "mz_iceberg_sinks", | ||
| ), |
There was a problem hiding this comment.
These three conversions also need entries in misc/python/materialize/checks/all_checks/builtin_version_pin.py: _STABLE_BUILTINS maps each stable mz_catalog builtin to the version that converted it to a view, and mz_sinks, mz_kafka_sinks and mz_iceberg_sinks currently sit at None there. The file's comment asks for this at conversion time, and #37465 recorded mz_kafka_sources in the conversion PR itself. Could you set all three to the version these steps end up at (currently that would be v26.38.0), keeping the two in lockstep?
Note nothing goes red when this is missed: every reader on current upgrade paths carries the #37610 tolerance fix (>= v26.35), so a None entry just keeps the check pinning through the conversion and passing. It only shows up by reading, which is presumably how #37725's tables got missed too: mz_kafka_connections, mz_ssh_tunnel_connections and mz_aws_privatelink_connections are still None on main despite converting in released v26.37.0, and mz_aws_connections is absent from the dict entirely (it was a stable mz_catalog table when the dict was seeded, so it looks like an oversight). Feel free to fold the backfill in here: those four would be v26.37.0.
There was a problem hiding this comment.
Good catch.
I left mz_aws_connections out, though. It's in mz_internal, not mz_catalog, and it was mz_internal even as a table before #37725, so I don't think it was ever a seeded mz_catalog entry.
The script builds CREATE VIEW ... FROM mz_catalog so it isn't safe to go in.
Problem: mz_sinks, mz_kafka_sinks and mz_iceberg_sinks are builtin tables that pack_sink_update fills on the leader environmentd, on every catalog change. Getting rid of those writes is the point of this whole series (SQL-118 / multi-envd), and the sink family is the last item in M1. Everything the three tables hold is already sitting in the persisted create_sql, so none of it has to be packed by Rust in the first place. Note mz_continual_tasks is named in the issue but no longer exists. The CT feature was removed in MaterializeInc#35967, and the v81_to_v82 migration asserts no CT items remain, so there is nothing there to convert. The Linear description wants correcting. Solution: Two parts. parse_catalog_create_sql gets a CreateSink arm, alongside the CreateSource one it mirrors. It emits sink_type, cluster_id, connection_id, envelope_type, format/key_format/value_format, the kafka topic, and the iceberg namespace/table. The deprecated format collapse stays in Rust rather than moving into the view SQL, because its edges are fiddly and here they are unit-testable. Only avro/avro and json/json collapse to a single name, and a key format only exists once the sink declares a KEY, which is easy to get wrong reading the statement alone. Then all three tables become BuiltinMaterializedViews over mz_catalog_raw and pack_sink_update goes away, since packing them was its only job. Each view keeps its columns, order, nullability, keys, comments, access and ontology as they were, and reuses its old OID so pg identity does not move. No privileges column, unlike what mz_sources grew, to keep this a straight parity conversion. - three MigrationStep::replacement entries at the current dev version. - the old 0.160.0 step naming mz_sinks as a Table had to go with them: it no longer resolves, and validate_migration_steps panics on that at catalog open. Since that only reproduces against a catalog already on disk, there is now a unit test asserting every step resolves. Testing: - New test/sqllogictest/mz_sinks.slt pins the column shape of all three views, that they are materialized views, that non-sink items are excluded, and the JSON keys the view SQL expects back from the helper. Sinks cannot be created in sqllogictest at all, since purification always talks to the broker, so derived values are not covered here. - mz-sinks.td picks up what nothing asserted before: key_format on a keyed bare format, the composite key-text-value-text form, and mz_kafka_sinks.topic. - test/iceberg/mode-append.td picks up envelope_type = append and mz_iceberg_sinks.namespace/table. The iceberg path had no catalog coverage. - Unit tests in jsonb.rs for the new arm: bare and key/value formats with and without a KEY, all three envelope values, the avro/json collapse against text/text and text/bytes, a missing TOPIC and a missing TABLE, and a non-sink statement as a regression guard. - The tables.td "cannot insert into system table" assertions move off mz_kafka_sinks and onto mz_storage_usage_by_shard, which the migration validator refuses to convert. - Wants the full and upgrade nightly. The migration steps and the fingerprint checks only run against an existing catalog. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
77a62d4 to
e894378
Compare
def-
left a comment
There was a problem hiding this comment.
No further complaints from qa side
Problem:
mz_sinks, mz_kafka_sinks and mz_iceberg_sinks are builtin tables that pack_sink_update fills on the leader environmentd, on every catalog change. Getting rid of those writes is the point of this whole series (SQL-118 / multi-envd), and the sink family is the last item in M1.
Everything the three tables hold is already sitting in the persisted create_sql, so none of it has to be packed by Rust in the first place.
Note mz_continual_tasks is named in the issue but no longer exists. The CT feature was removed in #35967, and the v81_to_v82 migration asserts no CT items remain, so there is nothing there to convert. The Linear description wants correcting.
Solution:
Two parts.
parse_catalog_create_sql gets a CreateSink arm, alongside the CreateSource one it mirrors. It emits sink_type, cluster_id, connection_id, envelope_type, format/key_format/value_format, the kafka topic, and the iceberg namespace/table.
The deprecated format collapse stays in Rust rather than moving into the view SQL, because its edges are fiddly and here they are unit-testable. Only avro/avro and json/json collapse to a single name, and a key format only exists once the sink declares a KEY, which is easy to get wrong reading the statement alone.
Then all three tables become BuiltinMaterializedViews over mz_catalog_raw and pack_sink_update goes away, since packing them was its only job. Each view keeps its columns, order, nullability, keys, comments, access and ontology as they were, and reuses its old OID so pg identity does not move. No privileges column, unlike what mz_sources grew, to keep this a straight parity conversion.
Testing: