Fix bugs in gin_fuzzy_search_limit processing.

entryGetItem()'s three code paths each contained bugs associated
with filtering the entries for gin_fuzzy_search_limit.

The posting-tree path failed to advance "advancePast" after having
decided to filter an item.  If we ran out of items on the current
page and needed to advance to the next, what would actually happen
is that entryLoadMoreItems() would re-load the same page.  Eventually,
the random dropItem() test would accept one of the same items it'd
previously rejected, and we'd move on --- but it could take awhile
with small gin_fuzzy_search_limit.  To add insult to injury, this
case would inevitably cause entryLoadMoreItems() to decide it needed
to re-descend from the root, making things even slower.

The posting-list path failed to implement gin_fuzzy_search_limit
filtering at all, so that all entries in the posting list would
be returned.

The bitmap-result path used a "gotitem" variable that it failed to
update in the one place where it'd actually make a difference, ie
at the one "continue" statement.  I think this was unreachable in
practice, because if we'd looped around then it shouldn't be the
case that the entries on the new page are before advancePast.
Still, the "gotitem" variable was contributing nothing to either
clarity or correctness, so get rid of it.

Refactor all three loops so that the termination conditions are
more alike and less unreadable.

The code coverage report showed that we had no coverage at all for
the re-descend-from-root code path in entryLoadMoreItems(), which
seems like a very bad thing, so add a test case that exercises it.
We also had exactly no coverage for gin_fuzzy_search_limit, so add a
simplistic test case that at least hits those code paths a little bit.

Back-patch to all supported branches.

Adé Heyward and Tom Lane

Discussion: https://postgr.es/m/CAEknJCdS-dE1Heddptm7ay2xTbSeADbkaQ8bU2AXRCVC2LdtKQ@mail.gmail.com

diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index 50fe38b711c0e706ed806ba8b34c8e3632152afe..7bdcbc858e39fd82d62fa7657aedb9aefac5cbd3 100644 (file)
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -813,9 +813,8 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
        /* A bitmap result */
        BlockNumber advancePastBlk = GinItemPointerGetBlockNumber(&advancePast);
        OffsetNumber advancePastOff = GinItemPointerGetOffsetNumber(&advancePast);
-       bool        gotitem = false;
 
-       do
+       for (;;)
        {
            /*
             * If we've exhausted all items on this block, move to next block
@@ -864,7 +863,6 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
                 * estimate number of results on this page to support correct
                 * reducing of result even if it's enabled.
                 */
-               gotitem = true;
                break;
            }
 
@@ -877,7 +875,7 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
                /*
                 * First, do a quick check against the last offset on the
                 * page. If that's > advancePast, so are all the other
-                * offsets.
+                * offsets, so just go back to the top to get the next page.
                 */
                if (entry->matchResult->offsets[entry->matchResult->ntuples - 1] <= advancePastOff)
                {
@@ -894,8 +892,11 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
                           entry->matchResult->blockno,
                           entry->matchResult->offsets[entry->offset]);
            entry->offset++;
-           gotitem = true;
-       } while (!gotitem || (entry->reduceResult == true && dropItem(entry)));
+
+           /* Done unless we need to reduce the result */
+           if (!entry->reduceResult || !dropItem(entry))
+               break;
+       }
    }
    else if (!BufferIsValid(entry->buffer))
    {
@@ -903,7 +904,7 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
         * A posting list from an entry tuple, or the last page of a posting
         * tree.
         */
-       do
+       for (;;)
        {
            if (entry->offset >= entry->nlist)
            {
@@ -913,13 +914,20 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
            }
 
            entry->curItem = entry->list[entry->offset++];
-       } while (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0);
-       /* XXX: shouldn't we apply the fuzzy search limit here? */
+
+           /* If we're not past advancePast, keep scanning */
+           if (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0)
+               continue;
+
+           /* Done unless we need to reduce the result */
+           if (!entry->reduceResult || !dropItem(entry))
+               break;
+       }
    }
    else
    {
        /* A posting tree */
-       do
+       for (;;)
        {
            /* If we've processed the current batch, load more items */
            while (entry->offset >= entry->nlist)
@@ -935,8 +943,20 @@ entryGetItem(GinState *ginstate, GinScanEntry entry,
 
            entry->curItem = entry->list[entry->offset++];
 
-       } while (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0 ||
-                (entry->reduceResult == true && dropItem(entry)));
+           /* If we're not past advancePast, keep scanning */
+           if (ginCompareItemPointers(&entry->curItem, &advancePast) <= 0)
+               continue;
+
+           /* Done unless we need to reduce the result */
+           if (!entry->reduceResult || !dropItem(entry))
+               break;
+
+           /*
+            * Advance advancePast (so that entryLoadMoreItems will load the
+            * right data), and keep scanning
+            */
+           advancePast = entry->curItem;
+       }
    }
 }
 

