Make heap apply retry loop configurable via spock.read_retry_count#475
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.
📝 WalkthroughWalkthroughAdds a SIGHUP-scoped GUC 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 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.
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
mason-sharp
left a comment
mason-sharp
left a comment
There was a problem hiding this comment.
Can you also update the docs for the new GUC?
Add a configuring.md entry describing the retry behavior, the 0-disables-retries semantics, the 0-100 range, the default of 5, and the SIGHUP reload behavior.
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 `@docs/configuring.md`:
- Around line 234-236: The fenced code block containing the configuration line
spock.read_retry_count = 5 is missing a language specifier; update that fenced
block to use the ini language identifier (i.e., change the backtick fence to
```ini) so the block is marked as INI for syntax highlighting and to satisfy the
MD040 lint rule, matching other examples like the postgresql.conf snippets.
🪄 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: 21dd50fa-a2b7-4010-8293-bbfcc9a46aad
📒 Files selected for processing (1)
docs/configuring.md
| ``` | ||
| spock.read_retry_count = 5 | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to the fenced code block.
The fenced code block should specify ini as the language identifier for proper syntax highlighting and to comply with markdown best practices.
📝 Proposed fix
-```
+```ini
spock.read_retry_count = 5</details>
Based on learnings: Static analysis flagged this as MD040 (fenced-code-language). Other configuration examples in this file (lines 264-274) use `ini` for postgresql.conf settings.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 234-234: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/configuring.md around lines 234 - 236, The fenced code block containing
the configuration line spock.read_retry_count = 5 is missing a language
specifier; update that fenced block to use the ini language identifier (i.e.,
change the backtick fence to ```ini) so the block is marked as INI for syntax
highlighting and to satisfy the MD040 lint rule, matching other examples like
the postgresql.conf snippets.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
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.