Skip to content

Commit c6d76d7

Browse files
tglsfdctglsfdc
committed
Add locking around WAL-replay modification of shared-memory variables.
Originally, most of this code assumed that no Postgres backends could be running concurrently with it, and so no locking could be needed. That assumption fails in Hot Standby. While it's still true that Hot Standby backends should never change values like nextXid, they can examine them, and consistency is important in some cases such as when computing a snapshot. Therefore, prudence requires that WAL replay code obtain the relevant locks when modifying such variables, even though it can examine them without taking a lock. We were following that coding rule in some places but not all. This commit applies the coding rule uniformly to all updates of ShmemVariableCache and MultiXactState fields; a search of the replay routines did not find any other cases that seemed to be at risk. In addition, this commit fixes a longstanding thinko in replay of NEXTOID and checkpoint records: we tried to advance nextOid only if it was behind the value in the WAL record, but the comparison would draw the wrong conclusion if OID wraparound had occurred since the previous value. Better to just unconditionally assign the new value, since OID assignment shouldn't be happening during replay anyway. The additional locking seems to be more in the nature of future-proofing than fixing any live bug, so I am not going to back-patch it. The NEXTOID fix will be back-patched separately.
1 parent 96abd81 commit c6d76d7

File tree

4 files changed

+75
-20
lines changed

4 files changed

+75
-20
lines changed

‎src/backend/access/transam/multixact.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1655,17 +1655,20 @@ CheckPointMultiXact(void)
16551655
* Set the next-to-be-assigned MultiXactId and offset
16561656
*
16571657
* This is used when we can determine the correct next ID/offset exactly
1658-
* from a checkpoint record. We need no locking since it is only called
1659-
* during bootstrap and XLog replay.
1658+
* from a checkpoint record. Although this is only called during bootstrap
1659+
* and XLog replay, we take the lock in case any hot-standby backends are
1660+
* examining the values.
16601661
*/
16611662
void
16621663
MultiXactSetNextMXact(MultiXactId nextMulti,
16631664
MultiXactOffset nextMultiOffset)
16641665
{
16651666
debug_elog4(DEBUG2, "MultiXact: setting next multi to %u offset %u",
16661667
nextMulti, nextMultiOffset);
1668+
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);
16671669
MultiXactState->nextMXact = nextMulti;
16681670
MultiXactState->nextOffset = nextMultiOffset;
1671+
LWLockRelease(MultiXactGenLock);
16691672
}
16701673

16711674
/*
@@ -1674,12 +1677,14 @@ MultiXactSetNextMXact(MultiXactId nextMulti,
16741677
*
16751678
* This is used when we can determine minimum safe values from an XLog
16761679
* record (either an on-line checkpoint or an mxact creation log entry).
1677-
* We need no locking since it is only called during XLog replay.
1680+
* Although this is only called during XLog replay, we take the lock in case
1681+
* any hot-standby backends are examining the values.
16781682
*/
16791683
void
16801684
MultiXactAdvanceNextMXact(MultiXactId minMulti,
16811685
MultiXactOffset minMultiOffset)
16821686
{
1687+
LWLockAcquire(MultiXactGenLock, LW_EXCLUSIVE);
16831688
if (MultiXactIdPrecedes(MultiXactState->nextMXact, minMulti))
16841689
{
16851690
debug_elog3(DEBUG2, "MultiXact: setting next multi to %u", minMulti);
@@ -1691,6 +1696,7 @@ MultiXactAdvanceNextMXact(MultiXactId minMulti,
16911696
minMultiOffset);
16921697
MultiXactState->nextOffset = minMultiOffset;
16931698
}
1699+
LWLockRelease(MultiXactGenLock);
16941700
}
16951701

16961702
/*

‎src/backend/access/transam/twophase.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1715,6 +1715,10 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
17151715
/*
17161716
* Examine subtransaction XIDs ... they should all follow main
17171717
* XID, and they may force us to advance nextXid.
1718+
*
1719+
* We don't expect anyone else to modify nextXid, hence we don't
1720+
* need to hold a lock while examining it. We still acquire the
1721+
* lock to modify it, though.
17181722
*/
17191723
subxids = (TransactionId *)
17201724
(buf + MAXALIGN(sizeof(TwoPhaseFileHeader)));
@@ -1726,8 +1730,10 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
17261730
if (TransactionIdFollowsOrEquals(subxid,
17271731
ShmemVariableCache->nextXid))
17281732
{
1733+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
17291734
ShmemVariableCache->nextXid = subxid;
17301735
TransactionIdAdvance(ShmemVariableCache->nextXid);
1736+
LWLockRelease(XidGenLock);
17311737
}
17321738
}
17331739

