Conversation
📝 WalkthroughWalkthroughThis PR widens the in-memory replay byte counter to uint64, runs delta-update conflict resolution inside an internal subtransaction while explicitly managing CurrentResourceOwner, and deep-copies subscription apply_delay interval values to allocated memory. ChangesReplication Safety and Sizing Improvements
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
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.
🧹 Nitpick comments (1)
src/spock_apply_heap.c (1)
762-780: 💤 Low valueConsider renaming
edatato avoid confusion with the common naming pattern.Throughout this file,
edatatypically refers toApplyExecutionData*(e.g., inspock_apply_heap_insert,spock_apply_heap_update,spock_apply_heap_delete). Usingedatahere forErrorData*could cause confusion during maintenance.Suggested rename
if (!is_insert) { - ErrorData *edata; + ErrorData *errdata; MemoryContextSwitchTo(MessageContext); - edata = CopyErrorData(); + errdata = CopyErrorData(); - if (edata->sqlerrcode == ERRCODE_UNIQUE_VIOLATION) + if (errdata->sqlerrcode == ERRCODE_UNIQUE_VIOLATION) { elog(spock_conflict_log_level, "CONFLICT: detected %s on %s.%s: %s", SpockConflictTypeName(SPOCK_CT_UPDATE_EXISTS), rel->nspname, RelationGetRelationName(rel->rel), - edata->message); + errdata->message); `#if` PG_VERSION_NUM >= 180000 spock_stat_report_subscription_conflict( MyApplyWorker->subid, SPOCK_CT_UPDATE_EXISTS); `#endif` } - FreeErrorData(edata); + FreeErrorData(errdata); }🤖 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_heap.c` around lines 762 - 780, Rename the local ErrorData* variable currently named edata in the error-handling block to a distinct name (e.g., errdata or err_info) to avoid confusion with ApplyExecutionData* variables named edata elsewhere; update all references in this block (the CopyErrorData() call, edata->sqlerrcode, edata->message, and FreeErrorData()) to the new name and ensure no other symbols (SpockConflictTypeName, SPOCK_CT_UPDATE_EXISTS, spock_stat_report_subscription_conflict, MyApplyWorker, spock_conflict_log_level, rel->nspname, RelationGetRelationName(rel->rel)) are modified.
🤖 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 `@src/spock_apply_heap.c`:
- Around line 762-780: Rename the local ErrorData* variable currently named
edata in the error-handling block to a distinct name (e.g., errdata or err_info)
to avoid confusion with ApplyExecutionData* variables named edata elsewhere;
update all references in this block (the CopyErrorData() call,
edata->sqlerrcode, edata->message, and FreeErrorData()) to the new name and
ensure no other symbols (SpockConflictTypeName, SPOCK_CT_UPDATE_EXISTS,
spock_stat_report_subscription_conflict, MyApplyWorker,
spock_conflict_log_level, rel->nspname, RelationGetRelationName(rel->rel)) are
modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81c2b3c6-5b6f-4d58-a4f3-309d7a93e77f
📒 Files selected for processing (3)
src/spock_apply.csrc/spock_apply_heap.csrc/spock_node.c
Strict ResourceOwner tracking raises a buffer-pin error when ExecSimpleRelationUpdate acquires pins under BeginInternalSubTransaction that are not released before ReleaseCurrentSubTransaction(). Redirect CurrentResourceOwner to TopTransactionResourceOwner for the duration of the ExecSimpleRelationUpdate call so that TupleDesc refs and buffer pins are registered under the top-level owner and survive the subtransaction release. Restore the saved resowner before releasing so CommitSubTransaction's assertion holds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Transactions with more than 2GB cause a crash in the subscriber.
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 (2)
src/spock_apply.c (1)
4475-4493:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a 64-bit spill-threshold expression too.
apply_replay_bytesis nowuint64, but the limit on Line 4476 is still computed asspock_replay_queue_size * 1024L * 1024L. On 32-bitlongbuilds that overflows above ~2047 MB before the comparison, so spill activation can still happen at the wrong boundary for large queue settings.Suggested fix
- if (!apply_replay_spilling && spock_replay_queue_size > 0 && - apply_replay_bytes + msg->len > spock_replay_queue_size * 1024L * 1024L) + if (!apply_replay_spilling && spock_replay_queue_size > 0 && + apply_replay_bytes + (uint64) msg->len > + (uint64) spock_replay_queue_size * UINT64CONST(1024) * UINT64CONST(1024))🤖 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 4475 - 4493, The comparison uses a 32-bit arithmetic expression for the threshold (spock_replay_queue_size * 1024L * 1024L) while apply_replay_bytes is uint64, which can overflow on 32-bit builds; change the threshold computation to 64-bit by casting or using 64-bit constants so the multiplication is done in uint64 (e.g., compute (uint64)spock_replay_queue_size * 1024ULL * 1024ULL) and replace the old expression in the if that checks apply_replay_bytes > threshold (the block setting apply_replay_spilling and creating apply_replay_spill_file).src/spock_apply_heap.c (1)
730-789:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winWrap the entire delta-apply subtransaction lifecycle in the local
PG_TRY/PG_CATCH.After
BeginInternalSubTransaction("SpockDeltaApply"), onlyExecSimpleRelationUpdate()is protected. An error inEvalPlanQualSetSlot(),SubTransactionIdSetCommitTsData(), orReleaseCurrentSubTransaction()skips the restore path entirely, and the currentPG_CATCHrethrows withoutRollbackAndReleaseCurrentSubTransaction(). In DISCARD/SUB_DISABLE flows this function already runs inside the caller's own internal subtransaction, so leaking the inner one will break later cleanup/assertions.Suggested structure
- if (is_delta_apply) - { - BeginInternalSubTransaction("SpockDeltaApply"); - save_resowner = CurrentResourceOwner; - CurrentResourceOwner = TopTransactionResourceOwner; - } - - EvalPlanQualSetSlot(epqstate, remoteslot); - PG_TRY(); { + if (is_delta_apply) + { + BeginInternalSubTransaction("SpockDeltaApply"); + save_resowner = CurrentResourceOwner; + CurrentResourceOwner = TopTransactionResourceOwner; + } + + EvalPlanQualSetSlot(epqstate, remoteslot); + ExecSimpleRelationUpdate(relinfo, estate, epqstate, localslot, remoteslot); + + if (is_delta_apply) + { + CurrentResourceOwner = save_resowner; + SubTransactionIdSetCommitTsData(GetCurrentTransactionId(), + local_ts, local_origin); + ReleaseCurrentSubTransaction(); + } } PG_CATCH(); { if (is_delta_apply) - CurrentResourceOwner = save_resowner; + { + CurrentResourceOwner = save_resowner; + RollbackAndReleaseCurrentSubTransaction(); + } /* * existing unique-violation logging... */ PG_RE_THROW(); } PG_END_TRY(); - - if (is_delta_apply) - { - CurrentResourceOwner = save_resowner; - SubTransactionIdSetCommitTsData(GetCurrentTransactionId(), - local_ts, local_origin); - ReleaseCurrentSubTransaction(); - }🤖 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_heap.c` around lines 730 - 789, Begin and protect the entire delta-apply subtransaction lifecycle (from BeginInternalSubTransaction("SpockDeltaApply") through SubTransactionIdSetCommitTsData() and ReleaseCurrentSubTransaction()) in a single PG_TRY/PG_CATCH so any error in EvalPlanQualSetSlot(), ExecSimpleRelationUpdate(), SubTransactionIdSetCommitTsData(), or ReleaseCurrentSubTransaction() restores CurrentResourceOwner and correctly rolls back/releases the subtransaction; specifically, move BeginInternalSubTransaction and saving/restoring save_resowner outside/around the try block, ensure the PG_CATCH calls RollbackAndReleaseCurrentSubTransaction() (or Release/Commit as appropriate) before resetting CurrentResourceOwner, and only call SubTransactionIdSetCommitTsData() and ReleaseCurrentSubTransaction() in the PG_TRY success path (or commit path) to avoid leaking the inner subtransaction.
🤖 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_heap.c`:
- Around line 730-789: Begin and protect the entire delta-apply subtransaction
lifecycle (from BeginInternalSubTransaction("SpockDeltaApply") through
SubTransactionIdSetCommitTsData() and ReleaseCurrentSubTransaction()) in a
single PG_TRY/PG_CATCH so any error in EvalPlanQualSetSlot(),
ExecSimpleRelationUpdate(), SubTransactionIdSetCommitTsData(), or
ReleaseCurrentSubTransaction() restores CurrentResourceOwner and correctly rolls
back/releases the subtransaction; specifically, move BeginInternalSubTransaction
and saving/restoring save_resowner outside/around the try block, ensure the
PG_CATCH calls RollbackAndReleaseCurrentSubTransaction() (or Release/Commit as
appropriate) before resetting CurrentResourceOwner, and only call
SubTransactionIdSetCommitTsData() and ReleaseCurrentSubTransaction() in the
PG_TRY success path (or commit path) to avoid leaking the inner subtransaction.
In `@src/spock_apply.c`:
- Around line 4475-4493: The comparison uses a 32-bit arithmetic expression for
the threshold (spock_replay_queue_size * 1024L * 1024L) while apply_replay_bytes
is uint64, which can overflow on 32-bit builds; change the threshold computation
to 64-bit by casting or using 64-bit constants so the multiplication is done in
uint64 (e.g., compute (uint64)spock_replay_queue_size * 1024ULL * 1024ULL) and
replace the old expression in the if that checks apply_replay_bytes > threshold
(the block setting apply_replay_spilling and creating apply_replay_spill_file).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d2d842c-bd5a-4dfd-8adf-814933a32928
📒 Files selected for processing (2)
src/spock_apply.csrc/spock_apply_heap.c
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
2 participants
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.
Three commits cherry-picked from v5 to main.