Repository navigation
Restore memory context in checkpoint retry catch - #70
Open
matt-welch wants to merge 1 commit into
Open
matt-welch wants to merge 1 commit into
matt-welch wants to merge 1 commit into
Conversation
- A checkpoint error raised before its transaction started left the worker in ErrorContext, since AbortCurrentTransaction() is a no-op when nothing is open to abort. The next FlushErrorState() anywhere in the worker then reset that context and freed data a checkpoint sweep was still iterating, crashing assert builds with SIGSEGV and leaving release builds leaking into ErrorContext. - Restore the saved context explicitly in the catch, matching the other worker catch sites, and add an Assert at the top of the heartbeat loop so the whole bug class trips a clean TRAP instead of an intermittent crash. - Make the existing regression test hold the injected failure for two cycles and check worker PID, signal termination, and ErrorContext growth, since one failure only poisons the context and a second is needed to free live data. Signed-off-by: Matt Welch <matt.welch@intel.com>
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.
Description
The vamana worker's per-index checkpoint retry absorbs errors so one bad index can't take the whole worker down, but its catch block never restored the caller's memory context. On most failures that doesn't matter, because the real transaction abort restores the context as a side effect. But a checkpoint error raised before its own transaction has started leaves nothing open to abort, so
AbortCurrentTransaction()is a no-op and the worker is left running inErrorContext. The next time anything in the worker callsFlushErrorState(), that call resetsErrorContextand frees whatever live data happened to be allocated there, including the list of cached index OIDs the checkpoint sweep was still iterating over. On an assert build this produces a segfault from reading freed, clobbered memory; on a release build there's no crash, but the worker keeps running with its loop-scoped allocations silently exposed to the next reset ofErrorContext.The fix restores the caller's saved memory context explicitly in the catch, after the (now conditional) transaction abort, matching the pattern already used at the other worker catch sites. It also adds an assertion at the top of the worker's heartbeat loop that catches this bug class as soon as it happens, as a clean assertion failure instead of a crash on some later, unrelated error. The existing regression test for this code path was updated to hold the injected failure across two consecutive checkpoint cycles (one cycle only leaves the context poisoned; a second cycle is what actually frees live data), and now checks that the same worker process survives with no signal termination, and that the worker's
ErrorContextusage does not grow once checkpoints resume succeeding.Real checkpoint failures that occur today (disk errors, lock conflicts, commit-time errors) all happen after the transaction has started, so the existing abort already restores the context correctly in those cases and this bug has not been observed in pr
oduction use. It was found through code inspection and reproduced with error injection. The fix is defensive: it closes the gap for any future change that does work ahead of the transaction start (a precheck, a GUC read, an added log call), which would otherwise make this reachable without needing error injection.
Related Issues
None.
Type of Change
Pre-Merge Checklist
Build
makecompletes without errors or warningsmake installcompletes successfullyTests
make installcheck) and TAP tests (test/t/) pass with no failurestest/sql/and/ortest/t/test/modules/: it builds and passes (make -C test/modules/<module> installcheck)Documentation
docs/updated if architecture or usage changedTesting Notes
Verified on two separate builds: one with
--enable-cassert --enable-debug --enable-injection-points, one without cassert (both with--enable-injection-points, required for the test's error injection).Confirmed the bug and the test's ability to catch it before applying the fix: on the assert build, the unmodified code crashed with SIGSEGV (signal 11) on all 3 runs attempted, each time after exactly two consecutive injected checkpoint failures logged a "will retry" line. On the release build, the unmodified code did not crash, but the test's
ErrorContextgrowth check failed, showing usage growing from 888 to 1968 bytes over about 4 seconds with no error in flight.After applying the fix,
test/t/36_pg_catch_return_value_integrity.plpassed 3 out of 3 runs on the assert build and passed on the release build (confirmed not skipped). Temporarily reverting only the memory-context restore, while keeping the new heartbeat-loop assertion, reproduced a clean assertion failure (TRAP: failed Assert("CurrentMemoryContext != ErrorContext")) in place of the earlier intermittent crash, confirming the assertion catches this bug class as intended; the fix was then restored.The full TAP suite and SQL regression suite were run against the fixed code. All regression test assertions passed. Two TAP files unrelated to this change (covering standby replay) reported a harness-level "no plan found" failure due to a pre-existing, unrelated assertion failure during standby replication slot cleanup; every individual test assertion in those files passed, and the failure reproduces identically whether run as part of the full suite or in isolation. This is a separate, pre-existing issue unaffected by this change and is not addressed by this PR.