diff --git a/src/test/regress/expected/gin.out b/src/test/regress/expected/gin.out
index e3c4805c2341ce8371ab8a986ddbb2075a75d836..83de5220fb9ceb842c6f6ca61b1da31b0cd8bdc5 100644 (file)
--- a/src/test/regress/expected/gin.out
+++ b/src/test/regress/expected/gin.out
@@ -35,6 +35,44 @@ insert into gin_test_tbl select array[1, 2, g] from generate_series(1, 1000) g;
 insert into gin_test_tbl select array[1, 3, g] from generate_series(1, 1000) g;
 delete from gin_test_tbl where i @> array[2];
 vacuum gin_test_tbl;
+-- Test for "rare && frequent" searches
+explain (costs off)
+select count(*) from gin_test_tbl where i @> array[1, 999];
+                      QUERY PLAN                       
+-------------------------------------------------------
+ Aggregate
+   ->  Bitmap Heap Scan on gin_test_tbl
+         Recheck Cond: (i @> '{1,999}'::integer[])
+         ->  Bitmap Index Scan on gin_test_idx
+               Index Cond: (i @> '{1,999}'::integer[])
+(5 rows)
+
+select count(*) from gin_test_tbl where i @> array[1, 999];
+ count 
+-------
+     3
+(1 row)
+
+-- Very weak test for gin_fuzzy_search_limit
+set gin_fuzzy_search_limit = 1000;
+explain (costs off)
+select count(*) > 0 as ok from gin_test_tbl where i @> array[1];
+                    QUERY PLAN                     
+---------------------------------------------------
+ Aggregate
+   ->  Bitmap Heap Scan on gin_test_tbl
+         Recheck Cond: (i @> '{1}'::integer[])
+         ->  Bitmap Index Scan on gin_test_idx
+               Index Cond: (i @> '{1}'::integer[])
+(5 rows)
+
+select count(*) > 0 as ok from gin_test_tbl where i @> array[1];
+ ok 
+----
+ t
+(1 row)
+
+reset gin_fuzzy_search_limit;
 -- Test optimization of empty queries
 create temp table t_gin_test_tbl(i int4[], j int4[]);
 create index on t_gin_test_tbl using gin (i, j);

diff --git a/src/test/regress/sql/gin.sql b/src/test/regress/sql/gin.sql
index 836717c996d879999e7d6c009239efcad1119109..abe35752652abcda4d52de7cfa2e6fcdfc3539c8 100644 (file)
--- a/src/test/regress/sql/gin.sql
+++ b/src/test/regress/sql/gin.sql
@@ -35,6 +35,22 @@ insert into gin_test_tbl select array[1, 3, g] from generate_series(1, 1000) g;
 delete from gin_test_tbl where i @> array[2];
 vacuum gin_test_tbl;
 
+-- Test for "rare && frequent" searches
+explain (costs off)
+select count(*) from gin_test_tbl where i @> array[1, 999];
+
+select count(*) from gin_test_tbl where i @> array[1, 999];
+
+-- Very weak test for gin_fuzzy_search_limit
+set gin_fuzzy_search_limit = 1000;
+
+explain (costs off)
+select count(*) > 0 as ok from gin_test_tbl where i @> array[1];
+
+select count(*) > 0 as ok from gin_test_tbl where i @> array[1];
+
+reset gin_fuzzy_search_limit;
+
 -- Test optimization of empty queries
 create temp table t_gin_test_tbl(i int4[], j int4[]);
 create index on t_gin_test_tbl using gin (i, j);