Refactor XLogInsert a bit. The rdata entries for backup blocks are now

constructed before acquiring WALInsertLock, which slightly reduces the time
the lock is held. Although I could not measure any benefit in benchmarks,
the code is more readable this way.

src/backend/access/transam/xlog.c

@@ -694,6 +694,7 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
 	uint32		freespace;
 	int			curridx;
 	XLogRecData *rdt;
+	XLogRecData *rdt_lastnormal;
 	Buffer		dtbuf[XLR_MAX_BKP_BLOCKS];
 	bool		dtbuf_bkp[XLR_MAX_BKP_BLOCKS];
 	BkpBlock	dtbuf_xlg[XLR_MAX_BKP_BLOCKS];
@@ -708,6 +709,7 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
 	bool		updrqst;
 	bool		doPageWrites;
 	bool		isLogSwitch = (rmid == RM_XLOG_ID && info == XLOG_SWITCH);
+	uint8		info_orig = info;
 
 	/* cross-check on whether we should be here or not */
 	if (!XLogInsertAllowed())
@@ -731,23 +733,18 @@ XLogInsert(RmgrId rmid, uint8 info, XLogRecData *rdata)
 	}
 
 	/*
-	 * Here we scan the rdata chain, determine which buffers must be backed
-	 * up, and compute the CRC values for the data.  Note that the record
-	 * header isn't added into the CRC initially since we don't know the final
-	 * length or info bits quite yet.  Thus, the CRC will represent the CRC of
-	 * the whole record in the order "rdata, then backup blocks, then record
-	 * header".
+	 * Here we scan the rdata chain, to determine which buffers must be backed
+	 * up.
 	 *
 	 * We may have to loop back to here if a race condition is detected below.
 	 * We could prevent the race by doing all this work while holding the
 	 * insert lock, but it seems better to avoid doing CRC calculations while
-	 * holding the lock.  This means we have to be careful about modifying the
-	 * rdata chain until we know we aren't going to loop back again.  The only
-	 * change we allow ourselves to make earlier is to set rdt->data = NULL in
-	 * chain items we have decided we will have to back up the whole buffer
-	 * for.  This is OK because we will certainly decide the same thing again
-	 * for those items if we do it over; doing it here saves an extra pass
-	 * over the chain later.
+	 * holding the lock.
+	 *
+	 * We add entries for backup blocks to the chain, so that they don't
+	 * need any special treatment in the critical section where the chunks are
+	 * copied into the WAL buffers. Those entries have to be unlinked from the
+	 * chain if we have to loop back here.
 	 */
 begin:;
 	for (i = 0; i < XLR_MAX_BKP_BLOCKS; i++)
@@ -764,7 +761,6 @@ begin:;
 	 */
 	doPageWrites = fullPageWrites || Insert->forcePageWrites;
 
-	INIT_CRC32(rdata_crc);
 	len = 0;
 	for (rdt = rdata;;)
 	{
@@ -772,7 +768,6 @@ begin:;
 		{
 			/* Simple data, just include it */
 			len += rdt->len;
-			COMP_CRC32(rdata_crc, rdt->data, rdt->len);
 		}
 		else
 		{
@@ -783,12 +778,12 @@ begin:;
 				{
 					/* Buffer already referenced by earlier chain item */
 					if (dtbuf_bkp[i])
+					{
 						rdt->data = NULL;
+						rdt->len = 0;
+					}
 					else if (rdt->data)
-					{
 						len += rdt->len;
-						COMP_CRC32(rdata_crc, rdt->data, rdt->len);
-					}
 					break;
 				}
 				if (dtbuf[i] == InvalidBuffer)
@@ -800,12 +795,10 @@ begin:;
 					{
 						dtbuf_bkp[i] = true;
 						rdt->data = NULL;
+						rdt->len = 0;
 					}
 					else if (rdt->data)
-					{
 						len += rdt->len;
-						COMP_CRC32(rdata_crc, rdt->data, rdt->len);
-					}
 					break;
 				}
 			}
@@ -820,47 +813,88 @@ begin:;
 	}
 
 	/*
-	 * Now add the backup block headers and data into the CRC
+	 * NOTE: We disallow len == 0 because it provides a useful bit of extra
+	 * error checking in ReadRecord.  This means that all callers of
+	 * XLogInsert must supply at least some not-in-a-buffer data.  However, we
+	 * make an exception for XLOG SWITCH records because we don't want them to
+	 * ever cross a segment boundary.
 	 */
