Skip to content

Commit 2f2007f

Browse files
tglsfdctglsfdc
committed
Fix assorted bugs by changing TS_execute's callback API to ternary logic.
Text search sometimes failed to find valid matches, for instance '!crew:A'::tsquery might fail to locate 'crew:1B'::tsvector during an index search. The root of the issue is that TS_execute's callback functions were not changed to use ternary (yes/no/maybe) reporting when we made the search logic itself do so. It's somewhat annoying to break that API, but on the other hand we now see that any code using plain boolean logic is almost certainly broken since the addition of phrase search. There seem to be very few outside callers of this code anyway, so we'll just break them intentionally to get them to adapt. This allows removal of tsginidx.c's private re-implementation of TS_execute, since that's now entirely duplicative. It's also no longer necessary to avoid use of CALC_NOT in tsgistidx.c, since the underlying callbacks can now do something reasonable. Back-patch into v13. We can't change this in stable branches, but it seems not quite too late to fix it in v13. Tom Lane and Pavel Borisov Discussion: https://postgr.es/m/CALT9ZEE-aLotzBg-pOp2GFTesGWVYzXA3=mZKzRDa_OKnLF7Mg@mail.gmail.com
1 parent 25244b8 commit 2f2007f

File tree

10 files changed

+395
-189
lines changed

10 files changed

+395
-189
lines changed

‎src/backend/tsearch/wparser_def.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1962,7 +1962,7 @@ typedef struct
19621962
/*
19631963
* TS_execute callback for matching a tsquery operand to headline words
19641964
*/
1965-
static bool
1965+
static TSTernaryValue
19661966
checkcondition_HL(void *opaque, QueryOperand *val, ExecPhraseData *data)
19671967
{
19681968
hlCheck *checkval = (hlCheck *) opaque;
@@ -1975,7 +1975,7 @@ checkcondition_HL(void *opaque, QueryOperand *val, ExecPhraseData *data)
19751975
{
19761976
/* if data == NULL, don't need to report positions */
19771977
if (!data)
1978-
return true;
1978+
return TS_YES;
19791979

19801980
if (!data->pos)
19811981
{
@@ -1992,9 +1992,9 @@ checkcondition_HL(void *opaque, QueryOperand *val, ExecPhraseData *data)
19921992
}
19931993

19941994
if (data && data->npos > 0)
1995-
return true;
1995+
return TS_YES;
19961996

1997-
return false;
1997+
return TS_NO;
19981998
}
19991999

20002000
/*

‎src/backend/utils/adt/tsginidx.c

Lines changed: 24 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,13 @@ typedef struct
178178
bool *need_recheck;
179179
} GinChkVal;
180180

181-
static GinTernaryValue
182-
checkcondition_gin_internal(GinChkVal *gcv, QueryOperand *val, ExecPhraseData *data)
181+
/*
182+
* TS_execute callback for matching a tsquery operand to GIN index data
183+
*/
184+
static TSTernaryValue
185+
checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
183186
{
187+
GinChkVal *gcv = (GinChkVal *) checkval;
184188
int j;
185189

186190
/*
@@ -193,112 +197,22 @@ checkcondition_gin_internal(GinChkVal *gcv, QueryOperand *val, ExecPhraseData *d
193197
/* convert item's number to corresponding entry's (operand's) number */
194198
j = gcv->map_item_operand[((QueryItem *) val) - gcv->first_item];
195199

196-
/* return presence of current entry in indexed value */
197-
return gcv->check[j];
198-
}
199-
200-
/*
201-
* Wrapper of check condition function for TS_execute.
202-
*/
203-
static bool
204-
checkcondition_gin(void *checkval, QueryOperand *val, ExecPhraseData *data)
205-
{
206-
return checkcondition_gin_internal((GinChkVal *) checkval,
207-
val,
208-
data) != GIN_FALSE;
209-
}
210-
211-
/*
212-
* Evaluate tsquery boolean expression using ternary logic.
213-
*
214-
* Note: the reason we can't use TS_execute() for this is that its API
215-
* for the checkcondition callback doesn't allow a MAYBE result to be
216-
* returned, but we might have MAYBEs in the gcv->check array.
217-
* Perhaps we should change that API.
218-
*/
219-
static GinTernaryValue
220-
TS_execute_ternary(GinChkVal *gcv, QueryItem *curitem, bool in_phrase)
221-
{
222-
GinTernaryValue val1,
223-
val2,
224-
result;
225-
226-
/* since this function recurses, it could be driven to stack overflow */
227-
check_stack_depth();
228-
229-
if (curitem->type == QI_VAL)
230-
return
231-
checkcondition_gin_internal(gcv,
232-
(QueryOperand *) curitem,
233-
NULL /* don't have position info */ );
234-
235-
switch (curitem->qoperator.oper)
200+
/*
201+
* return presence of current entry in indexed value; but TRUE becomes
202+
* MAYBE in the presence of a query requiring recheck
203+
*/
204+
if (gcv->check[j] == GIN_TRUE)
236205
{
237-
case OP_NOT:
238-
239-
/*
240-
* Below a phrase search, force NOT's result to MAYBE. We cannot
241-
* invert a TRUE result from the subexpression to FALSE, since
242-
* TRUE only says that the subexpression matches somewhere, not
243-
* that it matches everywhere, so there might be positions where
244-
* the NOT will match. We could invert FALSE to TRUE, but there's
245-
* little point in distinguishing TRUE from MAYBE, since a recheck
246-
* will have been forced already.
247-
*/
248-
if (in_phrase)
249-
return GIN_MAYBE;
250-
251-
result = TS_execute_ternary(gcv, curitem + 1, in_phrase);
252-
if (result == GIN_MAYBE)
253-
return result;
254-
return !result;
255-
256-
case OP_PHRASE:
257-
258-
/*
259-
* GIN doesn't contain any information about positions, so treat
260-
* OP_PHRASE as OP_AND with recheck requirement, and always
261-
* reporting MAYBE not TRUE.
262-
*/
263-
*(gcv->need_recheck) = true;
264-
/* Pass down in_phrase == true in case there's a NOT below */
265-
in_phrase = true;
266-
267-
/* FALL THRU */
268-
269-
case OP_AND:
270-
val1 = TS_execute_ternary(gcv, curitem + curitem->qoperator.left,
271-
in_phrase);
272-
if (val1 == GIN_FALSE)
273-
return GIN_FALSE;
274-
val2 = TS_execute_ternary(gcv, curitem + 1, in_phrase);
275-
if (val2 == GIN_FALSE)
276-
return GIN_FALSE;
277-
if (val1 == GIN_TRUE && val2 == GIN_TRUE &&
278-
curitem->qoperator.oper != OP_PHRASE)
279-
return GIN_TRUE;
280-
else
281-
return GIN_MAYBE;
282-
283-
case OP_OR:
284-
val1 = TS_execute_ternary(gcv, curitem + curitem->qoperator.left,
285-
in_phrase);
286-
if (val1 == GIN_TRUE)
287-
return GIN_TRUE;
288-
val2 = TS_execute_ternary(gcv, curitem + 1, in_phrase);
289-
if (val2 == GIN_TRUE)
290-
return GIN_TRUE;
291-
if (val1 == GIN_FALSE && val2 == GIN_FALSE)
292-
return GIN_FALSE;
293-
else
294-
return GIN_MAYBE;
295-
296-
default:
297-
elog(ERROR, "unrecognized operator: %d", curitem->qoperator.oper);
206+
if (val->weight != 0 || data != NULL)
207+
return TS_MAYBE;
298208
}
299209

300-
/* not reachable, but keep compiler quiet */
301-
return false;
210+
/*
211+
* We rely on GinTernaryValue and TSTernaryValue using equivalent value
212+
* assignments. We could use a switch statement to map the values if that
213+
* ever stops being true, but it seems unlikely to happen.
214+
*/
215+
return (TSTernaryValue) gcv->check[j];
302216
}
303217

304218
Datum
@@ -370,10 +284,11 @@ gin_tsquery_triconsistent(PG_FUNCTION_ARGS)
370284
gcv.map_item_operand = (int *) (extra_data[0]);
371285
gcv.need_recheck = &recheck;
372286

373-
res = TS_execute_ternary(&gcv, GETQUERY(query), false);
374-
375-
if (res == GIN_TRUE && recheck)
376-
res = GIN_MAYBE;
287+
if (TS_execute(GETQUERY(query),
288+
&gcv,
289+
TS_EXEC_CALC_NOT | TS_EXEC_PHRASE_NO_POS,
290+
checkcondition_gin))
291+
res = recheck ? GIN_MAYBE : GIN_TRUE;
377292
}
378293

