Fix transient clobbering of shared buffers during WAL replay.

tglsfdc · tglsfdc · commit 2b196f01efd8 · 2012-02-05T15:49:46.000-05:00
RestoreBkpBlocks was in the habit of zeroing and refilling the target
buffer; which was perfectly safe when the code was written, but is unsafe
during Hot Standby operation.  The reason is that we have coding rules
that allow backends to continue accessing a tuple in a heap relation while
holding only a pin on its buffer.  Such a backend could see transiently
zeroed data, if WAL replay had occasion to change other data on the page.
This has been shown to be the cause of bug #6425 from Duncan Rance (who
deserves kudos for developing a sufficiently-reproducible test case) as
well as Bridget Frey's re-report of bug #6200.  It most likely explains the
original report as well, though we don't yet have confirmation of that.

To fix, change the code so that only bytes that are supposed to change will
change, even transiently.  This actually saves cycles in RestoreBkpBlocks,
since it's not writing the same bytes twice.

Also fix seq_redo, which has the same disease, though it has to work a bit
harder to meet the requirement.

So far as I can tell, no other WAL replay routines have this type of bug.
In particular, the index-related replay routines, which would certainly be
broken if they had to meet the same standard, are not at risk because we
do not have coding rules that allow access to an index page when not
holding a buffer lock on it.

Back-patch to 9.0 where Hot Standby was added.
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
@@ -3556,9 +3556,9 @@ RestoreBkpBlocks(XLogRecPtr lsn, XLogRecord *record, bool cleanup)
 		}
 		else
 		{
-			/* must zero-fill the hole */
-			MemSet((char *) page, 0, BLCKSZ);
 			memcpy((char *) page, blk, bkpb.hole_offset);
+			/* must zero-fill the hole */
+			MemSet((char *) page + bkpb.hole_offset, 0, bkpb.hole_length);
 			memcpy((char *) page + (bkpb.hole_offset + bkpb.hole_length),
 				   blk + bkpb.hole_offset,
 				   BLCKSZ - (bkpb.hole_offset + bkpb.hole_length));
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
@@ -1358,6 +1358,7 @@ seq_redo(XLogRecPtr lsn, XLogRecord *record)
 	uint8		info = record->xl_info & ~XLR_INFO_MASK;
 	Buffer		buffer;
 	Page		page;
+	Page		localpage;
 	char	   *item;
 	Size		itemsz;
 	xl_seq_rec *xlrec = (xl_seq_rec *) XLogRecGetData(record);
@@ -1373,23 +1374,37 @@ seq_redo(XLogRecPtr lsn, XLogRecord *record)
 	Assert(BufferIsValid(buffer));
 	page = (Page) BufferGetPage(buffer);
 
-	/* Always reinit the page and reinstall the magic number */
-	/* See comments in DefineSequence */
-	PageInit((Page) page, BufferGetPageSize(buffer), sizeof(sequence_magic));
-	sm = (sequence_magic *) PageGetSpecialPointer(page);
+	/*
+	 * We must always reinit the page and reinstall the magic number (see
+	 * comments in fill_seq_with_data).  However, since this WAL record type
+	 * is also used for updating sequences, it's possible that a hot-standby
+	 * backend is examining the page concurrently; so we mustn't transiently
+	 * trash the buffer.  The solution is to build the correct new page
+	 * contents in local workspace and then memcpy into the buffer.  Then
+	 * only bytes that are supposed to change will change, even transiently.
+	 * We must palloc the local page for alignment reasons.
+	 */
+	localpage = (Page) palloc(BufferGetPageSize(buffer));
+
+	PageInit(localpage, BufferGetPageSize(buffer), sizeof(sequence_magic));
+	sm = (sequence_magic *) PageGetSpecialPointer(localpage);
 	sm->magic = SEQ_MAGIC;
 
 	item = (char *) xlrec + sizeof(xl_seq_rec);
 	itemsz = record->xl_len - sizeof(xl_seq_rec);
 	itemsz = MAXALIGN(itemsz);
-	if (PageAddItem(page, (Item) item, itemsz,
+	if (PageAddItem(localpage, (Item) item, itemsz,
 					FirstOffsetNumber, false, false) == InvalidOffsetNumber)
 		elog(PANIC, "seq_redo: failed to add item to page");
 
-	PageSetLSN(page, lsn);
-	PageSetTLI(page, ThisTimeLineID);
+	PageSetLSN(localpage, lsn);
+	PageSetTLI(localpage, ThisTimeLineID);
+
+	memcpy(page, localpage, BufferGetPageSize(buffer));
 	MarkBufferDirty(buffer);
 	UnlockReleaseBuffer(buffer);
+
+	pfree(localpage);
 }
 
 void

Original file line number	Diff line number	Diff line change
`@@ -3556,9 +3556,9 @@ RestoreBkpBlocks(XLogRecPtr lsn, XLogRecord *record, bool cleanup)`
`3556`	`3556`	`}`
`3557`	`3557`	`else`
`3558`	`3558`	`{`
`3559`		`- /* must zero-fill the hole */`
`3560`		`- MemSet((char *) page, 0, BLCKSZ);`
`3561`	`3559`	`memcpy((char *) page, blk, bkpb.hole_offset);`
	`3560`	`+ /* must zero-fill the hole */`
	`3561`	`+ MemSet((char *) page + bkpb.hole_offset, 0, bkpb.hole_length);`
`3562`	`3562`	`memcpy((char *) page + (bkpb.hole_offset + bkpb.hole_length),`
`3563`	`3563`	`blk + bkpb.hole_offset,`
`3564`	`3564`	`BLCKSZ - (bkpb.hole_offset + bkpb.hole_length));`