Batch 3: dollar-quote safety, efficiency/dedup cleanups, and gate-fixture coverage

fixtures/schema.sql

@@ -64,3 +64,109 @@ CREATE MATERIALIZED VIEW user_states AS
     SELECT state, count(*) AS total FROM users GROUP BY state;
 
 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.
+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 TYPE point_2d AS (
+    x DOUBLE PRECISION,
+    y DOUBLE PRECISION
+);
+
+CREATE TABLE locations OF point_2d;
+
+-- Table-level CHECK constraints
+CREATE TABLE products (
+    id       UUID    NOT NULL DEFAULT uuid_generate_v4() PRIMARY KEY,
+    price    NUMERIC NOT NULL,
+    quantity INTEGER NOT NULL,
+    CONSTRAINT products_price_positive CHECK (price >= 0),
+    CONSTRAINT products_quantity_nonneg CHECK (quantity >= 0)
+);
+
+-- 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';
+
+-- 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.
+CREATE TABLE public.widgets (
+    id   INTEGER PRIMARY KEY,
+    name TEXT NOT NULL
+);

src/build/acls.rs

@@ -8,7 +8,7 @@
 //! object's namespace and owner, and a dependency edge on the object's
 //! entry so the topological sort restores ACLs after their objects.
 
-use std::collections::BTreeMap;
+use std::collections::{BTreeMap, HashMap};
 
 use serde_json::{Map, Value};
 
@@ -81,6 +81,9 @@ pub(super) fn dump_acls(
             collect(&mut objects, acls, &role, false);
         }
     }
