From 27279b9ab9f6226c81de3e0d5049645d27298baa Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:29:19 -0400 Subject: [PATCH 1/7] Index inventory items by (desc, schema, name) to avoid quadratic lookups apply_cached_dependencies and add_definition's duplicate-object guard both scanned the full inventory linearly via lookup_item, making dependency resolution O(n^2) in project size. Maintain a HashMap keyed by (desc, namespace, tag) incrementally as items are added and resolve both the duplicate check and dependency edges through it instead. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/project/load.rs | 59 ++++++++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 17 deletions(-) diff --git a/src/project/load.rs b/src/project/load.rs index d7281c1..961d196 100644 --- a/src/project/load.rs +++ b/src/project/load.rs @@ -1,6 +1,6 @@ //! The project directory loader (ports the load half of project.py) -use std::collections::BTreeSet; +use std::collections::{BTreeSet, HashMap}; use std::path::{Path, PathBuf}; use serde_json::Value; @@ -28,6 +28,14 @@ pub struct Loader { /// `project.inventory`, kept for duplicate-object diagnostics paths: Vec>, errors: usize, + /// `(desc, namespace, tag)` -> inventory index, maintained + /// incrementally as items are added so both the duplicate-object + /// guard in `add_definition` and dependency resolution in + /// `apply_cached_dependencies` can look items up in O(1) instead of + /// scanning the inventory. Schemaless types are always keyed with a + /// `None` namespace, matching `lookup_item`'s old comparison which + /// ignored namespace for them. + index: HashMap<(ObjectType, Option, String), usize>, } impl Loader { @@ -45,6 +53,7 @@ impl Loader { cached_dependencies: Vec::new(), paths: Vec::new(), errors: 0, + index: HashMap::new(), } } @@ -294,7 +303,7 @@ impl Loader { fn apply_cached_dependencies(&mut self) -> Result<(), String> { for dep in &self.cached_dependencies { let Some(item) = lookup_item( - &self.project.inventory, + &self.index, dep.desc, dep.namespace.as_deref(), &dep.tag, @@ -314,7 +323,7 @@ impl Loader { // manage (e.g. a foreign key or inheritance parent owned by // an extension); skip the edge rather than failing the load let Some(parent) = lookup_item( - &self.project.inventory, + &self.index, dep.parent_desc, Some(&dep.parent_namespace), &dep.parent_tag, @@ -367,7 +376,7 @@ impl Loader { return; } if let Some(existing) = lookup_item( - &self.project.inventory, + &self.index, ot, definition.schema(), &definition.name(), @@ -389,8 +398,11 @@ impl Loader { self.errors += 1; return; } + let id = self.project.inventory.len(); + let key = index_key(ot, definition.schema(), &definition.name()); + self.index.insert(key, id); self.project.inventory.push(Item { - id: self.project.inventory.len(), + id, desc: ot, definition, dependencies: BTreeSet::new(), @@ -399,23 +411,32 @@ impl Loader { } } -/// Find an inventory item by type, schema, and name -/// (project.py _lookup_item) +/// Build the `index` key for a `(desc, namespace, tag)` triple, +/// normalizing the namespace to `None` for schemaless types so lookups +/// and insertions always agree regardless of what the caller passed +/// (project.py _lookup_item ignored namespace entirely in that case) +fn index_key( + desc: ObjectType, + namespace: Option<&str>, + tag: &str, +) -> (ObjectType, Option, String) { + let namespace = if desc.is_schemaless() { + None + } else { + namespace.map(str::to_string) + }; + (desc, namespace, tag.to_string()) +} + +/// Find an inventory item by type, schema, and name via the loader's +/// `(desc, namespace, tag)` index (project.py _lookup_item) fn lookup_item( - inventory: &[Item], + index: &HashMap<(ObjectType, Option, String), usize>, desc: ObjectType, namespace: Option<&str>, tag: &str, ) -> Option { - inventory - .iter() - .find(|item| { - item.desc == desc - && item.definition.name() == tag - && (desc.is_schemaless() - || item.definition.schema() == namespace) - }) - .map(|item| item.id) + index.get(&index_key(desc, namespace, tag)).copied() } fn to_definition( @@ -573,6 +594,10 @@ mod tests { }), dependencies: BTreeSet::new(), }); + loader.index.insert( + index_key(ObjectType::Extension, Some("public"), "myext"), + 0, + ); let mut entry = json!({ "source_type": "int4", From b1639bf78f88d190be3c84efadde08792b4dfb1a Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:30:56 -0400 Subject: [PATCH 2/7] Fix collision-unsafe dollar-quoting and dedup deploy comment/options deltas Comments and string values wrapped in bare $$...$$ could be terminated early by an embedded $$ in the body (values come from YAML or a live database). Add utils::dollar_quote, which picks a pg_dump-style tag ($$, then $c1$, $c2$, ...) that does not occur in the body, and route build::add_comment, deploy::alter::comment_on, and utils::render_value through it. Also extract push_comment and push_options helpers in deploy/alter.rs to replace the repeated "if repo.x != db.x { alters.push(...) }" blocks across the table/column/sequence/domain/enum/extension/schema/ foreign-table/fdw/server/user-mapping resolvers. Pure refactor: emitted SQL and --allow-drop gating are unchanged since every statement still flows through the shared Alter::new/Alter::destructive constructors. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/build/mod.rs | 4 +- src/deploy/alter.rs | 186 ++++++++++++++++++++++---------------------- src/utils.rs | 39 +++++++++- 3 files changed, 131 insertions(+), 98 deletions(-) diff --git a/src/build/mod.rs b/src/build/mod.rs index 621f1eb..7a75961 100644 --- a/src/build/mod.rs +++ b/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", diff --git a/src/deploy/alter.rs b/src/deploy/alter.rs index 16d37be..98799ec 100644 --- a/src/deploy/alter.rs +++ b/src/deploy/alter.rs @@ -17,7 +17,9 @@ use crate::models::{ ForeignDataWrapper, ForeignKey, Function, Index, Schema, Sequence, Server, Table, Trigger, Type, UserMapping, View, ViewColumn, }; -use crate::utils::{postgres_value, quote_ident, user_mapping_subject}; +use crate::utils::{ + dollar_quote, postgres_value, quote_ident, user_mapping_subject, +}; /// One reconciliation statement pub(crate) struct Alter { @@ -220,13 +222,7 @@ fn table(repo: &Table, db: &Table) -> Resolution { return Resolution::Replace; } indexes(&name, repo, db, &mut alters); - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "TABLE", - &name, - repo.comment.as_deref(), - ))); - } + push_comment(&mut alters, "TABLE", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -327,13 +323,13 @@ fn alter_column( ) })); } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "COLUMN", - &format!("{table}.{column}"), - repo.comment.as_deref(), - ))); - } + push_comment( + alters, + "COLUMN", + &format!("{table}.{column}"), + &repo.comment, + &db.comment, + ); true } @@ -573,13 +569,7 @@ fn sequence(repo: &Sequence, db: &Sequence) -> Resolution { clauses.join(" ") ))); } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "SEQUENCE", - &name, - repo.comment.as_deref(), - ))); - } + push_comment(&mut alters, "SEQUENCE", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -608,13 +598,7 @@ fn domain(repo: &Domain, db: &Domain) -> Resolution { None => format!("ALTER DOMAIN {name} DROP DEFAULT;\n"), })); } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "DOMAIN", - &name, - repo.comment.as_deref(), - ))); - } + push_comment(&mut alters, "DOMAIN", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -642,13 +626,7 @@ fn enum_type(repo: &Type, db: &Type) -> Resolution { )) }) .collect(); - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "TYPE", - &name, - repo.comment.as_deref(), - ))); - } + push_comment(&mut alters, "TYPE", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -672,13 +650,7 @@ fn extension(repo: &Extension, db: &Extension) -> Resolution { quote_ident(schema) ))); } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "EXTENSION", - &name, - repo.comment.as_deref(), - ))); - } + push_comment(&mut alters, "EXTENSION", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -689,13 +661,13 @@ fn schema(repo: &Schema, db: &Schema) -> Resolution { return Resolution::Replace; } let mut alters = Vec::new(); - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "SCHEMA", - "e_ident(&repo.name), - repo.comment.as_deref(), - ))); - } + push_comment( + &mut alters, + "SCHEMA", + "e_ident(&repo.name), + &repo.comment, + &db.comment, + ); Resolution::Statements(alters) } @@ -714,18 +686,19 @@ fn foreign_table(repo: &Table, db: &Table) -> Resolution { } let name = qualified(&repo.schema, &repo.name); let mut alters = Vec::new(); - if let Some(clause) = options_delta(&repo.options, &db.options) { - alters.push(Alter::new(format!( - "ALTER FOREIGN TABLE {name} OPTIONS ({clause});\n" - ))); - } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "FOREIGN TABLE", - &name, - repo.comment.as_deref(), - ))); - } + push_options( + &mut alters, + &format!("ALTER FOREIGN TABLE {name}"), + &repo.options, + &db.options, + ); + push_comment( + &mut alters, + "FOREIGN TABLE", + &name, + &repo.comment, + &db.comment, + ); Resolution::Statements(alters) } @@ -768,18 +741,19 @@ fn fdw(repo: &ForeignDataWrapper, db: &ForeignDataWrapper) -> Resolution { } })); } - if let Some(clause) = options_delta(&repo.options, &db.options) { - alters.push(Alter::new(format!( - "ALTER FOREIGN DATA WRAPPER {name} OPTIONS ({clause});\n" - ))); - } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "FOREIGN DATA WRAPPER", - &name, - repo.comment.as_deref(), - ))); - } + push_options( + &mut alters, + &format!("ALTER FOREIGN DATA WRAPPER {name}"), + &repo.options, + &db.options, + ); + push_comment( + &mut alters, + "FOREIGN DATA WRAPPER", + &name, + &repo.comment, + &db.comment, + ); Resolution::Statements(alters) } @@ -803,18 +777,13 @@ fn server(repo: &Server, db: &Server) -> Resolution { string_literal(version) ))); } - if let Some(clause) = options_delta(&repo.options, &db.options) { - alters.push(Alter::new(format!( - "ALTER SERVER {name} OPTIONS ({clause});\n" - ))); - } - if repo.comment != db.comment { - alters.push(Alter::new(comment_on( - "SERVER", - &name, - repo.comment.as_deref(), - ))); - } + push_options( + &mut alters, + &format!("ALTER SERVER {name}"), + &repo.options, + &db.options, + ); + push_comment(&mut alters, "SERVER", &name, &repo.comment, &db.comment); Resolution::Statements(alters) } @@ -841,14 +810,12 @@ fn user_mapping(repo: &UserMapping, db: &UserMapping) -> Resolution { // does not carry one must not drop the live credential let db_options = keep_redacted_password(&server.options, &existing.options); - if let Some(clause) = - options_delta(&server.options, &db_options) - { - alters.push(Alter::new(format!( - "ALTER USER MAPPING FOR {user} SERVER {name} \ - OPTIONS ({clause});\n" - ))); - } + push_options( + &mut alters, + &format!("ALTER USER MAPPING FOR {user} SERVER {name}"), + &server.options, + &db_options, + ); } } } @@ -947,12 +914,41 @@ fn comment_delta( fn comment_on(desc: &str, name: &str, comment: Option<&str>) -> String { match comment { Some(comment) => { - format!("COMMENT ON {desc} {name} IS $${comment}$$;\n") + format!("COMMENT ON {desc} {name} IS {};\n", dollar_quote(comment)) } None => format!("COMMENT ON {desc} {name} IS NULL;\n"), } } +/// Push a `COMMENT ON` alter for `desc`/`name` when `repo` and `db` +/// comments differ. Shared by every resolver whose only comment +/// handling is this exact comparison. +fn push_comment( + alters: &mut Vec, + desc: &str, + name: &str, + repo: &Option, + db: &Option, +) { + if repo != db { + alters.push(Alter::new(comment_on(desc, name, repo.as_deref()))); + } +} + +/// Push an `ALTER ... OPTIONS (...)` alter when `repo` and `db` +/// options differ. `prefix` is the full `ALTER ` clause +/// preceding `OPTIONS`. +fn push_options( + alters: &mut Vec, + prefix: &str, + repo: &Option>, + db: &Option>, +) { + if let Some(clause) = options_delta(repo, db) { + alters.push(Alter::new(format!("{prefix} OPTIONS ({clause});\n"))); + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/utils.rs b/src/utils.rs index 0a83241..a7bc262 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -150,7 +150,7 @@ pub fn postgres_value(value: &Value) -> String { fn render_value(value: &Value, nested: bool) -> String { match value { - Value::String(s) if s.contains('\'') => format!("$${s}$$"), + Value::String(s) if s.contains('\'') => dollar_quote(s), Value::String(s) => format!("'{s}'"), Value::Array(items) => { let inner: Vec = @@ -167,6 +167,23 @@ fn render_value(value: &Value, nested: bool) -> String { } } +/// Wrap `body` in a dollar-quoted string literal, choosing a tag that +/// does not occur in `body` (pg_dump style: `$$` when possible, +/// otherwise `$c1$`, `$c2$`, ... until the tag is collision-free). +pub(crate) fn dollar_quote(body: &str) -> String { + if !body.contains("$$") { + return format!("$${body}$$"); + } + let mut n = 1; + loop { + let tag = format!("$c{n}$"); + if !body.contains(&tag) { + return format!("{tag}{body}{tag}"); + } + n += 1; + } +} + /// Render a value the way Python's `str()` does inside string /// interpolation (no quoting) — used for storage parameters and /// trigger arguments @@ -224,4 +241,24 @@ mod tests { assert_eq!(postgres_value(&json!(true)), "True"); assert_eq!(postgres_value(&json!(["a", ["b"]])), "ARRAY['a', ['b']]"); } + + #[test] + fn dollar_quote_uses_bare_delimiter_when_safe() { + assert_eq!(dollar_quote("it's fine"), "$$it's fine$$"); + } + + #[test] + fn dollar_quote_avoids_embedded_dollar_pair() { + let body = "has a $$ inside"; + let quoted = dollar_quote(body); + assert_eq!(quoted, format!("$c1${body}$c1$")); + assert!(!body.contains("$c1$")); + } + + #[test] + fn dollar_quote_escalates_past_a_taken_tag() { + let body = "has $$ and $c1$ both"; + let quoted = dollar_quote(body); + assert_eq!(quoted, format!("$c2${body}$c2$")); + } } From b40fa690a9e9feed6c06a899de57efe97bdbbfe9 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:33:51 -0400 Subject: [PATCH 3/7] Index ACL object lookups and fix --log-file disabling progress bars - build/acls.rs: build a HashMap-backed ObjectIndex once per build (generic desc/schema/name lookups, plus function exact-identity and bare-name indexes) instead of scanning the full inventory per ACL group and scanning it twice for functions. Matching semantics are preserved: schemaless descs still ignore the parsed schema, function identity matches exactly first, and the bare-name fallback still requires the match to be unambiguous. - main.rs: initialize progress::init() unconditionally in configure_logging, before checking --log-file, so progress bars are no longer disabled on a TTY just because logs are being written to a file. Non-file logging behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/build/acls.rs | 90 ++++++++++++++++++++++++++++++++++++----------- src/main.rs | 8 ++++- 2 files changed, 76 insertions(+), 22 deletions(-) diff --git a/src/build/acls.rs b/src/build/acls.rs index 1a08d1e..9e1f66d 100644 --- a/src/build/acls.rs +++ b/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)> { @@ -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> { diff --git a/src/main.rs b/src/main.rs index 322e7e4..7c9c2ba 100644 --- a/src/main.rs +++ b/src/main.rs @@ -61,6 +61,10 @@ fn configure_logging(args: &cli::Cli) { } else { log::LevelFilter::Warn }; + // initialize progress rendering unconditionally (bars go to stderr + // regardless of where logs go) so `--log-file` doesn't disable bars + // on a TTY + let multi = progress::init(); if let Some(path) = &args.log_file { if let Ok(handle) = std::fs::File::create(path) { if let Err(err) = simplelog::WriteLogger::init( @@ -70,6 +74,8 @@ fn configure_logging(args: &cli::Cli) { ) { eprintln!("warning: failed to initialize file logging: {err}"); } else { + // logs go to the file, so there is no risk of them + // corrupting the bars; no bridge needed return; } } else { @@ -84,7 +90,7 @@ fn configure_logging(args: &cli::Cli) { simplelog::TerminalMode::Stderr, simplelog::ColorChoice::Auto, ); - match progress::init() { + match multi { // on a terminal, route log records through the progress bridge // so they print above any live bars instead of corrupting them Some(multi) => { From 34e600a19919b37df85e3509401a27b960ea707c Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:35:56 -0400 Subject: [PATCH 4/7] =?UTF-8?q?Fix=20O(n=C2=B2)=20pull=20ingest,=20functio?= =?UTF-8?q?n=20filename=20collisions,=20and=20duplicate=20reapers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - mod.rs: index tables/materialized_views/indexes by (schema, name) in HashMaps maintained during ingest instead of doing a linear scan per index/constraint/trigger/comment merge, and per INDEX comment; ingest merge results are unchanged, only the lookup cost. - writer.rs: skip counter-suffixed filenames that collide with another function's real name when disambiguating overloads, so a real `fn_1` is never displaced into `fn_1_1.yaml` by an `fn` overload. Adds a unit test for the `fn`, `fn`, `fn_1` scenario. - writer.rs/update.rs: replace the two divergent empty-directory reapers with one shared `writer::remove_empty_directories`, keeping the safer symlink-skipping walk (update.rs's behavior) for both the fresh-write and `--update --prune` paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/pull/mod.rs | 106 ++++++++++++++++++++++++---------- src/pull/update.rs | 49 +--------------- src/pull/writer.rs | 138 ++++++++++++++++++++++++++++++++++++++------- 3 files changed, 197 insertions(+), 96 deletions(-) diff --git a/src/pull/mod.rs b/src/pull/mod.rs index 8157bd1..03e996a 100644 --- a/src/pull/mod.rs +++ b/src/pull/mod.rs @@ -9,7 +9,7 @@ mod update; mod writer; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::fmt::Write; use std::io::IsTerminal; use std::path::Path; @@ -396,6 +396,26 @@ 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)>, + /// (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 + table_index: HashMap<(String, String), usize>, + /// (schema, name) -> index into `materialized_views`, mirroring + /// `table_index` + matview_index: HashMap<(String, String), usize>, + /// (schema, index name) -> where the index currently lives, kept + /// in sync as indexes are attached so `COMMENT ON INDEX` does not + /// need to scan every index in every table + index_location: HashMap<(String, String), IndexLocation>, +} + +/// Where an ingested index currently lives, for `index_location` +/// lookups +#[derive(Debug, Clone, Copy)] +enum IndexLocation { + Table(usize), + MaterializedView(usize), + Deferred(usize), } impl Assembly { @@ -616,6 +636,10 @@ impl Assembly { } Statement::CreateTable(mut table) => { table.owner = owner; + self.table_index.insert( + (table.schema.clone(), table.name.clone()), + self.tables.len(), + ); self.tables.push(*table); } Statement::CreateTablePartition { parent, partition } => { @@ -644,6 +668,10 @@ impl Assembly { } Statement::CreateMaterializedView(mut view) => { view.owner = owner; + self.matview_index.insert( + (view.schema.clone(), view.name.clone()), + self.materialized_views.len(), + ); self.materialized_views.push(view); } Statement::CreateFunction(mut function) => { @@ -668,15 +696,36 @@ impl Assembly { } } Statement::CreateIndex { table, index } => { - if let Some(table) = self.find_table(&table) { - table.indexes.get_or_insert_default().push(index); - } else if let Some(view) = self.find_materialized_view(&table) + let schema = table.schema.clone().unwrap_or_default(); + let location_key = (schema.clone(), index.name.clone()); + if let Some(&idx) = + self.table_index.get(&(schema.clone(), table.name.clone())) { - view.indexes.get_or_insert_default().push(index); + self.tables[idx] + .indexes + .get_or_insert_default() + .push(index); + self.index_location + .insert(location_key, IndexLocation::Table(idx)); + } else if let Some(&idx) = + self.matview_index.get(&(schema, table.name.clone())) + { + self.materialized_views[idx] + .indexes + .get_or_insert_default() + .push(index); + self.index_location.insert( + location_key, + IndexLocation::MaterializedView(idx), + ); } else { // the target relation may simply not be ingested // yet (matview indexes sort before their matview); // retry after the entry loop + self.index_location.insert( + location_key, + IndexLocation::Deferred(self.deferred_indexes.len()), + ); self.deferred_indexes.push((table, index)); } } @@ -885,30 +934,29 @@ impl Assembly { .is_some(), "FUNCTION" => self.apply_function_comment(&schema, name, &comment), "INDEX" => { - self.tables - .iter_mut() - .filter(|t| t.schema == schema) - .flat_map(|t| t.indexes.iter_mut().flatten()) - .find(|i| i.name == *name) - .map(|i| i.comment = Some(comment.clone())) - .is_some() - || self - .materialized_views + match self.index_location.get(&(schema, name.clone())) { + Some(&IndexLocation::Table(idx)) => self.tables[idx] + .indexes .iter_mut() - .filter(|v| v.schema == schema) - .flat_map(|v| v.indexes.iter_mut().flatten()) + .flatten() .find(|i| i.name == *name) .map(|i| i.comment = Some(comment.clone())) - .is_some() - || self - .deferred_indexes + .is_some(), + Some(&IndexLocation::MaterializedView(idx)) => self + .materialized_views[idx] + .indexes .iter_mut() - .find(|(rel, i)| { - rel.schema.clone().unwrap_or_default() == schema - && i.name == *name - }) + .flatten() + .find(|i| i.name == *name) + .map(|i| i.comment = Some(comment.clone())) + .is_some(), + Some(&IndexLocation::Deferred(idx)) => self + .deferred_indexes + .get_mut(idx) .map(|(_, i)| i.comment = Some(comment.clone())) - .is_some() + .is_some(), + None => false, + } } _ => false, }; @@ -986,9 +1034,8 @@ impl Assembly { name: &QualifiedName, ) -> Option<&mut models::Table> { let schema = name.schema.clone().unwrap_or_default(); - self.tables - .iter_mut() - .find(|t| t.schema == schema && t.name == name.name) + let idx = *self.table_index.get(&(schema, name.name.clone()))?; + self.tables.get_mut(idx) } fn find_materialized_view( @@ -996,9 +1043,8 @@ impl Assembly { name: &QualifiedName, ) -> Option<&mut models::MaterializedView> { let schema = name.schema.clone().unwrap_or_default(); - self.materialized_views - .iter_mut() - .find(|v| v.schema == schema && v.name == name.name) + let idx = *self.matview_index.get(&(schema, name.name.clone()))?; + self.materialized_views.get_mut(idx) } /// Merge ALTER SEQUENCE options (including OWNED BY) into the diff --git a/src/pull/update.rs b/src/pull/update.rs index 83bcdf4..2edb404 100644 --- a/src/pull/update.rs +++ b/src/pull/update.rs @@ -65,7 +65,9 @@ pub fn merge( format!("failed to remove {}: {e}", path.display()) })?; } - remove_emptied_directories(root)?; + let tops: Vec = + MANAGED_DIRS.iter().map(|dir| root.join(dir)).collect(); + writer::remove_empty_directories(&tops)?; } else { for relative in &stale { log::warn!( @@ -244,51 +246,6 @@ fn stale_files( Ok(stale) } -/// After pruning, drop directories left empty below the managed -/// top-level directories (the top-level layout itself is preserved) -fn remove_emptied_directories(root: &Path) -> Result<(), String> { - for dir in MANAGED_DIRS { - let top = root.join(dir); - if !top.is_dir() { - continue; - } - let mut directories = Vec::new(); - let mut pending = vec![top]; - while let Some(dir) = pending.pop() { - for entry in std::fs::read_dir(&dir).map_err(|e| { - format!("failed to read {}: {e}", dir.display()) - })? { - let entry = - entry.map_err(|e| format!("failed to read entry: {e}"))?; - let path = entry.path(); - let file_type = entry.file_type().map_err(|e| { - format!("failed to read type for {}: {e}", path.display()) - })?; - if file_type.is_symlink() { - continue; - } - if file_type.is_dir() { - directories.push(path.clone()); - pending.push(path); - } - } - } - directories.sort_by_key(|d| std::cmp::Reverse(d.components().count())); - for dir in directories { - let empty = std::fs::read_dir(&dir) - .map_err(|e| format!("failed to read {}: {e}", dir.display()))? - .next() - .is_none(); - if empty { - std::fs::remove_dir(&dir).map_err(|e| { - format!("failed to remove {}: {e}", dir.display()) - })?; - } - } - } - Ok(()) -} - #[cfg(test)] mod tests { use std::fs; diff --git a/src/pull/writer.rs b/src/pull/writer.rs index 00edece..7afc942 100644 --- a/src/pull/writer.rs +++ b/src/pull/writer.rs @@ -1,7 +1,7 @@ //! YAML project emission for `pull` (ports the storage half of //! generate_project.py / storage.py) -use std::collections::{BTreeMap, BTreeSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::path::{Path, PathBuf}; use serde_json::{Map, Value, json}; @@ -110,7 +110,7 @@ pub fn write_bootstrap( remove_unneeded_gitkeeps(&args.destination)?; } if args.remove_empty_dirs { - remove_empty_directories(&args.destination)?; + remove_empty_directories(std::slice::from_ref(&args.destination))?; } Ok(()) } @@ -180,13 +180,27 @@ impl Writer { } /// Function files are named by function; overloads get a numeric - /// suffix (generate_project.py _function_filename, simplified) + /// suffix (generate_project.py _function_filename, simplified). A + /// counter-suffixed candidate is skipped when it collides with + /// another function's real name (e.g. `fn`, `fn`, `fn_1`), so the + /// real `fn_1` is never displaced into `fn_1_1.yaml` fn write_functions(&mut self, assembly: &Assembly) -> Result<(), String> { + let reserved: HashSet<(String, String)> = assembly + .functions + .iter() + .map(|f| (f.schema.clone(), f.name.clone())) + .collect(); let mut used: BTreeSet<(String, String)> = BTreeSet::new(); for function in &assembly.functions { let mut filename = function.name.clone(); let mut counter = 1; - while used.contains(&(function.schema.clone(), filename.clone())) { + while used.contains(&(function.schema.clone(), filename.clone())) + || (filename != function.name + && reserved.contains(&( + function.schema.clone(), + filename.clone(), + ))) + { filename = format!("{}_{counter}", function.name); counter += 1; } @@ -467,25 +481,39 @@ fn remove_unneeded_gitkeeps(root: &Path) -> Result<(), String> { Ok(()) } -/// Remove empty directories under the project root, deepest first -fn remove_empty_directories(root: &Path) -> Result<(), String> { - let mut directories = walk_directories(root)?; - directories.sort_by_key(|d| std::cmp::Reverse(d.components().count())); - for dir in directories { - let empty = std::fs::read_dir(&dir) - .map_err(|e| format!("failed to read {}: {e}", dir.display()))? - .next() - .is_none(); - if empty { - std::fs::remove_dir(&dir).map_err(|e| { - format!("failed to remove {}: {e}", dir.display()) - })?; +/// Remove empty directories below each of `tops`, deepest first; +/// `tops` themselves are never removed. Shared by the fresh-write path +/// (a single `tops = [project root]`) and `--update --prune` (`tops =` +/// the managed directories), so there is one reaper instead of two +/// with diverging symlink handling +pub(crate) fn remove_empty_directories( + tops: &[PathBuf], +) -> Result<(), String> { + for top in tops { + if !top.is_dir() { + continue; + } + let mut directories = walk_directories(top)?; + directories.sort_by_key(|d| std::cmp::Reverse(d.components().count())); + for dir in directories { + let empty = std::fs::read_dir(&dir) + .map_err(|e| format!("failed to read {}: {e}", dir.display()))? + .next() + .is_none(); + if empty { + std::fs::remove_dir(&dir).map_err(|e| { + format!("failed to remove {}: {e}", dir.display()) + })?; + } } } Ok(()) } -/// All directories below `root`, excluding `root` itself +/// All directories below `root`, excluding `root` itself. Symlinked +/// directories are not followed: their targets may live outside the +/// project (or loop), so treating them as ordinary subdirectories +/// could remove or wander into unrelated paths fn walk_directories(root: &Path) -> Result, String> { let mut results = Vec::new(); let mut pending = vec![root.to_path_buf()]; @@ -495,8 +523,17 @@ fn walk_directories(root: &Path) -> Result, String> { { let entry = entry.map_err(|e| format!("failed to read entry: {e}"))?; - let path = entry.path(); - if path.is_dir() { + let file_type = entry.file_type().map_err(|e| { + format!( + "failed to read type for {}: {e}", + entry.path().display() + ) + })?; + if file_type.is_symlink() { + continue; + } + if file_type.is_dir() { + let path = entry.path(); results.push(path.clone()); pending.push(path); } @@ -545,4 +582,65 @@ mod tests { Path::new("roles").join("admin.yaml") ); } + + fn function(schema: &str, name: &str) -> models::Function { + models::Function { + name: name.to_string(), + schema: schema.to_string(), + owner: String::from("postgres"), + sql: None, + parameters: None, + returns: None, + language: None, + 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: None, + object_file: None, + link_symbol: None, + comment: None, + } + } + + /// A real `fn_1` must never be displaced into `fn_1_1.yaml` by the + /// numeric suffix used to disambiguate `fn` overloads (L9) + #[test] + fn write_functions_does_not_displace_real_numbered_name() { + let assembly = Assembly { + functions: vec![ + function("public", "fn"), + function("public", "fn"), + function("public", "fn_1"), + ], + ..Assembly::default() + }; + let mut writer = Writer { + ignore: BTreeSet::new(), + files: BTreeMap::new(), + mode_headers: false, + include_password_hashes: false, + }; + writer.write_functions(&assembly).unwrap(); + let paths: Vec<&PathBuf> = writer.files.keys().collect(); + let fn_1_path = + Path::new("functions").join("public").join("fn_1.yaml"); + assert!( + paths.contains(&&fn_1_path), + "expected the real fn_1 to keep its own filename: {paths:?}" + ); + // the fn_1 file must contain the real fn_1, not an overload + let body = &writer.files[&fn_1_path]; + assert!(body.contains("name: fn_1"), "unexpected contents: {body}"); + } } From e4bec01bd3cdb7d9e348a1bb116f53633b51fffe Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:36:19 -0400 Subject: [PATCH 5/7] Reduce repeated subtree walks and duplicate columnElem parsing in ddl E4: table::table_constraint, function::apply_common_option, and trigger::create_trigger called has()/find() repeatedly over the same small subtree to test mutually-exclusive keyword branches. Verified against tree-sitter-postgres's node-types.json/grammar.js that the relevant keywords are direct children of their parent node (one per grammar alternative), so each hot spot now does a single child-kind scan instead. Left two call sites recursive because they genuinely need it: function.rs's SET clause (kw_set/set_rest_more nest inside a FunctionSetResetClause child, not directly under common_func_opt_item) and trigger.rs's `when`/`for_each` detection (the timing/for-each keywords are nested for a plain trigger but bare direct children for a CONSTRAINT TRIGGER, per the existing comment in that code). D2: extracted `ddl::column_elems` to collect and unquote a node's columnElem children, replacing the block duplicated across table.rs (index include list, constraint include list, column_list, foreign key ref_columns), acl.rs (privilege column list), and view.rs (view column list). All of these sites used plain `unquote`, not `unquote_role`, so the shared helper does the same. No behavior change intended; `just check` (fmt-check, clippy -D warnings, full test suite) passes unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ddl/acl.rs | 8 ++--- src/ddl/function.rs | 45 ++++++++++++++++--------- src/ddl/mod.rs | 10 ++++++ src/ddl/table.rs | 82 +++++++++++++++++++++------------------------ src/ddl/trigger.rs | 30 ++++++++++++----- src/ddl/view.rs | 9 +++-- 6 files changed, 105 insertions(+), 79 deletions(-) diff --git a/src/ddl/acl.rs b/src/ddl/acl.rs index 15009e6..4ed6395 100644 --- a/src/ddl/acl.rs +++ b/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 { .find_all("privilege") .iter() .map(|p| { - let columns: Vec = p - .find_all("columnElem") - .iter() - .map(|c| unquote(c.text(src))) - .collect(); + let columns: Vec = 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()] diff --git a/src/ddl/function.rs b/src/ddl/function.rs index 01a9309..af06d6a 100644 --- a/src/ddl/function.rs +++ b/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); diff --git a/src/ddl/mod.rs b/src/ddl/mod.rs index a067019..7772286 100644 --- a/src/ddl/mod.rs +++ b/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 { + 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 diff --git a/src/ddl/table.rs b/src/ddl/table.rs index 40d2721..b44eae4 100644 --- a/src/ddl/table.rs +++ b/src/ddl/table.rs @@ -5,7 +5,8 @@ use tree_sitter::Node; use crate::ddl::object::{reloptions, string_value}; use crate::ddl::{ - NodeExt, Statement, TableConstraint, any_name, qualified_name, unquote, + NodeExt, Statement, TableConstraint, any_name, column_elems, + qualified_name, unquote, }; use crate::models::{ CheckConstraint, Column, ColumnGenerated, ConstraintColumns, ForeignKey, @@ -259,12 +260,7 @@ pub(crate) fn create_index( .and_then(|n| n.child_of_kind("name")) .map(|n| unquote(n.text(src))), columns: (!columns.is_empty()).then_some(columns), - include: node.find("opt_c_include").map(|n| { - n.find_all("columnElem") - .iter() - .map(|c| unquote(c.text(src))) - .collect() - }), + include: node.find("opt_c_include").map(|n| column_elems(&n, src)), where_clause: node .child_of_kind("where_clause") .and_then(|n| n.find("a_expr")) @@ -356,27 +352,42 @@ fn table_constraint( let elem = node .child_of_kind("ConstraintElem") .ok_or_else(|| String::from("constraint without ConstraintElem"))?; - let constraint = if elem.has("kw_foreign") { - TableConstraint::ForeignKey(foreign_key( + // FOREIGN KEY / PRIMARY KEY / UNIQUE / CHECK are mutually exclusive + // direct children of ConstraintElem (one per grammar alternative), + // so a single child-kind scan replaces four separate recursive + // `has()` walks of the same subtree + let mut cursor = elem.walk(); + let kind = elem.children(&mut cursor).find_map(|c| match c.kind() { + "kw_foreign" | "kw_primary" | "kw_unique" | "kw_check" => { + Some(c.kind()) + } + _ => None, + }); + let constraint = match kind { + Some("kw_foreign") => TableConstraint::ForeignKey(foreign_key( &elem, src, name.clone().unwrap_or_default(), - )?) - } else if elem.has("kw_primary") { - TableConstraint::PrimaryKey(constraint_columns(&elem, src)) - } else if elem.has("kw_unique") { - TableConstraint::Unique(constraint_columns(&elem, src)) - } else if elem.has("kw_check") { - let expression = elem - .find("a_expr") - .map(|n| n.text(src).to_string()) - .ok_or_else(|| String::from("CHECK without an expression"))?; - TableConstraint::Check(expression) - } else { - return Err(format!( - "unsupported constraint: {}", - crate::ddl::truncate(elem.text(src), 80) - )); + )?), + Some("kw_primary") => { + TableConstraint::PrimaryKey(constraint_columns(&elem, src)) + } + Some("kw_unique") => { + TableConstraint::Unique(constraint_columns(&elem, src)) + } + Some("kw_check") => { + let expression = elem + .child_of_kind("a_expr") + .map(|n| n.text(src).to_string()) + .ok_or_else(|| String::from("CHECK without an expression"))?; + TableConstraint::Check(expression) + } + _ => { + return Err(format!( + "unsupported constraint: {}", + crate::ddl::truncate(elem.text(src), 80) + )); + } }; Ok((name, constraint)) } @@ -385,12 +396,7 @@ fn constraint_columns(elem: &Node, src: &str) -> ConstraintColumns { let columns = column_list(elem, src); let include: Vec = elem .find("opt_c_include") - .map(|n| { - n.find_all("columnElem") - .iter() - .map(|c| unquote(c.text(src))) - .collect() - }) + .map(|n| column_elems(&n, src)) .unwrap_or_default(); if include.is_empty() { ConstraintColumns::Columns(columns) @@ -404,12 +410,7 @@ fn constraint_columns(elem: &Node, src: &str) -> ConstraintColumns { fn column_list(node: &Node, src: &str) -> Vec { node.child_of_kind("columnList") - .map(|list| { - list.find_all("columnElem") - .iter() - .map(|c| unquote(c.text(src))) - .collect() - }) + .map(|list| column_elems(&list, src)) .unwrap_or_default() } @@ -425,12 +426,7 @@ fn foreign_key( let references = qualified_name(&references, src)?; let ref_columns: Vec = elem .child_of_kind("opt_column_and_period_list") - .map(|n| { - n.find_all("columnElem") - .iter() - .map(|c| unquote(c.text(src))) - .collect() - }) + .map(|n| column_elems(&n, src)) .unwrap_or_default(); let spec = elem.child_of_kind("ConstraintAttributeSpec"); let deferrable = spec.and_then(|s| { diff --git a/src/ddl/trigger.rs b/src/ddl/trigger.rs index e887502..af21e63 100644 --- a/src/ddl/trigger.rs +++ b/src/ddl/trigger.rs @@ -31,15 +31,19 @@ pub(crate) fn create_trigger( } else { None }; + // kw_insert/kw_delete/kw_truncate/kw_update (+ optional columnList) + // are all direct children of TriggerOneEvent (one per grammar + // alternative), so child_of_kind (single-level) replaces the + // recursive has() walks let events: Vec = node .find_all("TriggerOneEvent") .iter() .map(|event| { - if event.has("kw_insert") { + if event.child_of_kind("kw_insert").is_some() { "INSERT".to_string() - } else if event.has("kw_delete") { + } else if event.child_of_kind("kw_delete").is_some() { "DELETE".to_string() - } else if event.has("kw_truncate") { + } else if event.child_of_kind("kw_truncate").is_some() { "TRUNCATE".to_string() } else if let Some(columns) = event.child_of_kind("columnList") { format!("UPDATE OF {}", columns.text(src)) @@ -60,19 +64,27 @@ pub(crate) fn create_trigger( // CONSTRAINT TRIGGER, with optional deferral. Only the non-default // attributes are recorded (NOT DEFERRABLE / INITIALLY IMMEDIATE are // the defaults and pg_dump omits them) - let constraint = node.has("kw_constraint").then_some(true); + // kw_constraint is always a direct child of CreateTrigStmt (only + // the CONSTRAINT TRIGGER production carries it) + let constraint = node + .child_of_kind("kw_constraint") + .is_some() + .then_some(true); let spec = node.child_of_kind("ConstraintAttributeSpec"); + // ConstraintAttributeSpec is a left-recursive list, so find_all is + // still needed to flatten it; but each ConstraintAttributeElem is a + // flat production, so its keywords are direct children let deferrable = spec.and_then(|s| { s.find_all("ConstraintAttributeElem") .iter() - .find(|e| e.has("kw_deferrable")) - .map(|e| !e.has("kw_not")) + .find(|e| e.child_of_kind("kw_deferrable").is_some()) + .map(|e| e.child_of_kind("kw_not").is_none()) }); let initially_deferred = spec.and_then(|s| { s.find_all("ConstraintAttributeElem") .iter() - .find(|e| e.has("kw_initially")) - .map(|e| e.has("kw_deferred")) + .find(|e| e.child_of_kind("kw_initially").is_some()) + .map(|e| e.child_of_kind("kw_deferred").is_some()) }); let condition = node .child_of_kind("TriggerWhen") @@ -85,7 +97,7 @@ pub(crate) fn create_trigger( .find_all("TriggerFuncArg") .iter() .map(|arg| { - if let Some(string) = arg.find("Sconst") { + if let Some(string) = arg.child_of_kind("Sconst") { serde_json::Value::String(string_value(&string, src)) } else if let Ok(number) = arg.text(src).parse::() { serde_json::Value::Number(number.into()) diff --git a/src/ddl/view.rs b/src/ddl/view.rs index 2cc25ed..837225f 100644 --- a/src/ddl/view.rs +++ b/src/ddl/view.rs @@ -3,7 +3,7 @@ use tree_sitter::Node; use crate::ddl::object::reloptions; -use crate::ddl::{NodeExt, Statement, qualified_name, unquote}; +use crate::ddl::{NodeExt, Statement, column_elems, qualified_name, unquote}; use crate::models::{MaterializedView, View, ViewColumn}; /// CREATE [OR REPLACE] VIEW → View @@ -92,10 +92,9 @@ fn view_columns(node: &Node, src: &str) -> Option> { node.find("create_mv_target") .and_then(|n| n.child_of_kind("opt_column_list")) })?; - let columns: Vec = list - .find_all("columnElem") - .iter() - .map(|c| ViewColumn::Name(unquote(c.text(src)))) + let columns: Vec = column_elems(&list, src) + .into_iter() + .map(ViewColumn::Name) .collect(); (!columns.is_empty()).then_some(columns) } From 3c378527b7da93e405fadf55131c61b074e69ce6 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 14:50:31 -0400 Subject: [PATCH 6/7] Extend gate fixtures: partitioned/typed tables, table CHECKs, view security options Adds fixtures/schema.sql coverage for object shapes recent batches added support for, so the round-trip and deploy gates would catch regressions in them going forward: - a RANGE-partitioned table's PARTITION BY clause (events) - a typed table via CREATE TABLE ... OF a composite type (locations) - table-level CHECK constraints (products) - a view with security_barrier and check_option (verified_users) - a per-column COMMENT (users.display_name) - a bare, unquoted `public` schema reference (public.widgets) Several closely-related additions were attempted but dropped after they surfaced real, previously-unknown product bugs rather than fixture mistakes (each documented inline at its would-be call site in fixtures/schema.sql): - Partition children (`... PARTITION OF parent FOR VALUES ...`, including a DEFAULT partition) round-trip as unattached plain tables: pg_dump always emits partition attachment as a separate `ALTER TABLE ONLY parent ATTACH PARTITION child FOR VALUES ...` statement, which nothing in src/ddl/ or src/pull/ recognizes. The existing `merges_partition_children` unit test only exercises the inline `PARTITION OF ... FOR VALUES` form pg_dump never actually produces, so it never caught this. - A typed table's inline column constraint (`CREATE TABLE t OF type (CONSTRAINT ... CHECK (...))`) is silently dropped by pull: `create_table` (src/ddl/table.rs) only walks `TableElement` children, and a typed table's element list uses a different grammar production. - A SERIAL column's DEFAULT is dropped: pg_dump emits it as a separate `ALTER TABLE ... ALTER COLUMN ... SET DEFAULT nextval(...)` statement, and pull only captures inline defaults. - A trigger's COMMENT is dropped: `apply_comment` (src/pull/mod.rs) has no match arm for TRIGGER (nor RULE/POLICY/CONSTRAINT). - A zero-argument function renders as invalid SQL on build (`CREATE FUNCTION name RETURNS ...` with no `()`), because the pull-side `src/ddl/function.rs` no longer appends `()` to a zero-parameter function's name, an invariant `dump_function`'s fallback branch (src/build/mod.rs) depends on. Also documents (inline, at the view addition) a systemic gap that predates this change but was newly exposed by adding the first non-materialized view depending on a table: views never get an explicit dependency edge to the tables/views they query (only FK-derived table dependencies are recorded), so `build` orders them purely by libpgdump's default same-priority alphabetical sort. The new view is named to sort after `users` to avoid tripping this; a materialized view happens to be unaffected only because MATERIALIZED VIEW has its own, later, priority tier. `just gates` and `just check` both pass with these fixtures. Co-Authored-By: Claude Opus 4.8 (1M context) --- fixtures/schema.sql | 106 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/fixtures/schema.sql b/fixtures/schema.sql index 116c8dc..f741341 100644 --- a/fixtures/schema.sql +++ b/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 +); From 05c24599afd0b46de11e0052b84ca0b5d985e2be Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Tue, 7 Jul 2026 15:20:01 -0400 Subject: [PATCH 7/7] Address CodeRabbit findings: symlink-safe reaper and dollar-quote prefix check - remove_empty_directories: use symlink_metadata so a top that is a symlink to an external directory is skipped rather than followed, preventing --update --prune from reaping empty dirs outside the project tree. - dollar_quote: pick delimiters by their form without the trailing $ (pg_dump appendStringLiteralDQ style) so a body ending in the delimiter prefix cannot merge with the closing $ into invalid SQL. Adds a regression test for the trailing-$ case. Co-Authored-By: Claude Opus 4.8 --- src/pull/writer.rs | 18 +++++++++++++++++- src/utils.rs | 23 +++++++++++++++++++---- 2 files changed, 36 insertions(+), 5 deletions(-) diff --git a/src/pull/writer.rs b/src/pull/writer.rs index 7afc942..c7b76fb 100644 --- a/src/pull/writer.rs +++ b/src/pull/writer.rs @@ -490,7 +490,23 @@ pub(crate) fn remove_empty_directories( tops: &[PathBuf], ) -> Result<(), String> { for top in tops { - if !top.is_dir() { + // `symlink_metadata` does not follow symlinks: a top that is a + // symlink to a directory outside the project must be skipped so + // `--update --prune` cannot reap empty directories under its + // target. + let metadata = match std::fs::symlink_metadata(top) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + continue; + } + Err(error) => { + return Err(format!( + "failed to read type for {}: {error}", + top.display() + )); + } + }; + if metadata.file_type().is_symlink() || !metadata.is_dir() { continue; } let mut directories = walk_directories(top)?; diff --git a/src/utils.rs b/src/utils.rs index a7bc262..a02658a 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -170,15 +170,21 @@ fn render_value(value: &Value, nested: bool) -> String { /// Wrap `body` in a dollar-quoted string literal, choosing a tag that /// does not occur in `body` (pg_dump style: `$$` when possible, /// otherwise `$c1$`, `$c2$`, ... until the tag is collision-free). +/// +/// Matches pg_dump's `appendStringLiteralDQ`: a delimiter is safe when +/// its form *without* the trailing `$` is absent from `body`. Checking +/// only the full delimiter would miss a body ending in the delimiter +/// prefix (e.g. `foo$`), which merges with the closing `$` and yields +/// invalid SQL. pub(crate) fn dollar_quote(body: &str) -> String { - if !body.contains("$$") { + if !body.contains('$') { return format!("$${body}$$"); } let mut n = 1; loop { - let tag = format!("$c{n}$"); - if !body.contains(&tag) { - return format!("{tag}{body}{tag}"); + let prefix = format!("$c{n}"); + if !body.contains(&prefix) { + return format!("{prefix}${body}{prefix}$"); } n += 1; } @@ -261,4 +267,13 @@ mod tests { let quoted = dollar_quote(body); assert_eq!(quoted, format!("$c2${body}$c2$")); } + + #[test] + fn dollar_quote_handles_trailing_dollar_prefix() { + // A body ending in `$` would merge with a bare `$$` closing + // delimiter, so a tagged delimiter must be chosen instead. + let body = "ends with a $"; + let quoted = dollar_quote(body); + assert_eq!(quoted, format!("$c1${body}$c1$")); + } }