catalog: convert the connection-detail tables to materialized views (SQL-497) - #37725
catalog: convert the connection-detail tables to materialized views (SQL-497)#37725mtabebe wants to merge 3 commits into
Conversation
48e66e8 to
e2243b3
Compare
e2243b3 to
8253589
Compare
…views Problem: mz_kafka_connections and mz_ssh_tunnel_connections were populated by Rust packers (pack_kafka_connection_update and pack_ssh_tunnel_connection_update) that ran on every catalog change on the leader environmentd. Solution: Add parse_connection_details, a connection-type-agnostic SQL helper that pulls the broker addresses and any explicit progress topic (kafka) or the two public keys (ssh) out of a connection's persisted create_sql. It returns jsonb null for anything that is not one of those connection types, so callers filter on IS NOT NULL and gate on the connection type separately, the way mz_connections already does. Convert both tables to BuiltinMaterializedView over mz_internal.mz_catalog_raw. Each view parses create_sql with the helper and reconstructs the default kafka progress topic (_materialize-progress-<env>-<conn>) in SQL, matching KafkaConnection::progress_topic. The two packers and their dispatch arms are removed, along with the now-unused imports. Testing: - Unit tests in jsonb.rs cover kafka (single broker, broker list, explicit and defaulted progress topic), ssh, a connection type without a detail view, and a non-connection statement. - New lockdown SLTs (mz_kafka_connections.slt, mz_ssh_tunnel_connections.slt) pin the column shape and nullability, confirm the views are empty on a fresh env, check the progress-topic default against its format string, and verify the wrong connection type is excluded. - connection-create-drop.td and test/ssh-connection/* query these views unchanged. - Regenerated oid.slt, information_schema_tables.slt, catalog_server_explain.slt, mz_catalog_server_index_accounting.slt, and catalog.td. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…erialized views Problem: mz_aws_connections and mz_aws_privatelink_connections were populated by Rust packers (pack_aws_connection_update and pack_aws_privatelink_connection_update) that ran on every catalog change on the leader environmentd. These connections carry values that are not in create_sql: the AWS principal, external id, and example trust policy for an assume-role aws connection, and the privatelink principal ARN. All of these derive from environment context (the AWS account id, external-id prefix, and connection role ARN), not from the connection definition. Solution: Extend parse_connection_details with an aws branch that pulls the eight create_sql-derived fields (endpoint, region, the inline/secret credentials, and the assume-role options) out of the connection's persisted create_sql. A secret reference resolves to the referenced secret's catalog id. Expose the three environment-context values that the views need through nullary functions that fold to a literal at plan time, the same mechanism mz_environment_id uses: mz_aws_account_id, mz_aws_external_id_prefix, and mz_aws_connection_role_arn. The account id is plumbed onto CatalogConfig (the external-id prefix and role ARN already live on its ConnectionContext). Each folds to NULL on an environment without the corresponding context. Like mz_environment_id, they are deliberately not gated by restrict_to_user_objects, because a fold cannot be gated meaningfully. The system connection views stay blocked for restricted sessions because they are system relations. Convert both tables to BuiltinMaterializedView. The aws view reconstructs the assume-role principal, external id, and trust policy from the context functions, matching AwsAssumeRole::external_id / example_trust_policy. The privatelink view reconstructs the principal ARN, matching AwsPrincipalContext::to_principal_string, and keeps WHERE principal IS NOT NULL so it emits no row when the AWS principal context is absent, matching the packer that skipped the row in that case. The two packers, their dispatch, and now-unused imports are removed. Testing: - Unit tests in jsonb.rs cover the aws branch: inline access key, all-secret credentials with a session token, and assume-role. - New lockdown SLTs (mz_aws_connections.slt, mz_aws_privatelink_connections.slt) pin the column shape and nullability and the wrong-type exclusion. The sqllogictest env configures an AWS context, so the built-in mz_analytics assume-role connection doubles as the SQL<->Rust parity check for principal, external_id, and example_trust_policy. Privatelink has no context there, so the view is empty, matching the packer's skip. - Regenerated oid.slt, information_schema_tables.slt, catalog_server_explain.slt, mz_catalog_server_index_accounting.slt, and catalog.td. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8253589 to
efed4ce
Compare
| name: ConnectionOptionName, | ||
| ) -> Option<String> { | ||
| values.iter().find_map(|o| match &o.value { | ||
| Some(WithOptionValue::Value(Value::String(s))) if o.name == name => Some(s.clone()), |
There was a problem hiding this comment.
The old packer read planned values, and planning accepts these option values in more forms than Value::String: the blanket impl in src/sql/src/plan/with_options.rs (impl<V: TryFromValue<Value>, ...> TryFromValue<WithOptionValue<T>> for V) also converts Ident and 1-part UnresolvedItemName to strings, and neither normalization nor AstDisplay canonicalizes them, so unquoted values persist unquoted. In-tree proof: the SHOW CREATE golden in test/testdrive/connection-create-drop.td:62 shows a persisted SECURITY PROTOCOL = plaintext. So CREATE CONNECTION ... (PROGRESS TOPIC = my_topic) plans fine, but this helper returns null for it, and the view then silently reports the default progress topic instead of the real one. Unquoted aws options (e.g. ASSUME ROLE SESSION NAME) similarly come out NULL.
Could you match WithOptionValue::Ident and 1-part WithOptionValue::UnresolvedItemName here too, mirroring the blanket impl? Two related suggestions:
- Consider making this a shared helper used by all the catalog-raw parsers. The already-merged
parse_kafka_source_details(catalog: convert mz_postgres_sources and mz_kafka_sources to materialized views #37465) has the identical gap, where it is worse: an unquotedTOPIC = tmakes the wholemz_kafka_sourcesview error ("missing TOPIC option"), and an unquotedGROUP ID PREFIXsilently drops the prefix from the reconstructed group id. A shared helper would fix both and keep the next conversion from reintroducing the class. - Longer-term: it would be great to make this parity executable, in the spirit of the mz_audit_events round-trip test, e.g. comparing parser output against planned state for the statements the test corpus creates. Both instances of this bug would have been caught mechanically while the packer (the oracle) still existed.
Minor, same neighborhood: planning also coerces ENDPOINT = '' to None (see the TODO in plan/statement/ddl/connection.rs), so the packer emitted NULL where the view now yields ''. Probably fine to just fold the same coercion into the helper or note it.
| pub const MV_MZ_SQL_SERVER_SOURCE_TABLES_OID: u32 = 17109; | ||
| pub const MV_MZ_KAFKA_SOURCE_TABLES_OID: u32 = 17110; | ||
| pub const FUNC_PARSE_SOURCE_EXPORT_DETAILS_OID: u32 = 17111; | ||
| pub const MV_MZ_KAFKA_CONNECTIONS_OID: u32 = 17112; |
There was a problem hiding this comment.
We agreed on the opposite convention in #36912 (comment): reuse the old OIDs and just rename the constant (seven of the eight conversions before that had done so), and you even realigned mz_indexes (17088 back to 16705) in that PR for consistency: #36912 (comment). Then #37679 minted new OIDs, and my comment there somehow got missed (#37679 (comment)). This PR now also makes the same mistake.
Please rename in place here (TABLE_MZ_*_CONNECTIONS_OID values 16695/16709/16756/16757), and realign #37679's four mz_*_source_tables OIDs (17107-17110 back to 16948/16949/17057/17025) in this PR or a follow-up. #37679 merged after the 26.36 release cut, so neither PR's new OIDs have shipped in any release and the realignment is invisible to users. (It would be safe even post-release: builtin OIDs are compile-time constants, not part of the fingerprint or durable state, and realigning restores the values every released version has had.)
(Or if there is a reason to deviate from the agreed convention, please say so explicitly and remove the dead consts instead.)
| ) AS d(details) | ||
| WHERE | ||
| r.data->>'kind' = 'Item' AND | ||
| details IS NOT NULL AND |
There was a problem hiding this comment.
nit: details IS NOT NULL reads like it excludes the non-kafka items, but the helper returns jsonb null for those, which passes SQL IS NOT NULL. The connection_type filter does the real work. The predicate is also fully redundant: when the create_sql path is SQL NULL, connection_type is SQL NULL too and the type filter already drops the row. Suggest removing it (or replacing it with a comment), in all three views here. Same pattern as discussed on the #37679 review.
|
Btw., it looks like our review comments on #37679 somehow got missed when that PR merged. Recap so they don't get lost:
|
|
Ok, I've pushed an update. I'm not sure what happened with the other PR. I had definitely made the changes locally and committed them. I thought I had pushed them but I guess I didn't. My mistake. Regarding not following the feedback. My bad, I had stacked this PR on top of the other one, and should have updated it based on the feedback. Sorry 😞 Thanks for taking the time to look. |
aljoscha
left a comment
aljoscha
left a comment
There was a problem hiding this comment.
I did take a peek at this one, but maybe not as thoroughly a Gábor. Looks quite nice! The one thing that made me ponder is the AWS/injection stuff. Looks complicated, but there's a probably a good reason. Why do we need that again?
|
No problem, it happens! |
| CdcV2 => "materialize", | ||
| }) | ||
| } | ||
| None if source_type == "kafka" => Some("none"), |
There was a problem hiding this comment.
This default also fires for new-syntax kafka sources (no FORMAT/ENVELOPE on the source, envelopes defined per-table), which currently report envelope_type NULL — was that flip intended? The history is murky in both directions: until d38735c (2025-09) the packer reported 'none' for them, but only because the planner built a hardcoded placeholder primary_export for a collection that "will not output" anything (the code there said so, with a TODO to remove the field). Since that commit the packer returned NULL (DataSourceDesc::Ingestion { .. } => None), and the current view preserves that, so 'none' here changes about a year of released behavior.
NULL seems preferable to us: the new-syntax source's own relation is the progress relation, nothing is ingested into it with any envelope, and envelope_type = 'none' on a source whose tables use ENVELOPE UPSERT would read oddly. The planner's own old-vs-new-syntax discriminator is the progress subsource (plan_create_source builds OldSyntaxIngestion iff progress_subsource is present, and durable old-syntax create_sql always carries EXPOSE PROGRESS AS), so gating on stmt.progress_subsource.is_some() reproduces the current behavior exactly. If you take the gate, catalog_kafka_source_omitted_envelope_defaults_none's statement should gain EXPOSE PROGRESS AS (that's the realistic persisted shape) plus a new-syntax case asserting the field stays absent. Either way, could you update the envelope_type column comment to spell out the chosen behavior for table-based kafka sources?
There was a problem hiding this comment.
Good feedback, I'll gate the 'none' default on stmt.progress_subsource.is_some() so only old-syntax sources get it and new-syntax stays NULL
The reason is the AWS columns came from the environment ConnectionContext which the packer could pull and fill in. But we don't have that with the materialized view anymore. |
3 participants
Converts the four connection-detail builtin tables to materialized views over mz_catalog_raw, so they are derived from each connection's persisted create_sql instead of being written by a Rust packer on every catalog change on the leader environmentd (the SQL-118 / multi-envd motivation).
Do this in two parts:
RBAC note. The three new fold functions are deliberately not gated by restrict_to_user_objects, exactly like mz_environment_id.
Testing: