forked frompostgres/postgres
- Notifications
You must be signed in to change notification settings - Fork6
Commitf332241
committed
Fix race conditions in synchronous standby management.
We have repeatedly seen the buildfarm reach the Assert(false) inSyncRepGetSyncStandbysPriority. This apparently is due to failing toconsider the possibility that the sync_standby_priority values inshared memory might be inconsistent; but they will be whenever onlysome of the walsenders have updated their values after a change inthe synchronous_standby_names setting. That function is vastly toocomplex for what it does, anyway, so rewriting it seems better thantrying to apply a band-aid fix.Furthermore, the API of SyncRepGetSyncStandbys is broken by design:it returns a list of WalSnd array indexes, but there is nothingguaranteeing that the contents of the WalSnd array remain stable.Thus, if some walsender exits and then a new walsender processtakes over that WalSnd array slot, a caller might make use ofWAL position data that it should not, potentially leading toincorrect decisions about whether to release transactions thatare waiting for synchronous commit.To fix, replace SyncRepGetSyncStandbys with a new functionSyncRepGetCandidateStandbys that copies all the required datafrom shared memory while holding the relevant mutexes. If theassociated walsender process then exits, this data is still safe tomake release decisions with, since we know that that much WAL *was*sent to a valid standby server. This incidentally means that we nolonger need to treat sync_standby_priority as protected by theSyncRepLock rather than the per-walsender mutex.SyncRepGetSyncStandbys is no longer used by the core code, so removeit entirely in HEAD. However, it seems possible that external code isrelying on that function, so do not remove it from the back branches.Instead, just remove the known-incorrect Assert. When the bug occurs,the function will return a too-short list, which callers should treatas meaning there are not enough sync standbys, which seems like areasonably safe fallback until the inconsistent state is resolved.Moreover it's bug-compatible with what has been happening in non-assertbuilds. We cannot do anything about the walsender-replacement racecondition without an API/ABI break.The bogus assertion exists back to 9.6, but 9.6 is sufficientlydifferent from the later branches that the patch doesn't apply at all.I chose to just remove the bogus assertion in 9.6, feeling that theprobability of a bad outcome from the walsender-replacement racecondition is too low to justify rewriting the whole patch for 9.6.Discussion:https://postgr.es/m/21519.1585272409@sss.pgh.pa.us1 parent3cb02e3 commitf332241
File tree
4 files changed
+187
-294
lines changed- src
- backend/replication
- include/replication
4 files changed
+187
-294
lines changed0 commit comments
Comments
(0)