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)
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 · 0 duplication
Metric Results Complexity 35 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
2 times, most recently
from
68e54db to
9d85fa7
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>
Add the 5.0.0 baseline and the 5.0.6->5.0.7 / 5.0.7->5.0.8 step files that v5_STABLE ships, and rename the dev migration to 5.0.8->6.0.0-devel. Move the sub_skip_schema relabel and the pause/resume/sync_event/wait_for_sync_event additions out of the 6.0.0-devel script, since they now belong to 5.0.7. Required so that an installation running any released 5.0.x can ALTER EXTENSION spock UPDATE all the way to 6.0.0-devel without losing intermediate steps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Record SPOCK_VERSION_NUM in spock.local_node on create_node, and re-check it on every get_local_node() call. Catches the operator mistake of upgrading the extension binary without running ALTER EXTENSION spock UPDATE (stale schema), and the inverse rollback case (binary older than schema). Lookup is by attribute name, not attnum, so a future DROP COLUMN / VACUUM FULL renumber cannot accidentally read the wrong slot. Covered by a regression test and a TAP safety-net test that exercises both directions plus the "column missing entirely" path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stop relying on the per-attribute reloption pair (log_old_value, delta_apply_function) -- which required a core patch to recognise -- and represent the same intent as a pg_seclabel row with provider 'spock' and label 'spock.delta_apply'. Update conflicts.md / troubleshooting.md to the spock.delta_apply() helper form, and stop wiping seclabel rows on internal table drops during ALTER EXTENSION UPDATE -- only the extension drop itself should clear them. Register the provider before the IsBinaryUpgrade early-return so pg_restore can replay labels during pg_upgrade. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
pg_dump --binary-upgrade against a spock 5.x source still emits ALTER TABLE ... SET (log_old_value=true, delta_apply_function=...). Add a ProcessUtility hook, gated on IsBinaryUpgrade so it costs nothing in normal operation, that intercepts those AlterTableCmds, strips the legacy DefElems, and synthesises the equivalent SECURITY LABEL FOR spock statement. Unrelated reloptions on the same SET clause survive untouched. Self-retiring: deleting the file and the register call from _PG_init is the cleanup once 5.x is out of support. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Build two PG trees + two spock versions from the local working copies, run the upgrade, and assert that columns marked with the legacy 5.x delta_apply reloption land in the new cluster as a spock-provider SECURITY LABEL with the canonical 'spock.delta_apply' value. This is the upgrade.sh scenario, expressed in PostgreSQL::Test idioms so it runs under the standard make check_prove target and gates the binary-upgrade shim. Co-Authored-By: Claude Opus 4.7 (1M context) <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>
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.
Summary
Lays the groundwork for upgrading a spock 5.x cluster to 6.0.0-devel via
pg_upgrade. Six logically separable commits:Remove unused apply-heap helper functions
fill_missing_defaults(),init_apply_exec_state(),finish_apply_exec_state()had no callers.Add core-patchset version safety net
Defines
SPOCK_CORE_PATCHSET_VERSION(compile-time) andSpockCorePatchsetVersion(runtime) in core via patches/{15..18}/..., and_PG_initereport(ERROR)s on mismatch. An unpatched server fails earlier on the missing extern symbol.Add per-node version safety net
New int4 NOT NULL column
spock.local_node.node_versioncarriesSPOCK_VERSION_NUMat create.get_local_node()looks the column up by name (DROP COLUMN / VACUUM FULL safe), and ereport(ERROR)s with hint"Run ALTER EXTENSION spock UPDATE." if the stamp doesn't match the running binary. Always errors regardless of
missing_ok. Covered bytests/regress/sql/version_guard.sqlandtests/tap/t/020_version_safety_net.pl.Restructure 5.x -> 6.0.0-devel SQL upgrade chain
Splits the previous combined
spock--5.0.6--6.0.0-devel.sqlinto:sql/spock--5.0.0.sql— full 5.0.0 install matching v5_STABLEsql/spock--5.0.6--5.0.7.sql— pause/resume_apply_workers,wait_for_sync_event(wait_if_disabled),sync_event(transactional), and thesub_skip_schematext->text[] relabel; matches v5_STABLE byte-for-bytesql/spock--5.0.7--6.0.0-devel.sql— only the 6.0.0-devel deltas (apply-group progress, lag_tracker rework, conflict stats, delta_apply helper, sub_alter_options, node_version)Result: a v5_STABLE 5.0.7 user upgrading via
ALTER EXTENSION spock UPDATE TO '6.0.0-devel'runs only the 5.0.7->6.0.0 step, which DROP-IF-EXISTS-then-CREATEs every signature it changes so collisions with v5_STABLE-installed objects are handled cleanly. In passing adaptstests/tap/t/002_create_subscriber.plto the newsync_event() / wait_for_sync_event()signatures.Add 5.x -> 6.x binary-upgrade compatibility shim
ProcessUtility hook installed only under
IsBinaryUpgradethat interceptsALTER TABLE ... SET (log_old_value=..., delta_apply_function=...)frompg_dump --binary-upgradeand rewrites it to the canonical 6.xSECURITY LABEL FOR spock ON COLUMN ... IS 'spock.delta_apply'.Self-contained in
src/spock_bucompat_5x.c(~450 lines); retirement is two edits (rm the file + remove the register call). Design doc atdocs/internals-doc/binary-upgrade-compat-shim.mdcovers contracts C1-C10. The security-label provider registration is moved before theIsBinaryUpgradeearly-return so synthesised statements find the provider during pg_restore. In passing converts the "spock extension is not created yet" elog to a proper ereport with errcode.Update user docs for 6.x SECURITY LABEL form
docs/conflicts.mdanddocs/troubleshooting.md: replaces legacy reloption examples withspock.delta_apply()helper calls; adds an "Upgrading from spock 5.x" subsection cross-linking to the bucompat design doc.