diff --git a/CHANGELOG b/CHANGELOG index 74f3b96..124e35e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,3 +1,36 @@ +2026-07-28 v1.0.26 + + Bug fixes: + - Resolved issue #235 (materialized view + recreation did not restore its indexes). A + materialized view can be indexed, but the index + rows PostgreSQL reports for one were only ever + distributed into the dump's table list, which + holds ordinary and partitioned tables alone, so + they matched nothing and never reached the + dump. Nothing re-emitted them either: a + materialized view is always dropped and + recreated when its definition changes, and DROP + MATERIALIZED VIEW takes the indexes with it, so + the migration silently left the view unindexed. + Because neither dump carried the indexes the + second diff was empty, and the loss went + unreported. The dump now captures the indexes + of every materialized view alongside the view + itself. A view that is recreated has them + rebuilt with it, and one whose definition did + not change has its index set reconciled in + place (created, dropped, redefined or + re-commented) so that adding an index no longer + forces a full rebuild of the view's contents. + A view restored after DROP FUNCTION ... CASCADE + gets the same treatment, and with + --output-for-production the builds and drops + run CONCURRENTLY after the transaction commits, + exactly as a table's do. Dumps written by older + versions carry no index data; they stay + readable and simply report no indexes. + 2026-07-17 v1.0.25 Bug fixes: diff --git a/app/Cargo.lock b/app/Cargo.lock index fc79602..d86b432 100644 --- a/app/Cargo.lock +++ b/app/Cargo.lock @@ -1146,7 +1146,7 @@ checksum = "e3148f5046208a5d56bcfc03053e3ca6334e51da8dfb19b6cdc8b306fae3283e" [[package]] name = "pgc" -version = "1.0.25" +version = "1.0.26" dependencies = [ "chrono", "clap", diff --git a/app/Cargo.toml b/app/Cargo.toml index d3d8187..07e9aaf 100644 --- a/app/Cargo.toml +++ b/app/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "pgc" -version = "1.0.25" +version = "1.0.26" edition = "2024" license = "MIT" authors = ["nettrash "] diff --git a/app/src/comparer/core.rs b/app/src/comparer/core.rs index 9a660a9..aa408bb 100644 --- a/app/src/comparer/core.rs +++ b/app/src/comparer/core.rs @@ -417,6 +417,44 @@ impl Comparer { } } + /// Emit the index changes of an [`IndexAlterPlan`] in default (non-production) + /// mode: drops first, then comment-only changes, then (re)creates — the same + /// ordering `Table::build_alter_script` uses for a table's own indexes. Used + /// for a materialized view that is not being recreated, whose indexes + /// therefore survive and have to be reconciled in place (issue #235). + fn emit_index_alter_plan(script: &mut String, plan: &IndexAlterPlan, use_drop: bool) { + for old_index in &plan.drop { + let drop_cmd = format!( + "drop index if exists {}.{};", + old_index.schema, old_index.name + ) + .with_empty_lines(); + if use_drop { + script.push_str(&drop_cmd); + } else { + script.push_str(&format!("-- {drop_cmd}")); + } + } + for index in &plan.comment_changes { + if let Some(comment) = &index.comment { + script.append_block(&format!( + "comment on index {}.{} is '{}';", + index.schema, + index.name, + comment.replace('\'', "''") + )); + } else { + script.append_block(&format!( + "comment on index {}.{} is null;", + index.schema, index.name + )); + } + } + for index in &plan.create { + script.push_str(&index.get_script()); + } + } + /// Emit the index changes of an ALTER for production: drops (concurrent /// unless on a partitioned table), comment-only changes (in-txn), then /// (re)creates (concurrent / partition-aware). Mirrors the ordering of @@ -3220,7 +3258,11 @@ impl Comparer { } /// Emit the CREATE (or CREATE OR REPLACE) script for a single target view. - fn emit_view_create(&mut self, to_view: &View) { + /// + /// `prod_ctx` is `Some` only in production mode, where a materialized view's + /// indexes are split out of the CREATE and built concurrently after the + /// transaction commits, exactly as a new table's are. + fn emit_view_create(&mut self, to_view: &View, prod_ctx: Option<&PartitionContext>) { self.script .push_str(format!("/* View: {}.{}*/\n", to_view.schema, to_view.name).as_str()); @@ -3233,6 +3275,8 @@ impl Comparer { .unwrap_or(true); if !drop_was_active { // DROP was commented out, so CREATE would fail; comment it out too. + // The indexes ride along: without the view they have nothing to + // build on. let create_script = to_view.get_script(); self.script.push_str(&format!( "-- use_drop=false: materialized view {}.{} requires drop+recreate; create commented out (manual intervention needed)\n", @@ -3244,6 +3288,13 @@ impl Comparer { .map(|l| format!("-- {}\n", l)) .collect::(), ); + } else if let Some(ctx) = prod_ctx { + self.script.push_str(&to_view.get_script_without_indexes()); + for index in &to_view.indexes { + let split = production::index_create_split(index, ctx, true); + self.script.push_str(&split.in_txn); + self.production_post_script.push_str(&split.post_commit); + } } else { self.script.push_str(&to_view.get_script()); } @@ -4270,10 +4321,33 @@ impl Comparer { // ────────────────────────────────────────────────────────── // Phase 5 – Emit creates / updates in dependency order // ────────────────────────────────────────────────────────── + // A materialized view's indexes are emitted with it (Phase 5) or + // reconciled in place (Phase 6); production mode routes both through the + // concurrent-build split, which needs the partition topology. A + // materialized view is never a partitioned parent, so the maps only ever + // steer these calls down the plain-relation branch — they are built here + // for the same reason the table path builds them: `index_create_split` + // and `index_drop_statement` take the context unconditionally. + let view_partition_maps = if self.output_for_production { + Some(self.build_partition_context_maps()) + } else { + None + }; + let view_prod_ctx = + view_partition_maps + .as_ref() + .map( + |(parents, children, partitioned_indexes)| PartitionContext { + partitioned_parents: parents, + children, + partitioned_indexes, + }, + ); + for &(is_view, orig_idx) in &emit_order { if is_view { let view = self.to.views[orig_idx].clone(); - self.emit_view_create(&view); + self.emit_view_create(&view, view_prod_ctx.as_ref()); } else { // Resolve the matching FROM-side routine by index so // `emit_routine_diff` can borrow the `Routine` out of @@ -4296,18 +4370,44 @@ impl Comparer { } // ────────────────────────────────────────────────────────── - // Phase 6 – Unchanged views: emit owner changes only + // Phase 6 – Unchanged views: owner changes and index diffs // ────────────────────────────────────────────────────────── + // A materialized view that is not being recreated keeps its indexes, so + // added / removed / redefined ones have to be reconciled in place — + // index changes deliberately stay out of `View::hash` so that adding an + // index does not force a full rebuild of the view's contents (#235). for &idx in &no_action_view_indices { let to_view = &self.to.views[idx]; - if let Some(&fidx) = from_view_map.get(&(to_view.schema.clone(), to_view.name.clone())) - { - let fv = &self.from.views[fidx]; - if fv.owner != to_view.owner { - self.script.push_str( - format!("/* View: {}.{}*/\n", to_view.schema, to_view.name).as_str(), + let Some(&fidx) = from_view_map.get(&(to_view.schema.clone(), to_view.name.clone())) + else { + continue; + }; + let from_view = &self.from.views[fidx]; + let owner_changed = from_view.owner != to_view.owner; + let plan = from_view.index_alter_plan(to_view); + let has_index_changes = !plan.create.is_empty() + || !plan.drop.is_empty() + || !plan.comment_changes.is_empty(); + if !owner_changed && !has_index_changes { + continue; + } + + self.script + .push_str(format!("/* View: {}.{}*/\n", to_view.schema, to_view.name).as_str()); + if owner_changed { + self.script.push_str(&to_view.get_owner_script()); + } + if has_index_changes { + if let Some(ctx) = view_prod_ctx.as_ref() { + Self::emit_index_alter_plan_prod( + &mut self.script, + &mut self.production_post_script, + &plan, + self.use_drop, + ctx, ); - self.script.push_str(&to_view.get_owner_script()); + } else { + Self::emit_index_alter_plan(&mut self.script, &plan, self.use_drop); } } } @@ -5828,12 +5928,19 @@ fn inject_if_not_exists_into_add_column(script: &str) -> String { /// that PostgreSQL never actually dropped, and an unconditional CREATE /// would fail against a surviving view. fn view_recreate_block(view: &View) -> String { - let script = view.get_script(); - if view.is_materialized { - inject_if_not_exists_into_create_materialized_view(&script) - } else { - rewrite_create_view_to_create_or_replace(&script) + if !view.is_materialized { + return rewrite_create_view_to_create_or_replace(&view.get_script()); + } + // The view's indexes went with it if CASCADE really did drop it, so they are + // recreated too — each guarded the same way and for the same reason as the + // view itself, since an unconditional CREATE INDEX would fail against the + // indexes of a view PostgreSQL never actually dropped. + let mut block = + inject_if_not_exists_into_create_materialized_view(&view.get_script_without_indexes()); + for index in &view.indexes { + block.push_str(&inject_if_not_exists_into_create_index(&index.get_script())); } + block } /// Rewrites the lowercase `create view ` prefix produced by diff --git a/app/src/comparer/core_tests.rs b/app/src/comparer/core_tests.rs index e192bc6..ae8a242 100644 --- a/app/src/comparer/core_tests.rs +++ b/app/src/comparer/core_tests.rs @@ -11466,3 +11466,307 @@ async fn dependent_view_is_dropped_before_incompatible_base_view() { "dependent must drop before the view it reads:\n{script}" ); } + +// ── Issue #235: indexes on a materialized view ────────────────────────────── + +fn mv235_index(name: &str, indexdef: &str) -> TableIndex { + TableIndex { + schema: "test_schema".to_string(), + table: "mv".to_string(), + name: name.to_string(), + catalog: None, + indexdef: indexdef.to_string(), + is_partition_index: false, + comment: None, + } +} + +fn mv235_view(definition: &str, indexes: Vec) -> View { + let mut view = View::new( + "mv".to_string(), + definition.to_string(), + "test_schema".to_string(), + vec!["test_schema.base".to_string()], + ); + view.is_materialized = true; + view.indexes = indexes; + view.hash(); + view +} + +const MV235_IX_VAL: &str = "CREATE INDEX ix_val ON test_schema.mv USING btree (val)"; +const MV235_IX_ID: &str = "CREATE UNIQUE INDEX ix_id ON test_schema.mv USING btree (id)"; + +#[tokio::test] +async fn issue235_recreated_matview_restores_its_indexes() { + // The definition changes, so the view is dropped and rebuilt. DROP + // MATERIALIZED VIEW takes the indexes with it and nothing put them back: + // the migration silently left the view unindexed, and because neither dump + // carried the indexes the round-2 diff was empty and never reported it. + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + from_dump.views.push(mv235_view( + "SELECT id, val FROM test_schema.base;", + vec![ + mv235_index("ix_id", MV235_IX_ID), + mv235_index("ix_val", MV235_IX_VAL), + ], + )); + to_dump.views.push(mv235_view( + "SELECT id, val, num FROM test_schema.base;", + vec![ + mv235_index("ix_id", MV235_IX_ID), + mv235_index("ix_val", MV235_IX_VAL), + ], + )); + + let mut comparer = Comparer::new(from_dump, to_dump, true, false, true, GrantsMode::Ignore); + comparer.drop_views().await.unwrap(); + comparer.compare_routines_and_views().await.unwrap(); + let script = comparer.get_script(); + + let drop_pos = script + .find("drop materialized view if exists test_schema.mv;") + .expect("changed materialized view must be dropped"); + let create_pos = script + .find("create materialized view test_schema.mv as") + .expect("changed materialized view must be recreated"); + let id_pos = script + .find("CREATE UNIQUE INDEX ix_id ON test_schema.mv USING btree (id);") + .expect("the unique index must be recreated with the view"); + let val_pos = script + .find("CREATE INDEX ix_val ON test_schema.mv USING btree (val);") + .expect("the plain index must be recreated with the view"); + + assert!(drop_pos < create_pos, "drop must precede create:\n{script}"); + assert!( + create_pos < id_pos && create_pos < val_pos, + "indexes must be built after the view exists:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_unchanged_matview_reconciles_indexes_in_place() { + // Same definition on both sides, so the view survives; only the index set + // moves. It must not be dropped and rebuilt just to change an index. + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let definition = "SELECT id, val FROM test_schema.base;"; + let mut recommented = mv235_index("ix_cmt", "CREATE INDEX ix_cmt ON test_schema.mv (num)"); + recommented.comment = Some("after".to_string()); + + from_dump.views.push(mv235_view( + definition, + vec![ + mv235_index("ix_cmt", "CREATE INDEX ix_cmt ON test_schema.mv (num)"), + mv235_index("ix_val", MV235_IX_VAL), + ], + )); + to_dump.views.push(mv235_view( + definition, + vec![ + mv235_index("ix_cmt", "CREATE INDEX ix_cmt ON test_schema.mv (num)"), + mv235_index("ix_id", MV235_IX_ID), + recommented, + ], + )); + + let mut comparer = Comparer::new(from_dump, to_dump, true, false, true, GrantsMode::Ignore); + comparer.drop_views().await.unwrap(); + comparer.compare_routines_and_views().await.unwrap(); + let script = comparer.get_script(); + + assert!( + !script.to_lowercase().contains("drop materialized view"), + "an index change must not rebuild the view's contents:\n{script}" + ); + assert!( + !script.to_lowercase().contains("create materialized view"), + "an index change must not rebuild the view's contents:\n{script}" + ); + assert!( + script.contains("drop index if exists test_schema.ix_val;"), + "the removed index must be dropped:\n{script}" + ); + assert!( + script.contains("CREATE UNIQUE INDEX ix_id ON test_schema.mv USING btree (id);"), + "the added index must be created:\n{script}" + ); + assert!( + script.contains("comment on index test_schema.ix_cmt is 'after';"), + "a comment-only change must not touch the index itself:\n{script}" + ); + assert!( + !script.contains("drop index if exists test_schema.ix_cmt;"), + "a comment-only change must not drop the index:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_matview_with_identical_indexes_emits_nothing() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let definition = "SELECT id, val FROM test_schema.base;"; + from_dump.views.push(mv235_view( + definition, + vec![mv235_index("ix_val", MV235_IX_VAL)], + )); + to_dump.views.push(mv235_view( + definition, + vec![mv235_index("ix_val", MV235_IX_VAL)], + )); + + let mut comparer = Comparer::new(from_dump, to_dump, true, false, false, GrantsMode::Ignore); + comparer.drop_views().await.unwrap(); + comparer.compare_routines_and_views().await.unwrap(); + let script = comparer.get_script(); + + assert!( + script.trim().is_empty(), + "an unchanged view with unchanged indexes must produce no SQL:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_matview_index_drop_is_commented_out_when_use_drop_is_false() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let definition = "SELECT id, val FROM test_schema.base;"; + from_dump.views.push(mv235_view( + definition, + vec![mv235_index("ix_val", MV235_IX_VAL)], + )); + to_dump.views.push(mv235_view(definition, Vec::new())); + + let mut comparer = Comparer::new(from_dump, to_dump, false, false, true, GrantsMode::Ignore); + comparer.drop_views().await.unwrap(); + comparer.compare_routines_and_views().await.unwrap(); + let script = comparer.get_script(); + + assert!( + script.contains("-- drop index if exists test_schema.ix_val;"), + "use_drop=false must comment the index drop out, as it does for a table:\n{script}" + ); + assert!( + !script + .lines() + .any(|l| !l.trim_start().starts_with("--") && l.contains("drop index")), + "no active drop may survive use_drop=false:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_production_mode_builds_matview_indexes_concurrently_after_commit() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + from_dump.views.push(mv235_view( + "SELECT id, val FROM test_schema.base;", + vec![mv235_index( + "ix_stale", + "CREATE INDEX ix_stale ON test_schema.mv (num)", + )], + )); + to_dump.views.push(mv235_view( + "SELECT id, val, num FROM test_schema.base;", + vec![mv235_index("ix_id", MV235_IX_ID)], + )); + + let mut comparer = Comparer::new(from_dump, to_dump, true, true, true, GrantsMode::Ignore); + comparer.set_output_for_production(true); + comparer.compare().await.unwrap(); + let script = comparer.get_script(); + + let commit_pos = script.find("commit;").expect("script must contain commit;"); + let create_pos = script + .find("create materialized view if not exists test_schema.mv as") + .expect("the view itself is still built inside the transaction"); + let concurrent_pos = script + .find("CREATE UNIQUE INDEX CONCURRENTLY IF NOT EXISTS ix_id ON test_schema.mv USING btree (id);") + .expect("a materialized view's index must be built concurrently in production mode"); + + assert!(create_pos < commit_pos, "view create is in-txn:\n{script}"); + assert!( + concurrent_pos > commit_pos, + "CREATE INDEX CONCURRENTLY cannot run inside a transaction block:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_production_mode_drops_matview_indexes_concurrently() { + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + let definition = "SELECT id, val FROM test_schema.base;"; + from_dump.views.push(mv235_view( + definition, + vec![mv235_index("ix_val", MV235_IX_VAL)], + )); + to_dump.views.push(mv235_view(definition, Vec::new())); + + let mut comparer = Comparer::new(from_dump, to_dump, true, true, true, GrantsMode::Ignore); + comparer.set_output_for_production(true); + comparer.compare().await.unwrap(); + let script = comparer.get_script(); + + let commit_pos = script.find("commit;").expect("script must contain commit;"); + let drop_pos = script + .find("drop index concurrently if exists test_schema.ix_val;") + .expect("an in-place index drop must be concurrent in production mode"); + assert!( + drop_pos > commit_pos, + "DROP INDEX CONCURRENTLY cannot run inside a transaction block:\n{script}" + ); +} + +#[tokio::test] +async fn issue235_cascade_recreated_matview_guards_its_indexes() { + // Phase 7 restores a materialized view that DROP FUNCTION ... CASCADE may + // have taken out. The match is textual and can false-positive on a view + // PostgreSQL never dropped, so the recreate is guarded — and the indexes + // that would have gone with it need the same guard, or an unconditional + // CREATE INDEX fails against the surviving one. + let mut from_dump = Dump::new(DumpConfig::default()); + let mut to_dump = Dump::new(DumpConfig::default()); + + from_dump + .routines + .push(issue179_compute_routine("integer", "SELECT x * 2;")); + to_dump.routines.push(issue179_compute_routine( + "bigint", + "SELECT (x * 2)::bigint;", + )); + + let mut view = issue189_view("mv_things", true); + let index = TableIndex { + schema: "test_deps".to_string(), + table: "mv_things".to_string(), + name: "ix_mv_things".to_string(), + catalog: None, + indexdef: "CREATE INDEX ix_mv_things ON test_deps.mv_things USING btree (c)".to_string(), + is_partition_index: false, + comment: None, + }; + view.indexes = vec![index]; + from_dump.views.push(view.clone()); + to_dump.views.push(view); + + let mut comparer = Comparer::new(from_dump, to_dump, true, false, true, GrantsMode::Ignore); + comparer.compare_routines_and_views().await.unwrap(); + let script = comparer.get_script(); + + assert!( + script.contains("create materialized view if not exists test_deps.mv_things as"), + "the cascade recreate must stay guarded:\n{script}" + ); + assert!( + script.contains( + "CREATE INDEX IF NOT EXISTS ix_mv_things ON test_deps.mv_things USING btree (c);" + ), + "the recreated view's indexes must be guarded the same way:\n{script}" + ); +} diff --git a/app/src/dump/core.rs b/app/src/dump/core.rs index c2482ea..b78476e 100644 --- a/app/src/dump/core.rs +++ b/app/src/dump/core.rs @@ -15,6 +15,7 @@ use crate::dump::schema::Schema; use crate::dump::sequence::Sequence; use crate::dump::statistic::Statistic; use crate::dump::table::{PgCatalogCaps, Table}; +use crate::dump::table_index::TableIndex; use crate::dump::text_search::{TextSearchConfig, TextSearchDict}; use crate::dump::view::View; use crate::{config::dump_config::DumpConfig, dump::extension::Extension}; @@ -1544,6 +1545,37 @@ impl Dump { }); } + // Indexes per materialized view (fetched sequentially for the same + // pool-budget reason as the view columns above). Keyed by the *raw* + // catalog schema/name because that is what `build_materialized_views_query` + // stores on the view itself. + let matview_indexes_query = Self::build_materialized_view_indexes_query(schema_filter); + let matview_index_rows = sqlx::query(matview_indexes_query.as_str()) + .fetch_all(pool) + .await + .map_err(|e| { + Error::other(format!("Failed to fetch materialized view indexes: {e}.")) + })?; + let mut matview_indexes_map: HashMap<(String, String), Vec> = HashMap::new(); + for row in &matview_index_rows { + let raw_schema: String = row.get("raw_schemaname"); + let raw_name: String = row.get("raw_matviewname"); + matview_indexes_map + .entry((raw_schema, raw_name)) + .or_default() + .push(TableIndex { + schema: row.get("schemaname"), + table: row.get("matviewname"), + name: row.get("indexname"), + catalog: row.get("tablespace"), + indexdef: row.get("indexdef"), + // A materialized view cannot be partitioned, so none of its + // indexes is inherited from a partitioned parent. + is_partition_index: false, + comment: row.get("index_comment"), + }); + } + let mut views = Vec::new(); if regular_rows.is_empty() { @@ -1595,6 +1627,8 @@ impl Dump { storage_parameters: None, tablespace: None, columns, + // Only materialized views can be indexed. + indexes: Vec::new(), }; view.hash(); println!( @@ -1617,6 +1651,10 @@ impl Dump { let column_comments = col_comments_map .remove(&(schema.clone(), name.clone())) .unwrap_or_default(); + let mut indexes = matview_indexes_map + .remove(&(schema.clone(), name.clone())) + .unwrap_or_default(); + indexes.sort_by_key(|i| i.name.to_lowercase()); let storage_opts: Option> = row.get("storage_options"); let storage_parameters = storage_opts.and_then(|v| { // Filter out security_invoker from reloptions (it's handled separately) @@ -1660,6 +1698,7 @@ impl Dump { // Materialized views never use CREATE OR REPLACE, so the // OR-REPLACE compatibility column list is not collected for them. columns: Vec::new(), + indexes, }; view.hash(); println!( @@ -1793,6 +1832,51 @@ impl Dump { ) } + /// Indexes defined on materialized views. + /// + /// `Table::build_indexes_bulk_query` also sees these rows (`pg_indexes` + /// spans relkind `r`/`m`/`p`) but distributes them by `(schema, table)` into + /// the dump's table list, which only holds relkind `r`/`p` — so a + /// materialized view's indexes matched nothing and were silently discarded, + /// and a recreated view came back without them (issue #235). They are + /// captured here instead, alongside the view they belong to. + /// + /// Unlike a table, a materialized view can carry no constraints at all, so + /// there is no primary-key/unique-constraint-backed index to filter out: + /// every index on one is a standalone `CREATE [UNIQUE] INDEX`. + fn build_materialized_view_indexes_query(schema_filter: &str) -> String { + format!( + "select + quote_ident(n.nspname) as schemaname, + quote_ident(mv.relname) as matviewname, + n.nspname as raw_schemaname, + mv.relname as raw_matviewname, + quote_ident(ic.relname) as indexname, + (select spcname from pg_catalog.pg_tablespace ts where ts.oid = ic.reltablespace) as tablespace, + pg_catalog.pg_get_indexdef(ic.oid) as indexdef, + d.description as index_comment + from pg_catalog.pg_index idx + join pg_catalog.pg_class ic on ic.oid = idx.indexrelid + join pg_catalog.pg_class mv on mv.oid = idx.indrelid + join pg_catalog.pg_namespace n on n.oid = mv.relnamespace + left join pg_catalog.pg_description d + on d.objoid = ic.oid + and d.classoid = 'pg_class'::regclass + and d.objsubid = 0 + where mv.relkind = 'm' + and n.nspname not in ('pg_catalog', 'information_schema') + and n.nspname in {schema_filter} + and not exists ( + select 1 from pg_catalog.pg_depend ext_dep + where ext_dep.classid = 'pg_class'::regclass + and ext_dep.objid = ic.oid + and ext_dep.objsubid = 0 + and ext_dep.deptype = 'e' + ) + order by n.nspname, mv.relname, ic.relname;" + ) + } + fn build_view_column_comments_query(schema_filter: &str) -> String { format!( "select diff --git a/app/src/dump/core_tests.rs b/app/src/dump/core_tests.rs index 06f4d29..5292615 100644 --- a/app/src/dump/core_tests.rs +++ b/app/src/dump/core_tests.rs @@ -761,6 +761,46 @@ fn build_materialized_views_query_filters_by_pg_class() { ); } +// Issue #235: a materialized view's indexes were fetched only by the table-side +// bulk query, which distributes rows into the dump's table list (relkind r/p) — +// so they matched nothing and never reached the dump. +#[test] +fn build_materialized_view_indexes_query_targets_matviews_only() { + let query = Dump::build_materialized_view_indexes_query("('public')"); + assert!( + query.contains("mv.relkind = 'm'"), + "expected the index query to be anchored on the indexed relation being a \ + materialized view: {query}" + ); + assert!( + query.contains("mv.oid = idx.indrelid"), + "the relkind filter has to be on the indexed relation, not the index itself" + ); + assert!( + query.contains("n.nspname in ('public')"), + "expected the schema filter to be applied: {query}" + ); + assert!( + query.contains("d.classoid = 'pg_class'::regclass"), + "expected pg_class classoid filter for materialized view index comments" + ); + assert!( + query.contains("ext_dep.deptype = 'e'"), + "expected extension-owned indexes to be excluded, as every other dump query does" + ); +} + +// The join key has to be the raw catalog name: build_materialized_views_query +// stores the view's schema/name unquoted, so a quote_ident'd key would not match +// and the indexes would silently be dropped again. +#[test] +fn build_materialized_view_indexes_query_exposes_raw_join_keys() { + let query = Dump::build_materialized_view_indexes_query("('public')"); + assert!(query.contains("n.nspname as raw_schemaname")); + assert!(query.contains("mv.relname as raw_matviewname")); + assert!(query.contains("quote_ident(ic.relname) as indexname")); +} + #[test] fn build_view_column_comments_query_filters_by_pg_class() { let query = Dump::build_view_column_comments_query("('public')"); diff --git a/app/src/dump/view.rs b/app/src/dump/view.rs index 56696d2..275a0a3 100644 --- a/app/src/dump/view.rs +++ b/app/src/dump/view.rs @@ -1,5 +1,7 @@ use serde::{Deserialize, Serialize}; +use crate::dump::table::IndexAlterPlan; +use crate::dump::table_index::TableIndex; use crate::utils::string_extensions::StringExt; /// One output column of a regular view, as PostgreSQL records it in @@ -86,6 +88,19 @@ pub struct View { /// [`View::or_replace_compatible`]; not part of the hash. #[serde(default, skip_serializing_if = "Vec::is_empty")] pub columns: Vec, + /// Indexes defined on a materialized view, ordered by name. Always empty for + /// regular views — PostgreSQL only allows indexing a materialized one — and + /// for dumps written before this field existed (issue #235). + /// + /// Deliberately excluded from `View::hash`: a materialized view is dropped and + /// fully rebuilt whenever its hash changes, so hashing the index list would + /// turn "an index was added" into a full refresh of the view's contents, and + /// would additionally make every materialized view compare as changed against + /// an older dump that carries no index data. The comparer diffs this list on + /// its own and emits plain `CREATE INDEX` / `DROP INDEX` for a view whose + /// definition did not change. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub indexes: Vec, } impl View { @@ -113,6 +128,7 @@ impl View { storage_parameters: None, tablespace: None, columns: Vec::new(), + indexes: Vec::new(), }; view.hash(); view @@ -190,8 +206,22 @@ impl View { )); } - /// Returns a string to create the view. + /// Returns a string to create the view, including the indexes of a + /// materialized view. A materialized view is always dropped and recreated + /// rather than replaced in place, and `DROP MATERIALIZED VIEW` takes its + /// indexes with it, so the CREATE has to put them back (issue #235). pub fn get_script(&self) -> String { + let mut script = self.get_script_without_indexes(); + for index in &self.indexes { + script.push_str(&index.get_script()); + } + script + } + + /// The CREATE script without the materialized view's indexes. Used by the + /// production output path, which emits them separately so they can be built + /// concurrently, mirroring `Table::get_script_without_triggers_no_indexes`. + pub fn get_script_without_indexes(&self) -> String { let keyword = self.view_keyword(); let with_clause = if self.security_invoker { " with (security_invoker = true)" @@ -289,6 +319,41 @@ impl View { .with_empty_lines() } + /// Structured index diff between `self` (FROM) and `to_view` (TO) for a + /// materialized view whose definition did not change, so it is not being + /// dropped and recreated and its indexes have to be reconciled in place. + /// Mirrors `Table::index_alter_plan`; a materialized view can never carry a + /// partition-inherited index, so there is nothing to skip. + pub fn index_alter_plan<'a>(&'a self, to_view: &'a View) -> IndexAlterPlan<'a> { + let mut plan = IndexAlterPlan::default(); + + for new_index in &to_view.indexes { + if let Some(old_index) = self.indexes.iter().find(|i| i.name == new_index.name) { + if old_index != new_index { + if crate::dump::table_index::indexdefs_equivalent( + &old_index.indexdef, + &new_index.indexdef, + ) { + plan.comment_changes.push(new_index); + } else { + plan.drop.push(old_index); + plan.create.push(new_index); + } + } + } else { + plan.create.push(new_index); + } + } + + for old_index in &self.indexes { + if !to_view.indexes.iter().any(|i| i.name == old_index.name) { + plan.drop.push(old_index); + } + } + + plan + } + pub fn get_owner_script(&self) -> String { if self.owner.is_empty() { return String::new(); diff --git a/app/src/dump/view_tests.rs b/app/src/dump/view_tests.rs index ab3631a..4f616e9 100644 --- a/app/src/dump/view_tests.rs +++ b/app/src/dump/view_tests.rs @@ -510,3 +510,176 @@ fn get_alter_script_compatible_append_uses_or_replace() { ); assert!(!script.to_lowercase().contains("drop view")); } + +// ── Issue #235: indexes on a materialized view ────────────────────────────── + +fn mv_index(name: &str, indexdef: &str) -> TableIndex { + TableIndex { + schema: "analytics".to_string(), + table: "active_users".to_string(), + name: name.to_string(), + catalog: None, + indexdef: indexdef.to_string(), + is_partition_index: false, + comment: None, + } +} + +fn indexed_materialized_view(indexes: Vec) -> View { + let mut view = create_materialized_view("select id from public.users"); + view.indexes = indexes; + view.hash(); + view +} + +#[test] +fn matview_get_script_recreates_its_indexes() { + let view = indexed_materialized_view(vec![ + mv_index( + "ix_id", + "CREATE UNIQUE INDEX ix_id ON analytics.active_users USING btree (id)", + ), + mv_index( + "ix_name", + "CREATE INDEX ix_name ON analytics.active_users USING btree (name)", + ), + ]); + + let script = view.get_script(); + assert!(script.contains("create materialized view analytics.active_users")); + assert!( + script.contains("CREATE UNIQUE INDEX ix_id ON analytics.active_users USING btree (id);"), + "DROP MATERIALIZED VIEW takes the indexes with it, so the CREATE must put \ + them back: {script}" + ); + assert!(script.contains("CREATE INDEX ix_name ON analytics.active_users USING btree (name);")); +} + +#[test] +fn matview_index_comment_is_emitted_with_the_index() { + let mut index = mv_index( + "ix_id", + "CREATE INDEX ix_id ON analytics.active_users USING btree (id)", + ); + index.comment = Some("lookup by id".to_string()); + let view = indexed_materialized_view(vec![index]); + + assert!( + view.get_script() + .contains("comment on index analytics.ix_id is 'lookup by id';") + ); +} + +#[test] +fn matview_get_script_without_indexes_omits_them() { + let view = indexed_materialized_view(vec![mv_index( + "ix_id", + "CREATE UNIQUE INDEX ix_id ON analytics.active_users USING btree (id)", + )]); + + let script = view.get_script_without_indexes(); + assert!(script.contains("create materialized view analytics.active_users")); + assert!( + !script.contains("CREATE UNIQUE INDEX"), + "the production path emits the indexes itself: {script}" + ); +} + +#[test] +fn regular_view_script_is_unchanged_by_the_index_field() { + // A regular view can never be indexed, so its script must be byte-identical + // to what it was before the field existed. + let view = create_view("select id from public.users"); + assert_eq!(view.get_script(), view.get_script_without_indexes()); +} + +#[test] +fn matview_indexes_do_not_affect_the_hash() { + // Hashing the index list would turn "an index was added" into a full drop + + // rebuild of the view's contents, and would make every materialized view + // look changed against a dump written before the field existed. + let plain = indexed_materialized_view(Vec::new()); + let indexed = indexed_materialized_view(vec![mv_index( + "ix_id", + "CREATE UNIQUE INDEX ix_id ON analytics.active_users USING btree (id)", + )]); + + assert_eq!(plain.hash, indexed.hash); +} + +#[test] +fn view_without_index_field_deserializes_from_an_older_dump() { + let json = r#"{ + "schema": "analytics", + "name": "active_users", + "definition": "select id from public.users", + "table_relation": [], + "is_materialized": true + }"#; + let view: View = serde_json::from_str(json).expect("older dumps must stay readable"); + assert!(view.indexes.is_empty()); +} + +#[test] +fn matview_index_alter_plan_classifies_every_change() { + let from = indexed_materialized_view(vec![ + mv_index( + "ix_dropped", + "CREATE INDEX ix_dropped ON analytics.active_users USING btree (amount)", + ), + mv_index( + "ix_redefined", + "CREATE INDEX ix_redefined ON analytics.active_users USING btree (name)", + ), + mv_index( + "ix_recommented", + "CREATE INDEX ix_recommented ON analytics.active_users USING btree (id)", + ), + ]); + + let mut recommented = mv_index( + "ix_recommented", + "CREATE INDEX ix_recommented ON analytics.active_users USING btree (id)", + ); + recommented.comment = Some("after".to_string()); + let to = indexed_materialized_view(vec![ + mv_index( + "ix_redefined", + "CREATE INDEX ix_redefined ON analytics.active_users USING btree (name DESC)", + ), + recommented, + mv_index( + "ix_added", + "CREATE INDEX ix_added ON analytics.active_users USING btree (active)", + ), + ]); + + let plan = from.index_alter_plan(&to); + + let dropped: Vec<&str> = plan.drop.iter().map(|i| i.name.as_str()).collect(); + let created: Vec<&str> = plan.create.iter().map(|i| i.name.as_str()).collect(); + let recommented: Vec<&str> = plan + .comment_changes + .iter() + .map(|i| i.name.as_str()) + .collect(); + + assert_eq!(dropped, vec!["ix_redefined", "ix_dropped"]); + assert_eq!(created, vec!["ix_redefined", "ix_added"]); + assert_eq!(recommented, vec!["ix_recommented"]); +} + +#[test] +fn matview_index_alter_plan_ignores_an_unchanged_index() { + let index = mv_index( + "ix_id", + "CREATE INDEX ix_id ON analytics.active_users USING btree (id)", + ); + let from = indexed_materialized_view(vec![index.clone()]); + let to = indexed_materialized_view(vec![index]); + + let plan = from.index_alter_plan(&to); + assert!(plan.drop.is_empty()); + assert!(plan.create.is_empty()); + assert!(plan.comment_changes.is_empty()); +} diff --git a/data/test/README.md b/data/test/README.md index 4bb260d..b1561b7 100644 --- a/data/test/README.md +++ b/data/test/README.md @@ -232,6 +232,28 @@ These schemas are designed to test comparison capabilities for the following Pos `innorm` is identical in both schemas; the matview exists only in TO so the diff creates it and applying it re-parses the expression. The companion partial index `ix_innorm_trust` (§6) exercises the same non-idempotency in an index predicate. +- **Indexes on a materialized view** (issue #235): `pg_indexes` rows for a + materialized view were only ever distributed into the dump's *table* list, so they + matched nothing and never reached the dump. Nothing re-emitted them either, and + because `DROP MATERIALIZED VIEW` takes the indexes with it, a recreated view came + back unindexed — silently, since neither dump carried the indexes and the second + diff was therefore empty. Three views over the shared base table `mv235_base` cover + the paths: + - **Modified**: `mv235_recreated` — gains the `amount` column, so it is dropped and + rebuilt; its unique index, its partial index and that index's comment must all be + recreated with it. + - **Unchanged**: `mv235_stable` — same definition in both, so the view survives and + its indexes are reconciled in place: `ix_mv235_stable_drop` removed, + `ix_mv235_stable_redef` redefined `ASC` → `DESC` (drop + rebuild, not a + comment-only edit), `ix_mv235_stable_cmt` re-commented without touching the index, + and `ix_mv235_stable_added` created. Index changes stay out of `View::hash` on + purpose: hashing them would turn "an index was added" into a full rebuild of the + view's contents. + - **Added**: `mv235_new` (TO-only) — its index is emitted with the initial CREATE. + + In production mode every one of these builds runs `CREATE INDEX CONCURRENTLY` (and + drops run `DROP INDEX CONCURRENTLY`) in the post-commit section, exactly as a + table's indexes do. ### 14. Row-Level Security Policies - **Modified**: `users_rls_select` — changed to `RESTRICTIVE`, role changed to `tenant_reader`, added `AND two_factor_enabled = TRUE` condition diff --git a/data/test/schema_a.sql b/data/test/schema_a.sql index cec6b6f..fe68093 100644 --- a/data/test/schema_a.sql +++ b/data/test/schema_a.sql @@ -1870,3 +1870,47 @@ CREATE TABLE test_schema.cascade_part_2025 PARTITION OF test_schema.cascade_part FOR VALUES FROM ('2025-01-01') TO ('2026-01-01'); CREATE INDEX ix_cascade_part_fn ON test_schema.cascade_part (test_schema.cascade_part_fn(s)); + +-- ============================================================================= +-- Regression: indexes on a materialized view (issue #235) +-- ============================================================================= +-- A materialized view can be indexed, but `pg_indexes` rows for one were only +-- ever distributed into the dump's *table* list, so they matched nothing and +-- were dropped on the floor. Nothing then re-emitted them: a materialized view +-- is always dropped and recreated when its definition changes, and +-- DROP MATERIALIZED VIEW takes the indexes with it, so the migration silently +-- left the view unindexed. Neither dump carried the indexes, so the second diff +-- was empty and the loss went unreported. +-- +-- mv235_recreated: definition changes in TO, so the view is dropped and rebuilt +-- and every index has to be recreated with it. +-- mv235_stable: definition identical in both, so the view survives and its +-- indexes are reconciled in place (added / removed / redefined +-- / re-commented) — index changes stay out of the view hash so +-- that adding an index never forces a full rebuild. +-- mv235_new: TO-only, so its index rides along with the initial CREATE. +CREATE TABLE test_schema.mv235_base ( + id integer PRIMARY KEY, + label text, + amount numeric, + active boolean +); + +CREATE MATERIALIZED VIEW test_schema.mv235_recreated AS +SELECT id, label FROM test_schema.mv235_base; + +CREATE UNIQUE INDEX ix_mv235_recreated_id ON test_schema.mv235_recreated (id); +CREATE INDEX ix_mv235_recreated_partial ON test_schema.mv235_recreated (label) + WHERE label IS NOT NULL; +COMMENT ON INDEX test_schema.ix_mv235_recreated_partial IS 'partial index on a matview'; + +CREATE MATERIALIZED VIEW test_schema.mv235_stable AS +SELECT id, label, amount FROM test_schema.mv235_base; + +-- Dropped in TO. +CREATE INDEX ix_mv235_stable_drop ON test_schema.mv235_stable (amount); +-- Definition changes in TO (ASC -> DESC): drop + rebuild, not a comment-only edit. +CREATE INDEX ix_mv235_stable_redef ON test_schema.mv235_stable (label); +-- Only the comment changes in TO: the index itself must be left alone. +CREATE INDEX ix_mv235_stable_cmt ON test_schema.mv235_stable (id, amount); +COMMENT ON INDEX test_schema.ix_mv235_stable_cmt IS 'comment before'; diff --git a/data/test/schema_b.sql b/data/test/schema_b.sql index 5963357..67352d9 100644 --- a/data/test/schema_b.sql +++ b/data/test/schema_b.sql @@ -2122,3 +2122,40 @@ CREATE TABLE test_schema.cascade_part_2025 PARTITION OF test_schema.cascade_part FOR VALUES FROM ('2025-01-01') TO ('2026-01-01'); CREATE INDEX ix_cascade_part_fn ON test_schema.cascade_part (test_schema.cascade_part_fn(s)); + +-- ============================================================================= +-- Regression: indexes on a materialized view (issue #235) +-- ============================================================================= +-- See schema_a.sql for the full description. Here mv235_recreated gains a column +-- (forcing the drop+recreate that used to lose every index), mv235_stable keeps +-- its definition while its index set changes underneath it, and mv235_new is a +-- TO-only view whose index has to be created with it. +CREATE TABLE test_schema.mv235_base ( + id integer PRIMARY KEY, + label text, + amount numeric, + active boolean +); + +CREATE MATERIALIZED VIEW test_schema.mv235_recreated AS +SELECT id, label, amount FROM test_schema.mv235_base; + +CREATE UNIQUE INDEX ix_mv235_recreated_id ON test_schema.mv235_recreated (id); +CREATE INDEX ix_mv235_recreated_partial ON test_schema.mv235_recreated (label) + WHERE label IS NOT NULL; +COMMENT ON INDEX test_schema.ix_mv235_recreated_partial IS 'partial index on a matview'; + +CREATE MATERIALIZED VIEW test_schema.mv235_stable AS +SELECT id, label, amount FROM test_schema.mv235_base; + +-- ix_mv235_stable_drop is gone. +CREATE INDEX ix_mv235_stable_redef ON test_schema.mv235_stable (label DESC); +CREATE INDEX ix_mv235_stable_cmt ON test_schema.mv235_stable (id, amount); +COMMENT ON INDEX test_schema.ix_mv235_stable_cmt IS 'comment after'; +-- TO-only index on a view that is not being recreated. +CREATE UNIQUE INDEX ix_mv235_stable_added ON test_schema.mv235_stable (id); + +CREATE MATERIALIZED VIEW test_schema.mv235_new AS +SELECT id, label, active FROM test_schema.mv235_base WHERE active; + +CREATE INDEX ix_mv235_new_label ON test_schema.mv235_new (label);