Conversation
danolivo
added
enhancement
|
Warning Review limit reached
More reviews will be available in 5 minutes and 25 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds DDL replication changes to return a boolean status, skip temp-relation-derived statements, and gate replication-set tracking on that status. Also adds a PG18 single-node installcheck workflow and Bash harness that build, start, test, and verify a three-node Spock mesh. ChangesDDL replication safety
PG18 single-node installcheck
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|---|---|
| Duplication | 1 |
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: 4
🧹 Nitpick comments (1)
.github/workflows/installcheck-multi-pg.yml (1)
50-50: 🏗️ Heavy liftConsider pinning actions to commit SHAs for supply-chain security.
Currently, the workflow references
actions/checkout@v4andactions/upload-artifact@v4using mutable tags. Pinning to full commit SHAs (e.g.,actions/checkout@<sha>) prevents tag-moving attacks and improves supply-chain security by ensuring the exact action code is reviewed and locked.Example pinning (verify current SHAs before applying):
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1Note: This requires looking up and maintaining SHA references, which adds maintenance overhead when updating action versions.
Also applies to: 67-67
🤖 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 @.github/workflows/installcheck-multi-pg.yml at line 50, The workflow uses mutable action tags (actions/checkout@v4 and actions/upload-artifact@v4); replace those with the corresponding immutable commit SHAs (e.g., actions/checkout@<commit-sha> and actions/upload-artifact@<commit-sha>) by looking up the current, verified commit SHAs for the desired versions and updating the `uses:` entries for the steps that reference `actions/checkout@v4` and `actions/upload-artifact@v4` so the workflow pins to exact commits.
🤖 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 @.github/workflows/installcheck-multi-pg.yml:
- Around line 49-50: Update the GitHub Actions checkout step named "Checkout
spock" (the actions/checkout@v4 usage) to include persist-credentials: false to
avoid persisting the GITHUB_TOKEN to disk; locate the step with name "Checkout
spock" and add a persist-credentials: false key under that step (properly
indented) so the checkout action explicitly disables credential persistence.
In `@tests/run-multi-pg-installcheck.sh`:
- Around line 204-218: on_err currently logs failure and kills builders but does
not stop any started Postgres nodes; update on_err to invoke stop_all_nodes (or
the existing node-teardown function used in main) unless the run was started
with the "--keep" option, i.e. check the same keep-flag variable used elsewhere
in the script and call stop_all_nodes || true before exit; retain
dump_logs_on_failure and kill_outstanding_builders behavior and make the
stop_all_nodes call idempotent/safe so it can run from the ERR trap without
breaking command-substitution contexts.
- Around line 338-342: When a partial patch application leaves a checkout in a
modified state, reruns hit already-applied patches because _do_patch_pg() only
writes .spock-patches-applied at the end; update the logic so that before
reapplying patches (in patch_pg() or at the start of _do_patch_pg()) you detect
the absence of the .spock-patches-applied marker and then either reset the
existing checkout (e.g., git reset --hard && git clean -fdx) or force a fresh
clone via clone_pg(); ensure the reset/clean happens whenever the marker is not
present but the repository directory exists so partial applies are wiped before
attempting the patch sequence again.
- Around line 232-260: Validate BASE_DIR (after resolving with BASE_DIR="$(cd
"${BASE_DIR}" && pwd)") before running mkdir/rm: ensure it's non-empty and not a
dangerous root like "/" or other top-level system roots (at minimum reject ""
and "/"), or require a harness-owned sentinel file (e.g. check for
"${BASE_DIR}/.spock-harness") before performing rm -rf on LOG_DIR and PID_DIR;
if validation fails, print an error and exit non‑zero. Update the logic around
BASE_DIR, SOCK_DIR, LOG_DIR, PID_DIR, mkdir -p and rm -rf to enforce this guard.
---
Nitpick comments:
In @.github/workflows/installcheck-multi-pg.yml:
- Line 50: The workflow uses mutable action tags (actions/checkout@v4 and
actions/upload-artifact@v4); replace those with the corresponding immutable
commit SHAs (e.g., actions/checkout@<commit-sha> and
actions/upload-artifact@<commit-sha>) by looking up the current, verified commit
SHAs for the desired versions and updating the `uses:` entries for the steps
that reference `actions/checkout@v4` and `actions/upload-artifact@v4` so the
workflow pins to exact commits.
🪄 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: cca87c39-c1bb-4f8e-ab01-1367688b0037
📒 Files selected for processing (3)
.github/workflows/installcheck-multi-pg.yml.gitignoretests/run-multi-pg-installcheck.sh
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/run-multi-pg-installcheck.sh (3)
243-260:⚠️ Potential issue | 🟠 MajorValidate
--base-dirbefore deleting under it.This path is accepted verbatim and then used for
rm -rf "${LOG_DIR}" "${PID_DIR}". A typo like--base-dir /or another top-level system path turns the harness into a host-filesystem cleanup step.🤖 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 `@tests/run-multi-pg-installcheck.sh` around lines 243 - 260, The script currently accepts --base-dir verbatim and then runs rm -rf "${LOG_DIR}" "${PID_DIR}" which can wipe critical paths; before deleting, validate and canonicalize BASE_DIR (the variable set by --base-dir) and assert it is non-empty, not "/" (or other top-level paths), and that LOG_DIR and PID_DIR are actual subdirectories of that canonicalized BASE_DIR; if the checks fail, abort with an error. Use the existing BASE_DIR, LOG_DIR, PID_DIR variables (and the canonicalization step BASE_DIR="$(cd "${BASE_DIR}" && pwd)") to perform these checks and refuse to run rm -rf unless the safe-guard conditions pass.
206-218:⚠️ Potential issue | 🟠 MajorDefault cleanup still does not run on failure.
on_err()only kills builders, and the explicitwait_for_builders || fail .../wait_for_all_ready || fail ...paths inmain()bypass the ERR trap entirely. A failed run can still leave started postmasters and sockets behind even when--keepwas not set.Also applies to: 1012-1016
🤖 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 `@tests/run-multi-pg-installcheck.sh` around lines 206 - 218, The failure paths bypass the ERR trap so started postmasters/sockets can be left running; update cleanup so failures always perform default teardown: modify on_err() to call stop_all_nodes and the normal teardown (or the same function main() uses for cleanup) in addition to kill_outstanding_builders and dump_logs_on_failure, honoring the --keep flag if set, and change the explicit failure paths in main() (the points that call wait_for_builders || fail... and wait_for_all_ready || fail...) to invoke on_err with the proper exit code (or call the shared teardown function) instead of exiting directly so cleanup runs consistently.
363-381:⚠️ Potential issue | 🟠 MajorA partial patch failure still leaves reruns stuck on a dirty checkout.
The marker is only written after the entire patch loop succeeds. If one patch fails after earlier ones applied, the next run reuses that modified tree and re-enters
git applyon already-applied patches until the checkout is manually reset.Also applies to: 390-394
🤖 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 `@tests/run-multi-pg-installcheck.sh` around lines 363 - 381, The loop in _do_patch_pg applies patches but always writes the .spock-patches-applied marker even if some git apply invocations fail; modify _do_patch_pg so that each git apply is checked and on any failure the function immediately prints an error, returns non-zero (or exits), and does not write the marker, and only touch "${src}/.spock-patches-applied" after the loop completes successfully (i.e., after all git apply calls returned success and any==1); reference the _do_patch_pg function, the git apply invocation and the .spock-patches-applied marker when making this change.
🤖 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 @.github/workflows/installcheck-multi-pg.yml:
- Around line 52-53: The workflow uses mutable action tags; replace the tagged
usages of the actions with the provided immutable commit SHAs so the steps
"uses: actions/checkout@v4" and "uses: actions/upload-artifact@v4" are updated
to "uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5" and "uses:
actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02" respectively
to pin the actions to immutable SHAs.
In `@tests/run-multi-pg-installcheck.sh`:
- Around line 338-342: The --force flag is currently not forcing a fresh
source+patch because clone_pg() reuses existing checkouts and patch_pg() skips
when a marker exists; update both to honor FORCE by having clone_pg() delete or
reclone the ${src} directory (or run a fresh git clone) when FORCE/--force is
set so it cannot reuse a stale checkout, and have patch_pg() remove or ignore
the existing marker (the patch-applied sentinel used there) when FORCE is set so
patches are re-applied; modify the logic around clone_pg() and patch_pg() to
check the FORCE variable and remove the existing source tree and marker before
proceeding.
---
Duplicate comments:
In `@tests/run-multi-pg-installcheck.sh`:
- Around line 243-260: The script currently accepts --base-dir verbatim and then
runs rm -rf "${LOG_DIR}" "${PID_DIR}" which can wipe critical paths; before
deleting, validate and canonicalize BASE_DIR (the variable set by --base-dir)
and assert it is non-empty, not "/" (or other top-level paths), and that LOG_DIR
and PID_DIR are actual subdirectories of that canonicalized BASE_DIR; if the
checks fail, abort with an error. Use the existing BASE_DIR, LOG_DIR, PID_DIR
variables (and the canonicalization step BASE_DIR="$(cd "${BASE_DIR}" && pwd)")
to perform these checks and refuse to run rm -rf unless the safe-guard
conditions pass.
- Around line 206-218: The failure paths bypass the ERR trap so started
postmasters/sockets can be left running; update cleanup so failures always
perform default teardown: modify on_err() to call stop_all_nodes and the normal
teardown (or the same function main() uses for cleanup) in addition to
kill_outstanding_builders and dump_logs_on_failure, honoring the --keep flag if
set, and change the explicit failure paths in main() (the points that call
wait_for_builders || fail... and wait_for_all_ready || fail...) to invoke on_err
with the proper exit code (or call the shared teardown function) instead of
exiting directly so cleanup runs consistently.
- Around line 363-381: The loop in _do_patch_pg applies patches but always
writes the .spock-patches-applied marker even if some git apply invocations
fail; modify _do_patch_pg so that each git apply is checked and on any failure
the function immediately prints an error, returns non-zero (or exits), and does
not write the marker, and only touch "${src}/.spock-patches-applied" after the
loop completes successfully (i.e., after all git apply calls returned success
and any==1); reference the _do_patch_pg function, the git apply invocation and
the .spock-patches-applied marker when making this change.
🪄 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: 36b9fb51-6a70-4c1d-bc1a-50ab582519ee
📒 Files selected for processing (3)
.github/workflows/installcheck-multi-pg.yml.gitignoretests/run-multi-pg-installcheck.sh
✅ Files skipped from review due to trivial changes (1)
- .gitignore
danolivo
changed the title
danolivo
changed the title
danolivo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/installcheck-single-pg18.yml (1)
51-52: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUnpinned GitHub Actions references. Both action references use mutable tags (
@v4) instead of immutable commit SHAs, creating a supply-chain security risk. The checkout action should also setpersist-credentials: false.
.github/workflows/installcheck-single-pg18.yml#L51-L52: Pinactions/checkout@v4toactions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683and addpersist-credentials: false..github/workflows/installcheck-single-pg18.yml#L72-L72: Pinactions/upload-artifact@v4toactions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02.🤖 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 @.github/workflows/installcheck-single-pg18.yml around lines 51 - 52, Replace mutable GitHub Actions tags with immutable commit SHAs to eliminate supply-chain security risks. At .github/workflows/installcheck-single-pg18.yml lines 51-52, update the actions/checkout action reference from `@v4` to the pinned commit SHA and add persist-credentials: false to the checkout step to prevent storing credentials. At .github/workflows/installcheck-single-pg18.yml lines 72-72, update the actions/upload-artifact action reference from `@v4` to its pinned commit SHA.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_functions.c`:
- Around line 2363-2370: The query_temp_like_source() function only checks
top-level CreateStmt nodes by iterating through results of
pg_parse_query(query), but when the original query is wrapped in EXPLAIN
ANALYZE, the top-level statement is ExplainStmt rather than CreateStmt, causing
the CREATE TABLE ... LIKE detection to be missed. At the recursion point around
line 2644-2646, the original query text (still containing EXPLAIN wrapper) is
being passed to query_temp_like_source(), which fails to detect the nested
CREATE TABLE ... LIKE statement. Strip the EXPLAIN wrapper from the query before
passing it to the recursive query_temp_like_source() call at line 2644-2646 so
that the function can properly identify and skip temp-source CREATE statements
at all nesting levels.
In `@tests/run-single-pg18-installcheck.sh`:
- Line 717: The variable `${out}` in the command substitution within the log
statement needs to be double-quoted to prevent word splitting and globbing
issues. Wrap `${out}` with double quotes in the echo command that pipes to tr to
ensure the variable is treated as a single word and special characters are
properly handled.
---
Duplicate comments:
In @.github/workflows/installcheck-single-pg18.yml:
- Around line 51-52: Replace mutable GitHub Actions tags with immutable commit
SHAs to eliminate supply-chain security risks. At
.github/workflows/installcheck-single-pg18.yml lines 51-52, update the
actions/checkout action reference from `@v4` to the pinned commit SHA and add
persist-credentials: false to the checkout step to prevent storing credentials.
At .github/workflows/installcheck-single-pg18.yml lines 72-72, update the
actions/upload-artifact action reference from `@v4` to its pinned commit SHA.
🪄 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: 15678d0c-1949-48e1-b7b3-83eb783f731f
⛔ Files ignored due to path filters (2)
tests/regress/expected/autoddl.outis excluded by!**/*.outtests/regress/expected/exception_row_capture.outis excluded by!**/*.out
📒 Files selected for processing (8)
.github/workflows/installcheck-single-pg18.yml.gitignoreinclude/spock.hsrc/spock_autoddl.csrc/spock_functions.csrc/spock_relcache.ctests/regress/sql/autoddl.sqltests/run-single-pg18-installcheck.sh
✅ Files skipped from review due to trivial changes (1)
- .gitignore
The auto-DDL path in spock_auto_replicate_ddl() interpolated the search_path GUC value into the queued command with a bare "%s", guarded only by strlen() > 0. That is unsafe because GetConfigOptionByName() does not always return something that is valid SQL on its own. Fix this issue. Change error message on missed entry in spock cache: highlight the fact that the root of the issue might be schema mismatch.
Auto-DDL ships the raw command text to be re-parsed and executed on
subscribers. A permanent table built from a temporary one -
CREATE TABLE ... (LIKE temp) or CREATE TABLE ... AS SELECT ... FROM temp
- would therefore ship a command that cannot run on the subscriber,
where the temporary relation does not exist, stalling the apply worker.
Detect those cases in spock_auto_replicate_ddl() and skip replication
with an explanatory message instead:
- CREATE TABLE ... (LIKE ...): by the time auto-DDL runs (post
execution) transformCreateStmt() has already expanded the LIKE
clause out of tableElts, so the executed parse tree no longer shows
the source relation. Re-parse the raw command text -- the exact
text we would ship and the subscriber would re-parse -- which still
carries the LIKE clause, and resolve each source against the
catalogue.
- CREATE TABLE AS: walk the analysed defining query with the existing
isQueryUsingTempRelation(), which catches references at any nesting
level.
spock_auto_replicate_ddl() now returns whether the statement was
queued, and spock_autoddl_process() adds the relation to the
replication set only when it was. Otherwise a table whose CREATE was
not replicated would have its later DML fail to apply downstream.
Note this detects only relations named directly in the statement; a
temporary relation reached indirectly (e.g. through a function body)
cannot be detected statically.
Add regression coverage for LIKE-of-temp, CTAS-of-temp (including a
nested subquery), and a permanent-LIKE sanity case.
Add a self-contained test rig (tests/run-single-pg18-installcheck.sh) that builds PostgreSQL REL_18_STABLE and the Spock extension once, wires three single-node PG18 clusters into a full Spock mesh (6 subscriptions, exception_behaviour='discard', auto-DDL on), runs `make installcheck` against one node as a stress workload, and asserts that every subscription stays enabled and that spock.sync_event() round-trips on every directed edge.
Preparatory refactor with no change in behaviour: move the relation resolution (including partition expansion) and the ExecuteTruncateGuts() call out of handle_truncate() into a new apply_truncate() helper. This isolates the part of the TRUNCATE apply path that can throw, so the next commit can wrap it in a subtransaction for DISCARD-mode exception handling without obscuring the material change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
In DISCARD mode handle_truncate() had no per-action exception handling. On the retry (use_try_block) pass it re-ran the truncate directly, so a failure -- most commonly a relation that does not exist on the subscriber -- escaped as an ERROR. With use_try_block set and xact_had_exception clear, apply_work()'s outer handler re-throws to exit the worker; the background worker then restarts, replays from the same position, and fails identically. The result is an apply-worker crash/restart loop that stalls every later change from that origin, sync_event included. Wrap apply_truncate() in a subtransaction on the DISCARD retry path, mirroring handle_insert/update/delete and handle_sql_or_exception(): on failure, roll back the savepoint, set xact_had_exception, log the discarded TRUNCATE, and carry on. TRANSDISCARD and SUB_DISABLE keep their unconditional skip -- the whole transaction is being abandoned, so the truncate must not be attempted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/installcheck-single-pg18.yml (1)
51-52: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin the GitHub Actions to immutable SHAs.
This workflow reintroduces mutable
@v4refs at Line 52 and Line 72, so the exact code executed by CI can change underneath the branch.Suggested hardening
- name: Checkout spock - uses: actions/checkout@v4 + uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 - name: Collect logs on failure if: ${{ failure() }} - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02Also applies to: 70-72
🤖 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 @.github/workflows/installcheck-single-pg18.yml around lines 51 - 52, The workflow is using mutable GitHub Actions refs, so replace every `@v4` usage in this workflow with a pinned immutable commit SHA. Update the affected `actions/checkout` steps (including the one named `Checkout spock`) and any other actions around the referenced blocks so CI always runs the exact same code. Keep the existing workflow behavior unchanged aside from pinning the action versions.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_apply.c`:
- Around line 1951-1983: The TRUNCATE error path in spock_apply.c leaves cached
relation handles stale after RollbackAndReleaseCurrentSubTransaction() swallows
an apply_truncate() failure. Update the PG_CATCH cleanup around apply_truncate()
to explicitly clear or close any cached SpockRelation->rel entries before
continuing, using the same exception handling block that copies edata and
restores ApplyOperationContext. Keep the success path with
ReleaseCurrentSubTransaction() unchanged, but move the cache invalidation into
the failure path so discarded relations do not retain stale pointers.
In `@tests/run-single-pg18-installcheck.sh`:
- Around line 74-76: The test harness keeps postmasters running because
KEEP_RUNNING defaults to enabled and the ERR trap exits before stop_all_nodes()
is reached. Update the control flow in run-single-pg18-installcheck.sh around
KEEP_RUNNING, the --keep handling, and the cleanup trap so stop_all_nodes()
always runs on normal completion and failure unless the user explicitly
requested to keep nodes alive. Make sure the affected paths in the main workflow
and error handling still use the existing stop_all_nodes/init_node logic
consistently.
---
Duplicate comments:
In @.github/workflows/installcheck-single-pg18.yml:
- Around line 51-52: The workflow is using mutable GitHub Actions refs, so
replace every `@v4` usage in this workflow with a pinned immutable commit SHA.
Update the affected `actions/checkout` steps (including the one named `Checkout
spock`) and any other actions around the referenced blocks so CI always runs the
exact same code. Keep the existing workflow behavior unchanged aside from
pinning the action versions.
🪄 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: 04c30b61-9804-44e8-8350-cb3297d8a563
⛔ Files ignored due to path filters (2)
tests/regress/expected/autoddl.outis excluded by!**/*.outtests/regress/expected/exception_row_capture.outis excluded by!**/*.out
📒 Files selected for processing (9)
.github/workflows/installcheck-single-pg18.yml.gitignoreinclude/spock.hsrc/spock_apply.csrc/spock_autoddl.csrc/spock_functions.csrc/spock_relcache.ctests/regress/sql/autoddl.sqltests/run-single-pg18-installcheck.sh
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (5)
- include/spock.h
- src/spock_relcache.c
- src/spock_autoddl.c
- tests/regress/sql/autoddl.sql
- src/spock_functions.c
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.
Problem
Spock's automatic DDL replication ships the raw command text to be re-parsed and executed on every subscriber. When a permanent table is created from a temporary one, the shipped command is valid on the origin but cannot run on the subscriber, where the temporary relation does not exist:
CREATE TABLE foo (LIKE some_temp_table)CREATE TABLE foo AS SELECT ... FROM some_temp_tableThe replicated command fails on the subscriber with
relation "..." does not exist, and because queued changes apply in order, the apply worker stalls — every later change from that origin (includingspock.sync_event()) is blocked behind the un-appliable command. The existing temp-relation guard only checked the target of the CREATE, not relations it copies from.Root cause
By the time auto-DDL runs (post-execution, in the ProcessUtility hook)
transformCreateStmt()has already expanded theLIKEclause out ofCreateStmt->tableElts, so the executed parse tree no longer shows the source relation. Inspecting that tree finds nothing. The information only survives in the raw command text — the exact text we would ship and the subscriber would re-parse.Solution
In
spock_auto_replicate_ddl(), detect temporary-relation references and skip replication with an explanatory message instead of shipping a command that cannot apply:CREATE TABLE ... (LIKE ...)— re-parse the raw command text (which still carries theLIKEclause), resolve each source relation against the catalogue, and check its persistence. Name resolution uses the livesearch_path, matching what the executor did on the origin.CREATE TABLE AS— walk the analysed defining query with the existingisQueryUsingTempRelation(), which catches references at any nesting level (e.g. inside a subquery).spock_auto_replicate_ddl()now returns whether the statement was queued.spock_autoddl_process()adds the relation to the replication set only when it was — otherwise a table whoseCREATEwas not replicated would have its later DML fail to apply downstream, reintroducing the same stall.Limitations
Detection covers only relations named directly in the statement. A temporary relation reached indirectly — through a function body or dynamic SQL — cannot be detected statically; that is a separate, fundamentally undecidable case.
Also in this branch
Replaces the multi-PG mesh installcheck rig with a single-PG18 one (
tests/run-single-pg18-installcheck.sh+ GitHub Actions workflow). The multi-version rig conflated genuine bugs with inherent cross-version DDL-shipping fragility (e.g. a PG16-builtregress.dylibfailing to load on a PG18 subscriber), which is expected and out of scope for the regression mesh. The single-PG18 rig builds PostgreSQL REL_18_STABLE and Spock once, wires three PG18 nodes into a full mesh, stresses it withmake installcheck, and asserts every subscription stays enabled andspock.sync_event()round-trips on every edge.