Conversation
danolivo
added
enhancement
📝 WalkthroughWalkthroughTwo independent changes: (1) ChangesDDL Replication Safety Fixes
PG18 Single-Node Installcheck Infrastructure
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 docstrings
🧪 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 | 0 |
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
Comment on lines
+49
to
+50
| - name: Checkout spock | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Set persist-credentials: false to prevent credential leakage.
The checkout action should explicitly set persist-credentials: false to ensure the GitHub token is not persisted to disk after checkout completes. While the artifacts being uploaded are only logs and test output (not the git checkout itself), this is a security best practice to prevent accidental credential exposure.
🔒 Proposed fix
- name: Checkout spock
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - name: Checkout spock | |
| uses: actions/checkout@v4 | |
| - name: Checkout spock | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 49-50: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 50-50: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 around lines 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.
Comment on lines
+204
to
+218
| trap 'on_err $? $LINENO' ERR | ||
|
|
||
| on_err() { | ||
| local rc=$1 line=$2 | ||
| log "Aborted: exit ${rc} at line ${line}" | ||
| dump_logs_on_failure || true | ||
| # Deliberately NO stop_all_nodes here. It used to live in this | ||
| # trap, but combined with set -E it would fire from inside command | ||
| # substitutions and silently kill every cluster on a single | ||
| # transient psql hiccup. Teardown lives in main()'s normal flow | ||
| # (where it has the correct context); kill_outstanding_builders is | ||
| # still safe to call here because it operates on PIDs we know are | ||
| # ours. | ||
| kill_outstanding_builders || true | ||
| exit "${rc}" |
There was a problem hiding this comment.
Always tear down started clusters on failure unless --keep is set.
Right now the only stop_all_nodes() call is on the success path in main(). Any error after some postmasters have started—including the already-hit wait_for_builders || fail "one or more builders failed" path—exits without cleanup, so a failed run can leave sockets/processes behind even though the script documents “servers will be stopped on script exit” by default.
Also applies to: 1012-1016, 1082-1086
🤖 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 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.
Comment on lines
+232
to
+260
| while [ "$#" -gt 0 ]; do | ||
| case "$1" in | ||
| --base-dir) BASE_DIR="$2"; shift 2 ;; | ||
| --keep) KEEP_RUNNING=1; shift ;; | ||
| --force) FORCE_REBUILD=1; shift ;; | ||
| --jobs) JOBS_TOTAL="$2"; shift 2 ;; | ||
| -h|--help) usage; exit 0 ;; | ||
| *) fail "unknown argument: $1" 4 ;; | ||
| esac | ||
| done | ||
|
|
||
| mkdir -p "${BASE_DIR}/src" \ | ||
| "${BASE_DIR}/bin" \ | ||
| "${BASE_DIR}/spock-build" \ | ||
| "${BASE_DIR}/pgdata" \ | ||
| "${BASE_DIR}/log" \ | ||
| "${BASE_DIR}/sock" \ | ||
| "${BASE_DIR}/pid" | ||
| BASE_DIR="$(cd "${BASE_DIR}" && pwd)" | ||
| SOCK_DIR="${BASE_DIR}/sock" | ||
| LOG_DIR="${BASE_DIR}/log" | ||
| PID_DIR="${BASE_DIR}/pid" | ||
|
|
||
| # Fresh log and pid directories per run: stale lines from a previous | ||
| # run would otherwise commingle with new output and make diagnosis | ||
| # painful. Deliberately scoped to log/ and pid/ -- src/, bin/, | ||
| # spock-build/, and pgdata/ are preserved so reuse-on-rerun still works. | ||
| rm -rf "${LOG_DIR}" "${PID_DIR}" | ||
| mkdir -p "${LOG_DIR}" "${PID_DIR}" |
There was a problem hiding this comment.
Guard --base-dir before the script starts deleting under it.
BASE_DIR is accepted verbatim and then used for mkdir -p plus rm -rf "${LOG_DIR}" "${PID_DIR}". A typo like --base-dir /var or --base-dir / turns this harness into a host-filesystem cleanup tool. Please reject unsafe roots (at minimum empty, /, and other top-level system paths) or require a harness-owned sentinel file before removing anything.
🤖 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 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.
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
Comment on lines
+52
to
+53
| - name: Checkout spock | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for repo in actions/checkout actions/upload-artifact; do
type="$(gh api "repos/${repo}/git/ref/tags/v4" --jq '.object.type')"
sha="$(gh api "repos/${repo}/git/ref/tags/v4" --jq '.object.sha')"
if [ "$type" = tag ]; then
sha="$(gh api "repos/${repo}/git/tags/${sha}" --jq '.object.sha')"
fi
printf '%s@v4 -> %s\n' "$repo" "$sha"
doneRepository: pgEdge/spock
Length of output: 191
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/installcheck-multi-pg.yml"
echo "== File excerpt around lines 45-85 =="
nl -ba "$FILE" | sed -n '45,85p'
echo
echo "== All uses of `@v4` in that workflow =="
rg -n "uses:\s*.*`@v4`" "$FILE" || true
echo
echo "== All uses of actions/checkout@v4 and actions/upload-artifact@v4 =="
rg -n "uses:\s*actions/(checkout|upload-artifact)`@v4`" "$FILE" || trueRepository: pgEdge/spock
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".github/workflows/installcheck-multi-pg.yml"
echo "== File excerpt around lines 45-85 =="
awk 'NR>=45 && NR<=85 {printf "%5d %s\n", NR, $0}' "$FILE"
echo
echo "== All uses of `@v4` in that workflow =="
rg -n "uses:\s*.*`@v4`" "$FILE" || true
echo
echo "== All uses of actions/checkout@v4 and actions/upload-artifact@v4 =="
rg -n "uses:\s*actions/(checkout|upload-artifact)`@v4`" "$FILE" || trueRepository: pgEdge/spock
Length of output: 1850
Pin GitHub Actions versions to immutable SHAs.
This workflow uses mutable tags:
- Line 53:
actions/checkout@v4→actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 - Line 73:
actions/upload-artifact@v4→actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 52-53: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 53-53: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 around lines 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.
Comment on lines
+338
to
+342
| if [ -d "${src}/.git" ] \ | ||
| && [ -f "${src}/src/test/regress/parallel_schedule" ]; then | ||
| log "${node}: [pg-clone] PG${ver} source already present, skipping" | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
--force does not actually force a fresh source+patch state.
clone_pg() still reuses an existing checkout, and patch_pg() still skips when the marker exists, so --force only rebuilds binaries from whatever source tree was already on disk. That contradicts the CLI contract and can silently test stale PostgreSQL or stale patches.
🛠️ Proposed fix
clone_pg() {
local ver="$1"
local node; node="$(ver_to_node "${ver}")"
local branch="REL_${ver}_STABLE"
local src; src="$(src_for "${ver}")"
+ if [ "${FORCE_REBUILD}" -ne 0 ]; then
+ rm -rf "${src}"
+ fi
+
if [ -d "${src}/.git" ] \
&& [ -f "${src}/src/test/regress/parallel_schedule" ]; then
log "${node}: [pg-clone] PG${ver} source already present, skipping"
return 0
fi
@@
patch_pg() {
local ver="$1"
local node; node="$(ver_to_node "${ver}")"
local src; src="$(src_for "${ver}")"
local patch_dir="${SPOCK_SRC}/patches/${ver}"
- if [ -f "${src}/.spock-patches-applied" ]; then
+ if [ "${FORCE_REBUILD}" -eq 0 ] && [ -f "${src}/.spock-patches-applied" ]; then
log "${node}: [pg-patch] patches already applied (marker present), skipping"
return 0
fi
run_phase "${node}" pg-patch _do_patch_pg "${ver}" "${src}" "${patch_dir}"
}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 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.
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.
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
Comment on lines
+2363
to
+2370
| foreach(rawlc, pg_parse_query(query)) | ||
| { | ||
| RawStmt *raw = lfirst_node(RawStmt, rawlc); | ||
| CreateStmt *cstmt; | ||
| ListCell *cell; | ||
|
|
||
| if (!IsA(raw->stmt, CreateStmt)) | ||
| continue; |
There was a problem hiding this comment.
EXPLAIN-wrapped CREATE TABLE ... LIKE temp can bypass temp-source skip logic.
At Line 2644 recursion keeps the original query text (still EXPLAIN ...), but query_temp_like_source() (Line 2363 onward) only checks top-level CreateStmt nodes. This can miss CREATE TABLE ... LIKE inside EXPLAIN ANALYZE, causing unsafe DDL to be queued and fail on subscribers.
💡 Suggested fix
- if (!IsA(raw->stmt, CreateStmt))
- continue;
-
- cstmt = (CreateStmt *) raw->stmt;
+ Node *stmt = raw->stmt;
+
+ /* Unwrap EXPLAIN wrapper in raw parse trees. */
+ if (IsA(stmt, ExplainStmt))
+ stmt = castNode(ExplainStmt, stmt)->query;
+
+ if (!IsA(stmt, CreateStmt))
+ continue;
+
+ cstmt = castNode(CreateStmt, stmt);Also applies to: 2644-2646
🤖 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 `@src/spock_functions.c` around lines 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.
| || { any_bad=1; log "${node}: NOT reachable -- treating as failure"; continue; } | ||
| if [ -n "${out}" ]; then | ||
| any_bad=1 | ||
| log "${node}: DISABLED subscriptions: $(echo ${out} | tr '\n' ' ')" |
There was a problem hiding this comment.
Quote variable to prevent word splitting.
The ${out} variable should be double-quoted to prevent word splitting and globbing issues.
🛠️ Suggested fix
- log "${node}: DISABLED subscriptions: $(echo ${out} | tr '\n' ' ')"
+ log "${node}: DISABLED subscriptions: $(echo "${out}" | tr '\n' ' ')"🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 717-717: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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-single-pg18-installcheck.sh` at 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.
Source: Linters/SAST tools
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.