Repository navigation
Fix standby crash and replay stall on index load after basebackup - #58
Conversation
VamanaFinishAndReleaseBuffer releases the metapage buffer from a PG_CATCH when GenericXLogFinish throws. By then errfinish() has reset InterruptHoldoffCount to zero, including the hold taken when the buffer's content lock was acquired, so the release's RESUME_INTERRUPTS() has nothing left to consume. The background worker catches these errors and keeps running, so the imbalance outlives the error: - On an assert-enabled build, the Assert(InterruptHoldoffCount > 0) in LWLockRelease aborts the worker and the postmaster reinitializes the server. - On a release build the count wraps to UINT32_MAX and the worker never processes an interrupt again. Query cancel is ignored and ProcSignalBarriers are never absorbed, so DROP DATABASE on a primary, and replay of CREATE DATABASE ... STRATEGY file_copy or DROP DATABASE on a standby, wait for the worker forever. Take the hold again before the release, as LWLockReleaseAll does for each lock it releases. Add an injection point inside the handler's PG_TRY, and a test in 04_bgw_robustness.pl that fires it on a metapage write and checks that the worker survives, that DROP DATABASE still completes and that the index serves searches once the failure is cleared. Without the fix the test trips the assertion on an assert-enabled build and leaves DROP DATABASE waiting on the worker on a release build. Signed-off-by: Matt Welch <matt.welch@intel.com>
An index's saved copy under vamana_indexes/ is written with plain file I/O and never replicates, but the metapage's hasSavedIndex flag is WAL-logged, so a standby can see the flag set with no directory behind it. LoadIndexFromPages answers that state by clearing the flag, which writes WAL. During recovery that fails with "cannot make new WAL entries during recovery", the load fails, and every search against the index on that standby fails with it. Two ordinary sequences lead there: - The standby's basebackup predates the save. The flag arrives later by replay and the directory never does. - The basebackup includes the saved copy. A standby creates an index's replication slot only when it loads the index, so at the first load that slot has not reached its initial consistent point. The check added in 9f4a9fa (PR #54) then correctly discards the copy, deletes its directory and retries, and the retry finds the flag set with the directory gone. Since that commit this happens on the first boot of every standby built from a backup that holds a saved copy. Through the error handler fixed in the previous commit it crash-looped the standby on assert-enabled builds and left the worker unable to absorb barriers on release builds. Clear the flag only on a server that persists indexes, which is the primary, and otherwise fall through to the heap rebuild that already handles a missing saved copy. The flag is the primary's to manage. The consistency check from 9f4a9fa stays in force on a standby. The saved copy lacks every row committed after it was written, and the standby's slot only starts after those rows, so draining it can never deliver them. Skipping the check there leaves them permanently missing from the standby's index; the heap rebuild is what keeps it complete. State that in the check's comment. Add 62_standby_saved_copy.pl and 63_standby_backup_before_save.pl, one per sequence. Each checks that the standby starts and its index answers queries, including rows committed after the save and after the standby started; that the worker never restarts and the log shows no crash and no attempted WAL write; that replay of CREATE DATABASE ... STRATEGY file_copy is not held up; and that the worker honours pg_terminate_backend and comes back serving. Without this change both fail on assert-enabled and release builds. Signed-off-by: Matt Welch <matt.welch@intel.com>
asonje
left a comment
asonje
left a comment
There was a problem hiding this comment.
Overall: solid, well-targeted fix. No logic issues found.
One observation about the larger issue, out of scope of this PR:
hasSavedIndex is node-local state stored in replicated storage. It describes a
directory on this node's filesystem, but it lives in a WAL-logged relation
page, so it travels independently of the thing it describes and can only be
corrected by writing WAL. That combination is the bug — the standby inherits an
assertion about local disk that isn't true locally, and the one mechanism
available to fix it is the one mechanism a standby doesn't have.
This PR closes the path that happened to reach it. The shape is still there:
nothing stops the next VamanaSetHasSavedIndex or VamanaWriteMetaPageDynamic
caller from landing in the same place, and each will need its own guard.
Two things worth thinking about before that happens:
-
vamana_indexes/is a cache of something fully reconstructible from the heap,
and it's in the basebackup only because it sits under$PGDATA. A standby
discards it anyway via the #54 slot check. Core's convention here is explicit
(pg_notify,pg_internal.init,pg_stat_tmpand friends are excluded): a
local cache shouldn't ship, the receiving node should rebuild. Tests 62 and 63
already demonstrate both standbys reach the same state. -
The flag can't simply be dropped in favour of
stat()— it's written
atomically withnextExternalIdand the other counters, so it's really the
validity bit for those, and the crash window atvamanaio.c:553-559currently
depends on it. But that's an argument for moving the validity bit next to the
files it describes, not for keeping it in a replicated page.
2 participants
Description
Since 9f4a9fa (PR #54), a streaming standby built from a basebackup that holds an index's saved copy cannot load that index. On an assert-enabled build its worker aborts on the first load and the standby crash-loops. On a release build the standby stays up, but every search against the index fails, query cancel is ignored by the worker, and standby replay of
CREATE DATABASE ... STRATEGY file_copyorDROP DATABASEstops until someone terminates the worker by hand. A standby whose basebackup predates the save reaches the same failure on its first search, independently of PR #54.An index's saved copy under
$PGDATA/vamana_indexes/is plain files that never replicate, while the metapage'shasSavedIndexflag is WAL-logged. A standby can therefore see the flag set with no directory.LoadIndexFromPagesresponds to that state by clearing the flag, and clearing it writes WAL, which fails during recovery. Two sequences reach that state on a standby:The failed WAL write lands in
VamanaFinishAndReleaseBuffer'sPG_CATCH. That handler releases the buffer aftererrfinish()has zeroedInterruptHoldoffCount, so the release decrements a count that is already zero. The worker catches the error and keeps running with the unbalanced count:LWLockRelease.UINT32_MAX. From then on the worker processes no interrupts at all. Cancel is ignored, and no ProcSignalBarrier is ever absorbed.This PR fixes both paths in two commits:
LWLockReleaseAlldoes for each lock. This applies to any error out ofGenericXLogFinish, on a primary or a standby.PR #54's consistency check stays in force on standbys. It is load-bearing there too: the saved copy lacks every row committed after it was written, and the standby's slot starts after those rows, so draining it can never deliver them. With the check skipped on a standby, the standby's index permanently missed 40 of 60 rows committed after the save. With the check in place, it returned all 60. The check's comment now says so.
Related Issues
N/A
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
04_bgw_robustness.plgains a block driven by a new injection point inside the handler'sPG_TRY. It checks that the worker survives a failed metapage write, thatDROP DATABASE(which waits on a ProcSignalBarrier) completes, and that the index serves searches afterwards. Without the first commit, it trips the assertion on an assert-enabled build and leavesDROP DATABASEwaiting on the worker on a release build.62_standby_saved_copy.plcovers the basebackup-after-save sequence, and63_standby_backup_before_save.plcovers basebackup-before-save. Each checks that:CREATE DATABASE ... STRATEGY file_copyis not held up;pg_terminate_backendand its replacement serves the full index.Without these changes, both files fail 10 of 12 checks on an assert-enabled build and 5 of 12 on a release build. With the first commit alone, both fail 4 of 12 on either build.
12_launcher.pland16_warmup.plalready reproduced the crash on assert-enabled builds and pass with this PR.Full TAP suite results:
18_build_offset_overflow.plis skipped behind its slow-tests gate, by design.08_standby_replay.pland09_standby_replay_faults.plfail on a separate, pre-existing standby assertion,Assert(IsTransactionState())inVamanaFreeCacheEntryResources. They fail identically with this PR reverted.
This run also needs a separate fix to the C initializer of
svs.max_build_memory. That fix was submitted on its own and was applied locally. Without it, no assert-enabled server starts withsvspreloaded.