Conversation
📝 WalkthroughWalkthroughThe changes add PostgreSQL compatibility macros, adjust log filtering and portability details, improve exception replay cleanup, and defer failover-slot operations when no usable connection string exists. ChangesCompatibility, replay, and failover
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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 5 |
| 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.
The two loops searching for this subscription's exception-log slot ran to i <= SpockCtx->total_workers, while the array holds exactly total_workers entries (see worker_shmem_size() and the memset in spock_worker_shmem_init). Every other loop over the array in the tree stops at < total_workers. Reading entry [total_workers] is outside the region that startup zeroes, so the slot_name compared there is whatever the adjacent shared memory happens to hold. Worse, an empty-looking name would be picked up as free_slot_index and namestrcpy() would then write past the end of the array. Nothing has been observed to break, because the region is allocated with worker_shmem_size() which over-allocates enough that the write lands in slack, but that is luck rather than design. (cherry picked from commit ad49bf9)
handle_begin() looked up this subscription's slot in the shared exception log with strncmp() bounded by strlen(MySubscription->name), which matches on any slot whose name merely starts with ours. Two subscriptions in a prefix relationship therefore share one slot: "test_subscription" matches the slot belonging to "test_subscription_parallel", and the shorter name wins because the comparison stops before the longer name's extra characters. Sharing the slot means sharing commit_lsn, and that is the marker handle_begin() uses to decide whether a transaction has already failed once. Subscriptions from the same provider are delivered the same remote transactions with the same commit LSNs, so a failure recorded by one subscription can make the other enter exception handling for a transaction that never failed -- which under transdiscard means the transaction is discarded. The initial_error_message and failed_action of the two subscriptions are mixed up as well. Compare with namestrcmp(), which is what the rest of the file uses for a NameData against a C string, and which is bounded by NAMEDATALEN. The comparison is now exact. In passing, drop the slot_name variable. Its only remaining use was the strlen() emptiness test for the free-slot search, which reads more directly as NameStr(...)[0] == '\0' and no longer needs a separate declaration. (cherry picked from commit 59ab49e)
…ninfo Without spock.failover_slots_dsn the primary's connection string is taken from WalRcv->conninfo, and that field is empty for as long as a connection attempt is in flight: WalReceiverMain() clears it under the walreceiver mutex before calling walrcv_connect() and fills it in only once the connection is fully established. The window therefore opens on every walreceiver start and restart, not merely while the standby comes up. Have make_sync_failover_slots_dsn() report whether a connection string is available rather than build an unusable one, and let both callers wait for the next cycle instead. Since the walreceiver rewrites the field with its spinlock held, copy it out under WalRcv->mutex too: an unlocked reader could otherwise catch the memset() or strlcpy() in progress and build a DSN from a truncated string. (cherry picked from commit d9e911c)
clear_transient_exception_state_on_connection_loss() open-codes the four assignments that put a shared exception log entry back into the "no failure recorded" state. A second caller is about to need exactly the same set, and a set of fields spelled out in two places is a set of fields that will drift -- in particular local_tuple, which is easy to overlook because it is the only one that is not a scalar marker. No functional change. A little refactoring in preparation for the next commit, to make the material change there more clear.
use_try_block lives in the apply worker's shared memory slot and is not per-transaction state, so once set it stays set until something clears it. It is cleared when a transaction commits, but an error raised between remote transactions never reaches that point: apply_work() switches to replay mode regardless, and the flag is left over for whichever transaction the provider sends next. handle_begin() only reasoned about the flag when the incoming commit_lsn matched the recorded failure. A non-matching transaction is a first attempt, but it inherited the stale flag and was applied as a read-only replay -- and under TRANSDISCARD or SUB_DISABLE it was then discarded, reported through the exception log as if the policy had fired on a genuine conflict. The transaction was never attempted and never conflicted, so this loses data and attributes the loss to an unrelated earlier error. Derive the flag from the comparison instead: a transaction that is not the recorded failure clears it, and drops the recorded root cause with it. Leaving initial_error_message behind would make the next genuine exception surface a stale "Initial error" belonging to a different transaction. Note the fix depends on apply_work() setting first_begin_at_startup back to true when it catches an exception, which is what brings the next BEGIN through this comparison at all; comment that coupling at both ends. In passing, fix two comments that no longer describe the code: case 3 of handle_begin()'s case analysis, and the claim in handle_commit() that it is the only place use_try_block may be cleared -- untrue even before this commit, as the path which allocates a fresh exception log slot clears it too.
Annotate the deliberate fall-through in handle_queued_message() with pg_fallthrough. clang does not accept a comment as the annotation the way gcc does, so it warned here. pg_fallthrough is new in PG19; add it to the older compat headers, picking the spelling from the compiler rather than the server version since it is a compiler feature. hash_get_num_entries() returns int64 as of PG19, which no longer matches the %lu in spock_group_shmem_startup()'s debug message. Cast and use INT64_FORMAT, which is right on every supported major and on platforms where long is 32 bits. Give generate_subscription_id() a (void) parameter list. An empty list is a declaration without a prototype, deprecated in all C versions and rejected outright in C23.
spock_error_callback() lowers two serialization-failure retry messages from LOG to DEBUG1 and then reconsiders whether they should still be emitted, because errstart() decided that for the original level. Both comparisons were the wrong way round: they suppressed the message exactly when it should have been shown and showed it when it should have been suppressed. With the default settings the branch was simply never taken -- log_min_messages of WARNING is not less than DEBUG1 -- so the messages went to the server log at DEBUG1 regardless of the threshold, which is the opposite of what this code exists to do. PG19 made the mistake visible: log_min_messages is now an array indexed by backend type, so comparing it against an elevel compares a pointer with an integer and clang warns. Add SPOCK_LOG_MIN_MESSAGES to the compat headers to hide that difference, and compare in the direction is_log_level_output() uses -- emit when elevel is at least the configured minimum. spock_dependency.c already tests its thresholds this way, which is what the comparison here should have looked like all along.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spock_apply.c (1)
3934-3937: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not enter replay mode when the replay queue is empty.
After
apply_replay_queue_start_replay()returns from a non-transaction queue,need_replaystill triggersMyApplyWorker->use_try_block = true. Replay loop then exits immediately and the next incoming transaction applies with replay mode on untilhandle_commit(), causing read-only/discard behavior, incorrect feedback handling, andSUB_DISABLEdisablement on unrelated errors. Enable replay state only whenapply_replay_heador spilled records exist.🤖 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 3934 - 3937, Update the need_replay branch in the stream replay flow so it enables MyApplyWorker->use_try_block and enters replay mode only when apply_replay_head or spilled replay records exist. When the replay queue is empty after apply_replay_queue_start_replay(), skip the replay transition and allow the next transaction to apply normally.
🤖 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.
Outside diff comments:
In `@src/spock_apply.c`:
- Around line 3934-3937: Update the need_replay branch in the stream replay flow
so it enables MyApplyWorker->use_try_block and enters replay mode only when
apply_replay_head or spilled replay records exist. When the replay queue is
empty after apply_replay_queue_start_replay(), skip the replay transition and
allow the next transaction to apply normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8994201e-a7ad-44ff-9576-def7524a4801
📒 Files selected for processing (10)
src/compat/15/spock_compat.hsrc/compat/16/spock_compat.hsrc/compat/17/spock_compat.hsrc/compat/18/spock_compat.hsrc/compat/19/spock_compat.hsrc/spock.csrc/spock_apply.csrc/spock_failover_slots.csrc/spock_group.csrc/spock_node.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spock_failover_slots.c
danolivo
changed the title
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.
Fixes
Exception replay mode leaked onto healthy transactions.
use_try_blocklives in the worker's shmem slot, not per-transaction state. An error raised between remote transactions never reaches the commit that clears it, andapply_work()enters replay mode anyway — so the flag landed on the next transaction from the stream.handle_begin()only reasoned about the flag whencommit_lsnmatched the recorded failure, so a non-matching transaction inherited it, was replayed read-only, and underTRANSDISCARD/SUB_DISABLEwas discarded. A transaction that never failed, lost, and blamed on an unrelated error. Now the flag is derived from the comparison, and the recorded root cause is dropped with it.Off-by-one scan of the exception log array. Both search loops ran to
i <= SpockCtx->total_workers, one past the end.[total_workers]is outside the region startup zeroes, and an empty-looking name there would be taken as the free slot, sonamestrcpy()wrote past the array. It landed in slack only becauseworker_shmem_size()over-allocates.Slot name matched by prefix instead of exactly.
strncmp()bounded bystrlen(MySubscription->name)madetest_subscriptionshare a slot withtest_subscription_parallel— and sharing the slot means sharingcommit_lsn, so one subscription's failure could push the other into exception handling for a transaction that never failed. Nownamestrcmp().Failover-slots worker died while the walreceiver reconnected.
WalRcv->conninfois empty for as long as a connection attempt is in flight, which is every walreceiver start and restart, not just standby startup — the worker died in that window for 60s at a time.make_sync_failover_slots_dsn()now reports whether a DSN is available instead of building an unusable one; both callers wait for the next cycle. The field is also copied underWalRcv->mutex, matching core'spg_stat_get_wal_receiver(); it was read unlocked while the walreceiver rewrote it under that spinlock.Log-level re-check was inverted.
spock_error_callback()downgrades two serialization-failure retry messages toDEBUG1, then re-tests whether they should still be emitted — backwards in both comparisons.DEBUG1is 14 and the defaultlog_min_messagesisWARNING= 19, so19 < 14never fired and the messages went to the server log regardless of the threshold, which is what this code exists to prevent. Now compares in the directionis_log_level_output()uses, asspock_dependency.calready does.Four PG19 compiler warnings.
log_min_messagesbecame an array indexed by backend type (pointer-vs-int comparison, the bug above);pg_fallthroughis new in PG19'sc.hand clang won't accept a comment as the annotation;hash_get_num_entries()now returnsint64, no longer matching%lu;generate_subscription_id()had an empty parameter list. New compat macros for the 15–18 headers.Refactor
clear_exception_log_entry()collects the four assignments that reset a shared exception log entry. Two call sites need the same set, andlocal_tupleis easy to overlook — it's the only non-scalar, and it dangles by the time either caller runs. No functional change.