‎src/backend/access/transam/xlog.c

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6625,12 +6625,20 @@ StartupXLOG(void)
66256625
errcontext.previous = error_context_stack;
66266626
error_context_stack = &errcontext;
66276627

6628-
/* nextXid must be beyond record's xid */
6628+
/*
6629+
* ShmemVariableCache->nextXid must be beyond record's xid.
6630+
*
6631+
* We don't expect anyone else to modify nextXid, hence we
6632+
* don't need to hold a lock while examining it. We still
6633+
* acquire the lock to modify it, though.
6634+
*/
66296635
if (TransactionIdFollowsOrEquals(record->xl_xid,
66306636
ShmemVariableCache->nextXid))
66316637
{
6638+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
66326639
ShmemVariableCache->nextXid = record->xl_xid;
66336640
TransactionIdAdvance(ShmemVariableCache->nextXid);
6641+
LWLockRelease(XidGenLock);
66346642
}
66356643

66366644
/*
@@ -6656,6 +6664,7 @@ StartupXLOG(void)
66566664
TransactionIdIsValid(record->xl_xid))
66576665
RecordKnownAssignedTransactionIds(record->xl_xid);
66586666

6667+
/* Now apply the WAL record itself */
66596668
RmgrTable[record->xl_rmid].rm_redo(EndRecPtr, record);
66606669

66616670
/* Pop the error context stack */
@@ -6971,8 +6980,10 @@ StartupXLOG(void)
69716980
XLogCtl->ckptXid = ControlFile->checkPointCopy.nextXid;
69726981

69736982
/* also initialize latestCompletedXid, to nextXid - 1 */
6983+
LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
69746984
ShmemVariableCache->latestCompletedXid = ShmemVariableCache->nextXid;
69756985
TransactionIdRetreat(ShmemVariableCache->latestCompletedXid);
6986+
LWLockRelease(ProcArrayLock);
69766987

