Skip to content

Commit 861d3aa

Browse files
committed
Fix race condition between hot standby and restoring a full-page image.
There was a window in RestoreBackupBlock where a page would be zeroed out, but not yet locked. If a backend pinned and locked the page in that window, it saw the zeroed page instead of the old page or new page contents, which could lead to missing rows in a result set, or errors. To fix, replace RBM_ZERO with RBM_ZERO_AND_LOCK, which atomically pins, zeroes, and locks the page, if it's not in the buffer cache already. In stable branches, the old RBM_ZERO constant is renamed to RBM_DO_NOT_USE, to avoid breaking any 3rd party extensions that might use RBM_ZERO. More importantly, this avoids renumbering the other enum values, which would cause even bigger confusion in extensions that use ReadBufferExtended, but haven't been recompiled. Backpatch to all supported versions; this has been racy since hot standby was introduced.
1 parent c2b4ed1 commit 861d3aa

File tree

6 files changed

+66
-25
lines changed

6 files changed

+66
-25
lines changed

src/backend/access/hash/hashpage.c

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -155,9 +155,8 @@ _hash_getinitbuf(Relation rel, BlockNumber blkno)
155155
if (blkno == P_NEW)
156156
elog(ERROR, "hash AM does not use P_NEW");
157157

158-
buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_ZERO, NULL);
159-
160-
LockBuffer(buf, HASH_WRITE);
158+
buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_ZERO_AND_LOCK,
159+
NULL);
161160

162161
/* ref count and lock type are correct */
163162

@@ -198,11 +197,13 @@ _hash_getnewbuf(Relation rel, BlockNumber blkno, ForkNumber forkNum)
198197
if (BufferGetBlockNumber(buf) != blkno)
199198
elog(ERROR, "unexpected hash relation size: %u, should be %u",
200199
BufferGetBlockNumber(buf), blkno);
200+
LockBuffer(buf, HASH_WRITE);
201201
}
202202
else
203-
buf = ReadBufferExtended(rel, forkNum, blkno, RBM_ZERO, NULL);
204-
205-
LockBuffer(buf, HASH_WRITE);
203+
{
204+
buf = ReadBufferExtended(rel, forkNum, blkno, RBM_ZERO_AND_LOCK,
205+
NULL);
206+
}
206207

207208
/* ref count and lock type are correct */
208209

src/backend/access/heap/heapam.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7070,9 +7070,8 @@ heap_xlog_newpage(XLogRecPtr lsn, XLogRecord *record)
70707070
* not do anything that assumes we are touching a heap.
70717071
*/
70727072
buffer = XLogReadBufferExtended(xlrec->node, xlrec->forknum, xlrec->blkno,
7073-
RBM_ZERO);
7073+
RBM_ZERO_AND_LOCK);
70747074
Assert(BufferIsValid(buffer));
7075-
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
70767075
page = (Page) BufferGetPage(buffer);
70777076

70787077
Assert(record->xl_len == SizeOfHeapNewpage + BLCKSZ);

src/backend/access/transam/xlog.c

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3187,12 +3187,8 @@ RestoreBackupBlockContents(XLogRecPtr lsn, BkpBlock bkpb, char *blk,
31873187
Page page;
31883188

31893189
buffer = XLogReadBufferExtended(bkpb.node, bkpb.fork, bkpb.block,
3190-
RBM_ZERO);
3190+
get_cleanup_lock ? RBM_ZERO_AND_CLEANUP_LOCK : RBM_ZERO_AND_LOCK);
31913191
Assert(BufferIsValid(buffer));
3192-
if (get_cleanup_lock)
3193-
LockBufferForCleanup(buffer);
3194-
else
3195-
LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
31963192

31973193
page = (Page) BufferGetPage(buffer);
31983194

src/backend/access/transam/xlogutils.c

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -258,16 +258,17 @@ XLogCheckInvalidPages(void)
258258
* The returned buffer is exclusively-locked.
259259
*
260260
* For historical reasons, instead of a ReadBufferMode argument, this only
261-
* supports RBM_ZERO (init == true) and RBM_NORMAL (init == false) modes.
261+
* supports RBM_ZERO_AND_LOCK (init == true) and RBM_NORMAL (init == false)
262+
* modes.
262263
*/
263264
Buffer
264265
XLogReadBuffer(RelFileNode rnode, BlockNumber blkno, bool init)
265266
{
266267
Buffer buf;
267268

268269
buf = XLogReadBufferExtended(rnode, MAIN_FORKNUM, blkno,
269-
init ? RBM_ZERO : RBM_NORMAL);
270-
if (BufferIsValid(buf))
270+
init ? RBM_ZERO_AND_LOCK : RBM_NORMAL);
271+
if (BufferIsValid(buf) && !init)
271272
LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
272273

