Conversation
📝 WalkthroughWalkthroughAdds a Spock patchset/version identity and runtime guard, records per-local-node version in the catalog with migration steps, enforces binary/schema version checks at startup and runtime, and adds regression/TAP tests plus Makefile/schedule updates to exercise mismatches and recovery. Changes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
TIP This summary will be updated as you push new changes. Give us feedback
![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
| +#define SPOCK_CORE_PATCHSET_VERSION 1 | ||
| +extern PGDLLIMPORT const int SpockCorePatchsetVersion; | ||
| + | ||
| #endif /* MISCADMIN_H */ | ||
| --- a/src/backend/utils/init/globals.c | ||
| +++ b/src/backend/utils/init/globals.c | ||
| @@ -157,6 +157,8 @@ | ||
| int VacuumCostBalance = 0; /* working state for vacuum */ | ||
| bool VacuumCostActive = false; | ||
|
|
||
| +int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; |
There was a problem hiding this comment.
Const qualifier mismatch between declaration and definition.
miscadmin.h declares extern PGDLLIMPORT const int SpockCorePatchsetVersion; (line 27), but globals.c defines it as non-const int SpockCorePatchsetVersion = ...; (line 36). This is a type mismatch on an external linkage object, which is undefined behavior in C and will typically produce a compiler/linker error (or at minimum, a warning that will break -Werror builds). The same issue likely exists in the PG15–17 patches in this PR.
🔧 Proposed fix
-+int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;
++const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;📝 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.
| +#define SPOCK_CORE_PATCHSET_VERSION 1 | |
| +extern PGDLLIMPORT const int SpockCorePatchsetVersion; | |
| + | |
| #endif /* MISCADMIN_H */ | |
| --- a/src/backend/utils/init/globals.c | |
| +++ b/src/backend/utils/init/globals.c | |
| @@ -157,6 +157,8 @@ | |
| int VacuumCostBalance = 0; /* working state for vacuum */ | |
| bool VacuumCostActive = false; | |
| +int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; | |
| `#define` SPOCK_CORE_PATCHSET_VERSION 1 | |
| extern PGDLLIMPORT const int SpockCorePatchsetVersion; | |
| `#endif` /* MISCADMIN_H */ | |
| ++ b/src/backend/utils/init/globals.c | |
| @@ -157,6 +157,8 @@ | |
| int VacuumCostBalance = 0; /* working state for vacuum */ | |
| bool VacuumCostActive = false; | |
| const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/18/pg18-000-spock-patchset-version.diff` around lines 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).
Two complementary checks prevent running Spock against an incompatible server or with stale catalog state: 1. Core patchset check: the pg18-000 patch exports SpockCorePatchsetVersion via miscadmin.h / globals.c. The extension references it in _PG_init(); an unpatched server fails at dlopen (undefined symbol), a version skew fails with ERROR. 2. Node version check: spock.local_node gains a node_version column populated with SPOCK_VERSION_NUM at node creation. get_local_node() compares it on every call -- a missing column (old schema) or a mismatched value triggers ERROR advising ALTER EXTENSION UPDATE.
The positional check (desc->natts < 3) failed to detect a dropped node_version column: DROP COLUMN marks the attribute as dropped but does not reduce natts, so fastgetattr silently read stale bytes and the version check passed incorrectly. VACUUM FULL makes it worse by renumbering attributes entirely. Replace with a name-and-type scan of the tuple descriptor that finds a live attribute called "node_version" with type int4. Add a SQL regression test (version_guard) and a TAP test (020_version_safety_net) covering correct version, tampered value, future value, dropped column, and recovery scenarios.
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
| +#define SPOCK_CORE_PATCHSET_VERSION 1 | ||
| +extern PGDLLIMPORT const int SpockCorePatchsetVersion; | ||
| + | ||
| #endif /* MISCADMIN_H */ | ||
| --- a/src/backend/utils/init/globals.c | ||
| +++ b/src/backend/utils/init/globals.c | ||
| @@ -155,3 +155,5 @@ int64 VacuumPageDirty = 0; | ||
|
|
||
| int VacuumCostBalance = 0; /* working state for vacuum */ | ||
| bool VacuumCostActive = false; | ||
| + | ||
| +int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; |
There was a problem hiding this comment.
Const-qualifier mismatch between declaration and definition.
The header declares extern PGDLLIMPORT const int SpockCorePatchsetVersion; (Line 18) but globals.c defines it as non-const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; (Line 28). This is a type mismatch that violates C's rules on compatible types for external linkage (C11 §6.2.7/§6.7.3) and will produce a compiler diagnostic on strict builds (and is technically undefined behavior). The same issue likely exists in the PG15/17/18 sibling patches.
🔧 Proposed fix
-+int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;
++const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION;Alternatively, drop const from the header declaration if a writable global is intended (not recommended — the value should be immutable).
📝 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.
| +#define SPOCK_CORE_PATCHSET_VERSION 1 | |
| +extern PGDLLIMPORT const int SpockCorePatchsetVersion; | |
| + | |
| #endif /* MISCADMIN_H */ | |
| --- a/src/backend/utils/init/globals.c | |
| +++ b/src/backend/utils/init/globals.c | |
| @@ -155,3 +155,5 @@ int64 VacuumPageDirty = 0; | |
| int VacuumCostBalance = 0; /* working state for vacuum */ | |
| bool VacuumCostActive = false; | |
| + | |
| +int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; | |
| `#define` SPOCK_CORE_PATCHSET_VERSION 1 | |
| extern PGDLLIMPORT const int SpockCorePatchsetVersion; | |
| `#endif` /* MISCADMIN_H */ | |
| ++ b/src/backend/utils/init/globals.c | |
| @@ -155,3 +155,5 @@ int64 VacuumPageDirty = 0; | |
| int VacuumCostBalance = 0; /* working state for vacuum */ | |
| bool VacuumCostActive = false; | |
| const int SpockCorePatchsetVersion = SPOCK_CORE_PATCHSET_VERSION; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/16/pg16-000-spock-patchset-version.diff` around lines 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.
| @@ -0,0 +1,130 @@ | |||
| use strict; | |||
| use warnings; | |||
| use Test::More tests => 10; | |||
There was a problem hiding this comment.
Test plan count mismatch: declares 10 tests but only 9 are run.
Counting the assertions: isnt (L39), isnt (L40), is (L45), like (L56), like (L58), like (L74), like (L92), like (L94), is (L117) — that is 9, not 10. The TAP harness will fail the file with "planned 10 tests but ran 9".
Either adjust the plan to tests => 9 or add the missing assertion (e.g., a second like in Scenario 3 asserting the ALTER EXTENSION spock UPDATE hint, which would mirror Scenarios 2 and 4 and is the most likely intended one).
🐛 Proposed fix — add the missing hint assertion in Scenario 3
like($output, qr/version mismatch.*999999/,
"error includes the future version number");
+like($output, qr/ALTER EXTENSION spock UPDATE/,
+ "error hints to run ALTER EXTENSION UPDATE (future version)");🤖 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` at 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.
1 participant
Problem
Spock has three independently versioned artefacts that must agree at runtime: the patched PostgreSQL core binary, the Spock shared library, and the Spock SQL schema. A mismatch between any of these can cause silent data corruption or crashes. Prior to this change, Spock had no mechanism to detect or report such mismatches.
Solution
Two complementary guards, each covering a different mismatch class:
Layer 1: Core patchset check (binary vs binary). A new patch (
pg<ver>-000-spock-patchset-version.diff) exportsSpockCorePatchsetVersionviamiscadmin.handglobals.c. The extension references this symbol in_PG_init():.sorefuses to load._PG_init()compares values and raises ERROR.Layer 2: Node version check (binary vs SQL schema).
spock.local_nodegains anode_version int4column stamped withSPOCK_VERSION_NUMat node creation.get_local_node()— called in every critical Spock code path (17 call sites) — looks up the column by name and type in the tuple descriptor, then compares its value against the compiled version number:HINT: Run ALTER EXTENSION spock UPDATE.The name-based lookup (rather than positional Anum access) is robust against
DROP COLUMN(which leaves gaps in the physical layout) andVACUUM FULL(which renumbers attributes).Upgrade path
The upgrade SQL (
spock--5.0.6--6.0.0-devel.sql) adds the column and stamps it viaspock.spock_version_num()to guarantee the SQL and C layers agree. Between binary upgrade andALTER EXTENSION UPDATE, the server is fully operational for non-Spock workloads; only Spock operations are blocked.Files changed
src/spock.c—_PG_init()patchset version checksrc/spock_node.c—create_local_node()stamps version;get_local_node()name-based column lookup and version comparisonsql/spock--6.0.0-devel.sql—node_versioncolumn in base schemasql/spock--5.0.6--6.0.0-devel.sql— upgrade SQLpatches/{15,16,17,18}/pg*-000-spock-patchset-version.diff— core patchestests/regress/sql/version_guard.sql— SQL regression testtests/tap/t/020_version_safety_net.pl— TAP test (5 scenarios)docs/version-safety-net.md— design report covering all mismatch scenarios