Batch 3: dollar-quote safety, efficiency/dedup cleanups, and gate-fixture coverage#55
Merged
Conversation
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) <noreply@anthropic.com>
…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) <noreply@anthropic.com>
- 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) <noreply@anthropic.com>
…apers - 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…curity 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) <noreply@anthropic.com>
…/loader-efficiency', 'fix/ddl-efficiency', 'fix/acls-and-progress' and 'test/gate-fixture-coverage' into fix/review-findings-batch3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds SQL fixture objects, introduces hash-map-based lookup indexes in ACL resolution, project loading, and pull ingestion, adds shared dollar-quoting for comment/string rendering, refactors DDL parsing helpers and direct child lookups, updates pull writer pruning and filename disambiguation, and adjusts main.rs progress bar setup. ChangesCore PR changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CreateIndexStmt
participant Assembly
participant Loader
participant ObjectIndex
participant Inventory
CreateIndexStmt->>Assembly: ingest table or matview index
Assembly->>Assembly: update table_index/matview_index/index_location
Loader->>Loader: store index_key for new definitions
ObjectIndex->>Inventory: build object and function maps
ObjectIndex->>ObjectIndex: resolve dependency targets from maps
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pull/writer.rs`:
- Around line 489-496: The remove_empty_directories path still follows symlinked
top entries because it uses top.is_dir() before calling walk_directories, which
can let --update --prune traverse outside the intended tree. Update
remove_empty_directories in the writer module to check each top with
symlink_metadata first, skip entries whose metadata indicates a symlink, and
only then proceed to walk_directories and prune empty directories for real
directories.
In `@src/utils.rs`:
- Around line 170-186: The dollar_quote helper currently checks only for the
full delimiter string, which can miss cases where the body ends with the
delimiter prefix and still create invalid SQL. Update dollar_quote to use
pg_dump-style collision checks by matching the delimiter prefix without the
trailing $ when deciding whether $$ or $c{n}$ is safe, and keep the fallback
loop in sync with that logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0dcbbe19-8c01-47d2-8987-fa7f875925c6
📒 Files selected for processing (16)
fixtures/schema.sqlsrc/build/acls.rssrc/build/mod.rssrc/ddl/acl.rssrc/ddl/function.rssrc/ddl/mod.rssrc/ddl/table.rssrc/ddl/trigger.rssrc/ddl/view.rssrc/deploy/alter.rssrc/main.rssrc/project/load.rssrc/pull/mod.rssrc/pull/update.rssrc/pull/writer.rssrc/utils.rs
…fix 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 <noreply@anthropic.com>
Owner
Author
|
pr-agent: PR is ready to merge.
Not merging per instructions — this is a readiness notification only. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Third batch from the 2026-07-06 code review: the remaining low-severity
correctness items, the efficiency (O(n²)→indexed) and duplication
cleanups, and — most importantly — an expansion of the Postgres gate
fixtures to close the coverage gaps that let earlier bugs slip past the
gates. Split across isolated worktrees by disjoint file ownership,
integrated, and validated with
just checkandjust gates.Solution
utils.rs,build/mod.rs,deploy/alter.rs): newdollar_quote()helper picks a tag not presentin the body (
$$→$c1$→ …) so a comment/value containing$$nolonger terminates the literal early. All three sites routed through it.
deploy/alter.rs):push_comment/push_optionsfold 14 identical delta sites; emittedSQL and
--allow-dropgating unchanged.pull/*):(schema,name)+index_locationmaps remove the O(n²) ingest scans; the overload filename counter no
longer displaces a real
fn_1; one shared symlink-skipping empty-dirreaper replaces the two divergent copies.
project/load.rs): a(desc,schema,name)index makeslookup_item/dependency resolution O(1); the duplicate-object guardstill works.
ddl/*): single-pass direct-child scans replacerepeated recursive
has()/find()walks (genuinely dual-depth sitesleft recursive, with reasoning); shared
column_elems()helper folds 7duplicated blocks.
build/acls.rs,main.rs,progress.rs): anObjectIndexmakes ACL lookups O(1); progress barsnow initialize even with
--log-file.fixtures/schema.sql): added round-trip/deploycoverage for a partitioned table's
PARTITION BY, a typed table, tableCHECK constraints, a
security_barrier+check_optionview, a columnCOMMENT, and a bare unquoted
publicreference.Verification
just check— fmt, clippy-D warnings, full suite: green.just gates— round-trip + deploy equivalence/convergence/safety:green on the expanded fixtures.
Trying to fixture the gap paths surfaced six real, previously-uncaught
bugs. They're documented inline in
fixtures/schema.sql; each is acandidate for a follow-up (batch 4). Highest-impact first:
CREATE FUNCTION name RETURNS …missing(); breaks every trigger function. (HIGH)pg_dumpemitsALTER TABLE ONLY parent ATTACH PARTITION child FOR VALUES …, a formpull doesn't recognize; the inline
PARTITION OFform it does handleis never what pg_dump produces. (HIGH — N-H3 is incomplete for real
dumps)
views alphabetically, so a view can be emitted before the table it
selects from. (HIGH, systemic)
ALTER TABLE … SET DEFAULT nextval(…)isn't captured.apply_commenthas noTRIGGER/RULE/POLICY/CONSTRAINTarm.Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
COMMENT ON/option payloads using safer dollar-quoting, preserving tricky characters.--update --pruneto avoid overwriting existing numbered overload files; improved empty-directory cleanup and symlink-safe traversal.