273274
return buf;
@@ -286,8 +287,8 @@ XLogReadBuffer(RelFileNode rnode, BlockNumber blkno, bool init)
286287
* dropped or truncated. If we don't see evidence of that later in the WAL
287288
* sequence, we'll complain at the end of WAL replay.)
288289
*
289-
* In RBM_ZERO and RBM_ZERO_ON_ERROR modes, if the page doesn't exist, the
290-
* relation is extended with all-zeroes pages up to the given block number.
290+
* In RBM_ZERO_* modes, if the page doesn't exist, the relation is extended
291+
* with all-zeroes pages up to the given block number.
291292
*
292293
* In RBM_NORMAL_NO_LOG mode, we return InvalidBuffer if the page doesn't
293294
* exist, and we don't check for all-zeroes. Thus, no log entry is made
@@ -341,14 +342,20 @@ XLogReadBufferExtended(RelFileNode rnode, ForkNumber forknum,
341342
do
342343
{
343344
if (buffer != InvalidBuffer)
345+
{
346+
if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
347+
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
344348
ReleaseBuffer(buffer);
349+
}
345350
buffer = ReadBufferWithoutRelcache(rnode, forknum,
346351
P_NEW, mode, NULL);
347352
}
348353
while (BufferGetBlockNumber(buffer) < blkno);
349354
/* Handle the corner case that P_NEW returns non-consecutive pages */
350355
if (BufferGetBlockNumber(buffer) != blkno)
351356
{
357+
if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
358+
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
352359
ReleaseBuffer(buffer);
353360
buffer = ReadBufferWithoutRelcache(rnode, forknum, blkno,
354361
mode, NULL);

src/backend/storage/buffer/bufmgr.c

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -214,14 +214,19 @@ ReadBuffer(Relation reln, BlockNumber blockNum)
214214
* valid, the page is zeroed instead of throwing an error. This is intended
215215
* for non-critical data, where the caller is prepared to repair errors.
216216
*
217-
* In RBM_ZERO mode, if the page isn't in buffer cache already, it's filled
218-
* with zeros instead of reading it from disk. Useful when the caller is
219-
* going to fill the page from scratch, since this saves I/O and avoids
217+
* In RBM_ZERO_AND_LOCK mode, if the page isn't in buffer cache already, it's
218+
* filled with zeros instead of reading it from disk. Useful when the caller
219+
* is going to fill the page from scratch, since this saves I/O and avoids
220220
* unnecessary failure if the page-on-disk has corrupt page headers.
221+
* The page is returned locked to ensure that the caller has a chance to
222+
* initialize the page before it's made visible to others.
221223
* Caution: do not use this mode to read a page that is beyond the relation's
222224
* current physical EOF; that is likely to cause problems in md.c when
223225
* the page is modified and written out. P_NEW is OK, though.
224226
*
227+
* RBM_ZERO_AND_CLEANUP_LOCK is the same as RBM_ZERO_AND_LOCK, but acquires
228+
* a cleanup-strength lock on the page.
229+
*
225230
* RBM_NORMAL_NO_LOG mode is treated the same as RBM_NORMAL here.
226231
*
227232
* If strategy is not NULL, a nondefault buffer access strategy is used.
@@ -363,6 +368,18 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
363368
isExtend,
364369
found);
365370

371+
/*
372+
* In RBM_ZERO_AND_LOCK mode, the caller expects the buffer to
373+
* be already locked on return.
374+
*/
375+
if (!isLocalBuf)
376+
{
377+
if (mode == RBM_ZERO_AND_LOCK)
378+
LWLockAcquire(bufHdr->content_lock, LW_EXCLUSIVE);
379+
else if (mode == RBM_ZERO_AND_CLEANUP_LOCK)
380+
LockBufferForCleanup(BufferDescriptorGetBuffer(bufHdr));
381+
}
382+
366383
return BufferDescriptorGetBuffer(bufHdr);
367384
}
368385

@@ -444,8 +461,11 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
444461
* Read in the page, unless the caller intends to overwrite it and
445462
* just wants us to allocate a buffer.
446463
*/
447-
if (mode == RBM_ZERO)
464+
if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK ||
465+
mode == RBM_DO_NOT_USE)
466+
{
448467
MemSet((char *) bufBlock, 0, BLCKSZ);
468+
}
449469
else
450470
{
451471
instr_time io_start,
@@ -486,6 +506,19 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
486506
}
487507
}
488508

509+
/*
510+
* In RBM_ZERO_AND_LOCK mode, grab the buffer content lock before marking
511+
* the page as valid, to make sure that no other backend sees the zeroed
512+
* page before the caller has had a chance to initialize it.
513+
*
514+
* Since no-one else can be looking at the page contents yet, there is no
515+
* difference between an exclusive lock and a cleanup-strength lock.
516+
* (Note that we cannot use LockBuffer() of LockBufferForCleanup() here,
517+
* because they assert that the buffer is already valid.)
518+
*/
519+
if (mode == RBM_ZERO_AND_LOCK || mode == RBM_ZERO_AND_CLEANUP_LOCK)
520+
LWLockAcquire(bufHdr->content_lock, LW_EXCLUSIVE);
521+
489522
if (isLocalBuf)
490523
{
491524
/* Only need to adjust flags */

src/include/storage/bufmgr.h

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,16 @@ typedef enum BufferAccessStrategyType
3636
typedef enum
3737
{
3838
RBM_NORMAL, /* Normal read */
39-
RBM_ZERO, /* Don't read from disk, caller will
40-
* initialize */
39+
RBM_DO_NOT_USE, /* This used to be RBM_ZERO. Only kept for
40+
* binary compatibility with 3rd party
41+
* extensions. */
4142
RBM_ZERO_ON_ERROR, /* Read, but return an all-zeros page on error */
42-
RBM_NORMAL_NO_LOG /* Don't log page as invalid during WAL
43+
RBM_NORMAL_NO_LOG, /* Don't log page as invalid during WAL
4344
* replay; otherwise same as RBM_NORMAL */
45+
RBM_ZERO_AND_LOCK, /* Don't read from disk, caller will
46+
* initialize. Also locks the page. */
47+
RBM_ZERO_AND_CLEANUP_LOCK /* Like RBM_ZERO_AND_LOCK, but locks the page
48+
* in "cleanup" mode */
4449
} ReadBufferMode;
4550

4651
/* in globals.c ... this duplicates miscadmin.h */

0 commit comments

Comments
 (0)