69776988
/*
69786989
* Start up the commit log and subtrans, if not already done for
@@ -8547,22 +8558,32 @@ xlog_redo(XLogRecPtr lsn, XLogRecord *record)
85478558
{
85488559
Oid nextOid;
85498560

8561+
/*
8562+
* We used to try to take the maximum of ShmemVariableCache->nextOid
8563+
* and the recorded nextOid, but that fails if the OID counter wraps
8564+
* around. Since no OID allocation should be happening during replay
8565+
* anyway, better to just believe the record exactly. We still take
8566+
* OidGenLock while setting the variable, just in case.
8567+
*/
85508568
memcpy(&nextOid, XLogRecGetData(record), sizeof(Oid));
8551-
if (ShmemVariableCache->nextOid < nextOid)
8552-
{
8553-
ShmemVariableCache->nextOid = nextOid;
8554-
ShmemVariableCache->oidCount = 0;
8555-
}
8569+
LWLockAcquire(OidGenLock, LW_EXCLUSIVE);
8570+
ShmemVariableCache->nextOid = nextOid;
8571+
ShmemVariableCache->oidCount = 0;
8572+
LWLockRelease(OidGenLock);
85568573
}
85578574
else if (info == XLOG_CHECKPOINT_SHUTDOWN)
85588575
{
85598576
CheckPoint checkPoint;
85608577

85618578
memcpy(&checkPoint, XLogRecGetData(record), sizeof(CheckPoint));
85628579
/* In a SHUTDOWN checkpoint, believe the counters exactly */
8580+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
85638581
ShmemVariableCache->nextXid = checkPoint.nextXid;
8582+
LWLockRelease(XidGenLock);
8583+
LWLockAcquire(OidGenLock, LW_EXCLUSIVE);
85648584
ShmemVariableCache->nextOid = checkPoint.nextOid;
85658585
ShmemVariableCache->oidCount = 0;
8586+
LWLockRelease(OidGenLock);
85668587
MultiXactSetNextMXact(checkPoint.nextMulti,
85678588
checkPoint.nextMultiOffset);
85688589
SetTransactionIdLimit(checkPoint.oldestXid, checkPoint.oldestXidDB);
@@ -8575,7 +8596,7 @@ xlog_redo(XLogRecPtr lsn, XLogRecord *record)
85758596
if (InArchiveRecovery &&
85768597
!XLogRecPtrIsInvalid(ControlFile->backupStartPoint) &&
85778598
XLogRecPtrIsInvalid(ControlFile->backupEndPoint))
8578-
ereport(ERROR,
8599+
ereport(PANIC,
85798600
(errmsg("online backup was canceled, recovery cannot continue")));
85808601

85818602
/*
@@ -8641,15 +8662,17 @@ xlog_redo(XLogRecPtr lsn, XLogRecord *record)
86418662
CheckPoint checkPoint;
86428663

86438664
memcpy(&checkPoint, XLogRecGetData(record), sizeof(CheckPoint));
8644-
/* In an ONLINE checkpoint, treat the counters like NEXTOID */
8665+
/* In an ONLINE checkpoint, treat the XID counter as a minimum */
8666+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
86458667
if (TransactionIdPrecedes(ShmemVariableCache->nextXid,
86468668
checkPoint.nextXid))
86478669
ShmemVariableCache->nextXid = checkPoint.nextXid;
8648-
if (ShmemVariableCache->nextOid < checkPoint.nextOid)
8649-
{
8650-
ShmemVariableCache->nextOid = checkPoint.nextOid;
8651-
ShmemVariableCache->oidCount = 0;
8652-
}
8670+
LWLockRelease(XidGenLock);
8671+
/* ... but still treat OID counter as exact */
8672+
LWLockAcquire(OidGenLock, LW_EXCLUSIVE);
8673+
ShmemVariableCache->nextOid = checkPoint.nextOid;
8674+
ShmemVariableCache->oidCount = 0;
8675+
LWLockRelease(OidGenLock);
86538676
MultiXactAdvanceNextMXact(checkPoint.nextMulti,
86548677
checkPoint.nextMultiOffset);
86558678
if (TransactionIdPrecedes(ShmemVariableCache->oldestXid,

‎src/backend/storage/ipc/procarray.c

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -654,17 +654,28 @@ ProcArrayApplyRecoveryInfo(RunningTransactions running)
654654
running->latestCompletedXid))
655655
ShmemVariableCache->latestCompletedXid = running->latestCompletedXid;
656656

657-
/* nextXid must be beyond any observed xid */
657+
Assert(TransactionIdIsNormal(ShmemVariableCache->latestCompletedXid));
658+
659+
LWLockRelease(ProcArrayLock);
660+
661+
/*
662+
* ShmemVariableCache->nextXid must be beyond any observed xid.
663+
*
664+
* We don't expect anyone else to modify nextXid, hence we don't need to
665+
* hold a lock while examining it. We still acquire the lock to modify
666+
* it, though.
667+
*/
658668
nextXid = latestObservedXid;
659669
TransactionIdAdvance(nextXid);
660670
if (TransactionIdFollows(nextXid, ShmemVariableCache->nextXid))
671+
{
672+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
661673
ShmemVariableCache->nextXid = nextXid;
674+
LWLockRelease(XidGenLock);
675+
}
662676

663-
Assert(TransactionIdIsNormal(ShmemVariableCache->latestCompletedXid));
664677
Assert(TransactionIdIsValid(ShmemVariableCache->nextXid));
665678

666-
LWLockRelease(ProcArrayLock);
667-
668679
KnownAssignedXidsDisplay(trace_recovery(DEBUG3));
669680
if (standbyState == STANDBY_SNAPSHOT_READY)
670681
elog(trace_recovery(DEBUG1), "recovery snapshots are now enabled");
@@ -1690,6 +1701,13 @@ GetOldestActiveTransactionId(void)
16901701

16911702
LWLockAcquire(ProcArrayLock, LW_SHARED);
16921703

1704+
/*
1705+
* It's okay to read nextXid without acquiring XidGenLock because (1) we
1706+
* assume TransactionIds can be read atomically and (2) we don't care if
1707+
* we get a slightly stale value. It can't be very stale anyway, because
1708+
* the LWLockAcquire above will have done any necessary memory
1709+
* interlocking.
1710+
*/
16931711
oldestRunningXid = ShmemVariableCache->nextXid;
16941712

16951713
/*
@@ -2609,7 +2627,9 @@ RecordKnownAssignedTransactionIds(TransactionId xid)
26092627
/* ShmemVariableCache->nextXid must be beyond any observed xid */
26102628
next_expected_xid = latestObservedXid;
26112629
TransactionIdAdvance(next_expected_xid);
2630+
LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
26122631
ShmemVariableCache->nextXid = next_expected_xid;
2632+
LWLockRelease(XidGenLock);
26132633
}
26142634
}
26152635

0 commit comments

Comments
 (0)