+    // build the lookup index once rather than rescanning the whole
+    // inventory for every ACL group below
+    let index = ObjectIndex::build(project);
     for ((section, object), acl) in &objects {
         let (key, keyword, dep_types) = SECTIONS[*section];
         // column grants attach to their table's entry
@@ -105,7 +108,7 @@ pub(super) fn dump_acls(
         let mut owner = builder.superuser.clone();
         let mut dependencies = Vec::new();
         if !dep_types.is_empty() {
-            match find_object(builder, project, dep_types, &target) {
+            match find_object(builder, &index, dep_types, &target) {
                 Some((dump_id, item_owner)) => {
                     dependencies.push(dump_id);
                     if let Some(item_owner) = item_owner {
@@ -248,10 +251,59 @@ pub(crate) fn statement(
     }
 }
 
+/// Lookup indexes over the project inventory, built once per build so
+/// each ACL group's object lookup is O(1) instead of a full inventory
+/// scan (previously repeated per group, and twice over for functions)
+struct ObjectIndex<'a> {
+    /// (desc, schema — `None` for schemaless descs, name) → item; a
+    /// name collision across desc/schema is not possible for valid
+    /// PostgreSQL objects, so keeping the first item seen is equivalent
+    /// to the original full-scan `find`
+    objects: HashMap<(ObjectType, Option<&'a str>, String), &'a Item>,
+    /// (schema, identity signature) → item, for exact function matches
+    functions_by_identity: HashMap<(Option<&'a str>, String), &'a Item>,
+    /// (schema, bare name) → matching items, for the unambiguous
+    /// bare-name fallback
+    functions_by_name: HashMap<(Option<&'a str>, String), Vec<&'a Item>>,
+}
+
+impl<'a> ObjectIndex<'a> {
+    fn build(project: &'a Project) -> Self {
+        let mut objects = HashMap::new();
+        let mut functions_by_identity = HashMap::new();
+        let mut functions_by_name: HashMap<_, Vec<&Item>> = HashMap::new();
+        for item in &project.inventory {
+            let schema = item.definition.schema();
+            let key_schema = if item.desc.is_schemaless() {
+                None
+            } else {
+                schema
+            };
+            objects
+                .entry((item.desc, key_schema, item.definition.name()))
+                .or_insert(item);
+            if let Definition::Function(f) = &item.definition {
+                functions_by_identity
+                    .entry((schema, f.identity()))
+                    .or_insert(item);
+                functions_by_name
+                    .entry((schema, item.definition.name()))
+                    .or_default()
+                    .push(item);
+            }
+        }
+        ObjectIndex {
+            objects,
+            functions_by_identity,
+            functions_by_name,
+        }
+    }
+}
+
 /// Find the granted-on object's entry id and owner
 fn find_object(
     builder: &Builder,
-    project: &Project,
+    index: &ObjectIndex,
     descs: &[ObjectType],
     object: &str,
 ) -> Option<(i32, Option<String>)> {
@@ -260,13 +312,14 @@ fn find_object(
         None => (None, object),
     };
     let item = if descs == [ObjectType::Function] {
-        find_function(project, schema, name)
+        find_function(index, schema, name)
     } else {
-        project.inventory.iter().find(|item| {
-            descs.contains(&item.desc)
-                && item.definition.name() == name
-                && (item.desc.is_schemaless()
-                    || item.definition.schema() == schema)
+        descs.iter().find_map(|desc| {
+            let key_schema = if desc.is_schemaless() { None } else { schema };
+            index
+                .objects
+                .get(&(*desc, key_schema, name.to_string()))
+                .copied()
         })
     }?;
     let dump_id = builder.dump_id_map.get(&item.id)?;
@@ -277,23 +330,18 @@ fn find_object(
 /// signature exactly, falling back to the bare name only when it is
 /// unambiguous, so overloads never bind to the wrong entry
 fn find_function<'a>(
-    project: &'a Project,
+    index: &ObjectIndex<'a>,
     schema: Option<&str>,
     name: &str,
 ) -> Option<&'a Item> {
-    let functions = project.inventory.iter().filter(|item| {
-        item.desc == ObjectType::Function && item.definition.schema() == schema
-    });
-    if let Some(item) = functions.clone().find(|item| {
-        matches!(&item.definition, Definition::Function(f)
-            if f.identity() == name)
-    }) {
-        return Some(item);
+    if let Some(item) =
+        index.functions_by_identity.get(&(schema, name.to_string()))
+    {
+        return Some(*item);
     }
     let base = name.split('(').next().unwrap_or(name);
-    let mut matches = functions.filter(|item| item.definition.name() == base);
-    let item = matches.next()?;
-    matches.next().is_none().then_some(item)
+    let matches = index.functions_by_name.get(&(schema, base.to_string()))?;
+    (matches.len() == 1).then(|| matches[0])
 }
 
 fn section<'a>(acls: &'a Acls, key: &str) -> Option<&'a Map<String, Value>> {

src/build/mod.rs

@@ -41,7 +41,7 @@ use crate::models::{
 use crate::progress;
 use crate::project::Project;
 use crate::utils::{
-    postgres_value, quote_ident, raw_value, user_mapping_subject,
+    dollar_quote, postgres_value, quote_ident, raw_value, user_mapping_subject,
 };
 
 pub fn build(project: &Project, destination: &Path) -> Result<(), String> {
@@ -295,7 +295,7 @@ impl Builder {
             desc.to_string(),
             name,
             String::from("IS"),
-            format!("$${comment}$$;\n"),
+            format!("{};\n", dollar_quote(comment)),
         ];
         self.add_entry(
             "COMMENT",

src/ddl/acl.rs

@@ -6,7 +6,7 @@ use tree_sitter::Node;
 use crate::ddl::object::{string_value, unstring};
 use crate::ddl::{
     Acl, AclTarget, NodeExt, Privilege, RoleDef, Statement, any_name,
-    qualified_name, truncate, unquote, unquote_role,
+    column_elems, qualified_name, truncate, unquote, unquote_role,
 };
 use crate::models::RoleOptions;
 
@@ -185,11 +185,7 @@ fn privileges(node: &Node, src: &str) -> Vec<Privilege> {
         .find_all("privilege")
         .iter()
         .map(|p| {
-            let columns: Vec<String> = p
-                .find_all("columnElem")
-                .iter()
-                .map(|c| unquote(c.text(src)))
-                .collect();
+            let columns: Vec<String> = column_elems(p, src);
             let name = match p.child_of_kind("opt_column_list") {
                 Some(list) => p.text(src)
                     [..list.start_byte() - p.start_byte()]

src/ddl/function.rs

@@ -100,46 +100,59 @@ pub(crate) fn create_function(
 }
 
 fn apply_common_option(function: &mut Function, node: &Node, src: &str) {
-    if node.has("kw_immutable") {
+    // every branch but the SET clause is keyed on a keyword that's a
+    // *direct* child of `common_func_opt_item` (one alternative per
+    // grammar production), so a single pass over the direct children
+    // replaces what were repeated recursive `has()` walks of the same
+    // small subtree
+    let mut cursor = node.walk();
+    let kinds: Vec<&str> =
+        node.children(&mut cursor).map(|c| c.kind()).collect();
+    let has = |kind: &str| kinds.contains(&kind);
+
+    if has("kw_immutable") {
         function.immutable = Some(true);
-    } else if node.has("kw_stable") {
+    } else if has("kw_stable") {
         function.stable = Some(true);
-    } else if node.has("kw_volatile") {
+    } else if has("kw_volatile") {
         function.volatile = Some(true);
-    } else if node.has("kw_leakproof") {
-        function.leak_proof = Some(!node.has("kw_not"));
-    } else if node.has("kw_strict") {
+    } else if has("kw_leakproof") {
+        function.leak_proof = Some(!has("kw_not"));
+    } else if has("kw_strict") {
         function.strict = Some(true);
-    } else if node.has("kw_called") {
+    } else if has("kw_called") {
         function.called_on_null_input = Some(true);
-    } else if node.has("kw_null") && node.has("kw_returns") {
+    } else if has("kw_null") && has("kw_returns") {
         function.called_on_null_input = Some(false);
-    } else if node.has("kw_security") {
+    } else if has("kw_security") {
         function.security = Some(
-            if node.has("kw_definer") {
+            if has("kw_definer") {
                 "DEFINER"
             } else {
                 "INVOKER"
             }
             .to_string(),
         );
-    } else if node.has("kw_parallel") {
+    } else if has("kw_parallel") {
         function.parallel = node
             .child_of_kind("ColId")
             .map(|n| n.text(src).to_uppercase());
-    } else if node.has("kw_cost") {
+    } else if has("kw_cost") {
         function.cost = node
-            .find("NumericOnly")
+            .child_of_kind("NumericOnly")
             .and_then(|n| n.text(src).parse().ok());
-    } else if node.has("kw_rows") {
+    } else if has("kw_rows") {
         function.rows = node
-            .find("NumericOnly")
+            .child_of_kind("NumericOnly")
             .and_then(|n| n.text(src).parse().ok());
-    } else if node.has("kw_support") {
+    } else if has("kw_support") {
         function.support = node
             .child_of_kind("any_name")
             .map(|n| n.text(src).to_string());
     } else if node.has("kw_set")
+        // `kw_set`/`set_rest_more` live inside a nested
+        // `FunctionSetResetClause` child, not directly under this
+        // node, so this one genuinely needs the recursive search
         && let Some(config) = node.find("set_rest_more")
     {
         let text = config.text(src);

src/ddl/mod.rs

@@ -388,6 +388,16 @@ pub(crate) fn unquote(value: &str) -> String {
     }
 }
 
+/// Collect a node's `columnElem` descendants as unquoted, case-folded
+/// column names (`columnList`, `opt_c_include`, etc. all wrap a flat or
+/// left-recursive list of `columnElem`)
+pub(crate) fn column_elems(node: &Node, src: &str) -> Vec<String> {
+    node.find_all("columnElem")
+        .iter()
+        .map(|c| unquote(c.text(src)))
+        .collect()
+}
+
 /// Like [`unquote`], but preserves the pseudo-role keyword `PUBLIC` as
 /// the canonical uppercase literal. Use only for role references
 /// (grantees, USER MAPPING subjects) where `PUBLIC` is a keyword rather