Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
_STABLE_BUILTINS: dict[str, MzVersion | None] = {
"mz_array_types": None,
"mz_audit_events": MzVersion.parse_mz("v26.33.0"),
"mz_aws_privatelink_connections": None,
"mz_aws_privatelink_connections": MzVersion.parse_mz("v26.37.0"),
"mz_base_types": None,
"mz_cluster_replica_sizes": None,
"mz_cluster_replicas": MzVersion.parse_mz("v26.30.0"),
Expand All @@ -45,17 +45,17 @@
"mz_default_privileges": MzVersion.parse_mz("v26.30.0"),
"mz_egress_ips": None,
"mz_functions": None,
"mz_iceberg_sinks": None,
"mz_iceberg_sinks": MzVersion.parse_mz("v26.38.0"),
"mz_index_columns": None,
"mz_kafka_connections": None,
"mz_kafka_sinks": None,
"mz_kafka_connections": MzVersion.parse_mz("v26.37.0"),
"mz_kafka_sinks": MzVersion.parse_mz("v26.38.0"),
"mz_kafka_sources": MzVersion.parse_mz("v26.34.0"),
"mz_list_types": None,
"mz_map_types": None,
"mz_operators": None,
"mz_pseudo_types": None,
"mz_sinks": None,
"mz_ssh_tunnel_connections": None,
"mz_sinks": MzVersion.parse_mz("v26.38.0"),
"mz_ssh_tunnel_connections": MzVersion.parse_mz("v26.37.0"),
"mz_system_privileges": MzVersion.parse_mz("v26.30.0"),
"mz_tables": None,
"mz_types": None,
Expand Down
100 changes: 9 additions & 91 deletions src/adapter/src/catalog/builtin_table_updates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,16 @@ use mz_catalog::SYSTEM_CONN_ID;
use mz_catalog::builtin::{
BuiltinTable, MZ_AGGREGATES, MZ_ARRAY_TYPES, MZ_BASE_TYPES, MZ_CLUSTER_REPLICA_SIZE_INTERNAL,
MZ_CLUSTER_REPLICA_SIZES, MZ_COLUMNS, MZ_EGRESS_IPS, MZ_FUNCTIONS,
MZ_HISTORY_RETENTION_STRATEGIES, MZ_ICEBERG_SINKS, MZ_INDEX_COLUMNS, MZ_KAFKA_SINKS,
MZ_LICENSE_KEYS, MZ_LIST_TYPES, MZ_MAP_TYPES, MZ_MATERIALIZED_VIEW_REFRESH_STRATEGIES,
MZ_OBJECT_DEPENDENCIES, MZ_OBJECT_GLOBAL_IDS, MZ_OPERATORS, MZ_PSEUDO_TYPES, MZ_REPLACEMENTS,
MZ_ROLE_AUTH, MZ_SESSIONS, MZ_SINKS, MZ_SOURCE_REFERENCES, MZ_STORAGE_USAGE_BY_SHARD,
MZ_SUBSCRIPTIONS, MZ_TABLES, MZ_TYPE_PG_METADATA, MZ_TYPES, MZ_VIEWS, MZ_WEBHOOKS_SOURCES,
MZ_HISTORY_RETENTION_STRATEGIES, MZ_INDEX_COLUMNS, MZ_LICENSE_KEYS, MZ_LIST_TYPES,
MZ_MAP_TYPES, MZ_MATERIALIZED_VIEW_REFRESH_STRATEGIES, MZ_OBJECT_DEPENDENCIES,
MZ_OBJECT_GLOBAL_IDS, MZ_OPERATORS, MZ_PSEUDO_TYPES, MZ_REPLACEMENTS, MZ_ROLE_AUTH,
MZ_SESSIONS, MZ_SOURCE_REFERENCES, MZ_STORAGE_USAGE_BY_SHARD, MZ_SUBSCRIPTIONS, MZ_TABLES,
MZ_TYPE_PG_METADATA, MZ_TYPES, MZ_VIEWS, MZ_WEBHOOKS_SOURCES,
};
use mz_catalog::durable::SourceReferences;
use mz_catalog::memory::error::Error;
use mz_catalog::memory::objects::{
CatalogEntry, CatalogItem, DataSourceDesc, Func, Index, MaterializedView, Sink, Table,
CatalogEntry, CatalogItem, DataSourceDesc, Func, Index, MaterializedView, Table,
TableDataSource, Type, View,
};
use mz_expr::MirScalarExpr;
Expand All @@ -50,7 +50,6 @@ use mz_sql::func::FuncImplCatalogDetails;
use mz_sql::names::SchemaSpecifier;
use mz_sql_parser::ast::display::AstDisplay;
use mz_storage_client::client::TableData;
use mz_storage_types::sinks::{IcebergSinkConnection, KafkaSinkConnection, StorageSinkConnection};
use smallvec::smallvec;

// DO NOT add any more imports from `crate` outside of `crate::catalog`.
Expand Down Expand Up @@ -192,9 +191,9 @@ impl CatalogState {
CatalogItem::MaterializedView(mview) => {
self.pack_materialized_view_update(id, mview, diff)
}
CatalogItem::Sink(sink) => {
self.pack_sink_update(id, oid, schema_id, name, owner_id, sink, diff)
}
// mz_sinks, mz_kafka_sinks and mz_iceberg_sinks read create_sql
// out of mz_catalog_raw, so there is nothing to pack here.
CatalogItem::Sink(_) => vec![],
CatalogItem::Type(ty) => {
self.pack_type_update(id, oid, schema_id, name, owner_id, privileges, ty, diff)
}
Expand Down Expand Up @@ -514,87 +513,6 @@ impl CatalogState {
updates
}

fn pack_sink_update(
&self,
id: CatalogItemId,
oid: u32,
schema_id: &SchemaSpecifier,
name: &str,
owner_id: &RoleId,
sink: &Sink,
diff: Diff,
) -> Vec<BuiltinTableUpdate<&'static BuiltinTable>> {
let mut updates = vec![];
match &sink.connection {
StorageSinkConnection::Kafka(KafkaSinkConnection {
topic: topic_name, ..
}) => {
updates.push(BuiltinTableUpdate::row(
&*MZ_KAFKA_SINKS,
Row::pack_slice(&[
Datum::String(&id.to_string()),
Datum::String(topic_name.as_str()),
]),
diff,
));
}
StorageSinkConnection::Iceberg(IcebergSinkConnection {
namespace, table, ..
}) => {
updates.push(BuiltinTableUpdate::row(
&*MZ_ICEBERG_SINKS,
Row::pack_slice(&[
Datum::String(&id.to_string()),
Datum::String(namespace.as_str()),
Datum::String(table.as_str()),
]),
diff,
));
}
};

let create_stmt = mz_sql::parse::parse(&sink.create_sql)
.unwrap_or_else(|_| panic!("create_sql cannot be invalid: {}", sink.create_sql))
.into_element()
.ast;

let envelope = sink.envelope();

// The combined format string is used for the deprecated `format` column.
let combined_format = sink.combined_format();
let (key_format, value_format) = match sink.formats() {
Some((key_format, value_format)) => (key_format, Some(value_format)),
None => (None, None),
};

updates.push(BuiltinTableUpdate::row(
&*MZ_SINKS,
Row::pack_slice(&[
Datum::String(&id.to_string()),
Datum::UInt32(oid),
Datum::String(&schema_id.to_string()),
Datum::String(name),
Datum::String(sink.connection.name()),
Datum::from(sink.connection_id().map(|id| id.to_string()).as_deref()),
// size column now deprecated w/o linked clusters
Datum::Null,
Datum::from(envelope),
// FIXME: These key/value formats are kinda leaky! Should probably live in
// the kafka sink table.
Datum::from(combined_format.as_ref().map(|f| f.as_ref())),
Datum::from(key_format),
Datum::from(value_format),
Datum::String(&sink.cluster_id.to_string()),
Datum::String(&owner_id.to_string()),
Datum::String(&sink.create_sql),
Datum::String(&create_stmt.to_ast_string_redacted()),
]),
diff,
));

updates
}

fn pack_index_update(
&self,
id: CatalogItemId,
Expand Down
29 changes: 23 additions & 6 deletions src/adapter/src/catalog/open/builtin_schema_migration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,6 @@ static MIGRATIONS: LazyLock<Vec<MigrationStep>> = LazyLock::new(|| {
MZ_INTERNAL_SCHEMA,
"mz_cluster_replica_metrics_history",
),
MigrationStep::replacement(
"0.160.0",
CatalogItemType::Table,
MZ_CATALOG_SCHEMA,
"mz_sinks",
),
MigrationStep::replacement(
"26.18.0-dev.0",
CatalogItemType::MaterializedView,
Expand Down Expand Up @@ -355,6 +349,29 @@ static MIGRATIONS: LazyLock<Vec<MigrationStep>> = LazyLock::new(|| {
MZ_CATALOG_SCHEMA,
"mz_aws_privatelink_connections",
),
// The three sink tables became materialized views, which moves their
// fingerprints. The old `0.160.0` `mz_sinks` step had to go at the same
// time, because it names the `Table` description and that no longer
// 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.38.0-dev.0",
CatalogItemType::MaterializedView,
MZ_CATALOG_SCHEMA,
"mz_sinks",
),
MigrationStep::replacement(
"26.38.0-dev.0",
CatalogItemType::MaterializedView,
MZ_CATALOG_SCHEMA,
"mz_kafka_sinks",
),
MigrationStep::replacement(
"26.38.0-dev.0",
CatalogItemType::MaterializedView,
MZ_CATALOG_SCHEMA,
"mz_iceberg_sinks",
),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

]
});

Expand Down
15 changes: 15 additions & 0 deletions src/adapter/src/catalog/open/builtin_schema_migration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -402,3 +402,18 @@ fn init_persist(sim: &mut turmoil::Sim) -> PersistLocation {
consensus_uri: "turmoil://consensus:7000".parse().unwrap(),
}
}

