Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 51865a0

Browse files
committed
Fix race condition in predicate-lock init code in EXEC_BACKEND builds.
Trading a little too heavily on letting the code path be the same whether we were creating shared data structures or only attaching to them, InitPredicateLocks() inserted the "scratch" PredicateLockTargetHash entry unconditionally. This is just wrong if we're in a postmaster child, which would only reach this code in EXEC_BACKEND builds. Most of the time, the hash_search(HASH_ENTER) call would simply report that the entry already existed, causing no visible effect since the code did not bother to check for that possibility. However, if this happened while some other backend had transiently removed the "scratch" entry, then that other backend's eventual RestoreScratchTarget would suffer an assert failure; this appears to be the explanation for a recent failure on buildfarm member culicidae. In non-assert builds, there would be no visible consequences there either. But nonetheless this is a pretty bad bug for EXEC_BACKEND builds, for two reasons: 1. Each new backend would perform the hash_search(HASH_ENTER) call without holding any lock that would prevent concurrent access to the PredicateLockTargetHash hash table. This creates a low but certainly nonzero risk of corruption of that hash table. 2. In the event that the race condition occurred, by reinserting the scratch entry too soon, we were defeating the entire purpose of the scratch entry, namely to guarantee that transaction commit could move hash table entries around with no risk of out-of-memory failure. The odds of an actual OOM failure are quite low, but not zero, and if it did happen it would again result in corruption of the hash table. The user-visible symptoms of such corruption are a little hard to predict, but would presumably amount to misbehavior of SERIALIZABLE transactions that'd require a crash or postmaster restart to fix. To fix, just skip the hash insertion if IsUnderPostmaster. I also inserted a bunch of assertions that the expected things happen depending on whether IsUnderPostmaster is true. That might be overkill, since most comparable code in other functions isn't quite that paranoid, but once burnt twice shy. In passing, also move a couple of lines to places where they seemed to make more sense. Diagnosis of problem by Thomas Munro, patch by me. Back-patch to all supported branches. Discussion: https://postgr.es/m/10593.1500670709@sss.pgh.pa.us
1 parent 971faef commit 51865a0

File tree

1 file changed

+21
-8
lines changed

1 file changed

+21
-8
lines changed

src/backend/storage/lmgr/predicate.c

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,7 @@ OldSerXidInit(void)
806806
oldSerXidControl = (OldSerXidControl)
807807
ShmemInitStruct("OldSerXidControlData", sizeof(OldSerXidControlData), &found);
808808

809+
Assert(found == IsUnderPostmaster);
809810
if (!found)
810811
{
811812
/*
@@ -1100,6 +1101,10 @@ InitPredicateLocks(void)
11001101
Size requestSize;
11011102
bool found;
11021103

1104+
#ifndef EXEC_BACKEND
1105+
Assert(!IsUnderPostmaster);
1106+
#endif
1107+
11031108
/*
11041109
* Compute size of predicate lock target hashtable. Note these
11051110
* calculations must agree with PredicateLockShmemSize!
@@ -1122,16 +1127,22 @@ InitPredicateLocks(void)
11221127
HASH_ELEM | HASH_BLOBS |
11231128
HASH_PARTITION | HASH_FIXED_SIZE);
11241129

1125-
/* Assume an average of 2 xacts per target */
1126-
max_table_size *= 2;
1127-
11281130
/*
11291131
* Reserve a dummy entry in the hash table; we use it to make sure there's
11301132
* always one entry available when we need to split or combine a page,
11311133
* because running out of space there could mean aborting a
11321134
* non-serializable transaction.
11331135
*/
1134-
hash_search(PredicateLockTargetHash, &ScratchTargetTag, HASH_ENTER, NULL);
1136+
if (!IsUnderPostmaster)
1137+
{
1138+
(void) hash_search(PredicateLockTargetHash, &ScratchTargetTag,
1139+
HASH_ENTER, &found);
1140+
Assert(!found);
1141+
}
1142+
1143+
/* Pre-calculate the hash and partition lock of the scratch entry */
1144+
ScratchTargetTagHash = PredicateLockTargetTagHashCode(&ScratchTargetTag);
1145+
ScratchPartitionLock = PredicateLockHashPartitionLock(ScratchTargetTagHash);
11351146

11361147
/*
11371148
* Allocate hash table for PREDICATELOCK structs. This stores per
@@ -1143,6 +1154,9 @@ InitPredicateLocks(void)
11431154
info.hash = predicatelock_hash;
11441155
info.num_partitions = NUM_PREDICATELOCK_PARTITIONS;
11451156

1157+
/* Assume an average of 2 xacts per target */
1158+
max_table_size *= 2;
1159+
11461160
PredicateLockHash = ShmemInitHash("PREDICATELOCK hash",
11471161
max_table_size,
11481162
max_table_size,
@@ -1169,6 +1183,7 @@ InitPredicateLocks(void)
11691183
PredXact = ShmemInitStruct("PredXactList",
11701184
PredXactListDataSize,
11711185
&found);
1186+
Assert(found == IsUnderPostmaster);
11721187
if (!found)
11731188
{
11741189
int i;
@@ -1247,6 +1262,7 @@ InitPredicateLocks(void)
12471262
RWConflictPool = ShmemInitStruct("RWConflictPool",
12481263
RWConflictPoolHeaderDataSize,
12491264
&found);
1265+
Assert(found == IsUnderPostmaster);
12501266
if (!found)
12511267
{
12521268
int i;
@@ -1278,6 +1294,7 @@ InitPredicateLocks(void)
12781294
ShmemInitStruct("FinishedSerializableTransactions",
12791295
sizeof(SHM_QUEUE),
12801296
&found);
1297+
Assert(found == IsUnderPostmaster);
12811298
if (!found)
12821299
SHMQueueInit(FinishedSerializableTransactions);
12831300

@@ -1286,10 +1303,6 @@ InitPredicateLocks(void)
12861303
* transactions.
12871304
*/
12881305
OldSerXidInit();
1289-
1290-
/* Pre-calculate the hash and partition lock of the scratch entry */
1291-
ScratchTargetTagHash = PredicateLockTargetTagHashCode(&ScratchTargetTag);
1292-
ScratchPartitionLock = PredicateLockHashPartitionLock(ScratchTargetTagHash);
12931306
}
12941307

12951308
/*

0 commit comments

Comments
 (0)