Convert tsginidx.c's GIN indexing logic to fully ternary operation.

Commit 2f2007fbb did this partially, but there were two remaining
warts.  checkcondition_gin handled some uncertain cases by setting
the out-of-band recheck flag, some by returning TS_MAYBE, and some
by doing both.  Meanwhile, TS_execute arbitrarily converted a
TS_MAYBE result to TS_YES.  Thus, if checkcondition_gin chose to
only return TS_MAYBE, the outcome would be TS_YES with no recheck
flag, potentially resulting in wrong query outputs.

The case where this'd happen is if there were GIN_MAYBE entries
in the indexscan results passed to gin_tsquery_[tri]consistent,
which so far as I can see would only happen if the tidbitmap used
to accumulate indexscan results grew large enough to become lossy.

I initially thought of fixing this by ensuring we always set the
recheck flag as well as returning TS_MAYBE in uncertain cases.
But that errs in the other direction, potentially forcing rechecks
of rows that provably match the query (since the recheck flag
remains set even if TS_execute later finds that the answer must be
TS_YES).  Instead, let's get rid of the out-of-band recheck flag
altogether and rely on returning TS_MAYBE.  This requires exporting
a version of TS_execute that will actually return the full ternary
result of the evaluation ... but we likely should have done that
to start with.

Unfortunately it doesn't seem practical to add a regression test case
that covers this: the amount of data needed to cause the GIN bitmap to
become lossy results in a longer runtime than I think we want to have
in the tests.  (I'm wondering about allowing smaller work_mem settings
to ameliorate that, but it'd be a matter for a separate patch.)

Per bug #16865 from Dimitri Nüscheler.  Back-patch to v13 where
the faulty commit came in.

Discussion: https://postgr.es/m/16865-4ffdc3e682e6d75b@postgresql.org

diff --git a/src/backend/utils/adt/tsginidx.c b/src/backend/utils/adt/tsginidx.c
index 6c913baabacd9a872b73187408392c2fda7d9480..7e0e31f69df6f11b9e9ff8c389e6a026fdf27603 100644 (file)
--- a/src/backend/utils/adt/tsginidx.c
+++ b/src/backend/utils/adt/tsginidx.c
@@ -175,7 +175,6 @@ typedef struct
        QueryItem  *first_item;
        GinTernaryValue *check;
        int                *map_item_operand;
-       bool       *need_recheck;
 } GinChkVal;
 
 /*
@@ -186,25 +185,22 @@ checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
 {
        GinChkVal  *gcv = (GinChkVal *) checkval;
        int                     j;
-
-       /*
-        * if any val requiring a weight is used or caller needs position
-        * information then set recheck flag
-        */
-       if (val->weight != 0 || data != NULL)
-               *(gcv->need_recheck) = true;
+       GinTernaryValue result;
 
        /* convert item's number to corresponding entry's (operand's) number */
        j = gcv->map_item_operand[((QueryItem *) val) - gcv->first_item];
 
+       /* determine presence of current entry in indexed value */
+       result = gcv->check[j];
+
        /*
-        * return presence of current entry in indexed value; but TRUE becomes
-        * MAYBE in the presence of a query requiring recheck
+        * If any val requiring a weight is used or caller needs position
+        * information then we must recheck, so replace TRUE with MAYBE.
         */
-       if (gcv->check[j] == GIN_TRUE)
+       if (result == GIN_TRUE)
        {
                if (val->weight != 0 || data != NULL)
-                       return TS_MAYBE;
+                       result = GIN_MAYBE;
        }
 
        /*
@@ -212,7 +208,7 @@ checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
         * assignments.  We could use a switch statement to map the values if that
         * ever stops being true, but it seems unlikely to happen.
         */
-       return (TSTernaryValue) gcv->check[j];
+       return (TSTernaryValue) result;
 }
 
 Datum
@@ -244,12 +240,23 @@ gin_tsquery_consistent(PG_FUNCTION_ARGS)
                                                 "sizes of GinTernaryValue and bool are not equal");
                gcv.check = (GinTernaryValue *) check;
                gcv.map_item_operand = (int *) (extra_data[0]);
