From ede9f74bbf872aa97b61d087f57c3dea2cc32e6a Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:30:17 -0400 Subject: [PATCH 01/11] Fix trigger comments dropped on pull COMMENT ON TRIGGER trg ON tbl already parsed correctly in ddl/object.rs (target.schema carries the owning table's qualified name, target.name the trigger name), but pull::Assembly::apply_comment had no TRIGGER arm, so the comment fell into the `_ => false` case and was silently discarded. Add apply_trigger_comment, mirroring the existing apply_column_comment two-name lookup, and a unit test asserting the comment lands on the trigger. RULE/POLICY/CONSTRAINT ... ON ... comments are deferred: their ddl-side parsing shares the same two-name shape (POLICY already has a ddl test), but there is no models::Trigger-equivalent field to attach a POLICY/RULE comment to without further model changes, which is out of scope here. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/pull/mod.rs | 90 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 03e996a..63c5a39 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -933,6 +933,7 @@ impl Assembly { .map(|v| v.comment = Some(comment.clone())) .is_some(), "FUNCTION" => self.apply_function_comment(&schema, name, &comment), + "TRIGGER" => self.apply_trigger_comment(target, &comment), "INDEX" => { match self.index_location.get(&(schema, name.clone())) { Some(&IndexLocation::Table(idx)) => self.tables[idx] @@ -1029,6 +1030,40 @@ impl Assembly { true } + /// `COMMENT ON TRIGGER trg ON schema.table` — the ddl layer puts + /// the owning table's qualified name into `target.schema` (mirrors + /// `apply_column_comment`'s two-name COMMENT shape) + fn apply_trigger_comment( + &mut self, + target: &QualifiedName, + comment: &str, + ) -> bool { + let Some(relation) = &target.schema else { + return false; + }; + let (schema, table) = match relation.split_once('.') { + Some((schema, table)) => (Some(schema.to_string()), table), + None => (None, relation.as_str()), + }; + let relation = QualifiedName { + schema, + name: table.to_string(), + }; + let Some(table) = self.find_table(&relation) else { + return false; + }; + let Some(trigger) = table + .triggers + .iter_mut() + .flatten() + .find(|t| t.name.as_deref() == Some(target.name.as_str())) + else { + return false; + }; + trigger.comment = Some(comment.to_string()); + true + } + fn find_table( &mut self, name: &QualifiedName, @@ -1519,6 +1554,61 @@ mod tests { } } + #[test] + fn trigger_comments_attach_to_owning_table() { + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Table, + "test", + "users", + "CREATE TABLE test.users (id uuid NOT NULL);", + ); + add( + &mut dump, + OT::Function, + "test", + "set_last_modified()", + "CREATE FUNCTION test.set_last_modified() RETURNS trigger \ + LANGUAGE plpgsql AS $$ BEGIN RETURN NEW; END; $$;", + ); + add( + &mut dump, + OT::Trigger, + "test", + "users trg_last_modified", + "CREATE TRIGGER trg_last_modified BEFORE UPDATE ON test.users \ + FOR EACH ROW EXECUTE FUNCTION test.set_last_modified();", + ); + add( + &mut dump, + OT::Comment, + "test", + "TRIGGER trg_last_modified ON users", + "COMMENT ON TRIGGER trg_last_modified ON test.users IS \ + 'keeps last_modified_at fresh';", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + let table = assembly + .tables + .iter() + .find(|t| t.name == "users") + .expect("users table"); + let trigger = table + .triggers + .as_ref() + .unwrap() + .iter() + .find(|t| t.name.as_deref() == Some("trg_last_modified")) + .expect("trigger"); + assert_eq!( + trigger.comment.as_deref(), + Some("keeps last_modified_at fresh") + ); + } + #[test] fn ingests_project_settings() { let assembly = assembled(); From 259201d014aabcb5734868a40a93c0fe886f1923 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:33:34 -0400 Subject: [PATCH 02/11] Fix typed-table inline column constraints dropped CREATE TABLE x OF type (col WITH OPTIONS ..., ...) uses a distinct element-list production (OptTypedTableElementList/TypedTableElement), not TableElement, so create_table's single TableElement walk never saw these columns/constraints and silently dropped them. TypedTableElement is columnOptions (a ColId + ColQualList of the same ColConstraintElem/ ColConstraint shapes the plain column() parser already understands) or TableConstraint (same as an inline table-level constraint). Add a second walk over TypedTableElement in create_table (src/ddl/table.rs) that feeds columnOptions nodes straight into the existing column() helper (data_type stays empty, matching build::render_typed_table_column which never emits it) and TableConstraint nodes into the existing table_constraint()/apply_constraint() path. Added parses_typed_table_inline_column_constraints and parses_typed_table_check_constraint unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/table.rs | 52 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/ddl/table.rs b/src/ddl/table.rs index b44eae4..713d212 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -139,6 +139,21 @@ pub(crate) fn create_table( table.like_table = Some(like_table(&like_clause, src)); } } + // `CREATE TABLE x OF type (col WITH OPTIONS ..., ...)` — a typed + // table's element list is a different production (no columnDef, + // since the columns themselves come from the composite type; only + // per-column constraints via `columnOptions`, plus ordinary table + // constraints, are legal here) + for element in node.find_all("TypedTableElement") { + if let Some(column_options) = element.child_of_kind("columnOptions") { + columns.push(column(&column_options, src)); + } else if let Some(constraint) = + element.child_of_kind("TableConstraint") + { + let (name, parsed) = table_constraint(&constraint, src)?; + apply_constraint(&mut table, name, parsed); + } + } if !columns.is_empty() { table.columns = Some(columns); } @@ -1027,6 +1042,43 @@ mod tests { assert_eq!(table.from_type, Some("test.person_type".into())); } + #[test] + fn parses_typed_table_inline_column_constraints() { + let statement = parse_one( + "CREATE TABLE test.person OF test.person_type (\n\ + name WITH OPTIONS NOT NULL,\n\ + age WITH OPTIONS DEFAULT 0\n\ + );", + ); + let Statement::CreateTable(table) = statement else { + panic!("expected CreateTable") + }; + assert_eq!(table.from_type, Some("test.person_type".into())); + let columns = table.columns.unwrap(); + assert_eq!(columns.len(), 2); + assert_eq!(columns[0].name, "name"); + assert_eq!(columns[0].data_type, ""); + assert_eq!(columns[0].nullable, Some(false)); + assert_eq!(columns[1].name, "age"); + assert_eq!(columns[1].default, Some(json!("0"))); + } + + #[test] + fn parses_typed_table_check_constraint() { + let statement = parse_one( + "CREATE TABLE test.person OF test.person_type (\n\ + CONSTRAINT person_age_check CHECK (age >= 0)\n\ + );", + ); + let Statement::CreateTable(table) = statement else { + panic!("expected CreateTable") + }; + let constraints = table.check_constraints.unwrap(); + assert_eq!(constraints.len(), 1); + assert_eq!(constraints[0].name, "person_age_check"); + assert_eq!(constraints[0].expression, "age >= 0"); + } + #[test] fn parses_create_table_like() { let statement = parse_one( From 41c6f645b216a1ff085e0804d08ff50458d820ad Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:38:28 -0400 Subject: [PATCH 03/11] Fix zero-argument functions emitting invalid CREATE/DROP/COMMENT SQL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dump_function's fallback branch used the bare d.name for func_name when a function had no structured `parameters`, which produced `CREATE FUNCTION schema.foo RETURNS ...` (missing `()`) for zero-argument functions that store a plain, unparenthesized name — invalid SQL that also broke the matching DROP FUNCTION and, since the default COMMENT ON FUNCTION target derives from the same name, COMMENT ON FUNCTION. Names that already carry an embedded `(argtypes)` signature (the on-disk convention used to disambiguate overloads) are left untouched, so parameterized functions render exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/build/mod.rs | 129 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 122 insertions(+), 7 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index 7a75961..9ef2e27 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -666,7 +666,12 @@ impl Builder { if let Some(sql) = &d.sql { return self.add_item(item, vec![sql.clone()], vec![], false); } - let func_name = match &d.parameters { + // `comment_target` is only set when `func_name` diverges from the + // default comment target (`namespace.d.name`) computed in + // `add_comment` — i.e. only for the bare, unparenthesized + // zero-argument case below, where `()` must be appended for a + // valid `COMMENT ON FUNCTION`. + let (func_name, comment_target) = match &d.parameters { Some(parameters) if !parameters.is_empty() => { let params: Vec = parameters .iter() @@ -683,13 +688,27 @@ impl Builder { value.join(" ") }) .collect(); - format!( + let func_name = format!( "{}({})", d.name.split('(').next().unwrap_or_default(), params.join(", ") - ) + ); + (func_name, None) + } + // no structured `parameters`: `d.name` may already carry an + // embedded `(argtypes)` signature (the on-disk convention for + // disambiguating overloads); only a bare, unparenthesized + // name needs `()` appended for a true zero-argument function + _ if d.name.contains('(') => (d.name.clone(), None), + _ => { + let func_name = format!("{}()", d.name); + let comment_target = if d.schema.is_empty() { + func_name.clone() + } else { + format!("{}.{}", quote_ident(&d.schema), func_name) + }; + (func_name, Some(comment_target)) } - _ => d.name.clone(), }; let mut create = vec![ "CREATE".into(), @@ -700,7 +719,7 @@ impl Builder { "LANGUAGE".into(), d.language.clone().unwrap_or_default(), ]; - let drop = vec!["DROP FUNCTION IF EXISTS".into(), func_name]; + let drop = vec!["DROP FUNCTION IF EXISTS".into(), func_name.clone()]; if let Some(transform_types) = &d.transform_types { let tts: Vec = transform_types .iter() @@ -763,7 +782,13 @@ impl Builder { if let Some(definition) = &d.definition { let create_sql = vec![format!("{} $$\n{}\n$$", create.join(" "), definition)]; - return self.add_item(item, create_sql, drop, false); + return self.add_item_with_comment_target( + item, + create_sql, + drop, + false, + comment_target, + ); } if let (Some(object_file), Some(link_symbol)) = (&d.object_file, &d.link_symbol) @@ -774,7 +799,13 @@ impl Builder { postgres_value(&Value::String(link_symbol.clone())) )); } - self.add_item(item, create, drop, false) + self.add_item_with_comment_target( + item, + create, + drop, + false, + comment_target, + ) } fn dump_group(&mut self, item: &Item) -> Result<(), String> { @@ -2815,4 +2846,88 @@ mod tests { DEFAULT;\n" ); } + + fn zero_arg_function(name: &str, comment: Option<&str>) -> Item { + Item { + id: 1, + desc: ObjectType::Function, + definition: Definition::Function(crate::models::Function { + name: name.into(), + schema: "test".into(), + owner: "app".into(), + sql: None, + parameters: None, + returns: Some("trigger".into()), + language: Some("plpgsql".into()), + transform_types: None, + window: None, + immutable: None, + stable: None, + volatile: None, + leak_proof: None, + called_on_null_input: None, + strict: None, + security: None, + parallel: None, + cost: None, + rows: None, + support: None, + configuration: None, + definition: Some("BEGIN\n RETURN NEW;\nEND;".into()), + object_file: None, + link_symbol: None, + comment: comment.map(String::from), + }), + dependencies: BTreeSet::new(), + } + } + + /// Render `item` and return the FUNCTION entry's create and drop SQL + fn function_entry(item: &Item) -> (String, String) { + let dump = libpgdump::new("t", "UTF-8", "18.0").unwrap(); + let mut builder = Builder { + dump, + dump_id_map: HashMap::new(), + superuser: "postgres".into(), + }; + builder.dump_item(item).unwrap(); + let entry = builder + .dump + .entries() + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Function) + .expect("a FUNCTION entry") + .clone(); + ( + entry.defn.unwrap_or_default(), + entry.drop_stmt.unwrap_or_default(), + ) + } + + #[test] + fn renders_zero_arg_function_with_parens() { + let item = zero_arg_function("bare_zero", None); + let (create, drop) = function_entry(&item); + assert!( + create.starts_with( + "CREATE FUNCTION bare_zero() RETURNS trigger LANGUAGE \ + plpgsql AS $$" + ), + "unexpected CREATE: {create}" + ); + assert_eq!(drop, "DROP FUNCTION IF EXISTS bare_zero();\n"); + } + + #[test] + fn comments_zero_arg_function_with_parens() { + let item = zero_arg_function("bare_zero", Some("a trigger fn")); + let defns = comment_defns(&item); + assert_eq!( + defns, + vec![ + "COMMENT ON FUNCTION test.bare_zero() IS $$a trigger \ + fn$$;\n;\n" + ] + ); + } } From f64687fa096d78174c7acde1343d64a721c80387 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:39:05 -0400 Subject: [PATCH 04/11] Fix SERIAL/column defaults from separate ALTER dropped on pull MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pg_dump emits column defaults (SERIAL's nextval(...), and any other DEFAULT) as a standalone `ALTER TABLE ONLY t ALTER COLUMN c SET DEFAULT ...` statement (libpgdump ObjectType::Default), not inline on the CREATE TABLE column def. alter_table() only recognized `kw_add` commands, so these fell through to Statement::Unsupported and the default was silently lost — SERIAL columns pulled with no default at all. - src/ddl/mod.rs: add Statement::SetColumnDefault { table, column, default }. - src/ddl/table.rs: alter_table() now also matches alter_table_cmd -> alter_column_default (kw_set + kw_default + a_expr; kw_drop + kw_default is left unhandled, same as before). Added parses_alter_table_set_column_default. - src/pull/mod.rs: OT::Default is now routed through the parser (was falling into push_remaining); Assembly::apply folds the default onto the matching column of the already-ingested table, with a deferred_defaults retry list (mirroring deferred_indexes/ deferred_partitions) for the case where the ALTER precedes its table. Added set_default_attaches_to_existing_column and deferred_default_attaches_after_table_is_ingested. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/mod.rs | 9 +++ src/ddl/table.rs | 52 ++++++++++++---- src/pull/mod.rs | 159 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 11 deletions(-) diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index 7772286..78b8eed 100644 --- a/src/ddl/mod.rs +++ b/src/ddl/mod.rs @@ -42,6 +42,15 @@ pub enum Statement { name: Option, constraint: TableConstraint, }, + /// ALTER TABLE ... ALTER COLUMN ... SET DEFAULT — pg_dump emits + /// column defaults this way (e.g. `nextval(...)` for SERIAL) rather + /// than inline on the CREATE TABLE column; folded onto the matching + /// column of the already-ingested table during pull assembly + SetColumnDefault { + table: QualifiedName, + column: String, + default: serde_json::Value, + }, CreateSchema(models::Schema), CreateDomain(models::Domain), CreateType(Box), diff --git a/src/ddl/table.rs b/src/ddl/table.rs index 713d212..3b1d6cd 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -336,18 +336,29 @@ pub(crate) fn alter_table( let table = qualified_name(&table, src)?; let mut statements = Vec::new(); for cmd in node.find_all("alter_table_cmd") { - if !cmd.has("kw_add") { - continue; + if cmd.has("kw_add") { + let Some(constraint) = cmd.find("TableConstraint") else { + continue; + }; + let (name, parsed) = table_constraint(&constraint, src)?; + statements.push(Statement::AddConstraint { + table: table.clone(), + name, + constraint: parsed, + }); + } else if let Some(default) = cmd.child_of_kind("alter_column_default") + && let Some(expr) = default.child_of_kind("a_expr") + { + let column = cmd + .child_of_kind("ColId") + .map(|n| unquote(n.text(src))) + .unwrap_or_default(); + statements.push(Statement::SetColumnDefault { + table: table.clone(), + column, + default: Value::String(expr.text(src).to_string()), + }); } - let Some(constraint) = cmd.find("TableConstraint") else { - continue; - }; - let (name, parsed) = table_constraint(&constraint, src)?; - statements.push(Statement::AddConstraint { - table: table.clone(), - name, - constraint: parsed, - }); } if statements.is_empty() { statements.push(Statement::Unsupported(format!( @@ -954,6 +965,25 @@ mod tests { assert_eq!(constraint, TableConstraint::Check("value > 0".into())); } + #[test] + fn parses_alter_table_set_column_default() { + let statement = parse_one( + "ALTER TABLE ONLY test.t ALTER COLUMN id SET DEFAULT \ + nextval('test.t_id_seq'::regclass);", + ); + let Statement::SetColumnDefault { + table, + column, + default, + } = statement + else { + panic!("expected SetColumnDefault, got {statement:?}") + }; + assert_eq!(table.to_string(), "test.t"); + assert_eq!(column, "id"); + assert_eq!(default, json!("nextval('test.t_id_seq'::regclass)")); + } + #[test] fn quoted_identifiers_unquote() { let statement = diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 63c5a39..1f81014 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -396,6 +396,10 @@ pub struct Assembly { /// PARTITION OF children whose parent table had not yet been /// ingested, replayed after the entry loop completes deferred_partitions: Vec<(QualifiedName, models::TablePartition)>, + /// ALTER TABLE ... ALTER COLUMN ... SET DEFAULT statements whose + /// table had not yet been ingested, replayed after the entry loop + /// completes + deferred_defaults: Vec<(QualifiedName, String, Value)>, /// (schema, name) -> index into `tables`, kept in sync as tables /// are ingested so index/constraint/trigger/comment merges are /// O(1) instead of a linear scan per lookup @@ -513,6 +517,7 @@ impl Assembly { | OT::Constraint | OT::FkConstraint | OT::CheckConstraint + | OT::Default | OT::Trigger | OT::ForeignTable | OT::ForeignDataWrapper @@ -553,6 +558,7 @@ impl Assembly { } self.apply_deferred_indexes(); self.apply_deferred_partitions(); + self.apply_deferred_defaults(); task.finish(); Ok(()) } @@ -587,6 +593,22 @@ impl Assembly { } } + /// Attach column defaults (`ALTER TABLE ... ALTER COLUMN ... SET + /// DEFAULT`) whose table was not yet ingested when the statement + /// was seen; warn for any that remain unresolved + fn apply_deferred_defaults(&mut self) { + for (table, column, default) in + std::mem::take(&mut self.deferred_defaults) + { + match self.find_table(&table) { + Some(t) => set_column_default(t, &column, default), + None => { + log::warn!("Column default on unknown table {table}"); + } + } + } + } + /// Parse a `pg_dumpall --roles-only` SQL dump, skipping comments, /// SET statements, and psql meta-commands (PG17 wraps the output /// in `\restrict` / `\unrestrict`) @@ -737,6 +759,16 @@ impl Assembly { Some(table) => ddl::apply_constraint(table, name, constraint), None => log::warn!("Constraint on unknown table {table}"), }, + Statement::SetColumnDefault { + table, + column, + default, + } => match self.find_table(&table) { + Some(t) => set_column_default(t, &column, default), + None => { + self.deferred_defaults.push((table, column, default)); + } + }, Statement::CreateTrigger { table, trigger } => { match self.find_table(&table) { Some(table) => { @@ -1297,6 +1329,29 @@ fn cancel_revokes(statements: Vec) -> Vec { } /// The quoted value from a `SET name = 'value';` entry definition +/// Set a table column's default expression, warning if the column +/// itself is not found (the table exists but not the column — should +/// not happen for well-formed pg_dump output, but is not fatal) +fn set_column_default( + table: &mut models::Table, + column: &str, + default: Value, +) { + match table + .columns + .iter_mut() + .flatten() + .find(|c| c.name == column) + { + Some(c) => c.default = Some(default), + None => log::warn!( + "Default on unknown column {column} of table {}.{}", + table.schema, + table.name + ), + } +} + fn set_value(defn: &str) -> Option { let start = defn.find('\'')? + 1; let end = defn.rfind('\'')?; @@ -1331,6 +1386,7 @@ fn extension(entry: &libpgdump::Entry) -> models::Extension { #[cfg(test)] mod tests { use libpgdump::ObjectType as OT; + use serde_json::json; use super::*; @@ -1609,6 +1665,109 @@ mod tests { ); } + #[test] + fn set_default_attaches_to_existing_column() { + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Sequence, + "test", + "t_id_seq", + "CREATE SEQUENCE test.t_id_seq START WITH 1 INCREMENT BY 1 \ + CACHE 1;", + ); + add( + &mut dump, + OT::Table, + "test", + "t", + "CREATE TABLE test.t (id bigint NOT NULL);", + ); + add( + &mut dump, + OT::Default, + "test", + "t id", + "ALTER TABLE ONLY test.t ALTER COLUMN id SET DEFAULT \ + nextval('test.t_id_seq'::regclass);", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + let table = assembly + .tables + .iter() + .find(|t| t.name == "t") + .expect("t table"); + let column = table + .columns + .as_ref() + .unwrap() + .iter() + .find(|c| c.name == "id") + .expect("id column"); + assert_eq!( + column.default, + Some(json!("nextval('test.t_id_seq'::regclass)")) + ); + } + + #[test] + fn deferred_default_attaches_after_table_is_ingested() { + // exercise the deferred-default retry path directly: the SET + // DEFAULT statement arrives before its table has been ingested + let mut assembly = Assembly::default(); + assembly.deferred_defaults.push(( + QualifiedName { + schema: Some("test".into()), + name: "t".into(), + }, + "id".into(), + json!("nextval('test.t_id_seq'::regclass)"), + )); + assembly.tables.push(models::Table { + name: "t".into(), + schema: "test".into(), + owner: String::new(), + sql: None, + unlogged: None, + from_type: None, + parents: None, + like_table: None, + columns: Some(vec![models::Column { + name: "id".into(), + data_type: "bigint".into(), + nullable: Some(false), + default: None, + collation: None, + check_constraint: None, + generated: None, + comment: None, + }]), + indexes: None, + primary_key: None, + check_constraints: None, + unique_constraints: None, + foreign_keys: None, + triggers: None, + partition: None, + partitions: None, + access_method: None, + storage_parameters: None, + tablespace: None, + index_tablespace: None, + server: None, + options: None, + comment: None, + }); + assembly.table_index.insert(("test".into(), "t".into()), 0); + assembly.apply_deferred_defaults(); + assert_eq!( + assembly.tables[0].columns.as_ref().unwrap()[0].default, + Some(json!("nextval('test.t_id_seq'::regclass)")) + ); + } + #[test] fn ingests_project_settings() { let assembly = assembled(); From 064890d274a08c5cdeb2bb708959e5dbcfccdc11 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:40:09 -0400 Subject: [PATCH 05/11] Emit dependency edges for views and materialized views MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Views got no dependency edges (only foreign-key-derived table edges existed), so build ordered them purely by libpgdump's same-priority alphabetical sort — a view could be emitted before a table it queries, breaking restore. pg_dump's archive TOC does carry per-entry dump-id dependencies, but wiring that through would require plumbing the raw libpgdump::Dump (and its dump-id → object mapping, built in ingest()) into the writer, touching src/pull/mod.rs which another pass owns. Instead, re-parse each view/matview's stored query text with tree-sitter-postgres and walk the CST for schema-qualified relation_expr nodes, matching them against the pulled inventory (tables/views/materialized_views). Unqualified references (CTE names, or anything pg_dump left unqualified) and references outside the inventory are skipped rather than guessed at, and self-references are guarded against. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/pull/writer.rs | 261 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 258 insertions(+), 3 deletions(-) diff --git a/src/pull/writer.rs b/src/pull/writer.rs index c7b76fb..cbe63ea 100644 --- a/src/pull/writer.rs +++ b/src/pull/writer.rs @@ -48,13 +48,39 @@ pub fn render( &value, )?; } + let relation_inventory = relation_inventory(assembly); for view in &assembly.views { - writer.save(nested("views", &view.schema, &view.name)?, view)?; + let mut value = serialize(view)?; + if let (Some(map), Some(dependencies)) = ( + value.as_object_mut(), + view_dependencies( + &view.schema, + &view.name, + view.query.as_deref(), + &relation_inventory, + ), + ) { + map.insert(String::from("dependencies"), dependencies); + } + writer + .save_value(nested("views", &view.schema, &view.name)?, &value)?; } for view in &assembly.materialized_views { - writer.save( + let mut value = serialize(view)?; + if let (Some(map), Some(dependencies)) = ( + value.as_object_mut(), + view_dependencies( + &view.schema, + &view.name, + view.query.as_deref(), + &relation_inventory, + ), + ) { + map.insert(String::from("dependencies"), dependencies); + } + writer.save_value( nested("materialized_views", &view.schema, &view.name)?, - view, + &value, )?; } writer.write_functions(assembly)?; @@ -377,6 +403,134 @@ fn table_dependencies(table: &models::Table) -> Option { (!references.is_empty()).then(|| json!({"tables": references})) } +/// A `schema.name` → dependency plural key ("tables", "views", +/// "materialized_views") map of every relation the pulled inventory +/// knows about, used to resolve a view query's relation references to +/// real, managed objects +fn relation_inventory( + assembly: &Assembly, +) -> BTreeMap<(String, String), &'static str> { + let mut inventory = BTreeMap::new(); + for table in &assembly.tables { + inventory.insert((table.schema.clone(), table.name.clone()), "tables"); + } + for view in &assembly.views { + inventory.insert((view.schema.clone(), view.name.clone()), "views"); + } + for view in &assembly.materialized_views { + inventory.insert( + (view.schema.clone(), view.name.clone()), + "materialized_views", + ); + } + inventory +} + +/// Views carry no foreign keys, so pg_dump's default same-priority +/// alphabetical sort is the only thing ordering them relative to the +/// tables/views they query; emit a `dependencies` key (mirroring +/// `table_dependencies`) so `build`/`load` order the referenced +/// relations first. Referenced relations are found by re-parsing the +/// view's `query` text and walking its CST for schema-qualified +/// `relation_expr` nodes; unqualified references (CTE names, or +/// anything the dump didn't schema-qualify) and references to objects +/// outside the pulled inventory are conservatively skipped rather than +/// guessed at. +fn view_dependencies( + schema: &str, + name: &str, + query: Option<&str>, + inventory: &BTreeMap<(String, String), &'static str>, +) -> Option { + let mut tables = BTreeSet::new(); + let mut views = BTreeSet::new(); + let mut materialized_views = BTreeSet::new(); + for (rel_schema, rel_name) in referenced_relations(query?) { + if rel_schema == schema && rel_name == name { + continue; // no self edges + } + let full = format!("{rel_schema}.{rel_name}"); + match inventory.get(&(rel_schema, rel_name)) { + Some(&"tables") => { + tables.insert(full); + } + Some(&"views") => { + views.insert(full); + } + Some(&"materialized_views") => { + materialized_views.insert(full); + } + _ => {} + } + } + if tables.is_empty() && views.is_empty() && materialized_views.is_empty() { + return None; + } + let mut object = Map::new(); + if !tables.is_empty() { + object.insert(String::from("tables"), json!(tables)); + } + if !views.is_empty() { + object.insert(String::from("views"), json!(views)); + } + if !materialized_views.is_empty() { + object.insert( + String::from("materialized_views"), + json!(materialized_views), + ); + } + Some(Value::Object(object)) +} + +/// Parse a view/materialized view's query text and return every +/// schema-qualified relation it references (`schema`, `name`), in +/// document order with duplicates kept (the caller dedupes via a +/// `BTreeSet`). Unqualified references (bare CTE names, or anything +/// pg_dump didn't schema-qualify) are skipped rather than guessed at. +/// Returns an empty list if the query fails to parse (should not +/// happen for text pg_dump itself emitted, but re-parsing is +/// defensive here rather than load-bearing). +fn referenced_relations(query: &str) -> Vec<(String, String)> { + use crate::ddl::{NodeExt, qualified_name}; + + let mut relations = Vec::new(); + let mut parser = tree_sitter::Parser::new(); + if parser + .set_language(&tree_sitter_postgres::LANGUAGE.into()) + .is_err() + { + return relations; + } + // The stored `query` is a bare SelectStmt (no trailing + // semicolon); append one to reparse it as a standalone statement, + // independent of the CREATE VIEW statement it came from. + let sql = format!("{query};"); + let Some(tree) = parser.parse(&sql, None) else { + return relations; + }; + let root = tree.root_node(); + if root.has_error() { + return relations; + } + for stmt in root.find_all("stmt") { + let Some(node) = stmt.named_child(0) else { + continue; + }; + for relation in node.find_all("relation_expr") { + let Some(qn) = relation.find("qualified_name") else { + continue; + }; + let Ok(name) = qualified_name(&qn, &sql) else { + continue; + }; + if let Some(schema) = name.schema { + relations.push((schema, name.name)); + } + } + } + relations +} + /// `user.yml` does not allow the `login` option (login is implied) fn user_options(state: &RoleState) -> Option { let mut options = state.options.clone(); @@ -659,4 +813,105 @@ mod tests { let body = &writer.files[&fn_1_path]; assert!(body.contains("name: fn_1"), "unexpected contents: {body}"); } + + #[test] + fn referenced_relations_finds_schema_qualified_table() { + let relations = referenced_relations("SELECT id FROM test.users"); + assert_eq!( + relations, + vec![(String::from("test"), String::from("users"))] + ); + } + + #[test] + fn referenced_relations_skips_unqualified_names() { + // a bare name could be a CTE, a same-schema table pg_dump + // chose not to qualify, or a system relation; conservatively + // skip anything that isn't schema-qualified + assert_eq!(referenced_relations("SELECT id FROM users"), Vec::new()); + } + + #[test] + fn referenced_relations_skips_cte_names() { + let relations = referenced_relations( + "WITH recent AS (SELECT id FROM test.orders) \ + SELECT id FROM recent", + ); + assert_eq!( + relations, + vec![(String::from("test"), String::from("orders"))] + ); + } + + fn inventory( + entries: &[(&str, &str, &'static str)], + ) -> BTreeMap<(String, String), &'static str> { + entries + .iter() + .map(|(schema, name, kind)| { + ((schema.to_string(), name.to_string()), *kind) + }) + .collect() + } + + #[test] + fn view_dependencies_emits_edge_for_known_table() { + let inv = inventory(&[("test", "users", "tables")]); + let deps = view_dependencies( + "test", + "us_users", + Some("SELECT id FROM test.users"), + &inv, + ); + assert_eq!(deps, Some(json!({"tables": ["test.users"]}))); + } + + #[test] + fn view_dependencies_emits_edge_for_known_view() { + let inv = inventory(&[("test", "base_view", "views")]); + let deps = view_dependencies( + "test", + "wrapper", + Some("SELECT id FROM test.base_view"), + &inv, + ); + assert_eq!(deps, Some(json!({"views": ["test.base_view"]}))); + } + + #[test] + fn view_dependencies_skips_unknown_relation() { + // referenced relation is schema-qualified but absent from the + // pulled inventory (a CTE the parser mis-detected, a system + // catalog, or an object outside the project) — must not + // fabricate an edge that would fail to load + let inv = inventory(&[]); + let deps = view_dependencies( + "test", + "v", + Some("SELECT id FROM test.unknown"), + &inv, + ); + assert_eq!(deps, None); + } + + #[test] + fn view_dependencies_guards_self_reference() { + // even if the view itself is (erroneously) present in the + // inventory map, referencing its own qualified name must not + // produce a self-edge + let inv = inventory(&[("test", "v", "views")]); + let deps = view_dependencies( + "test", + "v", + Some("SELECT id FROM test.v"), + &inv, + ); + assert_eq!(deps, None); + } + + #[test] + fn view_dependencies_none_without_query() { + let inv = inventory(&[("test", "users", "tables")]); + assert_eq!(view_dependencies("test", "v", None, &inv), None); + } } From fc275788aa7b2226bb91d2a044f579d1bb3ce3d6 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:48:22 -0400 Subject: [PATCH 06/11] Fix partition children dropped on pull from ATTACH PARTITION MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pg_dump does not emit `CREATE TABLE child PARTITION OF parent FOR VALUES ...`; it emits the child as a plain `CREATE TABLE` followed by a separate `TABLE ATTACH` TOC entry (`ALTER TABLE ONLY parent ATTACH PARTITION child FOR VALUES ...`). The parser only recognized the inline `PARTITION OF` form, and the ingest dispatch had no arm for the `TABLE ATTACH` object type, so partition membership was silently lost on round-trip: the child surfaced as a standalone table and the parent had no partitions. Parse the `partition_cmd` ATTACH form into a new `Statement::AttachPartition`, route `ObjectType::TableAttach` through the parser, and fold attached children into their parent during pull assembly — moving the bounds (and any comment picked up on the child table) into the parent's `partitions` and dropping the child's standalone table, mirroring the existing inline-`PARTITION OF` fold. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/mod.rs | 9 ++++ src/ddl/table.rs | 70 +++++++++++++++++++++++++++ src/pull/mod.rs | 123 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 202 insertions(+) diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index 78b8eed..ded34d8 100644 --- a/src/ddl/mod.rs +++ b/src/ddl/mod.rs @@ -42,6 +42,15 @@ pub enum Statement { name: Option, constraint: TableConstraint, }, + /// ALTER TABLE ONLY `parent` ATTACH PARTITION `child` FOR VALUES ... + /// — pg_dump emits partition children as a plain CREATE TABLE plus + /// this separate ATTACH rather than inline `PARTITION OF`; the pull + /// assembly folds the already-ingested child into the parent's + /// `partitions` and drops the standalone child table + AttachPartition { + parent: QualifiedName, + partition: models::TablePartition, + }, /// ALTER TABLE ... ALTER COLUMN ... SET DEFAULT — pg_dump emits /// column defaults this way (e.g. `nextval(...)` for SERIAL) rather /// than inline on the CREATE TABLE column; folded onto the matching diff --git a/src/ddl/table.rs b/src/ddl/table.rs index 3b1d6cd..c8bca11 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -360,6 +360,35 @@ pub(crate) fn alter_table( }); } } + // ATTACH PARTITION lives in a `partition_cmd` child (sibling of the + // `alter_table_cmd`s), carrying the child relation and its bounds + for cmd in node.find_all("partition_cmd") { + if !cmd.has("kw_attach") { + continue; + } + let child = cmd + .child_of_kind("qualified_name") + .ok_or_else(|| String::from("ATTACH PARTITION without a child"))?; + let child = qualified_name(&child, src)?; + let bound = + cmd.child_of_kind("PartitionBoundSpec").ok_or_else(|| { + String::from("ATTACH PARTITION without a FOR VALUES clause") + })?; + let bound = partition_bound(&bound, src); + statements.push(Statement::AttachPartition { + parent: table.clone(), + partition: TablePartition { + name: child.name, + schema: child.schema.unwrap_or_default(), + default: bound.default, + for_values_in: bound.for_values_in, + for_values_from: bound.for_values_from, + for_values_to: bound.for_values_to, + for_values_with: bound.for_values_with, + comment: None, + }, + }); + } if statements.is_empty() { statements.push(Statement::Unsupported(format!( "ALTER TABLE {table}: {}", @@ -1035,6 +1064,47 @@ mod tests { assert_eq!(partition.for_values_to, Some(json!("2025-01-01"))); } + #[test] + fn parses_attach_partition() { + // pg_dump's real form: child is a plain CREATE TABLE, attached + // later via ALTER TABLE ONLY parent ATTACH PARTITION ... + let statement = parse_one( + "ALTER TABLE ONLY test.events ATTACH PARTITION \ + test.events_2024 FOR VALUES FROM ('2024-01-01') \ + TO ('2025-01-01');", + ); + let Statement::AttachPartition { parent, partition } = statement + else { + panic!("expected AttachPartition") + }; + assert_eq!(parent.to_string(), "test.events"); + assert_eq!(partition.schema, "test"); + assert_eq!(partition.name, "events_2024"); + assert_eq!(partition.for_values_from, Some(json!("2024-01-01"))); + assert_eq!(partition.for_values_to, Some(json!("2025-01-01"))); + } + + #[test] + fn parses_attach_partition_list_and_default() { + let Statement::AttachPartition { partition, .. } = parse_one( + "ALTER TABLE ONLY test.t ATTACH PARTITION test.p \ + FOR VALUES IN ('a', 'b');", + ) else { + panic!("expected AttachPartition") + }; + assert_eq!( + partition.for_values_in, + Some(vec![json!("a"), json!("b")]) + ); + + let Statement::AttachPartition { partition, .. } = parse_one( + "ALTER TABLE ONLY test.t ATTACH PARTITION test.pd DEFAULT;", + ) else { + panic!("expected AttachPartition") + }; + assert_eq!(partition.default, Some(true)); + } + #[test] fn parses_partition_of_multicolumn_bounds() { let statement = parse_one( diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 1f81014..9f24d9d 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -400,6 +400,11 @@ pub struct Assembly { /// table had not yet been ingested, replayed after the entry loop /// completes deferred_defaults: Vec<(QualifiedName, String, Value)>, + /// ALTER TABLE ... ATTACH PARTITION children, replayed after the + /// entry loop so both parent and child tables are fully ingested; + /// each child is folded into its parent's `partitions` and its + /// standalone table is removed + attached_partitions: Vec<(QualifiedName, models::TablePartition)>, /// (schema, name) -> index into `tables`, kept in sync as tables /// are ingested so index/constraint/trigger/comment merges are /// O(1) instead of a linear scan per lookup @@ -518,6 +523,7 @@ impl Assembly { | OT::FkConstraint | OT::CheckConstraint | OT::Default + | OT::TableAttach | OT::Trigger | OT::ForeignTable | OT::ForeignDataWrapper @@ -559,6 +565,7 @@ impl Assembly { self.apply_deferred_indexes(); self.apply_deferred_partitions(); self.apply_deferred_defaults(); + self.apply_attached_partitions(); task.finish(); Ok(()) } @@ -609,6 +616,62 @@ impl Assembly { } } + /// Fold `ATTACH PARTITION` children into their parent tables: move + /// each child's bounds (and any table comment picked up during the + /// entry loop) into the parent's `partitions`, then drop the child's + /// standalone table so partitions are modeled only under the parent + fn apply_attached_partitions(&mut self) { + let attaches = std::mem::take(&mut self.attached_partitions); + if attaches.is_empty() { + return; + } + let mut removed = std::collections::HashSet::new(); + let mut folds = Vec::with_capacity(attaches.len()); + for (parent, mut partition) in attaches { + let key = (partition.schema.clone(), partition.name.clone()); + match self.table_index.get(&key).and_then(|&i| self.tables.get(i)) + { + Some(child) => { + if partition.comment.is_none() { + partition.comment = child.comment.clone(); + } + removed.insert(key); + } + None => log::warn!( + "ATTACH PARTITION of unknown child {}.{}", + partition.schema, + partition.name + ), + } + folds.push((parent, partition)); + } + self.tables.retain(|t| { + !removed.contains(&(t.schema.clone(), t.name.clone())) + }); + self.rebuild_table_index(); + for (parent, partition) in folds { + match self.find_table(&parent) { + Some(table) => { + table.partitions.get_or_insert_default().push(partition); + } + None => { + log::warn!("ATTACH PARTITION to unknown table {parent}") + } + } + } + } + + /// Rebuild `table_index` from the current `tables` order after a + /// removal has shifted positions + fn rebuild_table_index(&mut self) { + self.table_index = self + .tables + .iter() + .enumerate() + .map(|(i, t)| ((t.schema.clone(), t.name.clone()), i)) + .collect(); + } + /// Parse a `pg_dumpall --roles-only` SQL dump, skipping comments, /// SET statements, and psql meta-commands (PG17 wraps the output /// in `\restrict` / `\unrestrict`) @@ -677,6 +740,9 @@ impl Assembly { } } } + Statement::AttachPartition { parent, partition } => { + self.attached_partitions.push((parent, partition)); + } Statement::CreateSequence(mut sequence) => { sequence.owner = owner; self.sequences.push(sequence); @@ -1870,6 +1936,63 @@ mod tests { ); } + #[test] + fn folds_attached_partition_children() { + // pg_dump's real form: the child is a standalone CREATE TABLE + // and a separate `TABLE ATTACH` entry carries the bounds. The + // child must be folded into the parent and dropped as a + // top-level table. + let mut dump = libpgdump::new("fixtures", "UTF8", "18.0").unwrap(); + add(&mut dump, OT::Schema, "", "test", "CREATE SCHEMA test;"); + add( + &mut dump, + OT::Table, + "test", + "events", + "CREATE TABLE test.events (id bigint, ts timestamp) \ + PARTITION BY RANGE (ts);", + ); + add( + &mut dump, + OT::Table, + "test", + "events_2024", + "CREATE TABLE test.events_2024 (id bigint, ts timestamp);", + ); + add( + &mut dump, + OT::Comment, + "test", + "TABLE events_2024", + "COMMENT ON TABLE test.events_2024 IS 'The 2024 slice';", + ); + add( + &mut dump, + OT::TableAttach, + "test", + "events_2024", + "ALTER TABLE ONLY test.events ATTACH PARTITION \ + test.events_2024 FOR VALUES FROM ('2024-01-01') \ + TO ('2025-01-01');", + ); + let mut assembly = Assembly::default(); + assembly.ingest(&dump).unwrap(); + // the child no longer stands alone; only the parent remains + assert_eq!(assembly.tables.len(), 1); + let events = &assembly.tables[0]; + assert_eq!(events.name, "events"); + let partitions = events.partitions.as_ref().expect("partitions"); + assert_eq!(partitions.len(), 1); + assert_eq!(partitions[0].name, "events_2024"); + assert_eq!( + partitions[0].for_values_from, + Some(serde_json::json!("2024-01-01")) + ); + assert_eq!(partitions[0].for_values_to, Some(json!("2025-01-01"))); + // a comment on the child carries onto the partition + assert_eq!(partitions[0].comment.as_deref(), Some("The 2024 slice")); + } + #[test] fn merges_sequence_owned_by() { let assembly = assembled(); From 6e2508833437528a58aeed2a5350311384f62dd4 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 16:28:39 -0400 Subject: [PATCH 07/11] Fix for_values_to typo in partition schema contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RANGE-partition branch of the `partitions` oneOf required the misspelled key `for_vlaues_to` instead of `for_values_to`, so no RANGE partition (with `for_values_from`/`for_values_to`) could ever satisfy the schema — project load rejected every range-partitioned table. This was carried over verbatim from the Python schemata and only surfaced now that a fixture exercises range partition children. Co-Authored-By: Claude Opus 4.8 (1M context) --- schemata/table.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/schemata/table.yml b/schemata/table.yml index cf9b58f..e12a432 100644 --- a/schemata/table.yml +++ b/schemata/table.yml @@ -359,7 +359,7 @@ properties: - for_values_when - required: - for_values_from - - for_vlaues_to + - for_values_to not: required: - default From a425a5984dd04dbf21b2e2609759877109d3a731 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 16:28:48 -0400 Subject: [PATCH 08/11] Schema-qualify function refs and split sequence-backed defaults in build Two invalid-SQL bugs in the built archive, both surfaced by round-tripping a fixture through pg_restore: 1. CREATE/DROP FUNCTION rendered a bare, unqualified function name. pg_restore runs with search_path reset to '' and schema-qualifies every object, so a bare `CREATE FUNCTION name(...)` fails with "no schema has been selected to create in". Render the function reference schema-qualified (the COMMENT target was already qualified). Python emitted the bare form too, so the three test-project functions move into the documented build_parity deviations. 2. A SERIAL column's `nextval(...)` default was inlined into CREATE TABLE, but the sequence is OWNED BY that column: inlining makes the table depend on the sequence while the sequence depends on the table, an unrestorable cycle. Mirror pg_dump by holding the default out of CREATE TABLE and emitting it as a separate ALTER TABLE ... SET DEFAULT entry that depends on both the table and the sequence. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/build/mod.rs | 223 +++++++++++++++++++++++++++++++++++++++++- tests/build_parity.rs | 29 ++++++ tests/deploy.rs | 2 +- 3 files changed, 248 insertions(+), 6 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index 9ef2e27..bcbc8c3 100644 --- a/src/build/mod.rs +++ b/src/build/mod.rs @@ -26,6 +26,9 @@ //! `CREATE INDEX schema.name`, which PostgreSQL rejects) //! 11. Roles with `create: false` (e.g. PUBLIC) emit no entry (Python //! emitted CREATE ROLE anyway) +//! 12. CREATE/DROP FUNCTION references render schema-qualified (Python +//! emitted a bare name, which pg_restore rejects under its reset +//! `search_path` with "no schema has been selected to create in") mod acls; @@ -110,6 +113,7 @@ pub fn assemble(project: &Project) -> Result { entry.dependencies.extend(deps); } } + builder.split_sequence_defaults(project)?; let item_ids = builder .dump_id_map .iter() @@ -310,6 +314,78 @@ impl Builder { Ok(()) } + /// Emit the `ALTER TABLE ... SET DEFAULT nextval(...)` entries held + /// out of their `CREATE TABLE` by [`sequence_backed_default`]. Each + /// depends on both its table and the referenced sequence so + /// pg_restore orders it after both, breaking the SERIAL dependency + /// cycle the way pg_dump's separate `DEFAULT` TOC entries do. + fn split_sequence_defaults( + &mut self, + project: &Project, + ) -> Result<(), String> { + let mut sequence_ids: HashMap<(String, String), i32> = HashMap::new(); + for item in &project.inventory { + if let Definition::Sequence(s) = &item.definition + && let Some(&id) = self.dump_id_map.get(&item.id) + { + sequence_ids.insert((s.schema.clone(), s.name.clone()), id); + } + } + for item in &project.inventory { + let Definition::Table(d) = &item.definition else { + continue; + }; + // only the plain-column CREATE TABLE path renders defaults + // inline; sql-override, foreign, typed and LIKE tables do not + if d.sql.is_some() + || d.server.is_some() + || d.from_type.is_some() + || d.like_table.is_some() + { + continue; + } + let Some(&table_id) = self.dump_id_map.get(&item.id) else { + continue; + }; + for column in d.columns.as_deref().unwrap_or_default() { + let Some(default) = sequence_backed_default(column) else { + continue; + }; + let mut deps = vec![table_id]; + if let Some(key) = nextval_target(default) + && let Some(&seq_id) = sequence_ids.get(&key) + { + deps.push(seq_id); + } + let table = format!( + "{}.{}", + quote_ident(&d.schema), + quote_ident(&d.name) + ); + let col = quote_ident(&column.name); + let defn = vec![format!( + "ALTER TABLE ONLY {table} ALTER COLUMN {col} \ + SET DEFAULT {default}" + )]; + let drop = vec![format!( + "ALTER TABLE ONLY {table} ALTER COLUMN {col} \ + DROP DEFAULT" + )]; + self.add_entry( + "DEFAULT", + &d.schema, + &format!("{} {}", d.name, column.name), + &d.owner, + &defn, + &drop, + &deps, + None, + )?; + } + } + Ok(()) + } + /// `schema.name` with identifier quoting (ports _item_name) fn item_name(&self, item: &Item) -> String { let name = item.definition.name(); @@ -710,16 +786,27 @@ impl Builder { (func_name, Some(comment_target)) } }; + // pg_dump restores with an empty search_path and schema-qualifies + // every object name; a bare `CREATE FUNCTION name(...)` fails at + // restore with "no schema has been selected to create in". The + // COMMENT target is already qualified (via `namespace.tag`, or + // `comment_target` for the bare zero-arg case), so only the + // CREATE/DROP references need it here. + let qualified = if d.schema.is_empty() { + func_name.clone() + } else { + format!("{}.{}", quote_ident(&d.schema), func_name) + }; let mut create = vec![ "CREATE".into(), "FUNCTION".into(), - func_name.clone(), + qualified.clone(), "RETURNS".into(), d.returns.clone().unwrap_or_default(), "LANGUAGE".into(), d.language.clone().unwrap_or_default(), ]; - let drop = vec!["DROP FUNCTION IF EXISTS".into(), func_name.clone()]; + let drop = vec!["DROP FUNCTION IF EXISTS".into(), qualified.clone()]; if let Some(transform_types) = &d.transform_types { let tts: Vec = transform_types .iter() @@ -1211,7 +1298,16 @@ impl Builder { create.push("(".into()); let mut inner = Vec::new(); for column in d.columns.as_deref().unwrap_or_default() { - inner.push(render_table_column(column)); + // sequence-backed defaults are split into a separate + // SET DEFAULT entry (see split_sequence_defaults), so + // render the column without its default here + if sequence_backed_default(column).is_some() { + let mut stripped = column.clone(); + stripped.default = None; + inner.push(render_table_column(&stripped)); + } else { + inner.push(render_table_column(column)); + } } push_table_constraints(d, &mut inner); create.push(inner.join(", ")); @@ -2008,6 +2104,31 @@ pub(crate) fn render_default(value: &Value) -> String { postgres_value(value) } +/// A column default that references a sequence (`nextval(...)`, the +/// SERIAL form). pg_dump emits these as a separate `ALTER TABLE ... SET +/// DEFAULT` entry rather than inline on the column, because the sequence +/// is `OWNED BY` the column: inlining the default would make the table +/// depend on the sequence while the sequence depends on the table, an +/// unrestorable cycle. +fn sequence_backed_default(column: &Column) -> Option<&str> { + let default = column.default.as_ref()?.as_str()?; + default.contains("nextval(").then_some(default) +} + +/// Parse the `schema.name` a `nextval('schema.name'::regclass)` default +/// points at, so the split-out DEFAULT entry can depend on that sequence +fn nextval_target(default: &str) -> Option<(String, String)> { + let start = default.find('\'')? + 1; + let rest = &default[start..]; + let end = rest.find('\'')?; + let qualified = &rest[..end]; + let strip = |s: &str| s.trim_matches('"').to_string(); + match qualified.rsplit_once('.') { + Some((schema, name)) => Some((strip(schema), strip(name))), + None => Some((String::new(), strip(qualified))), + } +} + pub(crate) fn render_table_column(column: &Column) -> String { let mut sql = vec![column.name.clone(), column.data_type.clone()]; if let Some(collation) = &column.collation { @@ -2910,12 +3031,104 @@ mod tests { let (create, drop) = function_entry(&item); assert!( create.starts_with( - "CREATE FUNCTION bare_zero() RETURNS trigger LANGUAGE \ + "CREATE FUNCTION test.bare_zero() RETURNS trigger LANGUAGE \ plpgsql AS $$" ), "unexpected CREATE: {create}" ); - assert_eq!(drop, "DROP FUNCTION IF EXISTS bare_zero();\n"); + assert_eq!(drop, "DROP FUNCTION IF EXISTS test.bare_zero();\n"); + } + + #[test] + fn parses_nextval_target() { + assert_eq!( + nextval_target("nextval('test.widgets_id_seq'::regclass)"), + Some(("test".into(), "widgets_id_seq".into())) + ); + assert_eq!( + nextval_target("nextval('bare_seq'::regclass)"), + Some((String::new(), "bare_seq".into())) + ); + assert_eq!( + nextval_target("nextval('\"S\".\"Q\"'::regclass)"), + Some(("S".into(), "Q".into())) + ); + } + + #[test] + fn splits_sequence_backed_default_into_separate_entry() { + use std::path::PathBuf; + + use crate::models::Sequence; + + let mut col = column("id", "integer", true); + col.default = Some(json!("nextval('test.widgets_id_seq'::regclass)")); + let mut table = base_table("widgets"); + table.columns = Some(vec![col]); + let seq = Sequence { + name: "widgets_id_seq".into(), + schema: "test".into(), + owner: "app".into(), + sql: None, + data_type: None, + increment_by: None, + min_value: None, + max_value: None, + start_with: None, + cache: None, + cycle: None, + owned_by: Some("test.widgets.id".into()), + comment: None, + }; + let seq_item = Item { + id: 2, + desc: ObjectType::Sequence, + definition: Definition::Sequence(seq), + dependencies: BTreeSet::new(), + }; + let project = Project { + name: "t".into(), + encoding: "UTF-8".into(), + stdstrings: true, + superuser: "postgres".into(), + default_schema: "public".into(), + path: PathBuf::new(), + inventory: vec![table_item(1, table), seq_item], + }; + let output = assemble(&project).unwrap(); + let entries = output.dump.entries(); + // the CREATE TABLE must not inline the sequence default + let table_defn = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Table) + .and_then(|e| e.defn.clone()) + .expect("a TABLE entry"); + assert!( + !table_defn.contains("DEFAULT"), + "table inlined the sequence default: {table_defn}" + ); + // a separate DEFAULT entry carries it, depending on the sequence + let default_entry = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Default) + .expect("a DEFAULT entry"); + let defn = default_entry.defn.clone().unwrap(); + assert!( + defn.contains( + "ALTER TABLE ONLY test.widgets ALTER COLUMN id \ + SET DEFAULT nextval('test.widgets_id_seq'::regclass)" + ), + "unexpected DEFAULT defn: {defn}" + ); + let seq_dump_id = entries + .iter() + .find(|e| e.desc == libpgdump::ObjectType::Sequence) + .unwrap() + .dump_id; + assert!( + default_entry.dependencies.contains(&seq_dump_id), + "DEFAULT entry should depend on the sequence" + ); } #[test] diff --git a/tests/build_parity.rs b/tests/build_parity.rs index 3e68d54..7d258eb 100644 --- a/tests/build_parity.rs +++ b/tests/build_parity.rs @@ -66,6 +66,17 @@ const DEVIATIONS: &[(&str, &str, &str)] = &[ // argument signature required to disambiguate it ("COMMENT", "test", "(int4 AS timestamp)"), ("COMMENT", "test", "==="), + // Python emitted CREATE/DROP FUNCTION with a bare, unqualified + // name; pg_restore runs with search_path reset to '' and rejects it + // ("no schema has been selected to create in"), so the Rust build + // schema-qualifies the function reference + ("FUNCTION", "test", "disable_alter_domain()"), + ("FUNCTION", "test", "test_aggregate(integer, integer)"), + ( + "FUNCTION", + "test", + "utf8_to_latin1(integer, integer, cstring, internal, integer)", + ), ]; /// (desc, namespace, tag, required defn fragment) for the corrected @@ -162,6 +173,24 @@ const CORRECTED: &[(&str, &str, &str, &str)] = &[ "===", "COMMENT ON OPERATOR test.=== (box, box) IS", ), + ( + "FUNCTION", + "test", + "disable_alter_domain()", + "CREATE FUNCTION test.disable_alter_domain()", + ), + ( + "FUNCTION", + "test", + "test_aggregate(integer, integer)", + "CREATE FUNCTION test.test_aggregate(integer, integer)", + ), + ( + "FUNCTION", + "test", + "utf8_to_latin1(integer, integer, cstring, internal, integer)", + "CREATE FUNCTION test.utf8_to_latin1(IN source_encoding_id INTEGER,", + ), ]; /// (tag, comment) for COMMENT ON COLUMN entries: build now emits these diff --git a/tests/deploy.rs b/tests/deploy.rs index b1e8b84..2ce9eb3 100644 --- a/tests/deploy.rs +++ b/tests/deploy.rs @@ -130,7 +130,7 @@ fn table_alters_in_place_function_or_replaces() { "function must use CREATE OR REPLACE, not drop:\n{script}" ); assert!( - script.contains("CREATE OR REPLACE FUNCTION set_last_modified"), + script.contains("CREATE OR REPLACE FUNCTION test.set_last_modified"), "missing function OR REPLACE in:\n{script}" ); assert!( From 68d7cc6ed67b0b36475e0c7a90c637e967ca1c31 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 16:28:58 -0400 Subject: [PATCH 09/11] Resolve child-entry owners transitively in deploy Deploy mapped each archive entry to its owning inventory item by looking one dependency level deep. A COMMENT ON TRIGGER (and COMMENT ON INDEX) depends on the TRIGGER/INDEX entry, which is itself a child of the table rather than a modeled inventory item, so its owner set resolved empty and the entry was silently dropped: deploying a fresh table with a commented trigger lost the comment (caught by the equivalence gate). Walk the dependency graph through intermediate non-item entries until it reaches inventory items. This only recovers entries that were previously skipped for having no resolvable owner, so it cannot change any entry that already mapped to an item. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/deploy/mod.rs | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/deploy/mod.rs b/src/deploy/mod.rs index 5232491..765bafe 100644 --- a/src/deploy/mod.rs +++ b/src/deploy/mod.rs @@ -12,7 +12,7 @@ mod alter; mod diff; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap, HashSet}; use std::io::IsTerminal; use crate::deploy::alter::Resolution; @@ -211,6 +211,16 @@ fn plan( ); } } + // dump_id -> entry, for resolving a child entry's owning items + // through intermediate non-item entries (e.g. a COMMENT ON TRIGGER + // depends on the TRIGGER entry, which is itself a child of the table + // rather than a modeled inventory item) + let entries_by_id: HashMap = output + .dump + .entries() + .iter() + .map(|entry| (entry.dump_id, entry)) + .collect(); for entry in output.dump.entries() { if args.no_privileges && entry.desc == libpgdump::ObjectType::Acl { continue; @@ -218,11 +228,25 @@ fn plan( let direct = output.item_ids.get(&entry.dump_id); let owners: Vec = match direct { Some(id) => vec![*id], - None => entry - .dependencies - .iter() - .filter_map(|dep| output.item_ids.get(dep).copied()) - .collect(), + None => { + // walk the dependency graph until it reaches inventory + // items, so comments/ACLs on child entries still map to + // the object that owns them + let mut items = Vec::new(); + let mut seen = HashSet::new(); + let mut stack: Vec = entry.dependencies.clone(); + while let Some(dep) = stack.pop() { + if !seen.insert(dep) { + continue; + } + if let Some(id) = output.item_ids.get(&dep) { + items.push(*id); + } else if let Some(parent) = entries_by_id.get(&dep) { + stack.extend(parent.dependencies.iter().copied()); + } + } + items + } }; if owners.is_empty() { continue; From 1f0f6b3e6cccd0eb5fb475b956ff330657fadec9 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 16:29:05 -0400 Subject: [PATCH 10/11] Expand fixtures to lock in batch-4 bug fixes end-to-end Re-enable the objects batch 3 documented but had to leave out because the underlying bugs made them fail. Each now exercises a fix through the round-trip and deploy gates: - partition children via ATTACH PARTITION (pull fold) - a typed table with an inline column constraint - a zero-argument trigger function (build's `()` + schema qualification) - a trigger with a COMMENT ON TRIGGER (pull capture + deploy emission) - a view ordered alphabetically before its table (view dependency edges) - a SERIAL column (separate SET DEFAULT capture + build split) The trigger function body is authored in libpgfmt's normalized two-space form so the round-trip schema diff stays clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- fixtures/schema.sql | 132 ++++++++++++++++++++------------------------ 1 file changed, 61 insertions(+), 71 deletions(-) diff --git a/fixtures/schema.sql b/fixtures/schema.sql index f741341..0755357 100644 --- a/fixtures/schema.sql +++ b/fixtures/schema.sql @@ -65,49 +65,40 @@ CREATE MATERIALIZED VIEW user_states AS CREATE UNIQUE INDEX user_states_state ON user_states (state); --- Range-partitioned table, exercising the parent's PARTITION BY --- clause round-trip. --- --- Partition children (`CREATE TABLE child PARTITION OF parent FOR --- VALUES ...`, incl. a DEFAULT partition) were attempted here but are --- skipped: they uncovered a real, previously-unknown product bug. --- `pg_dump` never emits the inline `PARTITION OF ... FOR VALUES` form --- pglifecycle's parser handles (src/ddl/table.rs's `create_table`, --- matched via `kw_partition && kw_of`); it always splits partition --- attachment into a separate `ALTER TABLE ONLY parent ATTACH --- PARTITION child FOR VALUES ...` statement (TOC entry "Type: TABLE --- ATTACH"). No code anywhere in src/ddl/ or src/pull/ recognizes that --- ALTER TABLE subform, so it is silently dropped: the child comes --- back from `pull` as a plain, unattached table with no partition --- bound, and `build` has nothing to re-emit the ATTACH from. The --- existing pull unit test `merges_partition_children` --- (src/pull/mod.rs) only exercises the inline `PARTITION OF ... FOR --- VALUES` form, so it never caught this since that form doesn't --- occur in real `pg_dump` output. +-- Range-partitioned table with children, exercising the ATTACH +-- PARTITION round-trip. The children are authored inline with +-- `PARTITION OF ... FOR VALUES`, but `pg_dump` always re-emits them as +-- a plain `CREATE TABLE` plus a separate `ALTER TABLE ONLY parent +-- ATTACH PARTITION child FOR VALUES ...` (TOC entry "Type: TABLE +-- ATTACH"); pull now recognizes that form and folds the child back +-- into the parent's partitions (incl. a DEFAULT partition). CREATE TABLE events ( id BIGINT NOT NULL, created_at DATE NOT NULL, payload TEXT ) PARTITION BY RANGE (created_at); --- Typed table: composite type + CREATE TABLE OF. --- --- A column constraint (`CREATE TABLE locations OF point_2d --- (CONSTRAINT locations_x_check CHECK (x IS NOT NULL))`) was --- attempted here but is skipped: it uncovered another real product --- bug. `pg_dump` keeps a typed table's column constraint inline in --- the `CREATE TABLE ... OF type (...)` statement (unlike partition --- attachment above), but `create_table` (src/ddl/table.rs) only --- walks `TableElement` children to find constraints/columns; a typed --- table's parenthesized element list uses a different grammar --- production, so the CHECK constraint is silently dropped by pull --- and the round-trip loses it entirely. +CREATE TABLE events_2024 PARTITION OF events + FOR VALUES FROM ('2024-01-01') TO ('2025-01-01'); + +CREATE TABLE events_2025 PARTITION OF events + FOR VALUES FROM ('2025-01-01') TO ('2026-01-01'); + +CREATE TABLE events_default PARTITION OF events DEFAULT; + +-- Typed table: composite type + CREATE TABLE OF, with an inline +-- column constraint. `pg_dump` keeps a typed table's constraint inline +-- in the `CREATE TABLE ... OF type (...)` statement; pull now walks the +-- typed element list (a distinct grammar production) so the constraint +-- survives the round-trip. CREATE TYPE point_2d AS ( x DOUBLE PRECISION, y DOUBLE PRECISION ); -CREATE TABLE locations OF point_2d; +CREATE TABLE locations OF point_2d ( + CONSTRAINT locations_x_check CHECK (x IS NOT NULL) +); -- Table-level CHECK constraints CREATE TABLE products ( @@ -118,55 +109,54 @@ CREATE TABLE products ( CONSTRAINT products_quantity_nonneg CHECK (quantity >= 0) ); +-- View that sorts alphabetically BEFORE its underlying table +-- ("active_users" < "users"). Table, Sequence, View and ForeignTable +-- share libpgdump's priority tier, so without a recorded dependency +-- edge this view would restore before `users` and pg_restore would +-- fail with "relation ... does not exist". pull now derives view +-- dependency edges by re-parsing the stored query, so build orders it +-- after the tables it references. +CREATE VIEW active_users AS + SELECT id, name, surname FROM users WHERE state = 'verified'; + -- View with security_barrier and check_option. --- --- Named to sort alphabetically after `users`: build's dependency --- graph never records an edge from a view to the tables/views it --- queries (only tables get FK-derived `dependencies`, per --- src/pull/writer.rs's table_dependencies()), so a view with no --- recorded dependency is ordered purely by libpgdump's default --- same-priority (namespace, tag) sort -- Table, Sequence, View, and --- ForeignTable all share priority 22. A view alphabetically before --- its underlying table (e.g. "active_users" before "users") is --- restored first and pg_restore fails with "relation ... does not --- exist". This is a real, pre-existing gap (see commit message); it --- happens to be masked for user_states below because --- MATERIALIZED VIEW has its own, later, priority tier. CREATE VIEW verified_users WITH (security_barrier = true, check_option = 'local') AS SELECT id, name, surname FROM users WHERE state = 'verified'; +-- Zero-argument trigger function + trigger + trigger comment. Trigger +-- functions are always zero-arg, exercising build's `CREATE FUNCTION +-- name() RETURNS trigger` rendering (a missing `()` here is invalid +-- SQL); the `COMMENT ON TRIGGER` exercises pull's trigger-comment +-- match arm. +-- The body is authored in libpgfmt's normalized (two-space) form so the +-- round-trip gate's exact schema diff stays clean; pull reformats +-- function bodies through libpgfmt regardless of --style. +CREATE FUNCTION test.touch_last_modified() RETURNS trigger + LANGUAGE plpgsql AS $$ +BEGIN + NEW.last_modified_at := CURRENT_TIMESTAMP; + RETURN NEW; +END; +$$; + +CREATE TRIGGER users_touch_last_modified + BEFORE UPDATE ON users + FOR EACH ROW EXECUTE FUNCTION test.touch_last_modified(); + +COMMENT ON TRIGGER users_touch_last_modified ON users IS + 'Maintains last_modified_at on update'; + -- Per-object COMMENT: column --- --- A trigger + trigger-comment addition was attempted here but is --- skipped: it uncovered two real product bugs rather than a fixture --- issue: --- 1. src/pull/mod.rs `apply_comment` has no match arm for TRIGGER --- (or RULE/POLICY/CONSTRAINT), so `COMMENT ON TRIGGER ... ON t` --- always logs "Comment on unmatched object" and the comment is --- silently dropped. --- 2. src/build/mod.rs `dump_function` (and the pull-side --- src/ddl/function.rs, which no longer appends `()` to a --- zero-parameter function's name as the Python tokenizer did) --- renders zero-argument functions as `CREATE FUNCTION name --- RETURNS ...` with no parameter list, which is invalid SQL and --- fails pg_restore. This affects any zero-arg function, --- including the trigger functions PostgreSQL requires. COMMENT ON COLUMN users.display_name IS 'Optional user-facing display name'; -- Bare `public` schema reference, exercising case-folding of an --- unquoted `public` identifier. --- --- Uses a plain integer primary key rather than SERIAL: a SERIAL --- column's DEFAULT is emitted by pg_dump as a separate, later --- `ALTER TABLE ONLY public.widgets ALTER COLUMN id SET DEFAULT --- nextval(...)` statement (TOC entry "Type: DEFAULT"), which is --- another real, previously-unknown bug -- pull only captures a --- column's default when it is inline in the CREATE TABLE statement --- itself, so the sequence-owned default is silently dropped and the --- round-trip loses it. +-- unquoted `public` identifier. Uses a SERIAL primary key: pg_dump +-- emits the column default as a separate, later `ALTER TABLE ONLY +-- public.widgets ALTER COLUMN id SET DEFAULT nextval(...)` (TOC entry +-- "Type: DEFAULT"), which pull now folds back onto the column. CREATE TABLE public.widgets ( - id INTEGER PRIMARY KEY, + id SERIAL PRIMARY KEY, name TEXT NOT NULL ); From 7026ccaa66bc1b98b4b1e5820c1762dcdd954f4a Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 16:44:35 -0400 Subject: [PATCH 11/11] Capture index_tablespace for typed-table constraints on pull MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The TypedTableElement loop applied table constraints via apply_constraint but never captured OptConsTableSpace into table.index_tablespace, unlike the sibling TableElement loop. A typed table's PRIMARY KEY/UNIQUE ... USING INDEX TABLESPACE constraint silently lost its tablespace on pull — the same round-trip data loss class this branch fixes elsewhere. Mirror the sibling loop: first constraint carrying a tablespace wins. Reported by CodeRabbit. just check and just gates pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/table.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/ddl/table.rs b/src/ddl/table.rs index c8bca11..287b545 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -150,6 +150,15 @@ pub(crate) fn create_table( } else if let Some(constraint) = element.child_of_kind("TableConstraint") { + // USING INDEX TABLESPACE, on PRIMARY KEY/UNIQUE/EXCLUDE — + // same single table-level index_tablespace as the + // TableElement loop above; first constraint carrying one wins + if table.index_tablespace.is_none() { + table.index_tablespace = constraint + .find("OptConsTableSpace") + .and_then(|n| n.find("name")) + .map(|n| unquote(n.text(src))); + } let (name, parsed) = table_constraint(&constraint, src)?; apply_constraint(&mut table, name, parsed); }