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);