spock_apply: advance forwarded origin using pre-created disabled subscription - #547
spock_apply: advance forwarded origin using pre-created disabled subscription#547rasifr wants to merge 3 commits into
Conversation
rasifr
commented
Jul 27, 2026
rasifr
commented
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| 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.
📝 WalkthroughWalkthroughForwarded origins are resolved per transaction from ChangesForwarded Origin Forwarding
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 (4)
src/spock_apply.c (3)
1260-1260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameter shadows the file-scope static
remote_origin_id.Both new helpers take a parameter named exactly like the global (line 99). Renaming to
peer_origin_idavoids a future edit silently reading the parameter instead of the global (or vice versa).🤖 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_apply.c` at line 1260, Rename the remote_origin_id parameter in resolve_forwarded_origin_for_transaction to peer_origin_id, and apply the same rename to the parameter in the other new helper that shadows the file-scope remote_origin_id. Update all references within both helpers while preserving their behavior.
1259-1271: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffPer-transaction catalog scan on the apply hot path.
Every ORIGIN message now opens a transaction and does a
systablescan overspock.subscription. For high-throughput forwarding this is a measurable per-transaction cost. A cache invalidated viaCacheRegisterSyscacheCallback/relcache invalidation on the subscription catalog would keep the freshness guarantee described in the comment at line 105 without the per-transaction scan.🤖 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_apply.c` around lines 1259 - 1271, Replace the per-transaction resolve_forward_peer_origin catalog scan in resolve_forwarded_origin_for_transaction with a cache of subscription-origin mappings, refreshed through CacheRegisterSyscacheCallback or equivalent relcache invalidation. Preserve the existing InvalidRepOriginId and local-origin early returns, and ensure invalidation keeps cached mappings current while avoiding StartTransactionCommand/CommitTransactionCommand on each ORIGIN message.
1202-1205: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAmbiguity
ERRORin the apply path will restart-loop the worker.An ambiguous catalog state (two subscriptions to the same peer) is unrecoverable without operator action, so this raises on every reconnect. Consider logging a WARNING and returning
InvalidRepOriginId(skip the advance) instead, leaving hard rejection to the creation-time exclusivity validation.🤖 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_apply.c` around lines 1202 - 1205, Update the ambiguous forwarded-origin branch in the apply-path lookup to log a WARNING instead of raising ERROR, then return InvalidRepOriginId so the worker skips advancing and avoids a restart loop. Keep the existing creation-time exclusivity validation responsible for hard rejection.tests/tap/t/015_forward_origin_advance.pl (1)
122-122: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixed
sleepcalls make this suite timing-flaky.Replication latency assertions gated on 2–5s sleeps will fail intermittently under loaded CI. The file already demonstrates the right pattern at lines 247-252 — extract that retry loop into a helper (e.g.
wait_for_count($node, $sql, $expected, $timeout)) and use it for the row-count and origin-LSN checks.Note the
count_no_dupcheck at line 240 is the weakest: a 3s window may simply be too short for a re-replay to show up, so the assertion can pass even when the origin was not advanced. Prefer polling for a stable count over a longer window.Also applies to: 162-162, 181-184, 203-203, 238-238
🤖 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/tap/t/015_forward_origin_advance.pl` at line 122, Replace the fixed sleep calls in the replication assertions with a reusable polling helper, such as wait_for_count, modeled on the retry loop already demonstrated around lines 247-252. Use it for row-count and origin-LSN checks, and update count_no_dup to poll for a stable count over a longer timeout so re-replay is observed reliably.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/spock_apply.c`:
- Around line 4941-4959: Update maybe_advance_forwarded_origin to handle the
enable race before calling replorigin_advance: avoid advancing when the direct
subscription origin may be concurrently acquired, or catch
ERRCODE_OBJECT_IN_USE, roll back the failed advance transaction state as
required, and sleep/retry safely. Preserve existing early-exit checks and only
special-case the shared-origin ownership failure.
---
Nitpick comments:
In `@src/spock_apply.c`:
- Line 1260: Rename the remote_origin_id parameter in
resolve_forwarded_origin_for_transaction to peer_origin_id, and apply the same
rename to the parameter in the other new helper that shadows the file-scope
remote_origin_id. Update all references within both helpers while preserving
their behavior.
- Around line 1259-1271: Replace the per-transaction resolve_forward_peer_origin
catalog scan in resolve_forwarded_origin_for_transaction with a cache of
subscription-origin mappings, refreshed through CacheRegisterSyscacheCallback or
equivalent relcache invalidation. Preserve the existing InvalidRepOriginId and
local-origin early returns, and ensure invalidation keeps cached mappings
current while avoiding StartTransactionCommand/CommitTransactionCommand on each
ORIGIN message.
- Around line 1202-1205: Update the ambiguous forwarded-origin branch in the
apply-path lookup to log a WARNING instead of raising ERROR, then return
InvalidRepOriginId so the worker skips advancing and avoids a restart loop. Keep
the existing creation-time exclusivity validation responsible for hard
rejection.
In `@tests/tap/t/015_forward_origin_advance.pl`:
- Line 122: Replace the fixed sleep calls in the replication assertions with a
reusable polling helper, such as wait_for_count, modeled on the retry loop
already demonstrated around lines 247-252. Use it for row-count and origin-LSN
checks, and update count_no_dup to poll for a stable count over a longer timeout
so re-replay is observed reliably.
🪄 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 Plus
Run ID: 6ffa03d8-4f79-4238-88b6-846fb4693206
📒 Files selected for processing (3)
src/spock_apply.csrc/spock_functions.ctests/tap/t/015_forward_origin_advance.pl
c4c47af to
65f3e81
Compare
| * remain visible. | ||
| */ | ||
| static void | ||
| enforce_forwarding_exclusivity(Oid target_sub_id, const char *target_sub_name, |
There was a problem hiding this comment.
Blocks forward_origins whenever another subscription on this node is enabled (and blocks enabling a plain sub while one forwards). It is the right guard against the ERRCODE_OBJECT_IN_USE advance failure, but it changes behavior for anyone using forward_origins in a live mesh. Intended as join/catchup only? Worth a release note and doc mention.
|
|
||
| if (strcmp(key, "forward_origins") == 0) | ||
| { | ||
| enforce_forwarding_exclusivity(sub->id, sub->name, sub->enabled, result); |
There was a problem hiding this comment.
On a disabled sub, target_forwards is false, so if another enabled sub forwards this still errors with "cannot enable subscription ..." even though we are only setting forward_origins, not enabling. Consider skipping the check when sub->enabled is false.
65f3e81 to
ad7e6d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/spock_functions/sub_mgmt.md`:
- Around line 115-117: Use one precise exclusivity invariant across both
documents: in docs/spock_functions/sub_mgmt.md lines 115-117, state that a
subscription with forwarding active must be the only enabled subscription on the
node while allowing multiple disabled subscriptions to retain forwarding
configuration; in docs/spock_release_notes.md lines 171-177, qualify the
mutual-exclusivity statement to exclude disabled subscriptions.
- Around line 111-114: Update the forwarding worker documentation to state that
exactly one local subscription must match the relayed peer, replacing the
ambiguous “whichever local subscription matches” wording. Explicitly document
that multiple matching subscriptions are rejected rather than selected
arbitrarily.
In `@tests/tap/t/015_forward_origin_advance.pl`:
- Around line 121-122: Replace the fixed sleep calls in the test flow with
deterministic synchronization: use wait_for_sub_status for subscription-state
transitions and count/LSN polling for data replication, following the existing
polling pattern near lines 245-252. In particular, add wait_for_sub_status(3,
'sub_n3_n1', 'disabled', 30) after sub_disable('sub_n3_n1') and before enabling
sub_n3_n2, while preserving the later no-duplicate and exactly-once assertions.
- Line 36: Update the Test::More plan in the test file from 31 to the actual 26
assertions executed, preserving the existing test cases and cluster setup
assertions.
🪄 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 Plus
Run ID: 9b5ca5da-9fd1-46bb-821d-131136e2f09a
📒 Files selected for processing (8)
docs/spock_functions/functions/spock_sub_alter_options.mddocs/spock_functions/functions/spock_sub_create.mddocs/spock_functions/functions/spock_sub_enable.mddocs/spock_functions/sub_mgmt.mddocs/spock_release_notes.mdsrc/spock_apply.csrc/spock_functions.ctests/tap/t/015_forward_origin_advance.pl
🚧 Files skipped from review as they are similar to previous changes (2)
- src/spock_functions.c
- src/spock_apply.c
| use strict; | ||
| use warnings; | ||
| use Test::More tests => 22; | ||
| use Test::More tests => 31; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Count TAP assertions in the test file and inspect helper functions for hidden assertions.
grep -nE '^\s*(ok|is|pass|isnt|like)\(' tests/tap/t/015_forward_origin_advance.pl
echo "---"
grep -cE '^\s*(ok|is|pass|isnt|like)\(' tests/tap/t/015_forward_origin_advance.pl
echo "--- create_cluster / destroy_cluster definitions ---"
rg -n "sub create_cluster|sub destroy_cluster" tests/tap/t/SpockTest.pm -A 40Repository: pgEdge/spock
Length of output: 5335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- test file outline/header around plan and assertion calls ---"
sed -n '1,90p;100,270p' tests/tap/t/015_forward_origin_advance.pl | cat -n
echo "--- full create_cluster and destroy_cluster assertion/statements ---"
sed -n '200,430p' tests/tap/t/SpockTest.pm | cat -n
echo "--- assertion-like calls anywhere in tests/tap/t/015_forward_origin_advance.pl ---"
python3 - <<'PY'
import re
from pathlib import Path
p = Path('tests/tap/t/015_forward_origin_advance.pl')
text = p.read_text()
for i,line in enumerate(text.splitlines(),1):
if re.search(r'\b(pass|ok|is|isnt|like|unlike)\s*\(', line):
print(f'{i}: {line}')
PY
echo "--- all Test::More plan/import usage in file ---"
python3 - <<'PY'
import re
from pathlib import Path
text = Path('tests/tap/t/015_forward_origin_advance.pl').read_text()
for i,line in enumerate(text.splitlines(),1):
stripped = line.strip()
if 'Test::More' in stripped or stripped.startswith('plan') or stripped.startswith('use'):
print(f'{i}: {line}')
PYRepository: pgEdge/spock
Length of output: 27261
Align the declared plan with the actual assertion count.
tests => 31 is too high: this test runs 23 assertions plus three create_cluster assertions for the 3-node setup, totaling 26. Decrease the plan or add the missing assertions so Test::More does not fail due to a planned > ran mismatch.
🤖 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/tap/t/015_forward_origin_advance.pl` at line 36, Update the Test::More
plan in the test file from 31 to the actual 26 assertions executed, preserving
the existing test cases and cluster setup assertions.
…cription
Any node that subscribes with forward_origins='all' receives transactions
originally sourced from other peer nodes, forwarded through its immediate
provider. Each forwarded transaction's ORIGIN wire message carries the
original peer's Spock node OID, not the immediate provider's. A peer's
transactions can reach this node forwarded this way well before -- or
without -- a direct subscription to that peer ever being created here. If
one has been (or later is) created, tracking the forwarded progress on its
origin lets it start from the right position whenever it is enabled,
instead of a full resync. A replication origin only exists where a
subscription entry does, so this requires resolving the peer's node id to
the local subscription (if any) that names it as provider.
handle_origin() resolves the peer node ID to a matching local subscription,
excluding invalid origins and the direct provider, and stores the result in
forwarded_local_origin_id for the duration of the transaction. This is
resolved fresh for every ORIGIN message rather than cached across
transactions: a cached RepOriginId can be silently reassigned to an
unrelated origin once its owning subscription is dropped (RepOriginId is a
small, actively recycled space -- replorigin_create() always reuses the
lowest free id, and replorigin_advance() does no catalog validation against
it), and Spock's resolution has no cheap way to revalidate a stale value
later the way a simple name lookup would. Resolving within the same
transaction that uses it bounds that exposure to a single transaction's
replay. Multiple matching subscriptions are rejected as ambiguous.
At commit, maybe_advance_forwarded_origin() advances the resolved origin using
XactLastCommitEnd as this node's local position, rather than the provider's
end_lsn from a different WAL space. The WAL-logged origin becomes the durable
resume position. Because the advance is logged separately after the data
commit, a crash may replay the latest transaction, preserving at-least-once
semantics; delta-apply idempotency remains a separate follow-up.
t/015 redesigned to test this path:
- n1->n2->n3 cascade with forward_origins='all'
- disabled sub_n3_n1 pre-created on n3 before data arrives
- verifies origin is advanced beyond 0/0 after n1 inserts propagate
- verifies origin is stable after n2->n1 is disabled (gap detection)
- verifies sub_enable('sub_n3_n1') starts the apply worker from the
advanced position (not 0/0), so rows already received via the cascade
are not re-sent, and new inserts arrive exactly once via the direct sub
A forwarding worker advances the replication origin of any local subscription matching a forwarded peer. If that subscription is enabled concurrently, its apply worker owns the same origin, causing replorigin_advance() to raise ERRCODE_OBJECT_IN_USE. Normal apply error handling then disables the entire forwarding subscription, including any unrelated traffic it carries. Add enforce_forwarding_exclusivity() to reject configurations where an enabled forwarding subscription coexists with another enabled subscription on the same node. Enforce this for sub_create(enabled=true), sub_enable(), and sub_alter_options() when enabling forward_origins, covering both possible state-change orders.
ad7e6d7 to
1d9d4b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/tap/t/015_forward_origin_advance.pl`:
- Around line 34-38: Update the Test::More plan in 015_forward_origin_advance.pl
from 34 to the actual total of 29, accounting for the 26 direct assertions and
the 3 assertions produced by create_cluster(3, 'Create 3-node cluster').
🪄 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 Plus
Run ID: 9c9e5e82-40cf-4aa4-9018-ebb4a2169c76
📒 Files selected for processing (8)
docs/spock_functions/functions/spock_sub_alter_options.mddocs/spock_functions/functions/spock_sub_create.mddocs/spock_functions/functions/spock_sub_enable.mddocs/spock_functions/sub_mgmt.mddocs/spock_release_notes.mdsrc/spock_apply.csrc/spock_functions.ctests/tap/t/015_forward_origin_advance.pl
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/spock_functions/functions/spock_sub_enable.md
- docs/spock_functions/functions/spock_sub_create.md
- docs/spock_release_notes.md
- docs/spock_functions/functions/spock_sub_alter_options.md
- docs/spock_functions/sub_mgmt.md
- src/spock_functions.c
- src/spock_apply.c
| use strict; | ||
| use warnings; | ||
| use Test::More tests => 22; | ||
| use Test::More tests => 34; | ||
| use lib '.'; | ||
| use SpockTest qw(create_cluster destroy_cluster system_or_bail system_maybe command_ok get_test_config scalar_query psql_or_bail); | ||
| use SpockTest qw(create_cluster destroy_cluster system_or_bail get_test_config scalar_query psql_or_bail wait_for_sub_status); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the test plan count: declared 34, actual assertions total 29.
Count the assertions directly in this file: pass() at Lines 62, 68, 74, 81, 87, 102 (6); ok() at Lines 112-113, 114-115, 155-156, 183-184, 189-190, 216-217, 233-234, 250-251, 254-255, 274 (10); is() at Lines 118, 129, 147, 174, 206, 209, 225, 261, 277, 281 (10). That totals 26 assertions in this file. create_cluster(3, 'Create 3-node cluster') at Line 44 adds further assertions for the 3-node setup (established as 3 in a prior review of this same file). 26 + 3 = 29, not 34.
Test::More treats a planned-vs-run mismatch as a failure: "If you run less (or more) tests than you planned, the missing (or extras) will be considered failures." A previous review already flagged this exact defect class on this file (tests => 31 vs. 26 actual, a deficit of 5). The new plan (34) still has a deficit of 5 against the recomputed actual (29), so the same miscount persists after the number was changed.
Update tests => 34 to match the actual assertion count, or add the missing assertions.
🐛 Verification script
#!/bin/bash
set -euo pipefail
echo "=== Assertions directly in 015_forward_origin_advance.pl ==="
grep -nE '^\s*(ok|is|pass|isnt|like)\(' tests/tap/t/015_forward_origin_advance.pl
echo "--- count ---"
grep -cE '^\s*(ok|is|pass|isnt|like)\(' tests/tap/t/015_forward_origin_advance.pl
echo "=== create_cluster / destroy_cluster assertion counts ==="
rg -n "sub create_cluster|sub destroy_cluster" tests/tap/t/SpockTest.pm -A 60🤖 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/tap/t/015_forward_origin_advance.pl` around lines 34 - 38, Update the
Test::More plan in 015_forward_origin_advance.pl from 34 to the actual total of
29, accounting for the 26 direct assertions and the 3 assertions produced by
create_cluster(3, 'Create 3-node cluster').
|
|
||
| /* forward_origins only accepts the sentinel value "all" */ | ||
| if (strcmp(key, "forward_origins") == 0 && | ||
| strcmp(elem, "all") != 0) |
There was a problem hiding this comment.
Confirming, only "all" is supported at this time?
There was a problem hiding this comment.
yes. only "all" is supported.
3 participants
