Fix code for re-finding scan position in a multicolumn GIN index.

collectMatchBitmap() needs to re-find the index tuple it was previously
looking at, after transiently dropping lock on the index page it's on.
The tuple should still exist and be at its prior position or somewhere
to the right of that, since ginvacuum never removes tuples but
concurrent insertions could add one.  However, there was a thinko in
that logic, to the effect of expecting any inserted tuples to have the
same index "attnum" as what we'd been scanning.  Since there's no
physical separation of tuples with different attnums, it's not terribly
hard to devise scenarios where this fails, leading to transient "lost
saved point in index" errors.  (While I've duplicated this with manual
testing, it seems impossible to make a reproducible test case with our
available testing technology.)

Fix by just continuing the scan when the attnum doesn't match.

While here, improve the error message used if we do fail, so that it
matches the wording used in btree for a similar case.

collectMatchBitmap()'s posting-tree code path was previously not
exercised at all by our regression tests.  While I can't make
a regression test that exhibits the bug, I can at least improve
the code coverage here, so do that.  The test case I made for this
is an extension of one added by 4b754d6c1, so it only works in
HEAD and v13; didn't seem worth trying hard to back- it.

Per bug #16595 from Jesse Kinkead.  This has been broken since
multicolumn capability was added to GIN (commit 27cb66fdf),
so back- to all supported branches.

Discussion: https://postgr.es/m/16595-633118be8eef9ce2@postgresql.org

diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 7bdcbc858e39fd82d62fa7657aedb9aefac5cbd3..2cfccdedcf59f4148a080298854eac765d736b41 100644 (file)
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -264,24 +264,28 @@ collectMatchBitmap(GinBtreeData *btree, GinBtreeStack *stack,
            /* Search forward to re-find idatum */
            for (;;)
            {
-               Datum       newDatum;
-               GinNullCategory newCategory;
-
                if (moveRightIfItNeeded(btree, stack, snapshot) == false)
-                   elog(ERROR, "lost saved point in index");   /* must not happen !!! */
+                   ereport(ERROR,
+                           (errcode(ERRCODE_INTERNAL_ERROR),
+                            errmsg("failed to re-find tuple within index \"%s\"",
+                                   RelationGetRelationName(btree->index))));
 
                page = BufferGetPage(stack->buffer);
                itup = (IndexTuple) PageGetItem(page, PageGetItemId(page, stack->off));
 
-               if (gintuple_get_attrnum(btree->ginstate, itup) != attnum)
-                   elog(ERROR, "lost saved point in index");   /* must not happen !!! */
-               newDatum = gintuple_get_key(btree->ginstate, itup,
-                                           &newCategory);
+               if (gintuple_get_attrnum(btree->ginstate, itup) == attnum)
+               {
+                   Datum       newDatum;
+                   GinNullCategory newCategory;
+
+                   newDatum = gintuple_get_key(btree->ginstate, itup,
+                                               &newCategory);
 
-               if (ginCompareEntries(btree->ginstate, attnum,
-                                     newDatum, newCategory,
-                                     idatum, icategory) == 0)
-                   break;      /* Found! */
+                   if (ginCompareEntries(btree->ginstate, attnum,
+                                         newDatum, newCategory,
+                                         idatum, icategory) == 0)
+                       break;  /* Found! */
+               }
 
                stack->off++;
            }

diff --git a/src/test/regress/expected/gin.out b/src/test/regress/expected/gin.out
index 83de5220fb9ceb842c6f6ca61b1da31b0cd8bdc5..b335466fc4baebea6d5cc3b44d512890eb5e004f 100644 (file)
--- a/src/test/regress/expected/gin.out
+++ b/src/test/regress/expected/gin.out
@@ -199,6 +199,71 @@ from
   i @> '{1}' and j @> '{10}'               | 2               | 0                  | t
 (10 rows)
 
+reset enable_seqscan;
+reset enable_bitmapscan;
+-- re-purpose t_gin_test_tbl to test scans involving posting trees
+insert into t_gin_test_tbl select array[1, g, g/10], array[2, g, g/10]
+  from generate_series(1, 20000) g;
+select gin_clean_pending_list('t_gin_test_tbl_i_j_idx') is not null;
+ ?column? 
+----------
+ t
+(1 row)
+
+analyze t_gin_test_tbl;
+set enable_seqscan = off;
+set enable_bitmapscan = on;
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> array[50];
+                       QUERY PLAN                        
+---------------------------------------------------------
+ Aggregate
+   ->  Bitmap Heap Scan on t_gin_test_tbl
+         Recheck Cond: (j @> '{50}'::integer[])
+         ->  Bitmap Index Scan on t_gin_test_tbl_i_j_idx
+               Index Cond: (j @> '{50}'::integer[])
+(5 rows)
+
+select count(*) from t_gin_test_tbl where j @> array[50];
+ count 
+-------
+    11
+(1 row)
+
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> array[2];
+                       QUERY PLAN                        
+---------------------------------------------------------
+ Aggregate
+   ->  Bitmap Heap Scan on t_gin_test_tbl
+         Recheck Cond: (j @> '{2}'::integer[])
+         ->  Bitmap Index Scan on t_gin_test_tbl_i_j_idx
+               Index Cond: (j @> '{2}'::integer[])
+(5 rows)
+
+select count(*) from t_gin_test_tbl where j @> array[2];
+ count 
+-------
+ 20000
+(1 row)
+
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> '{}'::int[];
+                       QUERY PLAN                        
+---------------------------------------------------------
+ Aggregate
+   ->  Bitmap Heap Scan on t_gin_test_tbl
+         Recheck Cond: (j @> '{}'::integer[])
+         ->  Bitmap Index Scan on t_gin_test_tbl_i_j_idx
+               Index Cond: (j @> '{}'::integer[])
+(5 rows)
+
+select count(*) from t_gin_test_tbl where j @> '{}'::int[];
+ count 
+-------
+ 20006
+(1 row)
+
 reset enable_seqscan;
 reset enable_bitmapscan;
 drop table t_gin_test_tbl;

diff --git a/src/test/regress/sql/gin.sql b/src/test/regress/sql/gin.sql
index abe35752652abcda4d52de7cfa2e6fcdfc3539c8..efb8ef3e964cd3dbf0ae46752b0a71e03f57705e 100644 (file)
--- a/src/test/regress/sql/gin.sql
+++ b/src/test/regress/sql/gin.sql
@@ -138,4 +138,28 @@ from
 reset enable_seqscan;
 reset enable_bitmapscan;
 
+-- re-purpose t_gin_test_tbl to test scans involving posting trees
+insert into t_gin_test_tbl select array[1, g, g/10], array[2, g, g/10]
+  from generate_series(1, 20000) g;
+
+select gin_clean_pending_list('t_gin_test_tbl_i_j_idx') is not null;
+
+analyze t_gin_test_tbl;
+
+set enable_seqscan = off;
+set enable_bitmapscan = on;
+
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> array[50];
+select count(*) from t_gin_test_tbl where j @> array[50];
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> array[2];
+select count(*) from t_gin_test_tbl where j @> array[2];
+explain (costs off)
+select count(*) from t_gin_test_tbl where j @> '{}'::int[];
+select count(*) from t_gin_test_tbl where j @> '{}'::int[];
+
+reset enable_seqscan;
+reset enable_bitmapscan;
+
 drop table t_gin_test_tbl;