[R, Siva <sivasubr@amazon.com>]

Hi,

We recently encountered an issue where the opaque data flags on a gin data leaf page was corrupted while replaying a gin insert WAL record. Upon further examination of the redo code, we found a bug in ginRedoRecompress code, which extracts the WAL information and updates the page.

Specifically, when a new segment is inserted in the middle of a page, a memmove operation is performed [1] at the current point in the page to make room for the new segment. If this segment insertion is followed by delete segment actions that are yet to be processed and the total data size is very close to GinDataPageMaxDataSize, then we may move the data portion beyond the boundary causing the opaque data to be corrupted.

One way of solving this problem is to perform the replay work on a scratch space, perform sanity check on the total size of the data portion before copying it back to the actual page. While it involves additional memory allocation and memcpy operations, it is safer and similar to the 'do' code path where we ensure to make a copy of all segment past the first modified segment before placing them back on the page [2].

I have attached a patch for that approach here. Please let us know any comments or feedback.
Thanks!

Best
Siva

References:
[1] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginxlog.c;h=7515...
[2] - https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/gindatapage.c;h=...

Attachments:

  [application/octet-stream] rewrite-ginRedoRecompress-scratch-space-processing-replay_v1.patch (7.7K, ../1536091151804.6588@amazon.com/3-rewrite-ginRedoRecompress-scratch-space-processing-replay_v1.patch)
  download | inline diff:
From 9f971e7802d041573024eede7e0072372ef1d6e6 Mon Sep 17 00:00:00 2001
From: Siva R <sivasubr@amazon.com>
Date: Tue, 4 Sep 2018 19:19:16 +0000
Subject: [PATCH] Rewrite ginRedoRecompress to use a scratch space for
 processing replay actions instead of modifying contents on the actual page to
 avoid accidental overwrites of opaque data.

---
 src/backend/access/gin/ginxlog.c | 180 +++++++++++++++++++++------------------
 1 file changed, 98 insertions(+), 82 deletions(-)

diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 7515f8b..7ea798f 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -138,17 +138,21 @@ ginRedoRecompress(Page page, ginxlogRecompressDataLeaf *data)
 {
 	int			actionno;
 	int			segno;
-	GinPostingList *oldseg;
+	GinPostingList *segiter;
+	Pointer		segmentcopybegin;
 	Pointer		segmentend;
 	char	   *walbuf;
 	int			totalsize;
+	Pointer		scratch;
+	Pointer		scratchbegin;
+	bool		modified;
 
 	/*
 	 * If the page is in pre-9.4 format, convert to new format first.
 	 */
 	if (!GinPageIsCompressed(page))
 	{
-		ItemPointer uncompressed = (ItemPointer) GinDataPageGetData(page);
+		ItemPointer uncompressed = (ItemPointer)GinDataPageGetData(page);
 		int			nuncompressed = GinPageGetOpaque(page)->maxoff;
 		int			npacked;
 
@@ -180,63 +184,77 @@ ginRedoRecompress(Page page, ginxlogRecompressDataLeaf *data)
 		GinPageGetOpaque(page)->maxoff = InvalidOffsetNumber;
 	}
 
-	oldseg = GinDataLeafPageGetPostingList(page);
-	segmentend = (Pointer) oldseg + GinDataLeafPageGetPostingListSize(page);
-	segno = 0;
+	scratch = palloc(GinDataPageMaxDataSize);
+
+	segiter = GinDataLeafPageGetPostingList(page);
+	segmentend = (Pointer)segiter + GinDataLeafPageGetPostingListSize(page);
+	segmentcopybegin = segmentend;
 
-	walbuf = ((char *) data) + sizeof(ginxlogRecompressDataLeaf);
+	segno = 0;
+	modified = false;
+	scratchbegin = scratch;
+	walbuf = ((char *)data) + sizeof(ginxlogRecompressDataLeaf);
 	for (actionno = 0; actionno < data->nactions; actionno++)
 	{
-		uint8		a_segno = *((uint8 *) (walbuf++));
-		uint8		a_action = *((uint8 *) (walbuf++));
+		uint8		a_segno = *((uint8 *)(walbuf++));
+		uint8		a_action = *((uint8 *)(walbuf++));
 		GinPostingList *newseg = NULL;
 		int			newsegsize = 0;
 		ItemPointerData *items = NULL;
 		uint16		nitems = 0;
-		ItemPointerData *olditems;
-		int			nolditems;
-		ItemPointerData *newitems;
-		int			nnewitems;
-		int			segsize;
-		Pointer		segptr;
-		int			szleft;
-
-		/* Extract all the information we need from the WAL record */
-		if (a_action == GIN_SEGMENT_INSERT ||
-			a_action == GIN_SEGMENT_REPLACE)
-		{
-			newseg = (GinPostingList *) walbuf;
-			newsegsize = SizeOfGinPostingList(newseg);
-			walbuf += SHORTALIGN(newsegsize);
-		}
-
-		if (a_action == GIN_SEGMENT_ADDITEMS)
-		{
-			memcpy(&nitems, walbuf, sizeof(uint16));
-			walbuf += sizeof(uint16);
-			items = (ItemPointerData *) walbuf;
-			walbuf += nitems * sizeof(ItemPointerData);
-		}
 
 		/* Skip to the segment that this action concerns */
 		Assert(segno <= a_segno);
 		while (segno < a_segno)
 		{
-			oldseg = GinNextPostingListSegment(oldseg);
+			if (modified)
+			{
+				int			cursegsize;
+
+				/* Copy out the segment to scratch space */
+				cursegsize = SizeOfGinPostingList(segiter);
+				Assert(scratch + cursegsize - scratchbegin <= GinDataPageMaxDataSize);
+				memcpy(scratch, segiter, cursegsize);
+				scratch += cursegsize;
+			}
+
+			/* Move to next segment in the page */
+			segiter = GinNextPostingListSegment(segiter);
 			segno++;
 		}
 
 		/*
-		 * ADDITEMS action is handled like REPLACE, but the new segment to
-		 * replace the old one is reconstructed using the old segment from
-		 * disk and the new items from the WAL record.
+		 * Encountered first segment with an action. Every subsequent segment
+		 * needs to be in scratch space.
+		 */
+		if (!modified)
+		{
+			modified = true;
+			segmentcopybegin = (Pointer)segiter;
+		}
+
+		/*
+		 * Positioned after the last existing segment. Only INSERTs expected
+		 * here.
 		 */
+		if ((Pointer)segiter == segmentend)
+			Assert(a_action == GIN_SEGMENT_INSERT);
+
+		/* Process the action on the segment */
 		if (a_action == GIN_SEGMENT_ADDITEMS)
 		{
+			ItemPointerData *olditems;
+			int			nolditems;
+			ItemPointerData *newitems;
+			int			nnewitems;
 			int			npacked;
 
-			olditems = ginPostingListDecode(oldseg, &nolditems);
+			memcpy(&nitems, walbuf, sizeof(uint16));
+			walbuf += sizeof(uint16);
+			items = (ItemPointerData *)walbuf;
+			walbuf += nitems * sizeof(ItemPointerData);
 
+			olditems = ginPostingListDecode(segiter, &nolditems);
 			newitems = ginMergeItemPointers(items, nitems,
 											olditems, nolditems,
 											&nnewitems);
@@ -247,61 +265,59 @@ ginRedoRecompress(Page page, ginxlogRecompressDataLeaf *data)
 			Assert(npacked == nnewitems);
 
 			newsegsize = SizeOfGinPostingList(newseg);
-			a_action = GIN_SEGMENT_REPLACE;
+			memcpy(scratch, newseg, newsegsize);
+			scratch += newsegsize;
 		}
+		else if (a_action == GIN_SEGMENT_INSERT ||
+				 a_action == GIN_SEGMENT_REPLACE)
+		{
+			newseg = (GinPostingList *)walbuf;
+			newsegsize = SizeOfGinPostingList(newseg);
+			walbuf += SHORTALIGN(SizeOfGinPostingList(newseg));
 
-		segptr = (Pointer) oldseg;
-		if (segptr != segmentend)
-			segsize = SizeOfGinPostingList(oldseg);
+			/* Copy the new/replaced segment into scratch space */
+			memcpy(scratch, newseg, newsegsize);
+			scratch += newsegsize;
+		}
+		else if (a_action == GIN_SEGMENT_DELETE)
+		{
+			/* No op */
+		}
 		else
 		{
-			/*
-			 * Positioned after the last existing segment. Only INSERTs
-			 * expected here.
-			 */
-			Assert(a_action == GIN_SEGMENT_INSERT);
-			segsize = 0;
+			elog(ERROR, "unexpected GIN leaf action: %u", a_action);
 		}
-		szleft = segmentend - segptr;
 
-		switch (a_action)
+		/*
+		 * Advance to next segment on original page if the current segment
+		 * action is not insert.
+		 */
+		if (a_action != GIN_SEGMENT_INSERT)
 		{
-			case GIN_SEGMENT_DELETE:
-				memmove(segptr, segptr + segsize, szleft - segsize);
-				segmentend -= segsize;
-
-				segno++;
-				break;
-
-			case GIN_SEGMENT_INSERT:
-				/* make room for the new segment */
-				memmove(segptr + newsegsize, segptr, szleft);
-				/* copy the new segment in place */
-				memcpy(segptr, newseg, newsegsize);
-				segmentend += newsegsize;
-				segptr += newsegsize;
-				break;
-
-			case GIN_SEGMENT_REPLACE:
-				/* shift the segments that follow */
-				memmove(segptr + newsegsize,
-						segptr + segsize,
-						szleft - segsize);
-				/* copy the replacement segment in place */
-				memcpy(segptr, newseg, newsegsize);
-				segmentend -= segsize;
-				segmentend += newsegsize;
-				segptr += newsegsize;
-				segno++;
-				break;
-
-			default:
-				elog(ERROR, "unexpected GIN leaf action: %u", a_action);
+			segiter = GinNextPostingListSegment(segiter);
+			segno++;
 		}
-		oldseg = (GinPostingList *) segptr;
 	}
 
-	totalsize = segmentend - (Pointer) GinDataLeafPageGetPostingList(page);
+	/* Assert that there was some modification performed */
+	Assert(modified);
+
+	/* Copy out remainder of the page into scratch */
+	if ((Pointer)segiter < segmentend)
+		memcpy(scratch, segiter, (segmentend - ((Pointer)segiter)));
+
+	/*
+	 * Check that the new total size is within the max data page size allowed.
+	 * New total size is sum of scratch space size consumed and portion of
+	 * page that was unmodified.
+	 */
+	totalsize = (scratch - scratchbegin) + (segmentcopybegin - (Pointer)GinDataLeafPageGetPostingList(page));
+	Assert(totalsize <= GinDataPageMaxDataSize);
+
+	/* Copy the items back onto the page */
+	memcpy(segmentcopybegin, scratchbegin, (scratch - scratchbegin));
+
+	/* Set size on the page */
 	GinDataPageSetDataSize(page, totalsize);
 }
 
-- 
2.7.3.AMZN