379294
PG_RETURN_GIN_TERNARY_VALUE(res);

‎src/backend/utils/adt/tsgistidx.c

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -273,9 +273,9 @@ typedef struct
273273
} CHKVAL;
274274

275275
/*
276-
* is there value 'val' in array or not ?
276+
* TS_execute callback for matching a tsquery operand to GIST leaf-page data
277277
*/
278-
static bool
278+
static TSTernaryValue
279279
checkcondition_arr(void *checkval, QueryOperand *val, ExecPhraseData *data)
280280
{
281281
int32 *StopLow = ((CHKVAL *) checkval)->arrb;
@@ -288,23 +288,26 @@ checkcondition_arr(void *checkval, QueryOperand *val, ExecPhraseData *data)
288288
* we are not able to find a prefix by hash value
289289
*/
290290
if (val->prefix)
291-
return true;
291+
return TS_MAYBE;
292292

293293
while (StopLow < StopHigh)
294294
{
295295
StopMiddle = StopLow + (StopHigh - StopLow) / 2;
296296
if (*StopMiddle == val->valcrc)
297-
return true;
297+
return TS_MAYBE;
298298
else if (*StopMiddle < val->valcrc)
299299
StopLow = StopMiddle + 1;
300300
else
301301
StopHigh = StopMiddle;
302302
}
303303

304-
return false;
304+
return TS_NO;
305305
}
306306

307-
static bool
307+
/*
308+
* TS_execute callback for matching a tsquery operand to GIST non-leaf data
309+
*/
310+
static TSTernaryValue
308311
checkcondition_bit(void *checkval, QueryOperand *val, ExecPhraseData *data)
309312
{
310313
void *key = (SignTSVector *) checkval;
@@ -313,8 +316,12 @@ checkcondition_bit(void *checkval, QueryOperand *val, ExecPhraseData *data)
313316
* we are not able to find a prefix in signature tree
314317
*/
315318
if (val->prefix)
316-
return true;
317-
return GETBIT(GETSIGN(key), HASHVAL(val->valcrc, GETSIGLEN(key)));
319+
return TS_MAYBE;
320+
321+
if (GETBIT(GETSIGN(key), HASHVAL(val->valcrc, GETSIGLEN(key))))
322+
return TS_MAYBE;
323+
else
324+
return TS_NO;
318325
}
319326

320327
Datum
@@ -339,10 +346,9 @@ gtsvector_consistent(PG_FUNCTION_ARGS)
339346
if (ISALLTRUE(key))
340347
PG_RETURN_BOOL(true);
341348

342-
/* since signature is lossy, cannot specify CALC_NOT here */
343349
PG_RETURN_BOOL(TS_execute(GETQUERY(query),
344350
key,
345-
TS_EXEC_PHRASE_NO_POS,
351+
TS_EXEC_PHRASE_NO_POS | TS_EXEC_CALC_NOT,
346352
checkcondition_bit));
347353
}
348354
else

‎src/backend/utils/adt/tsrank.c

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -556,14 +556,18 @@ typedef struct
556556
#define QR_GET_OPERAND_DATA(q, v) \
557557
( (q)->operandData + (((QueryItem*)(v)) - GETQUERY((q)->query)) )
558558

559-
static bool
560-
checkcondition_QueryOperand(void *checkval, QueryOperand *val, ExecPhraseData *data)
559+
/*
560+
* TS_execute callback for matching a tsquery operand to QueryRepresentation
561+
*/
562+
static TSTernaryValue
563+
checkcondition_QueryOperand(void *checkval, QueryOperand *val,
564+
ExecPhraseData *data)
561565
{
562566
QueryRepresentation *qr = (QueryRepresentation *) checkval;
563567
QueryRepresentationOperand *opData = QR_GET_OPERAND_DATA(qr, val);
564568

565569
if (!opData->operandexists)
566-
return false;
570+
return TS_NO;
567571

568572
if (data)
569573
{
@@ -573,7 +577,7 @@ checkcondition_QueryOperand(void *checkval, QueryOperand *val, ExecPhraseData *d
573577
data->pos += MAXQROPOS - opData->npos;
574578
}
575579

576-
return true;
580+
return TS_YES;
577581
}
578582

579583
typedef struct

0 commit comments

Comments
 (0)