From: R, Siva <sivasubr@amazon.com>
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Bug in ginRedoRecompress that causes opaque data on page to be overrun
Date: Tue, 4 Sep 2018 19:59:11 +0000
Message-ID: <1536091151804.6588@amazon.com> (raw)
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.cindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sivasubr@amazon.com
Subject: Re: Bug in ginRedoRecompress that causes opaque data on page to be overrun
In-Reply-To: <1536091151804.6588@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox