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:
📝 WalkthroughWalkthroughAdds a runtime Spock patchset identity and initialization-time consistency check, records a per-local-node ChangesVersion Guard, Patchset Identity, and 5.x Binary-Upgrade Compatibility
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 | 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: 1
🧹 Nitpick comments (2)
src/spock_node.c (1)
607-607: Consider upgrading Assert to a runtime check for defense-in-depth.The
Assertvalidates the type only in debug builds. While the schema guaranteesint4, a corrupt catalog or manual tampering could cause silent misbehavior in release builds if the type is unexpected.🔧 Suggested defensive check
- Assert(TupleDescAttr(desc, ver_attnum - 1)->atttypid == INT4OID); + if (TupleDescAttr(desc, ver_attnum - 1)->atttypid != INT4OID) + { + systable_endscan(scan); + table_close(rel, for_update ? NoLock : RowExclusiveLock); + ereport(ERROR, + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE), + errmsg("spock.local_node.node_version has unexpected type"), + errhint("Run ALTER EXTENSION spock UPDATE."))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/spock_node.c` at line 607, The Assert call TupleDescAttr(desc, ver_attnum - 1)->atttypid == INT4OID should be converted to a runtime defensive check: read the attribute type via TupleDescAttr(desc, ver_attnum - 1)->atttypid, compare it to INT4OID, and if it does not match raise a proper error (e.g., elog(ERROR, ...)) or return a failure with a clear message referencing desc and ver_attnum instead of relying on Assert; update the surrounding function (wherever desc and ver_attnum are used) to handle the error path appropriately so callers don't proceed with an unexpected type.tests/tap/t/020_version_safety_net.pl (1)
125-130: Shell command construction could be safer.The
$sqlvariable is interpolated directly into the shell command. While all callers in this test use hardcoded SQL strings, this pattern is fragile if the test is later extended with dynamic SQL.🔧 Safer alternative using list form
sub psql_expect_error { my ($node_num, $sql) = `@_`; my $port = $cfg->{node_ports}[$node_num - 1]; - my $result = `$PG_BIN/psql -X -p $port -d regression -t -c "$sql" 2>&1`; + my `@cmd` = ("$PG_BIN/psql", "-X", "-p", $port, "-d", "regression", "-t", "-c", $sql); + open(my $fh, "-|", `@cmd`, "2>&1") or die "Cannot run psql: $!"; + my $result = do { local $/; <$fh> }; + close($fh); return $result; }Alternatively, consider using
IPC::Runor Perl'sqx{}with proper escaping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/020_version_safety_net.pl` around lines 125 - 130, In psql_expect_error, avoid interpolating $sql into a single-shell backtick command; instead construct the psql invocation without the shell by using IPC::Run (e.g., IPC::Run::run) or Perl's list form system/open3 to pass arguments (including "-c", $sql) so the SQL isn't interpreted by the shell, or at minimum escape $sql with quotemeta if switching to IPC::Run isn't possible; update psql_expect_error to call $PG_BIN/psql with arguments (port, database, "-t", "-c", $sql) via IPC::Run or a safe argument list to eliminate shell interpolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/tap/schedule`:
- Line 44: The entry "020_version_safety_net" in the TAP schedule is missing the
required "test:" prefix so the schedule parser skips it; update the schedule so
the line reads with the prefix (e.g. "test: 020_version_safety_net") so the
schedule parser and check_prove will pick up and execute the test.
---
Nitpick comments:
In `@src/spock_node.c`:
- Line 607: The Assert call TupleDescAttr(desc, ver_attnum - 1)->atttypid ==
INT4OID should be converted to a runtime defensive check: read the attribute
type via TupleDescAttr(desc, ver_attnum - 1)->atttypid, compare it to INT4OID,
and if it does not match raise a proper error (e.g., elog(ERROR, ...)) or return
a failure with a clear message referencing desc and ver_attnum instead of
relying on Assert; update the surrounding function (wherever desc and ver_attnum
are used) to handle the error path appropriately so callers don't proceed with
an unexpected type.
In `@tests/tap/t/020_version_safety_net.pl`:
- Around line 125-130: In psql_expect_error, avoid interpolating $sql into a
single-shell backtick command; instead construct the psql invocation without the
shell by using IPC::Run (e.g., IPC::Run::run) or Perl's list form system/open3
to pass arguments (including "-c", $sql) so the SQL isn't interpreted by the
shell, or at minimum escape $sql with quotemeta if switching to IPC::Run isn't
possible; update psql_expect_error to call $PG_BIN/psql with arguments (port,
database, "-t", "-c", $sql) via IPC::Run or a safe argument list to eliminate
shell interpolation.
🪄 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: cd68e04c-3e47-4656-994f-972c26f5a0ea
⛔ Files ignored due to path filters (1)
tests/regress/expected/version_guard.outis excluded by!**/*.out
📒 Files selected for processing (12)
Makefilepatches/15/pg15-000-spock-patchset-version.diffpatches/16/pg16-000-spock-patchset-version.diffpatches/17/pg17-000-spock-patchset-version.diffpatches/18/pg18-000-spock-patchset-version.diffsql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_node.ctests/regress/sql/version_guard.sqltests/tap/scheduletests/tap/t/020_version_safety_net.pl
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sql/spock--5.0.6--6.0.0-devel.sql (1)
435-437: Prefer dropping theDEFAULT 0after the backfill.The column definition leaves
node_versionwithDEFAULT 0, but runtime validation (insrc/spock_node.c:569-622) rejects any value that is NULL or not equal toSPOCK_VERSION_NUM. While the only existing insert path (C code insrc/spock_node.c:459) explicitly providesSPOCK_VERSION_NUM, the default is misleading and creates a trap for future code: any new insert path that omits the column would silently get0and immediately fail validation.Dropping the DEFAULT after the backfill enforces explicit provision at all insert sites:
Suggested migration shape
ALTER TABLE spock.local_node ADD COLUMN IF NOT EXISTS node_version int4 NOT NULL DEFAULT 0; UPDATE spock.local_node SET node_version = spock.spock_version_num(); +ALTER TABLE spock.local_node + ALTER COLUMN node_version DROP DEFAULT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/spock--5.0.6--6.0.0-devel.sql` around lines 435 - 437, Add a DDL step to remove the misleading default after the backfill: after updating spock.local_node.node_version with spock.spock_version_num(), run an ALTER TABLE on spock.local_node to DROP DEFAULT for the node_version column so future inserts must explicitly provide a value; reference the table/column names (spock.local_node, node_version) and the backfill call (spock.spock_version_num()) so the DROP DEFAULT is applied immediately after the UPDATE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patches/18/pg18-000-spock-patchset-version.diff`:
- Around line 26-36: The definition of SpockCorePatchsetVersion in globals.c
must match the declaration's const qualifier; change the definition to "const
int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;" so the symbol
SpockCorePatchsetVersion exactly matches the extern PGDLLIMPORT const int
declaration from miscadmin.h (also check and apply the same const fix in the
other PG15–17 patch files where SpockCorePatchsetVersion is defined).
---
Nitpick comments:
In `@sql/spock--5.0.6--6.0.0-devel.sql`:
- Around line 435-437: Add a DDL step to remove the misleading default after the
backfill: after updating spock.local_node.node_version with
spock.spock_version_num(), run an ALTER TABLE on spock.local_node to DROP
DEFAULT for the node_version column so future inserts must explicitly provide a
value; reference the table/column names (spock.local_node, node_version) and the
backfill call (spock.spock_version_num()) so the DROP DEFAULT is applied
immediately after the UPDATE.
🪄 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: 14cd7344-6be0-4e8d-9f9d-d4a909643982
📒 Files selected for processing (2)
patches/18/pg18-000-spock-patchset-version.diffsql/spock--5.0.6--6.0.0-devel.sql
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/tap/t/020_version_safety_net.pl (2)
125-130:psql_expect_errordoes not verify that psql actually failed.The helper returns combined output but never inspects the exit status, so a scenario where
spock.node_info()unexpectedly succeeds would still pass as long as the stdout happens to match the pattern (e.g., unlikely but masked regressions). Consider capturing$?and asserting non-zero, or usingIPC::Run/Test::More::okon the exit status in addition to the message-pattern checks.♻️ Optional hardening
sub psql_expect_error { my ($node_num, $sql) = `@_`; my $port = $cfg->{node_ports}[$node_num - 1]; my $result = `$PG_BIN/psql -X -p $port -d regression -t -c "$sql" 2>&1`; + my $rc = $? >> 8; + note("psql exited with $rc; output: $result") if $rc == 0; return $result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/020_version_safety_net.pl` around lines 125 - 130, psql_expect_error currently returns psql's combined output but never checks the exit status, so modify the function (psql_expect_error) to capture the exit status after the backtick call (check $?) and ensure it is non-zero; if the status is zero, fail/assert (e.g., croak/die or use Test::More::ok) so tests don't silently accept a successful psql run, and otherwise return the output (or return both output and status) so callers can still inspect the error message from $PG_BIN/psql using the configured $cfg->{node_ports}[$node_num - 1].
106-113: Scenario 5 restores the column withDEFAULT 0, which diverges from the canonical schema.
sql/spock--6.0.0-devel.sqldefinesnode_version int4 NOT NULL DEFAULT 0, so the literal column definition here matches. However, on a broader note: if the canonical schema ever changes the default (e.g., to removeDEFAULT 0once upgrade is complete), this test will silently drift. Consider a short comment pointing at the authoritative definition, or dropping the default after theUPDATE, so the restored table matches production state more closely. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/020_version_safety_net.pl` around lines 106 - 113, The test restores spock.local_node.node_version with DEFAULT 0 which may diverge from the authoritative schema; after the ALTER TABLE/UPDATE block (the ALTER TABLE spock.local_node ADD COLUMN node_version ... and UPDATE spock.local_node SET node_version = spock.spock_version_num()), remove the literal DEFAULT by issuing an ALTER TABLE ... ALTER COLUMN node_version DROP DEFAULT so the test leaves the column in the same default state as production, and add a short inline comment referencing sql/spock--6.0.0-devel.sql to indicate the canonical definition being mirrored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patches/16/pg16-000-spock-patchset-version.diff`:
- Around line 17-28: The declaration in the header uses "extern PGDLLIMPORT
const int SpockCorePatchsetVersion;" but the definition in globals.c is
non-const; update the definition in globals.c (symbol SpockCorePatchsetVersion)
to be const to match the header (i.e., define it as a const int initialized to
SPOCK_CORE_PATCHSET_VERSION), and apply the same const-fix to any sibling
patches (PG15/17/18) where SpockCorePatchsetVersion is defined.
In `@tests/tap/t/020_version_safety_net.pl`:
- Line 3: The test file declares Test::More tests => 10 but only runs 9
assertions causing a TAP failure; either change the plan to tests => 9 or add
the missing assertion in Scenario 3: insert a second assertion mirroring
Scenarios 2 and 4 that checks for the "ALTER EXTENSION spock UPDATE" hint (e.g.,
a like() on the Scenario 3 output similar to the existing like() calls),
ensuring the total assertion count matches the plan.
---
Nitpick comments:
In `@tests/tap/t/020_version_safety_net.pl`:
- Around line 125-130: psql_expect_error currently returns psql's combined
output but never checks the exit status, so modify the function
(psql_expect_error) to capture the exit status after the backtick call (check
$?) and ensure it is non-zero; if the status is zero, fail/assert (e.g.,
croak/die or use Test::More::ok) so tests don't silently accept a successful
psql run, and otherwise return the output (or return both output and status) so
callers can still inspect the error message from $PG_BIN/psql using the
configured $cfg->{node_ports}[$node_num - 1].
- Around line 106-113: The test restores spock.local_node.node_version with
DEFAULT 0 which may diverge from the authoritative schema; after the ALTER
TABLE/UPDATE block (the ALTER TABLE spock.local_node ADD COLUMN node_version ...
and UPDATE spock.local_node SET node_version = spock.spock_version_num()),
remove the literal DEFAULT by issuing an ALTER TABLE ... ALTER COLUMN
node_version DROP DEFAULT so the test leaves the column in the same default
state as production, and add a short inline comment referencing
sql/spock--6.0.0-devel.sql to indicate the canonical definition being mirrored.
🪄 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: 3a1c47b6-54b2-4ad8-a237-52c194b7cdd8
⛔ Files ignored due to path filters (1)
tests/regress/expected/version_guard.outis excluded by!**/*.out
📒 Files selected for processing (13)
Makefilepatches/15/pg15-000-spock-patchset-version.diffpatches/16/pg16-000-spock-patchset-version.diffpatches/17/pg17-000-spock-patchset-version.diffpatches/18/pg18-000-spock-patchset-version.diffsql/spock--5.0.6--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_node.ctests/regress/sql/version_guard.sqltests/tap/scheduletests/tap/t/002_create_subscriber.pltests/tap/t/020_version_safety_net.pl
✅ Files skipped from review due to trivial changes (2)
- sql/spock--6.0.0-devel.sql
- tests/regress/sql/version_guard.sql
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/tap/schedule
- Makefile
- src/spock.c
- patches/15/pg15-000-spock-patchset-version.diff
- sql/spock--5.0.6--6.0.0-devel.sql
- patches/17/pg17-000-spock-patchset-version.diff
- patches/18/pg18-000-spock-patchset-version.diff
- src/spock_node.c
danolivo
force-pushed
the
spoc-504
branch
5 times, most recently
from
9cd28e3 to
d40434a
Compare
danolivo
force-pushed
the
spoc-504
branch
5 times, most recently
from
d0043ea to
ef6ada3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/spock.c (1)
56-57: 💤 Low valueOptional: move
register_spock_compat_5xdeclaration into a shared header.An inline
externinspock.cworks but spreads the function's contract between the .c file and its single caller. Putting the prototype inspock.h(or a smallspock_bucompat_5x.h) keeps function declarations centralized and avoids drift if a second caller is ever added.🤖 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.c` around lines 56 - 57, Move the inline extern prototype for register_spock_compat_5x out of src/spock.c and into a shared header (e.g., declare it in spock.h or create spock_bucompat_5x.h) and then `#include` that header from src/spock.c; remove the redundant extern declaration from spock.c so the function contract is centralized and available to any future callers.tests/regress/sql/version_guard.sql (1)
1-43: ⚡ Quick winConsider adding the missing-column scenario for full guard coverage.
get_local_node()has two distinct error paths: value-mismatch ("spock version mismatch") and missing-column ("spock extension schema outdated"). This test covers only the value-mismatch path (vianode_version = 0and999999). The DROP-COLUMN path is the more interesting safety case — it's what protects againstnode_versionbeing lost via VACUUM FULL renumbering or an explicit DROP — and the relevant code-segment lookup-by-name vs. positional access is documented as the reason for the name-based scan insrc/spock_node.c.The TAP test reportedly covers it, but exercising both paths in regress keeps the schedule self-contained and keeps the safety-net coverage close to the schema migration that introduces the column.
♻️ Suggested addition
-- Restore before DDL. UPDATE spock.local_node SET node_version = spock.spock_version_num(); + +-- --------------------------------------------------------------- +-- Scenario: schema outdated -- node_version column dropped +-- (simulates pre-6.0 schema with a 6.x binary). +-- --------------------------------------------------------------- +ALTER TABLE spock.local_node DROP COLUMN node_version; + +\set VERBOSITY terse +SELECT * FROM spock.node_info(); +\set VERBOSITY default + +-- Restore the column for any subsequent regression tests. +ALTER TABLE spock.local_node + ADD COLUMN node_version int4 NOT NULL DEFAULT 0; +UPDATE spock.local_node SET node_version = spock.spock_version_num();🤖 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/regress/sql/version_guard.sql` around lines 1 - 43, Add a regression case that exercises the "missing-column" error path for get_local_node(): simulate dropping the node_version column from spock.local_node (or otherwise making it absent) and then call spock.node_info() to verify it raises the "spock extension schema outdated" error; after the check, restore the schema by recreating or resetting node_version to spock.spock_version_num() so subsequent DDL tests run. Place the new steps near the existing version-tampering scenarios in tests/regress/sql/version_guard.sql and reference get_local_node()/spock.node_info(), spock.local_node.node_version, and the rationale in src/spock_node.c when adding the test. Ensure verbosity is set to terse around the failing call as done for the other scenarios.
🤖 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/conflicts.md`:
- Around line 41-61: Replace the fenced SQL code blocks with indented (4-space)
code blocks to satisfy MD046: convert the three SELECT spock.delta_apply(...)
snippets, the to_drop => true example, and the final SELECT * FROM pg_seclabel
... snippet by removing the triple-backtick fences and indenting each SQL line
with four spaces; keep the SQL text unchanged (including spock.delta_apply,
provider = 'spock', label = 'spock.delta_apply') so linting passes.
- Line 64: The heading "#### Upgrading from spock 5.x" is one level too deep
(MD001); change that heading to "### Upgrading from spock 5.x" to restore proper
hierarchy so it follows the surrounding section structure.
In `@docs/internals-doc/binary-upgrade-compat-shim.md`:
- Around line 34-40: In docs/internals-doc/binary-upgrade-compat-shim.md update
all fenced code blocks to include language identifiers (e.g., use ```text for
file layout/log output or appropriate language for snippets) so markdownlint
MD040 is satisfied; specifically tag the block showing the file list (the one
containing src/spock_bucompat_5x.c, src/spock.c, sql/spock--6.0.0-devel.sql,
docs/... ) and the other fenced blocks referenced around the same area
(including the blocks corresponding to the content at lines ~109-111) with the
correct language identifiers.
In `@sql/spock--5.0.6--5.0.7.sql`:
- Around line 140-163: Before performing the direct catalog mutations on
pg_catalog.pg_attribute and pg_catalog.pg_statistic for table spock.subscription
and column sub_skip_schema, add a session-local GUC by issuing "SET LOCAL
allow_system_table_mods = on;" so the UPDATE and DELETE are permitted for
superusers; place this SET LOCAL immediately before the LOCK TABLE ... and the
catalog statements and ensure it applies to the same transaction/scope so no
other permission changes are required.
---
Nitpick comments:
In `@src/spock.c`:
- Around line 56-57: Move the inline extern prototype for
register_spock_compat_5x out of src/spock.c and into a shared header (e.g.,
declare it in spock.h or create spock_bucompat_5x.h) and then `#include` that
header from src/spock.c; remove the redundant extern declaration from spock.c so
the function contract is centralized and available to any future callers.
In `@tests/regress/sql/version_guard.sql`:
- Around line 1-43: Add a regression case that exercises the "missing-column"
error path for get_local_node(): simulate dropping the node_version column from
spock.local_node (or otherwise making it absent) and then call spock.node_info()
to verify it raises the "spock extension schema outdated" error; after the
check, restore the schema by recreating or resetting node_version to
spock.spock_version_num() so subsequent DDL tests run. Place the new steps near
the existing version-tampering scenarios in tests/regress/sql/version_guard.sql
and reference get_local_node()/spock.node_info(), spock.local_node.node_version,
and the rationale in src/spock_node.c when adding the test. Ensure verbosity is
set to terse around the failing call as done for the other scenarios.
🪄 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: c6568bd7-650f-4b51-aab7-5558abe2375d
⛔ Files ignored due to path filters (1)
tests/regress/expected/version_guard.outis excluded by!**/*.out
📒 Files selected for processing (20)
Makefiledocs/conflicts.mddocs/internals-doc/binary-upgrade-compat-shim.mddocs/troubleshooting.mdpatches/15/pg15-000-spock-patchset-version.diffpatches/16/pg16-000-spock-patchset-version.diffpatches/17/pg17-000-spock-patchset-version.diffpatches/18/pg18-000-spock-patchset-version.diffsql/spock--5.0.0.sqlsql/spock--5.0.6--5.0.7.sqlsql/spock--5.0.7--6.0.0-devel.sqlsql/spock--6.0.0-devel.sqlsrc/spock.csrc/spock_apply_heap.csrc/spock_bucompat_5x.csrc/spock_node.ctests/regress/sql/version_guard.sqltests/tap/scheduletests/tap/t/002_create_subscriber.pltests/tap/t/020_version_safety_net.pl
💤 Files with no reviewable changes (1)
- src/spock_apply_heap.c
✅ Files skipped from review due to trivial changes (4)
- sql/spock--6.0.0-devel.sql
- tests/tap/schedule
- Makefile
- patches/16/pg16-000-spock-patchset-version.diff
🚧 Files skipped from review as they are similar to previous changes (5)
- patches/17/pg17-000-spock-patchset-version.diff
- patches/18/pg18-000-spock-patchset-version.diff
- tests/tap/t/002_create_subscriber.pl
- src/spock_node.c
- tests/tap/t/020_version_safety_net.pl
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docs/conflicts.md (1)
41-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThese fenced SQL blocks still trip the repo’s MD046 rule.
The three SQL examples here are still fenced, so markdownlint will keep flagging this section until they are converted to indented code blocks.
🤖 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/conflicts.md` around lines 41 - 62, Replace the fenced ```sql blocks with indented code blocks (remove the triple-backticks and indent each line by four spaces) for the examples that call spock.delta_apply(...) (including the variant with to_drop => true) and the final SELECT from pg_seclabel so they become indented code blocks that satisfy MD046; ensure indentation is applied to every line of those three SQL snippets and no language fence remains.
🧹 Nitpick comments (1)
src/spock.c (1)
965-971: ⚡ Quick winAdd a remediation hint to the patchset-mismatch error.
This new guard will fail very early, so operators need the next step in the error itself. An
errhintpointing them to install the matching patched PostgreSQL build or rebuild the extension would make upgrade failures much less opaque.🛠️ Suggested improvement
if (SpockCorePatchsetVersion != SPOCK_CORE_PATCHSET_VERSION) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("spock core patchset version mismatch: " "server has v%d, extension expects v%d", SpockCorePatchsetVersion, - SPOCK_CORE_PATCHSET_VERSION))); + SPOCK_CORE_PATCHSET_VERSION), + errhint("Install the matching Spock-patched PostgreSQL binaries for this extension build, or rebuild the extension against the running server.")));🤖 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.c` around lines 965 - 971, The error raised when SpockCorePatchsetVersion != SPOCK_CORE_PATCHSET_VERSION lacks a remediation hint; update the ereport call in src/spock.c (the block using SpockCorePatchsetVersion, SPOCK_CORE_PATCHSET_VERSION and ereport(ERROR,...)) to include an errhint guiding operators to either install the matching patched PostgreSQL build or rebuild/reinstall the extension against the server's patchset version so upgrade failures show the next steps directly in the error message.
🤖 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/conflicts.md`:
- Around line 72-73: The documented canonical audit query is too broad: the
current query "SELECT * FROM pg_seclabel WHERE provider = 'spock'" omits the
label restriction and will over-report; update the query to include the label
predicate (e.g., add "AND label = 'spock.delta_apply'") so it matches the
definition of delta-apply columns and only returns rows where provider = 'spock'
and label = 'spock.delta_apply'.
In `@src/spock_bucompat_5x.c`:
- Around line 160-171: The ProcessUtility call uses a hardcoded
PROCESS_UTILITY_SUBCOMMAND which prevents top-level-only hooks from observing
the synthetic statement; change that call so it passes the original context
parameter instead of PROCESS_UTILITY_SUBCOMMAND (i.e. invoke
ProcessUtility(synth_pstmt, NULL, false, context, params, queryEnv, dest,
NULL)), keeping synth_pstmt, params, queryEnv and dest as-is so other registered
hooks (e.g. spock_autoddl.c checks) will see the statement in the same context
as the caller.
---
Duplicate comments:
In `@docs/conflicts.md`:
- Around line 41-62: Replace the fenced ```sql blocks with indented code blocks
(remove the triple-backticks and indent each line by four spaces) for the
examples that call spock.delta_apply(...) (including the variant with to_drop =>
true) and the final SELECT from pg_seclabel so they become indented code blocks
that satisfy MD046; ensure indentation is applied to every line of those three
SQL snippets and no language fence remains.
---
Nitpick comments:
In `@src/spock.c`:
- Around line 965-971: The error raised when SpockCorePatchsetVersion !=
SPOCK_CORE_PATCHSET_VERSION lacks a remediation hint; update the ereport call in
src/spock.c (the block using SpockCorePatchsetVersion,
SPOCK_CORE_PATCHSET_VERSION and ereport(ERROR,...)) to include an errhint
guiding operators to either install the matching patched PostgreSQL build or
rebuild/reinstall the extension against the server's patchset version so upgrade
failures show the next steps directly in the error message.
🪄 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: bcde85df-9252-4bfa-92a8-159a8b1ffc0a
📒 Files selected for processing (4)
docs/conflicts.mddocs/troubleshooting.mdsrc/spock.csrc/spock_bucompat_5x.c
danolivo
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
patches/16/pg16-000-spock-patchset-version.diff (1)
22-23: ⚡ Quick winConsider adding
constto both the declaration and definition for immutability.
SpockCorePatchsetVersionis a fixed compile-time identity that should never change after server start. Withoutconst, nothing prevents accidental writes to it at runtime. The previous review resolved the mismatch by droppingconstfrom both sides, but adding it back consistently to both the declaration and definition would make the intent clearer and allow compiler optimizations.♻️ Proposed change
-+extern PGDLLIMPORT int SpockCorePatchsetVersion; ++extern PGDLLIMPORT const int SpockCorePatchsetVersion;-+int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; ++const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;🤖 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 `@patches/16/pg16-000-spock-patchset-version.diff` around lines 22 - 23, Make SpockCorePatchsetVersion immutable by adding const to both its declaration and its definition: change the declaration "extern PGDLLIMPORT int SpockCorePatchsetVersion;" to "extern PGDLLIMPORT const int SpockCorePatchsetVersion;" and update the corresponding definition in the C file to "const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;" so the compile-time identity is enforced and consistent with the SPOCK_CORE_PATCHSET_VERSION macro.
🤖 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.
Nitpick comments:
In `@patches/16/pg16-000-spock-patchset-version.diff`:
- Around line 22-23: Make SpockCorePatchsetVersion immutable by adding const to
both its declaration and its definition: change the declaration "extern
PGDLLIMPORT int SpockCorePatchsetVersion;" to "extern PGDLLIMPORT const int
SpockCorePatchsetVersion;" and update the corresponding definition in the C file
to "const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;" so the
compile-time identity is enforced and consistent with the
SPOCK_CORE_PATCHSET_VERSION macro.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e58e3bc1-e14a-4618-84dc-55d3fc90759e
📒 Files selected for processing (4)
patches/15/pg15-000-spock-patchset-version.diffpatches/16/pg16-000-spock-patchset-version.diffpatches/17/pg17-000-spock-patchset-version.diffpatches/18/pg18-000-spock-patchset-version.diff
✅ Files skipped from review due to trivial changes (3)
- patches/15/pg15-000-spock-patchset-version.diff
- patches/17/pg17-000-spock-patchset-version.diff
- patches/18/pg18-000-spock-patchset-version.diff
danolivo
force-pushed
the
spoc-504
branch
4 times, most recently
from
202eb83 to
1f78b31
Compare
danolivo
force-pushed
the
spoc-504
branch
2 times, most recently
from
fcaee93 to
7d24c90
Compare
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
I did an initial pass and added some comments.
| + * Spock core-patchset identity. Bump the version when the patchset | ||
| + * changes in a way visible to the extension binary. | ||
| + */ | ||
| +#define SPOCK_CORE_PATCHSET_VERSION 1 |
There was a problem hiding this comment.
I think it is smart idea in general. It does replace one patch with another though (but smaller). I am thinking one day we could have a spock-lite with reduced functionality that would work with plain Postgres. I will think about this some more.
There was a problem hiding this comment.
@mason-sharp ,
It is trivial to guard our patches and the corresponding Spock machinery. That enables Spock to be used with upstream Postgres. But for production use, at least one patch (logical clock) is a requirement. So, for the safety and predictability, we should have a mechanism to detect patched postgres.
| @@ -0,0 +1,790 @@ | |||
| \echo Use "CREATE EXTENSION spock" to load this file. \quit | |||
There was a problem hiding this comment.
I don't think we need this file.
If users first start with 6.0.0, they would use spock--6.0.0.sql.
If users have 5.0.x, then they are at some version >= 5.0.0 already.
There was a problem hiding this comment.
Done. Although it is out of community coding standards, it is legal. So, I removed the base SQL script.
| node_id oid PRIMARY KEY REFERENCES node(node_id), | ||
| node_local_interface oid NOT NULL REFERENCES node_interface(if_id) | ||
| node_local_interface oid NOT NULL REFERENCES node_interface(if_id), | ||
| node_version int4 NOT NULL DEFAULT 0 |
There was a problem hiding this comment.
A version check is a good idea, but can't we use pg_extension.extversion and simplify this? It could still be fetched in get_local_node() and catch when the .so does not match the last recorded version from CREATE EXTENSION or ALTER EXTENSION. Perhaps spock_manager_main() could check, and delay to avoid too many error messages in the log.
There was a problem hiding this comment.
The pg_extension.extversion doesn't simplify. Our purpose is to determine which Spock version was installed when the node was created. That's crucial because different versions can leave distinct fingerprints in the system. If you know how to avoid it and guarantee the upgrade, please share the details.
There was a problem hiding this comment.
If we run ALTER EXTENSION UPDATE it will update both node_version here as well as pg_extension.extversion. Before running it, these two value would be the same I think.
| /*------------------------------------------------------------------------- | ||
| * | ||
| * spock_bucompat_5x.c | ||
| * Binary-upgrade compatibility: spock 5.x -> 6.x. |
There was a problem hiding this comment.
I like this, but I wonder if it would be simpler to just require on an upgrade (if we loosen the patch set versioning) that users must run a SQL upgrade script and then do a regular pg_upgrade after.
OTOH, if we defer until PG 19 only, this file as part of the upgrade process seems very useful/required.
Just adding comments while thinking about it.
There was a problem hiding this comment.
Right now, we keep things simple and don't require users to do anything beyond the standard pg_upgrade command. Not sure that the additional step is safer. One more reason: a separate module might be easily cut from the codebase when we decide not to support upgrades from 5.x anymore.
danolivo
force-pushed
the
spoc-504
branch
2 times, most recently
from
240458c to
58ed81a
Compare
Add a single integer (SPOCK_CORE_PATCHSET_VERSION compile-time, and SpockCorePatchsetVersion runtime global) in miscadmin.h and globals.c on every supported PG branch (15-18). This gives the spock extension a binary-level handshake with the patched server: an unpatched server fails to dynamic-link, and a server patched against a different generation produces a clear runtime mismatch later. No behaviour change yet -- the consumer side lands in the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In _PG_init, compare the runtime SpockCorePatchsetVersion exposed by the patched server against the SPOCK_CORE_PATCHSET_VERSION the extension was compiled against, and ereport() if they disagree. Catches the "extension binary upgraded but server binary still on the old patchset" footgun before any worker starts, so the failure mode is a clean error at LOAD instead of a subtle later crash. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 6.0.0 extension shipped only a direct spock--5.0.8--6.0.0.sql
shortcut. But origin/v5_STABLE now ships 5.0.10, and a cluster pinned at
5.0.9 or 5.0.10 has no update path to 6.0.0: ALTER EXTENSION spock UPDATE
(driven on connect by the manager worker) dead-ends with "no update path
from 5.0.10 to 6.0.0" after a pg_upgrade.
Replace the shortcut with the incremental chain that mirrors v5_STABLE's
own version scripts:
- add no-op spock--5.0.8--5.0.9.sql and spock--5.0.9--5.0.10.sql
(neither release changed the schema), and
- move the full 5.x -> 6.0.0 migration into spock--5.0.10--6.0.0.sql,
applied at the final step of the chain.
ALTER EXTENSION spock UPDATE now walks 5.0.8 -> 5.0.9 -> 5.0.10 -> 6.0.0
in a single command regardless of which 5.0.x the cluster is pinned at,
and future stable point releases extend the chain with another no-op hop
rather than a new direct-to-6.0.0 script.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ion DB
Drive the real pg_upgrade path for spock 5.x -> 6.x as a TAP test:
clone the local PostgreSQL and spock trees, build a v5 (origin/v5_STABLE)
variant and a HEAD variant side by side, run pg_upgrade between them, and
assert the upgrade is correct.
6.0.0 represents delta_apply with the same per-attribute reloption
(log_old_value / delta_apply_function) that v5 used, so a correct upgrade
carries the attoption across verbatim -- there is no shim to exercise.
The test:
- populates a regression database via core 'make installcheck' on the
old cluster and asserts it survives the upgrade with the same set of
user relations;
- marks columns in a custom database with the reloption form and asserts
the attoption survives byte-for-byte, naming a live spock.delta_apply();
- drives ALTER EXTENSION spock UPDATE as a hard assertion and checks
pg_extension.extversion (not just the C-library version) reaches 6.0.0.
Test-infra robustness so it runs under 'make check_prove':
- scrub inherited CFLAGS/CPPFLAGS (PGXS exports -Werror=vla and -I paths
into the target install) before the nested PG configure/build, which
otherwise fail the C99 probe and shadow freshly-built headers;
- generate derived headers before the parallel build to avoid the
src/common vs src/backend generated-header race on a fresh tree.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switch the post-INSERT wait from spock.sub_wait_for_sync (which polls for "caught up" and races with apply progress on busy CI) to the deterministic sync_event-on-provider / wait_for_sync_event-on-subscriber pattern. Removes a known source of flakiness on the buildfarm; pure test-only change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
origin/v5_STABLE now ships 5.0.10, so the 018 upgrade-schema-match test (which builds the old node from v5_STABLE) starts at 5.0.10, not 5.0.8 -- its "starts at 5.0.8" version assertion would otherwise fail. Retarget the test's version references accordingly. Generalise the release-note Upgrading section: the single ALTER EXTENSION spock UPDATE now walks the 5.0.8 -> 5.0.9 -> 5.0.10 -> 6.0.0 chain, so it applies to any 5.0.8-or-later release, not just 5.0.8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
danolivo
changed the title
2 participants
Summary
This branch closes two gaps that surface when a node built on the
v5_STABLEline is pg_upgraded to 6.0.0:ALTER EXTENSION spock UPDATEpath from the current stable line. The 6.0.0 extension shipped only a directspock--5.0.8--6.0.0.sqlshortcut, butorigin/v5_STABLEnow ships 5.0.10. A cluster pinned at 5.0.9 or 5.0.10 dead-ended onALTER EXTENSION spock UPDATEwith"no update path from 5.0.10 to 6.0.0"— and because the manager worker drives that UPDATE on connect, the worker crash-loops after the upgrade.It also adds a TAP test that drives the real pg_upgrade path end to end, plus supporting test/doc fixes.
Changes
Core patchset version handshake
patches/{15,16,17,18}/*-spock-patchset-version.diff— add a compile-timeSPOCK_CORE_PATCHSET_VERSIONand a runtimeSpockCorePatchsetVersionglobal (inmiscadmin.h/globals.c) on every supported PG branch. An unpatched server fails to dynamic-link the symbol; a server patched against a different generation reports a clear mismatch.src/spock.c— in_PG_init, compare the runtimeSpockCorePatchsetVersionagainst theSPOCK_CORE_PATCHSET_VERSIONthe extension was compiled against andereport()on disagreement. The failure mode becomes a clean error at LOAD rather than a later crash.Stepped 5.0.x → 6.0.0 upgrade chain
spock--5.0.8--6.0.0.sqlshortcut.spock--5.0.8--5.0.9.sqlandspock--5.0.9--5.0.10.sql(neither stable release changed the schema), mirroringv5_STABLE's own version scripts.spock--5.0.10--6.0.0.sql(body unchanged from the old shortcut), applied at the final step of the chain.ALTER EXTENSION spock UPDATEnow walks5.0.8 → 5.0.9 → 5.0.10 → 6.0.0in a single command regardless of which 5.0.x the cluster is pinned at. Future stable point releases extend the chain with another no-op hop instead of needing a fresh direct-to-6.0.0 script.Tests and docs
tests/tap/t/030_pg_upgrade_5x_to_6x.pl(new) — builds av5(origin/v5_STABLE) and aHEADvariant side by side, runs the real pg_upgrade between them, and asserts: a regression DB survives with the same user relations; the per-attributedelta_applyreloption (log_old_value/delta_apply_function) survives byte-for-byte; andALTER EXTENSION spock UPDATEdrivespg_extension.extversionto 6.0.0. Includes CFLAGS/CPPFLAGS scrubbing and pre-generated headers so it runs undermake check_prove.tests/tap/t/018_upgrade_schema_match.pl— retarget the upgrade-from version from 5.0.8 to 5.0.10 (the test builds the old node fromv5_STABLE, which now ships 5.0.10; its"starts at 5.0.8"assertion would otherwise fail).tests/tap/t/002_create_subscriber.pl— replace the flakyspock.sub_wait_for_syncpoll with the deterministicsync_event/wait_for_sync_eventpattern.docs/spock_release_notes.md— generalise the Upgrading section: the singleALTER EXTENSION spock UPDATEnow applies to any 5.0.8-or-later release and walks the chain automatically.Compatibility / upgrade impact