PG17: re-find the proclock after waiting instead of trusting a stale pointer - #84
Merged
Merged
Conversation
PG17 counterpart of 2859252 on patches18 (ORI-247, "PANIC: proclock table corrupted"). A plain cherry-pick is wrong here, for two reasons. The partition lock. On PG18 LockAcquireExtended() releases it before WaitOnLock(), so that fix re-takes it around the lookup. On PG17 ProcSleep() still takes the partition lock at entry and re-acquires it EXCLUSIVE before returning, so it is held here; acquiring it again would self-deadlock. The lookup is therefore a plain hash_search. The dontWait path. PG17 handles a failed conditional acquisition in this same block, and that path never slept -- ProcSleep() with dontWait returns without releasing the partition lock, so nobody can have run RemoveFromWaitQueue() on us and the captured pointers are still good. Worse, that path *needs* them: it deletes the proclock and decrements lock->nRequested itself. So the re-find is done only when we actually slept. Also guard the INCONSISTENT prints, which can now see a proclock that is gone rather than merely one without our bit, and keep the local lock's cached pointers in step on the granted path. Verified the same way as the PG18 fix: with a probe reporting a proclock that was freed while we slept, the contended-upsert workload against orioledb hits it 157 times in 60 seconds on PG17 -- 157 reads of freed shared memory that this change no longer performs -- and runs with no panic and no assertion failure.
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
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.
PG17 counterpart of 2859252 on
patches18(ORI-247,PANIC: proclock table corrupted).A plain cherry-pick is wrong here, for two reasons — both worth knowing
before reviewing:
1. The partition lock
On PG18
LockAcquireExtended()releases the partition lock beforeWaitOnLock(), so that fix re-takes it around the lookup. On PG17ProcSleep()still "must be held at entry, and will be held at exit" — itre-acquires it
LW_EXCLUSIVEimmediately before returning. Taking it againhere would self-deadlock. So the lookup is a plain
hash_search_with_hash_valuewith no acquire.
2. The
dontWaitpathPG17 handles a failed conditional acquisition inside this same block, and that
path never slept:
ProcSleep()withdontWaitreturns without ever releasingthe partition lock, so no one can have run
RemoveFromWaitQueue()on us and thecaptured pointers are still good. Worse, that path needs them — it deletes the
proclock and decrements
lock->nRequesteditself. The re-find is therefore doneonly when we actually slept.
Also guarded the
INCONSISTENTprints, which can now see a proclock that isgone rather than merely one without our bit, and kept the local lock's cached
pointers in step on the granted path.
Validation
Same method as the PG18 fix, on a PG17 build with orioledb: a probe reporting a
proclock that was freed while we slept fires 157 times in 60 seconds of the
contended-upsert workload — 157 reads of freed shared memory that this change no
longer performs — with no panic and no assertion failure.
PG16 port follows separately; its block has no
dontWaitcase, so it issimpler.