diff --git a/src/adapter/src/catalog/state.rs b/src/adapter/src/catalog/state.rs index 8fa4baaad0e34..b2b0d04b88c24 100644 --- a/src/adapter/src/catalog/state.rs +++ b/src/adapter/src/catalog/state.rs @@ -1121,7 +1121,24 @@ impl CatalogState { let mut uncached_expr = None; - let item = match plan { + // Carry over the plans (`optimized_plan`, `physical_plan`, + // `dataflow_metainfo`) from the previous incarnation of this item when + // re-parsing an existing item (e.g. after a RENAME). These fields live + // on the `CatalogItem` since #35834, but are not reconstructable from + // `create_sql` alone — they are populated by the sequencer `_finish` + // paths at create time, and by the expression-cache / bootstrap + // rendering path on boot. If we don't preserve them here, a RENAME + // silently drops the plans and dataflow metainfo for the affected + // MV/Index/CT. + let previous_plans = previous_item.as_ref().map(|item| { + ( + item.optimized_plan().cloned(), + item.physical_plan().cloned(), + item.dataflow_metainfo().cloned(), + ) + }); + + let mut item = match plan { Plan::CreateTable(CreateTablePlan { table, .. }) => { let collections = extra_versions .iter() @@ -1508,6 +1525,18 @@ impl CatalogState { } }; + // Carry over the plans (`optimized_plan`, `physical_plan`, + // `dataflow_metainfo`) from the previous incarnation of this item, if + // any. See the comment on `previous_plans` above. + if let Some((prev_optimized, prev_physical, prev_metainfo)) = previous_plans { + if let Some((optimized_plan, physical_plan, dataflow_metainfo)) = item.plan_fields_mut() + { + *optimized_plan = prev_optimized; + *physical_plan = prev_physical; + *dataflow_metainfo = prev_metainfo; + } + } + Ok((item, uncached_expr)) } diff --git a/src/catalog/src/memory/objects.rs b/src/catalog/src/memory/objects.rs index 0a9b612ac876c..42265d098a50b 100644 --- a/src/catalog/src/memory/objects.rs +++ b/src/catalog/src/memory/objects.rs @@ -1848,6 +1848,37 @@ impl CatalogItem { } } + /// Returns mutable references to the plan fields (`optimized_plan`, + /// `physical_plan`, `dataflow_metainfo`) on plan-bearing items + /// (`Index`, `MaterializedView`, `ContinualTask`), or `None` for + /// other item kinds. + pub fn plan_fields_mut( + &mut self, + ) -> Option<( + &mut Option>>, + &mut Option>>, + &mut Option>>, + )> { + match self { + CatalogItem::Index(idx) => Some(( + &mut idx.optimized_plan, + &mut idx.physical_plan, + &mut idx.dataflow_metainfo, + )), + CatalogItem::MaterializedView(mv) => Some(( + &mut mv.optimized_plan, + &mut mv.physical_plan, + &mut mv.dataflow_metainfo, + )), + CatalogItem::ContinualTask(ct) => Some(( + &mut ct.optimized_plan, + &mut ct.physical_plan, + &mut ct.dataflow_metainfo, + )), + _ => None, + } + } + /// Whether this item represents a storage collection. pub fn is_storage_collection(&self) -> bool { match self { diff --git a/test/sqllogictest/alter.slt b/test/sqllogictest/alter.slt index fcf4bc78a1563..0aa7e75f6d954 100644 --- a/test/sqllogictest/alter.slt +++ b/test/sqllogictest/alter.slt @@ -228,3 +228,58 @@ query T SELECT create_sql FROM (SHOW CREATE SOURCE s) ---- CREATE SOURCE materialize.public.s IN CLUSTER quickstart FROM LOAD GENERATOR COUNTER; + +# Regression tests: ALTER MATERIALIZED VIEW SET CLUSTER and +# ALTER MATERIALIZED VIEW / INDEX SET (RETAIN HISTORY ...) retract+add +# the catalog item via `apply_item_update`. `parse_item_inner` must carry +# the optimized_plan / physical_plan / dataflow_metainfo fields forward +# from the previous catalog entry, so that EXPLAIN can still show them. +# See also the analogous RENAME test in rename.slt. + +statement ok +CREATE TABLE t_alter_plans (a int) + +statement ok +CREATE MATERIALIZED VIEW mv_alter_plans AS SELECT a + 1 AS b FROM t_alter_plans + +statement ok +CREATE DEFAULT INDEX idx_alter_plans ON mv_alter_plans + +statement ok +ALTER MATERIALIZED VIEW mv_alter_plans SET CLUSTER quickstart + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_alter_plans + +statement ok +EXPLAIN PHYSICAL PLAN FOR MATERIALIZED VIEW mv_alter_plans + +statement ok +ALTER MATERIALIZED VIEW mv_alter_plans SET (RETAIN HISTORY FOR '1m') + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_alter_plans + +statement ok +EXPLAIN PHYSICAL PLAN FOR MATERIALIZED VIEW mv_alter_plans + +statement ok +ALTER MATERIALIZED VIEW mv_alter_plans RESET (RETAIN HISTORY) + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_alter_plans + +statement ok +ALTER INDEX idx_alter_plans SET (RETAIN HISTORY FOR '1m') + +statement ok +EXPLAIN OPTIMIZED PLAN FOR INDEX idx_alter_plans + +statement ok +EXPLAIN PHYSICAL PLAN FOR INDEX idx_alter_plans + +statement ok +ALTER INDEX idx_alter_plans RESET (RETAIN HISTORY) + +statement ok +EXPLAIN OPTIMIZED PLAN FOR INDEX idx_alter_plans diff --git a/test/sqllogictest/privilege_checks.slt b/test/sqllogictest/privilege_checks.slt index 40025daef1c8e..3246d0ee5f605 100644 --- a/test/sqllogictest/privilege_checks.slt +++ b/test/sqllogictest/privilege_checks.slt @@ -3609,8 +3609,52 @@ REVOKE USAGE ON SCHEMA mz_temp FROM PUBLIC db error: ERROR: cannot grant or revoke privileges on schema mz_temp because it is a temporary schema # Disable rbac checks. - simple conn=mz_system,user=mz_system ALTER SYSTEM SET enable_rbac_checks TO false; ---- COMPLETE 0 + +# Regression tests: ALTER ... OWNER TO ... and GRANT/REVOKE retract + re-add +# the affected catalog item via `apply_item_update`. Plans on MV / INDEX +# items must survive, so that EXPLAIN can still show them. +statement ok +CREATE ROLE plan_owner_other + +statement ok +CREATE TABLE t_owner_plans (a int) + +statement ok +CREATE MATERIALIZED VIEW mv_owner_plans AS SELECT a + 1 AS b FROM t_owner_plans + +statement ok +CREATE DEFAULT INDEX idx_owner_plans ON mv_owner_plans + +statement ok +ALTER MATERIALIZED VIEW mv_owner_plans OWNER TO plan_owner_other + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_owner_plans + +statement ok +EXPLAIN PHYSICAL PLAN FOR MATERIALIZED VIEW mv_owner_plans + +statement ok +ALTER INDEX idx_owner_plans OWNER TO plan_owner_other + +statement ok +EXPLAIN OPTIMIZED PLAN FOR INDEX idx_owner_plans + +statement ok +EXPLAIN PHYSICAL PLAN FOR INDEX idx_owner_plans + +statement ok +GRANT SELECT ON mv_owner_plans TO plan_owner_other + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_owner_plans + +statement ok +REVOKE SELECT ON mv_owner_plans FROM plan_owner_other + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_owner_plans diff --git a/test/sqllogictest/rename.slt b/test/sqllogictest/rename.slt index 5da99f1e11f49..58fbdd88543d6 100644 --- a/test/sqllogictest/rename.slt +++ b/test/sqllogictest/rename.slt @@ -655,3 +655,66 @@ SHOW CREATE VIEW temp_view; ---- mz_temp.temp_view CREATE TEMPORARY VIEW mz_temp.temp_view AS SELECT * FROM materialize.public_renamed.non_temp_table; + +# Regression test: RENAME on a MATERIALIZED VIEW / INDEX must preserve the +# plans and dataflow metainfo stored on the catalog item. Before the fix, +# `EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW ...` would fail after a +# rename with "cannot find dataflow metainformation for materialized view +# ... in catalog", because `parse_item_inner` hardcoded `None` for those +# fields when reconstructing the item from `create_sql`. + +statement ok +CREATE TABLE t_rename_metainfo (a int) + +statement ok +CREATE MATERIALIZED VIEW mv_rename_metainfo AS SELECT a + 1 AS b FROM t_rename_metainfo + +statement ok +ALTER MATERIALIZED VIEW mv_rename_metainfo RENAME TO mv_rename_metainfo_renamed + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW mv_rename_metainfo_renamed + +statement ok +EXPLAIN PHYSICAL PLAN FOR MATERIALIZED VIEW mv_rename_metainfo_renamed + +# Same, for indexes. + +statement ok +CREATE INDEX idx_rename_metainfo ON t_rename_metainfo (a) + +statement ok +ALTER INDEX idx_rename_metainfo RENAME TO idx_rename_metainfo_renamed + +statement ok +EXPLAIN OPTIMIZED PLAN FOR INDEX idx_rename_metainfo_renamed + +statement ok +EXPLAIN PHYSICAL PLAN FOR INDEX idx_rename_metainfo_renamed + +# Regression test: ALTER SCHEMA ... RENAME TO ... retracts+re-adds every +# item in the schema via `tx.update_items`; plans on MV / INDEX items in +# the schema must survive, so that EXPLAIN can still show them. +statement ok +CREATE SCHEMA s_rename_metainfo + +statement ok +CREATE TABLE s_rename_metainfo.t (a int) + +statement ok +CREATE MATERIALIZED VIEW s_rename_metainfo.mv AS SELECT a + 1 AS b FROM s_rename_metainfo.t + +statement ok +CREATE DEFAULT INDEX idx_in_schema ON s_rename_metainfo.mv + +statement ok +ALTER SCHEMA s_rename_metainfo RENAME TO s_rename_metainfo_renamed + +statement ok +EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW s_rename_metainfo_renamed.mv + +statement ok +EXPLAIN PHYSICAL PLAN FOR MATERIALIZED VIEW s_rename_metainfo_renamed.mv + +statement ok +EXPLAIN OPTIMIZED PLAN FOR INDEX s_rename_metainfo_renamed.idx_in_schema diff --git a/test/sqllogictest/transform/notice/index_too_wide_for_literal_constraints.slt b/test/sqllogictest/transform/notice/index_too_wide_for_literal_constraints.slt index 036b1796022a5..206590f1a3b93 100644 --- a/test/sqllogictest/transform/notice/index_too_wide_for_literal_constraints.slt +++ b/test/sqllogictest/transform/notice/index_too_wide_for_literal_constraints.slt @@ -297,6 +297,54 @@ Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6 Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠5`. Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠7`. Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements +# Rename a materialized view and verify its notices are preserved. +# Regression test for a bug where RENAME dropped dataflow_metainfo (and thus +# the associated mz_optimizer_notices rows) from the catalog item. +statement ok +ALTER MATERIALIZED VIEW mv2 RENAME TO mv2_renamed; + +query TTTTTTTT +SELECT + n.notice_type, n.message, n.redacted_message, n.hint, n.redacted_hint, n.action, n.redacted_action, n.action_type +FROM + mz_internal.mz_notices n JOIN + mz_catalog.mz_materialized_views mv ON(n.object_id = mv.id) +WHERE + mv.name IN ('mv1', 'mv2_renamed'); +---- +Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y␠on␠t6(x,␠y)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠5`. Index␠materialize.notices.t6_idx_x_y␠on␠t6(x,␠y)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements +Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y␠on␠t6(x,␠y)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠7`. Index␠materialize.notices.t6_idx_x_y␠on␠t6(x,␠y)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements +Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠5`. Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements +Index␠too␠wide␠for␠literal␠constraints Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠7`. Index␠materialize.notices.t6_idx_x_y_z␠on␠t6(x,␠y,␠z)␠is␠too␠wide␠to␠use␠for␠literal␠equalities␠`y␠=␠█`. If␠your␠literal␠equalities␠filter␠out␠many␠rows,␠create␠an␠index␠whose␠key␠exactly␠matches␠your␠literal␠equalities:␠(y). NULL CREATE␠INDEX␠ON␠t6(y); NULL sql_statements + +# Also check that EXPLAIN OPTIMIZED PLAN FOR MATERIALIZED VIEW works after +# the rename (this reads the stored optimized_plan + dataflow_metainfo off +# the catalog item, which is what the bug above broke). +query T multiline +EXPLAIN OPTIMIZED PLAN WITH(humanized expressions) AS VERBOSE TEXT FOR MATERIALIZED VIEW mv2_renamed; +---- +materialize.notices.mv2_renamed: + Filter (#1{y} = 7) + ReadIndex on=t6 t6_idx_x_y=[*** full scan ***] + +Used Indexes: + - materialize.notices.t6_idx_x_y (*** full scan ***) + +Target cluster: quickstart + +Notices: + - Notice: Index materialize.notices.t6_idx_x_y on t6(x, y) is too wide to use for literal equalities `y = 7`. + Hint: If your literal equalities filter out many rows, create an index whose key exactly matches your literal equalities: (y). + - Notice: Index materialize.notices.t6_idx_x_y_z on t6(x, y, z) is too wide to use for literal equalities `y = 7`. + Hint: If your literal equalities filter out many rows, create an index whose key exactly matches your literal equalities: (y). + +EOF + +# Rename back, so that later queries in this file (which filter by mv name) +# continue to work unchanged. +statement ok +ALTER MATERIALIZED VIEW mv2_renamed RENAME TO mv2; + # Drop one of the indexes associated with the notices. statement ok DROP INDEX t6_idx_x_y_z;