-               gcv.need_recheck = recheck;
 
-               res = TS_execute(GETQUERY(query),
-                                                &gcv,
-                                                TS_EXEC_PHRASE_NO_POS,
-                                                checkcondition_gin);
+               switch (TS_execute_ternary(GETQUERY(query),
+                                                                  &gcv,
+                                                                  TS_EXEC_PHRASE_NO_POS,
+                                                                  checkcondition_gin))
+               {
+                       case TS_NO:
+                               res = false;
+                               break;
+                       case TS_YES:
+                               res = true;
+                               break;
+                       case TS_MAYBE:
+                               res = true;
+                               *recheck = true;
+                               break;
+               }
        }
 
        PG_RETURN_BOOL(res);
@@ -266,10 +273,6 @@ gin_tsquery_triconsistent(PG_FUNCTION_ARGS)
        /* int32        nkeys = PG_GETARG_INT32(3); */
        Pointer    *extra_data = (Pointer *) PG_GETARG_POINTER(4);
        GinTernaryValue res = GIN_FALSE;
-       bool            recheck;
-
-       /* Initially assume query doesn't require recheck */
-       recheck = false;
 
        if (query->size > 0)
        {
@@ -282,13 +285,11 @@ gin_tsquery_triconsistent(PG_FUNCTION_ARGS)
                gcv.first_item = GETQUERY(query);
                gcv.check = check;
                gcv.map_item_operand = (int *) (extra_data[0]);
-               gcv.need_recheck = &recheck;
 
-               if (TS_execute(GETQUERY(query),
-                                          &gcv,
-                                          TS_EXEC_PHRASE_NO_POS,
-                                          checkcondition_gin))
-                       res = recheck ? GIN_MAYBE : GIN_TRUE;
+               res = TS_execute_ternary(GETQUERY(query),
+                                                                &gcv,
+                                                                TS_EXEC_PHRASE_NO_POS,
+                                                                checkcondition_gin);
        }
 
        PG_RETURN_GIN_TERNARY_VALUE(res);

diff --git a/src/backend/utils/adt/tsvector_op.c b/src/backend/utils/adt/tsvector_op.c
index 2939fb5c21037808712a8952e2dd90f506ff7ffa..9236ebcc8fe5cf8eb4c48ec03184c032862ca71b 100644 (file)
--- a/src/backend/utils/adt/tsvector_op.c
+++ b/src/backend/utils/adt/tsvector_op.c
@@ -1854,6 +1854,18 @@ TS_execute(QueryItem *curitem, void *arg, uint32 flags,
        return TS_execute_recurse(curitem, arg, flags, chkcond) != TS_NO;
 }
 
+/*
+ * Evaluate tsquery boolean expression.
+ *
+ * This is the same as TS_execute except that TS_MAYBE is returned as-is.
+ */
+TSTernaryValue
+TS_execute_ternary(QueryItem *curitem, void *arg, uint32 flags,
+                                  TSExecuteCallback chkcond)
+{
+       return TS_execute_recurse(curitem, arg, flags, chkcond);
+}
+
 /*
  * TS_execute recursion for operators above any phrase operator.  Here we do
  * not need to worry about lexeme positions.  As soon as we hit an OP_PHRASE

diff --git a/src/include/tsearch/ts_utils.h b/src/include/tsearch/ts_utils.h
index 69a9ba8524ed39f8b6280aab61aaabef9578eac7..4266560151f1465b5e9ea5ec6b4b54ce1423b010 100644 (file)
--- a/src/include/tsearch/ts_utils.h
+++ b/src/include/tsearch/ts_utils.h
@@ -199,6 +199,9 @@ typedef TSTernaryValue (*TSExecuteCallback) (void *arg, QueryOperand *val,
 
 extern bool TS_execute(QueryItem *curitem, void *arg, uint32 flags,
                                           TSExecuteCallback chkcond);
+extern TSTernaryValue TS_execute_ternary(QueryItem *curitem, void *arg,
+                                                                                uint32 flags,
+                                                                                TSExecuteCallback chkcond);
 extern bool tsquery_requires_match(QueryItem *curitem);
 
 /*