/// A step naming a builtin that no longer exists, or that changed
/// `CatalogItemType`, panics `validate_migration_steps` at catalog open. That
/// needs a catalog already on disk to reproduce, so the default CI suite never
/// sees it and only the upgrade nightly does. Much cheaper to catch here.
#[mz_ore::test]
fn test_migration_steps_resolve_to_builtins() {
for step in MIGRATIONS.iter() {
assert!(
BUILTIN_LOOKUP.contains_key(&step.object),
"migration step for non-existent builtin: {:?}",
step.object
);
}
}
6 changes: 3 additions & 3 deletions src/catalog/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1107,11 +1107,11 @@ pub static BUILTINS_STATIC: LazyLock<Vec<Builtin<NameReference>>> = LazyLock::ne
Builtin::Log(&MZ_COMPUTE_ERROR_COUNTS_RAW),
Builtin::Log(&MZ_COMPUTE_HYDRATION_TIMES_PER_WORKER),
Builtin::Log(&MZ_COMPUTE_OPERATOR_HYDRATION_STATUSES_PER_WORKER),
Builtin::Table(&MZ_KAFKA_SINKS),
Builtin::MaterializedView(&MZ_KAFKA_SINKS),
Builtin::MaterializedView(&MZ_KAFKA_CONNECTIONS),
Builtin::MaterializedView(&MZ_KAFKA_SOURCES),
Builtin::Table(&MZ_OBJECT_DEPENDENCIES),
Builtin::Table(&MZ_ICEBERG_SINKS),
Builtin::MaterializedView(&MZ_ICEBERG_SINKS),
Builtin::MaterializedView(&MZ_DATABASES),
Builtin::MaterializedView(&MZ_SCHEMAS),
Builtin::Table(&MZ_COLUMNS),
Expand All @@ -1125,7 +1125,7 @@ pub static BUILTINS_STATIC: LazyLock<Vec<Builtin<NameReference>>> = LazyLock::ne
Builtin::MaterializedView(&MZ_MYSQL_SOURCE_TABLES),
Builtin::MaterializedView(&MZ_SQL_SERVER_SOURCE_TABLES),
Builtin::MaterializedView(&MZ_KAFKA_SOURCE_TABLES),
Builtin::Table(&MZ_SINKS),
Builtin::MaterializedView(&MZ_SINKS),
Builtin::Table(&MZ_VIEWS),
Builtin::Table(&MZ_TYPES),
Builtin::Table(&MZ_TYPE_PG_METADATA),
Expand Down
Loading
Loading