Make the set of reserved schemas and extensions configurable.#530
Open
ibrarahmad wants to merge 5 commits into
Open
Make the set of reserved schemas and extensions configurable.#530ibrarahmad wants to merge 5 commits into
ibrarahmad wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces hard-coded reserved schemas and extensions with the ChangesReserved object catalog
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 2 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sql/spock--6.0.0.sql (1)
1-1: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUpsert race in
reserved_object_add, duplicated in both scripts.The UPDATE-then-INSERT upsert has a TOCTOU window: concurrent first-time adds of the same
(name, kind)can both miss theUPDATEand race on theINSERT, surfacing a raw unique-violation instead of a clean upsert.ON CONFLICTis unusable here since the table isuser_catalog_table(PostgreSQL rejectsON CONFLICTon catalog-marked relations), but the race can be closed with a caught exception.
sql/spock--6.0.0.sql#L380-398: wrap theINSERTin anEXCEPTION WHEN unique_violationblock that falls back toUPDATE.sql/spock--5.0.10--6.0.0.sql#L396-414: apply the identical fix so the upgrade script matches the fresh-install script.🔒 Proposed fix (apply to both files)
IF NOT FOUND THEN - INSERT INTO spock.reserved_object - (name, kind, exclude_from_dump, block_in_repset, builtin) - VALUES (p_name, p_kind, p_exclude_from_dump, p_block_in_repset, false); + BEGIN + INSERT INTO spock.reserved_object + (name, kind, exclude_from_dump, block_in_repset, builtin) + VALUES (p_name, p_kind, p_exclude_from_dump, p_block_in_repset, false); + EXCEPTION WHEN unique_violation THEN + UPDATE spock.reserved_object + SET exclude_from_dump = p_exclude_from_dump, + block_in_repset = p_block_in_repset + WHERE name = p_name AND kind = p_kind; + END; END IF;🤖 Prompt for 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. In `@sql/spock--6.0.0.sql` at line 1, Update the reserved_object_add implementation in both scripts by wrapping its INSERT path in an exception block that catches unique_violation and falls back to the existing UPDATE for the same name and kind. Keep the current update-then-insert behavior and apply the identical handling in both fresh-install and upgrade definitions.
🤖 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 `@docs/spock_functions/repset_mgmt.md`:
- Around line 362-367: Update the documented signature and parameter
descriptions for spock.reserved_object_remove to use the SQL-defined names
object_name and object_kind instead of p_name and p_kind, while preserving their
existing schema/extension meanings.
In `@sql/spock--5.0.10--6.0.0.sql`:
- Around line 396-414: Update spock.reserved_object_add to handle the concurrent
insert race: retain the existing UPDATE-first behavior, but wrap the INSERT in a
unique-violation exception handler and retry the UPDATE for the same (p_name,
p_kind) row when another call wins the insert. Preserve the current values and
builtin=false semantics without using ON CONFLICT.
---
Outside diff comments:
In `@sql/spock--6.0.0.sql`:
- Line 1: Update the reserved_object_add implementation in both scripts by
wrapping its INSERT path in an exception block that catches unique_violation and
falls back to the existing UPDATE for the same name and kind. Keep the current
update-then-insert behavior and apply the identical handling in both
fresh-install and upgrade definitions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45fbe9d4-f9ed-46b2-a73a-84d171486cf2
📒 Files selected for processing (7)
docs/spock_functions/repset_mgmt.mdinclude/spock_node.hsql/spock--5.0.10--6.0.0.sqlsql/spock--6.0.0.sqlsrc/spock_node.csrc/spock_sync.ctests/tap/t/027_reserved_object.pl
susan-pgedge
approved these changes
Jul 15, 2026
susan-pgedge
left a comment
susan-pgedge
left a comment
Member
There was a problem hiding this comment.
The .md files look good to me - I can't speak for the .c/.sql/.pl files!
mason-sharp
marked this pull request as draft
July 15, 2026 15:02
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/spock_functions/repset_mgmt.md (1)
108-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the Markdown table pipe style.
The table triggers MD055 because it uses trailing pipes while this repository’s configured style expects leading pipes only.
Proposed formatting fix
-| Object | exclude_from_dump | block_in_repset | replicate_ddl | -|-------------------|-------------------|-----------------|---------------| -| `spock` | yes | yes | yes | -| `snowflake` | yes | yes | yes | -| `lolor` | yes | no | yes | -| `pgedge_ace` (schema only) | yes | yes | no | +| Object | exclude_from_dump | block_in_repset | replicate_ddl +|-------------------|-------------------|-----------------|--------------- +| `spock` | yes | yes | yes +| `snowflake` | yes | yes | yes +| `lolor` | yes | no | yes +| `pgedge_ace` (schema only) | yes | yes | no🤖 Prompt for 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. In `@docs/spock_functions/repset_mgmt.md` around lines 108 - 113, Update the Markdown table containing the Object, exclude_from_dump, block_in_repset, and replicate_ddl columns to use leading pipes only, removing trailing pipes from the header, separator, and every data row while preserving all table content.Source: Linters/SAST tools
🤖 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/spock_autoddl.c`:
- Around line 358-378: Before the early return in the blocked-schema branch,
call remove_table_from_repsets(node->node->id, reloid, false) to evict any
existing replication-set membership. Keep the relation close and return behavior
unchanged after cleanup.
In `@src/spock_sync.c`:
- Around line 1327-1358: Update the table filtering logic around remoterel and
filtered_tables to evaluate each relation’s complete partition hierarchy, not
just its direct nspname. Recursively resolve descendant partition namespaces,
exclude parents whose hierarchy contains blocked or skip_schema partitions, and
split mixed-policy hierarchies when necessary so scans never copy rows from
excluded partitions or target partitions absent on the subscriber.
In `@tests/tap/t/034_reserved_object_ddl.pl`:
- Line 107: Update the assertion message associated with $fenced in
034_reserved_object_ddl.pl so it states that the pgedge_ace DDL was not
replicated to the subscriber, matching the test’s expected behavior.
---
Nitpick comments:
In `@docs/spock_functions/repset_mgmt.md`:
- Around line 108-113: Update the Markdown table containing the Object,
exclude_from_dump, block_in_repset, and replicate_ddl columns to use leading
pipes only, removing trailing pipes from the header, separator, and every data
row while preserving all table content.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff14f7de-fe8d-4e29-8e01-763251d3abc3
📒 Files selected for processing (12)
docs/spock_functions/repset_mgmt.mdinclude/spock.hinclude/spock_node.hsql/spock--5.0.10--6.0.0.sqlsql/spock--6.0.0.sqlsrc/spock_autoddl.csrc/spock_functions.csrc/spock_node.csrc/spock_sync.ctests/tap/scheduletests/tap/t/027_reserved_object.pltests/tap/t/034_reserved_object_ddl.pl
🚧 Files skipped from review as they are similar to previous changes (3)
- include/spock_node.h
- sql/spock--6.0.0.sql
- src/spock_node.c
mason-sharp
force-pushed
the
reserved_object_catalog
branch
from
July 22, 2026 04:03
83b5aa3 to
45212e1
Compare
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 `@docs/spock_functions/repset_mgmt.md`:
- Around line 108-113: Remove the trailing pipe characters from each data row in
the repset management table, including the rows for spock, snowflake, lolor, and
pgedge_ace, while preserving the table’s column separators and content so it
complies with MD055.
- Around line 99-103: Update the overview text in the repset management
documentation to say “three behaviours” instead of “two behaviours,” reflecting
the documented replicate_ddl behavior while leaving the rest of the section
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e5a9338-72e7-40e5-997d-79c33cae71cb
📒 Files selected for processing (12)
docs/spock_functions/repset_mgmt.mdinclude/spock.hinclude/spock_node.hsql/spock--5.0.10--6.0.0.sqlsql/spock--6.0.0.sqlsrc/spock_autoddl.csrc/spock_functions.csrc/spock_node.csrc/spock_sync.ctests/tap/scheduletests/tap/t/027_reserved_object.pltests/tap/t/034_reserved_object_ddl.pl
🚧 Files skipped from review as they are similar to previous changes (11)
- include/spock.h
- tests/tap/schedule
- src/spock_autoddl.c
- include/spock_node.h
- src/spock_functions.c
- tests/tap/t/034_reserved_object_ddl.pl
- sql/spock--6.0.0.sql
- src/spock_sync.c
- tests/tap/t/027_reserved_object.pl
- sql/spock--5.0.10--6.0.0.sql
- src/spock_node.c
ibrarahmad
force-pushed
the
reserved_object_catalog
branch
from
July 22, 2026 13:33
45212e1 to
080d713
Compare
ibrarahmad
marked this pull request as ready for review
July 22, 2026 13:34
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/spock_node.c`:
- Around line 261-265: Update the reserved_for_purpose calculation in the
surrounding flag-processing logic so a NULL replicate_ddl value is preserved as
not reserved rather than inverted to true. Mirror the NULL handling used by
spock_name_is_reserved, applying inversion only when the attribute is non-NULL,
before the existing !reserved_for_purpose check.
In `@tests/tap/t/027_reserved_object.pl`:
- Around line 167-180: Update the pg_restore invocation before the $rt callback
to use exit-on-error behavior and check its return status; immediately fail or
abort the test when restoration returns a nonzero code, while preserving the
existing reserved_object assertions for successful restores.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35a97481-3228-40f0-a5e6-f3add2898485
📒 Files selected for processing (12)
docs/spock_functions/repset_mgmt.mdinclude/spock.hinclude/spock_node.hsql/spock--5.0.10--6.0.0.sqlsql/spock--6.0.0.sqlsrc/spock_autoddl.csrc/spock_functions.csrc/spock_node.csrc/spock_sync.ctests/tap/scheduletests/tap/t/027_reserved_object.pltests/tap/t/034_reserved_object_ddl.pl
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/tap/schedule
- include/spock.h
- src/spock_autoddl.c
- include/spock_node.h
- tests/tap/t/034_reserved_object_ddl.pl
- sql/spock--5.0.10--6.0.0.sql
- src/spock_functions.c
- src/spock_sync.c
- sql/spock--6.0.0.sql
mason-sharp
requested changes
Jul 22, 2026
mason-sharp
force-pushed
the
reserved_object_catalog
branch
from
July 22, 2026 22:20
e09e11e to
c18a9f9
Compare
Spock keeps certain schemas and extensions out of the structure synchronization dump, and forbids their tables from being added to a replication set. This set was fixed and could only be changed by rebuilding the extension, and the two behaviours were maintained independently, so they could drift apart. Keep the set in a catalog instead, seeded with the same defaults as before: spock and snowflake are both excluded from the dump and blocked from replication sets, while lolor is excluded from the dump but still allowed in replication sets so that large objects survive dropping the extension on every node. The seeded entries are protected and cannot be removed, and an operator can now reserve additional schemas or extensions without rebuilding. Such additions are preserved across dump and restore, while the built-in entries are always re-created.
Render the extension config table deterministically in the init test, make the reserved-object add helper upsert race-safe, and give its remove helper consistent parameter names.
The spock.reserved_object catalog gains a third, independent policy flag, replicate_ddl, alongside exclude_from_dump and block_in_repset. It applies to schemas only: when false, AutoDDL keeps DDL that targets the schema node-local instead of shipping it to other nodes. Seed a built-in pgedge_ace schema with exclude_from_dump, block_in_repset, and replicate_ddl=false so pgEdge ACE's node-local state stays off the wire: it is left out of the structure-sync dump, its tables are never added to a replication set, and its DDL is not replicated to nodes that may lack the schema. The reserved-object accessor gains a DDL purpose that selects schemas whose replicate_ddl is false. In AutoDDL, stmt_targets_only_ddl_local() suppresses replication of DDL targeting only node-local schemas, and apply_repset_policy_for_reloid() evicts tables in a block_in_repset schema from every replication set; unclassifiable statements fall back to replicating. copy_replication_sets_data() likewise drops such tables from the initial data copy. Dump exclusion is now unconditional: the previous local-existence guard ran on the subscriber while pg_dump targets the provider, so an object present only on the provider was never excluded. replicate_ddl is nullable with a CHECK tying it to schema rows, so extension rows store NULL rather than an ignored value. Extend 027_reserved_object and add 034_reserved_object_ddl; update the docs.
Test one reserved name with a predicate instead of building a list, and read the catalog once per DROP rather than once per object. Document the classified DDL set and add dump/restore and operator-exclusion tests.
Replace reserved_purpose_attnum()'s attnum + *invert out-param with a single row_reserved_for_purpose() predicate that decides, in one place, which column and which polarity mean "reserved" for a given purpose. Callers no longer reconstruct the boolean logic themselves. This also fixes a latent inconsistency: spock_reserved_object_names() inverted replicate_ddl without checking for NULL, so a NULL flag (extensions) could be wrongly treated as node-local; it now matches spock_name_is_reserved() and treats NULL as not reserved. Promote name_in_list() from a static in spock_functions.c to a shared helper (declared in spock.h) and use it to collapse the duplicated schema-skip loops in copy_replication_sets_data() down to a single membership check. All changes are on DDL, repset-admin, and sync-setup paths; the per-row DML apply path is untouched. No functional change other than the NULL fix noted above. Also fold in two CodeRabbit fixes: the reserved_object round-trip test now checks pg_restore's exit status, and the docs table drops trailing pipes to satisfy markdownlint MD055.
mason-sharp
force-pushed
the
reserved_object_catalog
branch
from
July 24, 2026 21:07
c18a9f9 to
2faadbd
Compare
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
3 participants
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.
Spock keeps certain schemas and extensions out of the structure synchronization dump, and forbids their tables from being added to a replication set. This set was fixed and could only be changed by rebuilding the extension, and the two behaviours were maintained independently, so they could drift apart.
Keep the set in a catalog instead, seeded with the same defaults as before: spock and snowflake are both excluded from the dump and blocked from replication sets, while lolor is excluded from the dump but still allowed in replication sets so that large objects survive dropping the extension on every node. The seeded entries are protected and cannot be removed, and an operator can now reserve additional schemas or extensions without rebuilding. Such additions are preserved across dump and restore, while the built-in entries are always re-created.