+	if (len == 0 && !isLogSwitch)
+		elog(PANIC, "invalid xlog record length %u", len);
+
+	/*
+	 * Make additional rdata chain entries for the backup blocks, so that we
+	 * don't need to special-case them in the write loop.  This modifies the
+	 * original rdata chain, but we keep a pointer to the last regular entry,
+	 * rdt_lastnormal, so that we can undo this if we have to loop back to the
+	 * beginning.
+	 *
+	 * At the exit of this loop, write_len includes the backup block data.
+	 *
+	 * Also set the appropriate info bits to show which buffers were backed
+	 * up. The i'th XLR_SET_BKP_BLOCK bit corresponds to the i'th distinct
+	 * buffer value (ignoring InvalidBuffer) appearing in the rdata chain.
+	 */
+	rdt_lastnormal = rdt;
+	write_len = len;
 	for (i = 0; i < XLR_MAX_BKP_BLOCKS; i++)
 	{
-		if (dtbuf_bkp[i])
-		{
-			BkpBlock   *bkpb = &(dtbuf_xlg[i]);
+		BkpBlock   *bkpb;
 		char	   *page;
 
-			COMP_CRC32(rdata_crc,
-					   (char *) bkpb,
-					   sizeof(BkpBlock));
+		if (!dtbuf_bkp[i])
+			continue;
+
+		info |= XLR_SET_BKP_BLOCK(i);
+
+		bkpb = &(dtbuf_xlg[i]);
 		page = (char *) BufferGetBlock(dtbuf[i]);
+
+		rdt->next = &(dtbuf_rdt1[i]);
+		rdt = rdt->next;
+
+		rdt->data = (char *) bkpb;
+		rdt->len = sizeof(BkpBlock);
+		write_len += sizeof(BkpBlock);
+
+		rdt->next = &(dtbuf_rdt2[i]);
+		rdt = rdt->next;
+
 		if (bkpb->hole_length == 0)
 		{
-				COMP_CRC32(rdata_crc,
-						   page,
-						   BLCKSZ);
+			rdt->data = page;
+			rdt->len = BLCKSZ;
+			write_len += BLCKSZ;
+			rdt->next = NULL;
 		}
 		else
 		{
 			/* must skip the hole */
-				COMP_CRC32(rdata_crc,
-						   page,
-						   bkpb->hole_offset);
-				COMP_CRC32(rdata_crc,
-						   page + (bkpb->hole_offset + bkpb->hole_length),
-						   BLCKSZ - (bkpb->hole_offset + bkpb->hole_length));
-			}
+			rdt->data = page;
+			rdt->len = bkpb->hole_offset;
+			write_len += bkpb->hole_offset;
+
+			rdt->next = &(dtbuf_rdt3[i]);
+			rdt = rdt->next;
+
+			rdt->data = page + (bkpb->hole_offset + bkpb->hole_length);
+			rdt->len = BLCKSZ - (bkpb->hole_offset + bkpb->hole_length);
+			write_len += rdt->len;
+			rdt->next = NULL;
 		}
 	}
 
 	/*
-	 * NOTE: We disallow len == 0 because it provides a useful bit of extra
-	 * error checking in ReadRecord.  This means that all callers of
-	 * XLogInsert must supply at least some not-in-a-buffer data.  However, we
-	 * make an exception for XLOG SWITCH records because we don't want them to
-	 * ever cross a segment boundary.
+	 * Calculate CRC of the data, including all the backup blocks
+	 *
+	 * Note that the record header isn't added into the CRC initially since
+	 * we don't know the prev-link yet.  Thus, the CRC will represent the CRC
+	 * of the whole record in the order: rdata, then backup blocks, then
+	 * record header.
 	 */
-	if (len == 0 && !isLogSwitch)
-		elog(PANIC, "invalid xlog record length %u", len);
+	INIT_CRC32(rdata_crc);
+	for (rdt = rdata; rdt != NULL; rdt = rdt->next)
+		COMP_CRC32(rdata_crc, rdt->data, rdt->len);
 
 	START_CRIT_SECTION();
 
@@ -896,6 +930,8 @@ begin:;
 					 */
 					LWLockRelease(WALInsertLock);
 					END_CRIT_SECTION();
+					rdt_lastnormal->next = NULL;
+					info = info_orig;
 					goto begin;
 				}
 			}
@@ -910,70 +946,14 @@ begin:;
 	 */
 	if (Insert->forcePageWrites && !doPageWrites)
 	{
-		/* Oops, must redo it with full-page data */
+		/* Oops, must redo it with full-page data. */
 		LWLockRelease(WALInsertLock);
 		END_CRIT_SECTION();
+		rdt_lastnormal->next = NULL;
+		info = info_orig;
 		goto begin;
 	}
 
-	/*
-	 * Make additional rdata chain entries for the backup blocks, so that we
-	 * don't need to special-case them in the write loop.  Note that we have
-	 * now irrevocably changed the input rdata chain.  At the exit of this
-	 * loop, write_len includes the backup block data.
-	 *
-	 * Also set the appropriate info bits to show which buffers were backed
-	 * up. The i'th XLR_SET_BKP_BLOCK bit corresponds to the i'th distinct
-	 * buffer value (ignoring InvalidBuffer) appearing in the rdata chain.
-	 */
-	write_len = len;
-	for (i = 0; i < XLR_MAX_BKP_BLOCKS; i++)
-	{
-		BkpBlock   *bkpb;
-		char	   *page;
-
-		if (!dtbuf_bkp[i])
-			continue;
-
-		info |= XLR_SET_BKP_BLOCK(i);
-
-		bkpb = &(dtbuf_xlg[i]);
-		page = (char *) BufferGetBlock(dtbuf[i]);
-
-		rdt->next = &(dtbuf_rdt1[i]);
-		rdt = rdt->next;
-
-		rdt->data = (char *) bkpb;
-		rdt->len = sizeof(BkpBlock);
-		write_len += sizeof(BkpBlock);
-
-		rdt->next = &(dtbuf_rdt2[i]);
-		rdt = rdt->next;
-
-		if (bkpb->hole_length == 0)
-		{
-			rdt->data = page;
-			rdt->len = BLCKSZ;
-			write_len += BLCKSZ;
-			rdt->next = NULL;
-		}
-		else
-		{
-			/* must skip the hole */
-			rdt->data = page;
-			rdt->len = bkpb->hole_offset;
-			write_len += bkpb->hole_offset;
-
-			rdt->next = &(dtbuf_rdt3[i]);
-			rdt = rdt->next;
-
-			rdt->data = page + (bkpb->hole_offset + bkpb->hole_length);
-			rdt->len = BLCKSZ - (bkpb->hole_offset + bkpb->hole_length);
-			write_len += rdt->len;
-			rdt->next = NULL;
-		}
-	}
-
 	/*
 	 * If there isn't enough space on the current XLOG page for a record
 	 * header, advance to the next page (leaving the unused space as zeroes).