Bug in gin insert redo code path during re-compression of empty gin data leaf pages
Hi,
We came across an issue during replay of a gin insert record on a pre-9.4 uncompressed data leaf page that does not have any items in it. The engine version where the replay is done is 9.6.3. The redo logic attempts to compress the existing page items before trying to insert new items from the WAL record[1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginxlog.c;h=a40f1683dd80e4620ba6ccd93ae0e178e9616cf7;hb=refs/heads/REL9_6_STABLE#l147. In this particular case, we noticed that if the data leaf page does not have any items in it (see below on how such a page can be present), the compression should not be attempted as the algorithm expects the page to have at least one item [2]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginpostinglist.c;h=54d5f6f630c4638474d3208489c5fb6eb1ca1eeb;hb=refs/heads/REL9_6_STABLE#l201. This will lead to incorrect data size set on the page and also makes the assertion that expects npacked and nuncompressed to be equal, false [3]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginxlog.c;h=a40f1683dd80e4620ba6ccd93ae0e178e9616cf7;hb=refs/heads/REL9_6_STABLE#l163.
In Postgres 9.3, when the gin posting tree is vacuumed, the pages that are in the leftmost and rightmost branches are not deleted [4]https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginvacuum.c;h=31b765287f6ac9da798d2637046e706f67bdac7a;hb=refs/heads/REL9_3_STABLE#l430. These empty pages can be part of the database after upgrading to 9.6.3. We verified this by doing the following test:
Step 1: On Postgresql 9.3, create a table with gin index, insert and delete some data followed by vacuum.
* CREATE EXTENSION IF NOT EXISTS pg_trgm;
* CREATE TABLE gin_test (first_name text, last_name text);
* INSERT INTO gin_test SELECT md5(random()::text), md5(random()::text) FROM (SELECT * FROM generate_series(1,100000) AS id) AS x;
* CREATE INDEX gin_t_idx ON gin_test USING gin (first_name gin_trgm_ops, last_name gin_trgm_ops) WITH (fastupdate = OFF);
* DELETE FROM gin_test;
* VACUUM gin_test;
Step 2: Upgrade the database to 9.6.3 using pg_upgrade
* pg_upgrade -b $93_POSTGRES_DIR/bin -B $96_POSTGRES_DIR/bin -d $93_DATADIR -D $96_DATADIR
Step 3: Check the gin page opaque information of a uncompressed data leaf page to see if it has no items
* CREATE EXTENSION IF NOT EXISTS pageinspect;
* SELECT * FROM gin_metapage_info(get_raw_page('gin_t_idx', 0)); >>> Get number of data pages
* SELECT * FROM gin_page_opaque_info(get_raw_page('gin_t_idx', 8000)); >>> some block number close to total number of blocks
* This will return a page that has rightlink 2^32-1, maxoff 0 and flags {data,leaf}.
Attached is a patch that will skip compression of the posting list in the case that the data leaf page is empty.
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=a40f1683dd80e4620ba6ccd93ae0e178e9616cf7;hb=refs/heads/REL9_6_STABLE#l147
[2]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginpostinglist.c;h=54d5f6f630c4638474d3208489c5fb6eb1ca1eeb;hb=refs/heads/REL9_6_STABLE#l201
[3]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginxlog.c;h=a40f1683dd80e4620ba6ccd93ae0e178e9616cf7;hb=refs/heads/REL9_6_STABLE#l163
[4]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/gin/ginvacuum.c;h=31b765287f6ac9da798d2637046e706f67bdac7a;hb=refs/heads/REL9_3_STABLE#l430
Attachments:
skip-compression-of-pre-9.4-empty-gin-data-leaf-page-replay_v1.patchapplication/octet-stream; name=skip-compression-of-pre-9.4-empty-gin-data-leaf-page-replay_v1.patchDownload
From 956a533c5ec3171485624461cc13f786dbfc15e5 Mon Sep 17 00:00:00 2001
From: Siva R <sivasubr@amazon.com>
Date: Tue, 17 Jul 2018 22:01:42 +0000
Subject: [PATCH] Skip compression of pre-9.4 empty gin data leaf pages during
replay
---
src/backend/access/gin/ginxlog.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index a40f168..0797b92 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -151,15 +151,28 @@ ginRedoRecompress(Page page, ginxlogRecompressDataLeaf *data)
ItemPointer uncompressed = (ItemPointer) GinDataPageGetData(page);
int nuncompressed = GinPageGetOpaque(page)->maxoff;
int npacked;
- GinPostingList *plist;
- plist = ginCompressPostingList(uncompressed, nuncompressed,
- BLCKSZ, &npacked);
- Assert(npacked == nuncompressed);
+ totalsize = 0;
- totalsize = SizeOfGinPostingList(plist);
+ /*
+ * In 9.3, when we delete data leaf pages as part of vacuum, we skip
+ * deletion of the leftmost and rightmost pages even when they are
+ * empty. So try to compress the posting list only if there are items
+ * in it.
+ */
+ if (nuncompressed > 0)
+ {
+ GinPostingList *plist;
+
+ plist = ginCompressPostingList(uncompressed, nuncompressed,
+ BLCKSZ, &npacked);
+ totalsize = SizeOfGinPostingList(plist);
+
+ Assert(npacked == nuncompressed);
+
+ memcpy(GinDataLeafPageGetPostingList(page), plist, totalsize);
+ }
- memcpy(GinDataLeafPageGetPostingList(page), plist, totalsize);
GinDataPageSetDataSize(page, totalsize);
GinPageSetCompressed(page);
GinPageGetOpaque(page)->maxoff = InvalidOffsetNumber;
--
2.7.3.AMZN
Hi!
On Wed, Jul 18, 2018 at 1:40 AM R, Siva <sivasubr@amazon.com> wrote:
We came across an issue during replay of a gin insert record on a pre-9.4
uncompressed data leaf page that does not have any items in it. The engine
version where the replay is done is 9.6.3. The redo logic attempts to
compress the existing page items before trying to insert new items from
the WAL record[1]. In this particular case, we noticed that if the data
leaf page does not have any items in it (see below on how such a page can
be present), the compression should not be attempted as the algorithm
expects the page to have at least one item [2]. This will lead to incorrect
data size set on the page and also makes the assertion that expects npacked
and nuncompressed to be equal, false [3].In Postgres 9.3, when the gin posting tree is vacuumed, the pages that are
in the leftmost and rightmost branches are not deleted [4]. These empty
pages can be part of the database after upgrading to 9.6.3. We verified
this by doing the following test:
Thank you for reporting this bug. At the first glance it seems that your
proposed fix is correct. I'll review it in more details and commit.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Hi!
On Wed, Jul 18, 2018 at 11:59 AM Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
On Wed, Jul 18, 2018 at 1:40 AM R, Siva <sivasubr@amazon.com> wrote:
We came across an issue during replay of a gin insert record on a pre-9.4
uncompressed data leaf page that does not have any items in it. The engine
version where the replay is done is 9.6.3. The redo logic attempts to
compress the existing page items before trying to insert new items from
the WAL record[1]. In this particular case, we noticed that if the data
leaf page does not have any items in it (see below on how such a page can
be present), the compression should not be attempted as the algorithm
expects the page to have at least one item [2]. This will lead to incorrect
data size set on the page and also makes the assertion that expects npacked
and nuncompressed to be equal, false [3].In Postgres 9.3, when the gin posting tree is vacuumed, the pages that
are in the leftmost and rightmost branches are not deleted [4]. These empty
pages can be part of the database after upgrading to 9.6.3. We verified
this by doing the following test:Thank you for reporting this bug. At the first glance it seems that your
proposed fix is correct. I'll review it in more details and commit.
I've managed to reproduce assertion failure while upgrading from 9.3 to
master. The steps to reproduction are following.
1. On 9.3 run following.
* create table t as (select array[1] as a from generate_series(1,2000));
* create index t_idx on t using gin(a) with (fastupdate= off);
* delete from t;
* vacuum t;
2. pg_upgrade from 9.3 to master
3. On upgraded master set full_page_writes = off and run it
4. On master run following
* insert into t (select array[1] as a from generate_series(1,2000));
5. kill postmaster with -9
2018-07-19 14:30:11.437 MSK [57946] CONTEXT: WAL redo at 0/7000190 for
Gin/INSERT: isdata: T isleaf: T 1 segments: 0 (replace)
TRAP: FailedAssertion("!(npacked == nuncompressed)", File: "ginxlog.c",
Line: 161)
After applying your patch, I still have following assertion failure.
2018-07-19 14:40:34.449 MSK [60372] LOG: redo starts at 0/7000140
TRAP: FailedAssertion("!(a_action == 2)", File: "ginxlog.c", Line: 262)
So, the issue is still persists. I'm going to investigate this bug more
and come up with updated patch.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On Thu, Jul 19, 2018 at 2:43 PM Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
On Wed, Jul 18, 2018 at 11:59 AM Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:On Wed, Jul 18, 2018 at 1:40 AM R, Siva <sivasubr@amazon.com> wrote:
We came across an issue during replay of a gin insert record on a
pre-9.4 uncompressed data leaf page that does not have any items in it. The
engine version where the replay is done is 9.6.3. The redo logic attempts
to compress the existing page items before trying to insert new items from
the WAL record[1]. In this particular case, we noticed that if the data
leaf page does not have any items in it (see below on how such a page can
be present), the compression should not be attempted as the algorithm
expects the page to have at least one item [2]. This will lead to incorrect
data size set on the page and also makes the assertion that expects npacked
and nuncompressed to be equal, false [3].In Postgres 9.3, when the gin posting tree is vacuumed, the pages that
are in the leftmost and rightmost branches are not deleted [4]. These empty
pages can be part of the database after upgrading to 9.6.3. We verified
this by doing the following test:Thank you for reporting this bug. At the first glance it seems that your
proposed fix is correct. I'll review it in more details and commit.I've managed to reproduce assertion failure while upgrading from 9.3 to
master. The steps to reproduction are following.1. On 9.3 run following.
* create table t as (select array[1] as a from generate_series(1,2000));
* create index t_idx on t using gin(a) with (fastupdate= off);
* delete from t;
* vacuum t;2. pg_upgrade from 9.3 to master
3. On upgraded master set full_page_writes = off and run it
4. On master run following
* insert into t (select array[1] as a from generate_series(1,2000));
5. kill postmaster with -92018-07-19 14:30:11.437 MSK [57946] CONTEXT: WAL redo at 0/7000190 for
Gin/INSERT: isdata: T isleaf: T 1 segments: 0 (replace)
TRAP: FailedAssertion("!(npacked == nuncompressed)", File: "ginxlog.c",
Line: 161)After applying your patch, I still have following assertion failure.
2018-07-19 14:40:34.449 MSK [60372] LOG: redo starts at 0/7000140
TRAP: FailedAssertion("!(a_action == 2)", File: "ginxlog.c", Line: 262)So, the issue is still persists. I'm going to investigate this bug more
and come up with updated patch.
It appears that we need to handle empty posting list pages also
in disassembleLeaf(). Updated patch is attached. I'm going to commit it.
BTW, what is your full name for the commit message?
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Attachments:
skip-compression-of-pre-9.4-empty-gin-data-leaf-page-replay_v2.patchapplication/octet-stream; name=skip-compression-of-pre-9.4-empty-gin-data-leaf-page-replay_v2.patchDownload
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index aeaf8adab09..9f20513811e 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -1394,7 +1394,8 @@ disassembleLeaf(Page page)
{
/*
* A pre-9.4 format uncompressed page is represented by a single
- * segment, with an array of items.
+ * segment, with an array of items. The corner case is uncompressed
+ * page containing no items, which is represented as no segments.
*/
ItemPointer uncompressed;
int nuncompressed;
@@ -1402,15 +1403,18 @@ disassembleLeaf(Page page)
uncompressed = dataLeafPageGetUncompressed(page, &nuncompressed);
- seginfo = palloc(sizeof(leafSegmentInfo));
+ if (nuncompressed > 0)
+ {
+ seginfo = palloc(sizeof(leafSegmentInfo));
- seginfo->action = GIN_SEGMENT_REPLACE;
- seginfo->seg = NULL;
- seginfo->items = palloc(nuncompressed * sizeof(ItemPointerData));
- memcpy(seginfo->items, uncompressed, nuncompressed * sizeof(ItemPointerData));
- seginfo->nitems = nuncompressed;
+ seginfo->action = GIN_SEGMENT_REPLACE;
+ seginfo->seg = NULL;
+ seginfo->items = palloc(nuncompressed * sizeof(ItemPointerData));
+ memcpy(seginfo->items, uncompressed, nuncompressed * sizeof(ItemPointerData));
+ seginfo->nitems = nuncompressed;
- dlist_push_tail(&leaf->segments, &seginfo->node);
+ dlist_push_tail(&leaf->segments, &seginfo->node);
+ }
leaf->oldformat = true;
}
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index a6e321d77e4..7a1e94a1d56 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -153,15 +153,30 @@ ginRedoRecompress(Page page, ginxlogRecompressDataLeaf *data)
ItemPointer uncompressed = (ItemPointer) GinDataPageGetData(page);
int nuncompressed = GinPageGetOpaque(page)->maxoff;
int npacked;
- GinPostingList *plist;
- plist = ginCompressPostingList(uncompressed, nuncompressed,
- BLCKSZ, &npacked);
- Assert(npacked == nuncompressed);
+ /*
+ * Empty leaf pages are deleted as part of vacuum, but leftmost and
+ * rightmost pages are never deleted. So, pg_upgrade'd from pre-9.4
+ * instances might contain empty leaf pages, and we need to handle
+ * them correctly.
+ */
+ if (nuncompressed > 0)
+ {
+ GinPostingList *plist;
+
+ plist = ginCompressPostingList(uncompressed, nuncompressed,
+ BLCKSZ, &npacked);
+ totalsize = SizeOfGinPostingList(plist);
+
+ Assert(npacked == nuncompressed);
- totalsize = SizeOfGinPostingList(plist);
+ memcpy(GinDataLeafPageGetPostingList(page), plist, totalsize);
+ }
+ else
+ {
+ totalsize = 0;
+ }
- memcpy(GinDataLeafPageGetPostingList(page), plist, totalsize);
GinDataPageSetDataSize(page, totalsize);
GinPageSetCompressed(page);
GinPageGetOpaque(page)->maxoff = InvalidOffsetNumber;
Hi Alexander!
On Thu, Jul 19, 2018 at 2:43 PM Alexander Korotkov <a.korotkov@postgrespro.ru> wrote:
It appears that we need to handle empty posting list pages also in disassembleLeaf(). Updated patch is attached. I'm
going to commit it.
Thank you so much for reviewing the patch! You are right, we need to handle the regular insert/vacuum path for the empty page as well, thanks for fixing that. The updated patch looks good to me.
BTW, what is your full name for the commit message?
My full name is "Sivasubramanian Ramasubramanian".
Email : sivasubr@amazon.com
Thanks again!
Best
Siva
On Thu, Jul 19, 2018 at 6:17 PM R, Siva <sivasubr@amazon.com> wrote:
On Thu, Jul 19, 2018 at 2:43 PM Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:
It appears that we need to handle empty posting list pages also
in disassembleLeaf(). Updated patch is attached. I'm
going to commit it.
Thank you so much for reviewing the patch! You are right, we need to
handle the regular insert/vacuum path for the empty page as well, thanks
for fixing that. The updated patch looks good to me.BTW, what is your full name for the commit message?
My full name is "Sivasubramanian Ramasubramanian".
Email : sivasubr@amazon.com
Pushed, thanks!
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
On 2018-Jul-19, Alexander Korotkov wrote:
Pushed, thanks!
I think you missed branch REL_11_STABLE ...
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thu, Jul 19, 2018 at 9:29 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
On 2018-Jul-19, Alexander Korotkov wrote:
Pushed, thanks!
I think you missed branch REL_11_STABLE ...
Thank you for noticing! I also have just discovered that while looking at
buildfarm. Fixed.
------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company