Make heap apply retry loop configurable via spock.read_retry_count#475
Make heap apply retry loop configurable via spock.read_retry_count#475ibrarahmad wants to merge 1 commit into
Conversation
spock_apply_heap_update() and spock_apply_heap_delete() hardcoded the retry count at 5 when the local tuple could not be found. Expose this as a GUC (default 5, range 0..100, PGC_SIGHUP) so deployments hitting out-of-order arrivals can tune the loop without rebuilding.
📝 WalkthroughWalkthroughThis PR introduces a new ChangesConfigurable Read Retry Count
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)
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 | 0 |
| 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.
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_heap.c (1)
972-989:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInitialize lookup state and always perform an initial tuple read before retries.
With
spock.read_retry_count = 0, these loops never execute, sofoundis read uninitialized and the code may take an arbitrary branch. It also skips the initial local tuple lookup entirely.💡 Proposed fix
@@ - bool found; + bool found = false; @@ - retry = 0; - while (retry < spock_read_retry_count) - { - found = FindReplTupleInLocalRel(edata, relinfo->ri_RelationDesc, - edata->targetRel->idxoid, - remoteslot, &localslot, - false); - if (found) - break; - - wait_for_previous_transaction(); - retry++; - } + found = FindReplTupleInLocalRel(edata, relinfo->ri_RelationDesc, + edata->targetRel->idxoid, + remoteslot, &localslot, + false); + retry = 0; + while (!found && retry < spock_read_retry_count) + { + wait_for_previous_transaction(); + retry++; + found = FindReplTupleInLocalRel(edata, relinfo->ri_RelationDesc, + edata->targetRel->idxoid, + remoteslot, &localslot, + false); + }Apply the same pattern in both
spock_apply_heap_update()andspock_apply_heap_delete().Also applies to: 1089-1106
🤖 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 972 - 989, Initialize the lookup state and perform an initial FindReplTupleInLocalRel call before entering the retry loop to avoid reading the uninitialized variable `found` when `spock_read_retry_count` is 0; specifically in functions `spock_apply_heap_update()` and `spock_apply_heap_delete()` set `retry = 0`, initialize `found = false` (or equivalent), call `FindReplTupleInLocalRel(edata, relinfo->ri_RelationDesc, edata->targetRel->idxoid, remoteslot, &localslot, false)` once and handle the result before entering the while (retry < spock_read_retry_count) loop, then keep the existing wait_for_previous_transaction() / retry++ logic for subsequent attempts.
🤖 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 972-989: Initialize the lookup state and perform an initial
FindReplTupleInLocalRel call before entering the retry loop to avoid reading the
uninitialized variable `found` when `spock_read_retry_count` is 0; specifically
in functions `spock_apply_heap_update()` and `spock_apply_heap_delete()` set
`retry = 0`, initialize `found = false` (or equivalent), call
`FindReplTupleInLocalRel(edata, relinfo->ri_RelationDesc,
edata->targetRel->idxoid, remoteslot, &localslot, false)` once and handle the
result before entering the while (retry < spock_read_retry_count) loop, then
keep the existing wait_for_previous_transaction() / retry++ logic for subsequent
attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e3e32207-725a-4461-8734-9ffbaae7466f
📒 Files selected for processing (4)
include/spock.hsrc/spock.csrc/spock_apply_heap.ctests/tap/t/030_read_retry_count_guc.pl
| "concurrently-applying transaction to finish, then " | ||
| "searches the local relation again. Set to 0 to disable " | ||
| "retries (the row-missing path runs immediately). " | ||
| "Used in spock_apply_heap_update and " |
There was a problem hiding this comment.
This is a little bit too verbose, no need to mention which functions. Can you please shorten? Something like the first 4 lines and the "Set to 0" sentence?
3 participants
spock_apply_heap_update() and spock_apply_heap_delete() hardcoded the retry count at 5 when the local tuple could not be found. Expose this as a GUC (default 5, range 0..100, PGC_SIGHUP) so deployments hitting out-of-order arrivals can tune the loop without rebuilding.