Conversation
📝 WalkthroughWalkthroughThis PR introduces a version guard mechanism for Spock that validates schema and runtime patchset consistency. It adds patchset version constants across PostgreSQL 15–18 patches, extends the ChangesVersion Guard Implementation
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 35 complexity · -2 duplication
Metric Results Complexity 35 Duplication -2
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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
4 times, most recently
from
3aeba5c to
d0043ea
Compare
fill_missing_defaults(), init_apply_exec_state() and finish_apply_exec_state() in spock_apply_heap.c are no longer called from anywhere in the tree. Drop them. In passing also drops the build_delta_tuple() static-helper grouping comment that was specific to those callers.
When spock is loaded into a server binary that was built from a
different generation of the spock core patchset than this extension
expects, the result is silent ABI mismatch -- new code reads stale
struct layouts, missing symbols may exist as null lookups, etc.
Make the version coupling explicit:
* The core patchset (patches/{15,16,17,18}/pg{N}-000-...) defines
SPOCK_CORE_PATCHSET_VERSION as a compile-time constant in
miscadmin.h and SpockCorePatchsetVersion as a runtime int in
globals.c. Bump the macro when a patchset change is visible to
the extension binary.
* spock _PG_init reads both values and ereport(ERROR)s when they
disagree. An unpatched server never reaches this check -- the
dynamic linker fails first on the missing SpockCorePatchsetVersion
symbol.
The constant and the runtime variable live in core, so a future
patchset bump is one number change in two places, with no spock-side
churn.
Detect "binary upgraded but ALTER EXTENSION spock UPDATE not run" by
stamping each node with the spock version that wrote it and checking
the stamp on every read.
A new int4 NOT NULL column spock.local_node.node_version carries the
binary version (SPOCK_VERSION_NUM) at create_local_node() time.
get_local_node() looks up the column by name (not by Anum, since
DROP COLUMN leaves a gap and VACUUM FULL renumbers attributes), then
ereport(ERROR)s if it is missing or does not match the running binary.
Both error paths suggest "Run ALTER EXTENSION spock UPDATE." and use
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE so callers can distinguish
schema-mismatch from operational failures.
The check fires regardless of the missing_ok argument: returning NULL
would conflate "node not configured yet" with "node misconfigured",
and not all callers check the return value.
Coverage:
* tests/regress/sql/version_guard.sql exercises both directions
(node_version below and above the binary) plus the column-shape
invariants. Wired into REGRESS in Makefile.
* tests/tap/t/020_version_safety_net.pl drives the same scenarios
end-to-end on a live cluster. Wired into the tap schedule.
The 5.x -> 6.x upgrade story previously required a single
spock--5.0.6--6.0.0-devel.sql file that bundled all 5.x patch updates
plus the 6.x changes. That collapsed two unrelated concerns:
1. picking up patches v5_STABLE shipped in 5.0.7 (wait_for_sync_event
with wait_if_disabled, sync_event(transactional), the
sub_skip_schema text->text[] relabel);
2. landing 6.0.0-devel features (new conflict types, apply-group
progress, security-label-based delta_apply, etc.).
Split the chain so the 5.x patch level is reached first, then the
6.x-specific changes are applied as a separate step. Concretely:
* sql/spock--5.0.0.sql is now a true full-install file at the 5.0.0
schema level (matches what v5_STABLE ships).
* sql/spock--5.0.6--5.0.7.sql (also matches v5_STABLE) brings a 5.0.6
install up to 5.0.7 -- pause/resume_apply_workers,
wait_for_sync_event(wait_if_disabled), sync_event(transactional),
and the sub_skip_schema relabel with the LOCK TABLE +
pg_statistic cleanup that v5_STABLE uses.
* sql/spock--5.0.7--6.0.0-devel.sql carries only the 6.0.0-devel
deltas on top of 5.0.7, replacing the previous combined script.
* sql/spock--5.0.6--6.0.0-devel.sql is removed -- the chain now
routes through 5.0.7.
A v5_STABLE 5.0.7 user upgrading via "ALTER EXTENSION spock UPDATE TO
'6.0.0-devel'" will run only spock--5.0.7--6.0.0-devel.sql, which
DROP-IF-EXISTS-then-CREATEs every signature it changes so collisions
with v5_STABLE-installed objects are handled cleanly.
In passing also adapts tests/tap/t/002_create_subscriber.pl to use the
new sync_event() / wait_for_sync_event(...) signatures; the previous
spock.sub_wait_for_sync('test_subscription') call is no longer the
canonical way to synchronise.
During pg_upgrade from a spock-5.x cluster to a spock-6.x cluster,
pg_dump --binary-upgrade emits the legacy spock-5.x form for
delta-apply markers:
ALTER TABLE t ALTER COLUMN c SET (log_old_value=true,
delta_apply_function=spock.delta_apply);
spock 6.x records the same intent as a security label with provider
'spock':
SECURITY LABEL FOR spock ON COLUMN t.c IS 'spock.delta_apply';
Install a ProcessUtility hook in the new cluster that intercepts the
legacy form during pg_restore and rewrites it on the fly:
- the legacy DefElems are stripped from the AlterTableCmd;
- if the stripped cmd has unrelated keys (e.g. fillfactor) they
survive;
- if the stripped cmd has nothing left, the cmd is dropped entirely;
- a synthetic SECURITY LABEL statement is emitted in the same xact;
- one NOTICE per rewritten column lands in pg_upgrade.log.
Outside pg_upgrade the hook is not installed and the normal DDL path
pays nothing. The mechanism is one self-contained file
(src/spock_bucompat_5x.c, ~450 lines) plus one call from spock.c
_PG_init. Retirement is two edits: git rm the .c file and remove
the register_spock_compat_5x() call.
The security label provider is moved to BEFORE the IsBinaryUpgrade
early-return in _PG_init, so the synthesised SECURITY LABEL
statements find the provider during pg_restore.
In passing convert the "spock extension is not created yet" elog in
spock_object_relabel() to a proper ereport with
ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE; the message is reachable
from user-visible code paths (any SECURITY LABEL FOR spock ... before
CREATE EXTENSION) so it deserves an errcode.
Design contract (C1-C10) is documented in
docs/internals-doc/binary-upgrade-compat-shim.md.
A TAP test driving real pg_upgrade from a 5.x dump fixture is still
to do.
Replace the legacy 5.x reloption examples in docs/conflicts.md and
docs/troubleshooting.md with calls to the spock.delta_apply() helper.
The reloption form is still accepted at runtime via the
binary-upgrade compatibility shim during pg_upgrade, but the
documented way to mark a delta-apply column on 6.x is:
SELECT spock.delta_apply('t'::regclass, 'c');
Add a "Upgrading from spock 5.x" subsection to conflicts.md pointing
at the binary-upgrade compatibility shim doc and showing operators
how to audit the post-upgrade catalog via pg_seclabel.
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
Comment on lines
41
to
+61
| ```sql | ||
| ALTER TABLE pgbench_accounts ALTER COLUMN abalance | ||
| SET (log_old_value=true, delta_apply_function=spock.delta_apply); | ||
| ALTER TABLE pgbench_branches ALTER COLUMN bbalance | ||
| SET (log_old_value=true, delta_apply_function=spock.delta_apply); | ||
| ALTER TABLE pgbench_tellers ALTER COLUMN tbalance | ||
| SET (log_old_value=true, delta_apply_function=spock.delta_apply); | ||
| SELECT spock.delta_apply('pgbench_accounts'::regclass, 'abalance'); | ||
| SELECT spock.delta_apply('pgbench_branches'::regclass, 'bbalance'); | ||
| SELECT spock.delta_apply('pgbench_tellers'::regclass, 'tbalance'); | ||
| ``` | ||
|
|
||
| As a special safety-valve feature, if you ever need to re-set a | ||
| `log_old_value` column you can temporarily alter the column to | ||
| `log_old_value` is `false`. | ||
| To remove the marker, pass `to_drop => true`: | ||
|
|
||
| ```sql | ||
| SELECT spock.delta_apply('pgbench_accounts'::regclass, 'abalance', to_drop => true); | ||
| ``` | ||
|
|
||
| Under the hood, `spock.delta_apply()` writes a row into `pg_seclabel` | ||
| with `provider = 'spock'` and `label = 'spock.delta_apply'`. The | ||
| binary-upgrade compatibility shim that translates legacy spock 5.x | ||
| reloptions during `pg_upgrade` writes the same canonical label, so | ||
| operators can audit the catalog uniformly: | ||
|
|
||
| ```sql | ||
| SELECT * FROM pg_seclabel | ||
| WHERE provider = 'spock' AND label = 'spock.delta_apply'; |
There was a problem hiding this comment.
Markdown lint: code block style is inconsistent with repo rule (MD046).
These SQL blocks are fenced, but lint expects indented blocks. Converting these three snippets to indented style should clear the warnings.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 41-41: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
[warning] 49-49: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
[warning] 59-59: Code block style
Expected: indented; Actual: fenced
(MD046, code-block-style)
🤖 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 - 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.
| WHERE provider = 'spock' AND label = 'spock.delta_apply'; | ||
| ``` | ||
|
|
||
| #### Upgrading from spock 5.x |
There was a problem hiding this comment.
Fix heading level increment (MD001).
#### Upgrading from spock 5.x skips a level under the surrounding structure; promote it to ### to keep heading hierarchy valid.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 64-64: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 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` at 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.
Comment on lines
+34
to
+40
| ``` | ||
| src/spock_bucompat_5x.c -- the entire mechanism (~325 lines) | ||
| src/spock.c -- one extern decl + one call from _PG_init | ||
| sql/spock--6.0.0-devel.sql -- (no shim-related DDL; security-label | ||
| provider is registered in C) | ||
| docs/internals-doc/binary-upgrade-compat-shim.md -- this file | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced blocks (MD040).
Please tag these fences (for example, text for file layout and log output) to satisfy markdownlint and improve readability tooling.
Also applies to: 109-111
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/internals-doc/binary-upgrade-compat-shim.md` around lines 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.
Comment on lines
+140
to
+163
| LOCK TABLE spock.subscription IN ACCESS EXCLUSIVE MODE; | ||
|
|
||
| UPDATE pg_catalog.pg_attribute | ||
| SET atttypid = 'text[]'::regtype, | ||
| attndims = 1 | ||
| WHERE attrelid = 'spock.subscription'::regclass | ||
| AND attname = 'sub_skip_schema' | ||
| AND atttypid = 'text'::regtype; | ||
|
|
||
| /* | ||
| * Drop any pg_statistic rows for the column. Stats sampled when the | ||
| * column was labelled text encode varlena bytes with text semantics; | ||
| * after the relabel the planner would interpret the same stavalues | ||
| * arrays as text[], producing nonsense selectivities (and possibly | ||
| * crashing on operators that validate ArrayType structure). ANALYZE | ||
| * will repopulate as needed. | ||
| */ | ||
| DELETE FROM pg_catalog.pg_statistic | ||
| WHERE starelid = 'spock.subscription'::regclass | ||
| AND staattnum = ( | ||
| SELECT attnum | ||
| FROM pg_catalog.pg_attribute | ||
| WHERE attrelid = 'spock.subscription'::regclass | ||
| AND attname = 'sub_skip_schema'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On PostgreSQL 15 and 16, is allow_system_table_mods PGC_POSTMASTER or can it be changed per-session by a superuser?
💡 Result:
No, allow_system_table_mods is not PGC_POSTMASTER. It is PGC_SUSET (context 'superuser'), which means it can be changed at runtime by superusers using SET, including per-session.[1][2][3]
Citations:
- 1: https://postgresqlco.nf/doc/en/param/allow_system_table_mods/
- 2: https://postgr.es/c/c4a7a392e
- 3: https://www.postgresql.org/docs/current/runtime-config-developer.html
Add SET LOCAL allow_system_table_mods = on before catalog mutations.
Direct UPDATE pg_catalog.pg_attribute and DELETE FROM pg_catalog.pg_statistic are blocked even for superusers unless allow_system_table_mods is enabled. The upgrade script will fail with "permission denied" errors on stock servers where this GUC is off by default.
allow_system_table_mods is PGC_SUSET, so superusers can enable it per-session with SET LOCAL across all supported PostgreSQL versions (15–18). Wrap the catalog block with SET LOCAL allow_system_table_mods = on to make the upgrade self-contained:
Suggested fix
+SET LOCAL allow_system_table_mods = on;
+
LOCK TABLE spock.subscription IN ACCESS EXCLUSIVE MODE;
UPDATE pg_catalog.pg_attribute
SET atttypid = 'text[]'::regtype,
attndims = 1
WHERE attrelid = 'spock.subscription'::regclass
AND attname = 'sub_skip_schema'
AND atttypid = 'text'::regtype;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sql/spock--5.0.6--5.0.7.sql` around lines 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.
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 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