Skip to content

Commit 1c45507

Browse files
tglsfdctglsfdc
committed
Allow matchingsel() to be used with operators that might return NULL.
Although selfuncs.c will never call a target operator with null inputs, some functions might return null anyway. The existing coding will fail if that happens (since FunctionCall2Coll will punt), which seems undesirable given that matchingsel() has such a broad range of potential applicability --- in fact, we already have a problem because we apply it to jsonb_path_exists_opr, which can return null. Hence, rejigger the underlying functions mcv_selectivity and histogram_selectivity to cope, treating a null result as false. While we are at it, we can move the InitFunctionCallInfoData overhead out of the inner loops, which isn't a huge number of cycles but might save something considering we are likely calling functions as cheap as int4eq(). Plus, the number of loop cycles to be expected is much more than it was when this code was written, since typical settings of default_statistics_target are higher. In view of that consideration, let's apply the same change to var_eq_const, eqjoinsel_inner, and eqjoinsel_semi. We do not expect equality functions to ever return null for non-null inputs (and certainly that code has been that way a long time without complaints), but the cycle savings seem attractive, especially in the eqjoinsel loops where there's potentially an O(N^2) savings. Similar code exists in ineq_histogram_selectivity and get_variable_range, but I forebore from changing those for now. The performance argument for changing ineq_histogram_selectivity is really weak anyway, since that will only iterate log2(N) times. Nikita Glukhov and Tom Lane Discussion: https://postgr.es/m/9d3b0959-95d6-c37e-2c0b-287bcfe5c705@postgrespro.ru
1 parent 9d25e1a commit 1c45507

File tree

1 file changed

+128
-38
lines changed

1 file changed

+128
-38
lines changed

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

Lines changed: 128 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -345,25 +345,42 @@ var_eq_const(VariableStatData *vardata, Oid operator,
345345
STATISTIC_KIND_MCV, InvalidOid,
346346
ATTSTATSSLOT_VALUES | ATTSTATSSLOT_NUMBERS))
347347
{
348+
LOCAL_FCINFO(fcinfo, 2);
348349
FmgrInfo eqproc;
349350

350351
fmgr_info(opfuncoid, &eqproc);
351352

353+
/*
354+
* Save a few cycles by setting up the fcinfo struct just once.
355+
* Using FunctionCallInvoke directly also avoids failure if the
356+
* eqproc returns NULL, though really equality functions should
357+
* never do that.
358+
*/
359+
InitFunctionCallInfoData(*fcinfo, &eqproc, 2, sslot.stacoll,
360+
NULL, NULL);
361+
fcinfo->args[0].isnull = false;
362+
fcinfo->args[1].isnull = false;
363+
/* be careful to apply operator right way 'round */
364+
if (varonleft)
365+
fcinfo->args[1].value = constval;
366+
else
367+
fcinfo->args[0].value = constval;
368+
352369
for (i = 0; i < sslot.nvalues; i++)
353370
{
354-
/* be careful to apply operator right way 'round */
371+
Datum fresult;
372+
355373
if (varonleft)
356-
match = DatumGetBool(FunctionCall2Coll(&eqproc,
357-
sslot.stacoll,
358-
sslot.values[i],
359-
constval));
374+
fcinfo->args[0].value = sslot.values[i];
360375
else
361-
match = DatumGetBool(FunctionCall2Coll(&eqproc,
362-
sslot.stacoll,
363-
constval,
364-
sslot.values[i]));
365-
if (match)
376+
fcinfo->args[1].value = sslot.values[i];
377+
fcinfo->isnull = false;
378+
fresult = FunctionCallInvoke(fcinfo);
379+
if (!fcinfo->isnull && DatumGetBool(fresult))
380+
{
381+
match = true;
366382
break;
383+
}
367384
}
368385
}
369386
else
@@ -723,17 +740,36 @@ mcv_selectivity(VariableStatData *vardata, FmgrInfo *opproc,
723740
STATISTIC_KIND_MCV, InvalidOid,
724741
ATTSTATSSLOT_VALUES | ATTSTATSSLOT_NUMBERS))
725742
{
743+
LOCAL_FCINFO(fcinfo, 2);
744+
745+
/*
746+
* We invoke the opproc "by hand" so that we won't fail on NULL
747+
* results. Such cases won't arise for normal comparison functions,
748+
* but generic_restriction_selectivity could perhaps be used with
749+
* operators that can return NULL. A small side benefit is to not
750+
* need to re-initialize the fcinfo struct from scratch each time.
751+
*/
752+
InitFunctionCallInfoData(*fcinfo, opproc, 2, sslot.stacoll,
753+
NULL, NULL);
754+
fcinfo->args[0].isnull = false;
755+
fcinfo->args[1].isnull = false;
756+
/* be careful to apply operator right way 'round */
757+
if (varonleft)
758+
fcinfo->args[1].value = constval;
759+
else
760+
fcinfo->args[0].value = constval;
761+
726762
for (i = 0; i < sslot.nvalues; i++)
727763
{
728-
if (varonleft ?
729-
DatumGetBool(FunctionCall2Coll(opproc,
730-
sslot.stacoll,
731-
sslot.values[i],
732-
constval)) :
733-
DatumGetBool(FunctionCall2Coll(opproc,
734-
sslot.stacoll,
735-
constval,
736-
sslot.values[i])))
764+
Datum fresult;
765+
766+
if (varonleft)
767+
fcinfo->args[0].value = sslot.values[i];
768+
else
769+
fcinfo->args[1].value = sslot.values[i];
770+
fcinfo->isnull = false;
771+
fresult = FunctionCallInvoke(fcinfo);
772+
if (!fcinfo->isnull && DatumGetBool(fresult))
737773
mcv_selec += sslot.numbers[i];
738774
sumcommon += sslot.numbers[i];
739775
}
@@ -798,20 +834,39 @@ histogram_selectivity(VariableStatData *vardata, FmgrInfo *opproc,
798834
*hist_size = sslot.nvalues;
799835
if (sslot.nvalues >= min_hist_size)
800836
{
837+
LOCAL_FCINFO(fcinfo, 2);
801838
int nmatch = 0;
802839
int i;
803840

841+
/*
842+
* We invoke the opproc "by hand" so that we won't fail on NULL
843+
* results. Such cases won't arise for normal comparison
844+
* functions, but generic_restriction_selectivity could perhaps be
845+
* used with operators that can return NULL. A small side benefit
846+
* is to not need to re-initialize the fcinfo struct from scratch
847+
* each time.
848+
*/
849+
InitFunctionCallInfoData(*fcinfo, opproc, 2, sslot.stacoll,
850+
NULL, NULL);
851+
fcinfo->args[0].isnull = false;
852+
fcinfo->args[1].isnull = false;
853+
/* be careful to apply operator right way 'round */
854+
if (varonleft)
855+
fcinfo->args[1].value = constval;
856+
else
857+
fcinfo->args[0].value = constval;
858+
804859
for (i = n_skip; i < sslot.nvalues - n_skip; i++)
805860
{
806-
if (varonleft ?
807-
DatumGetBool(FunctionCall2Coll(opproc,
808-
sslot.stacoll,
809-
sslot.values[i],
810-
constval)) :
811-
DatumGetBool(FunctionCall2Coll(opproc,
812-
sslot.stacoll,
813-
constval,
814-
sslot.values[i])))
861+
Datum fresult;
862+
863+
if (varonleft)
864+
fcinfo->args[0].value = sslot.values[i];
865+
else
866+
fcinfo->args[1].value = sslot.values[i];
867+
fcinfo->isnull = false;
868+
fresult = FunctionCallInvoke(fcinfo);
869+
if (!fcinfo->isnull && DatumGetBool(fresult))
815870
nmatch++;
816871
}
817872
result = ((double) nmatch) / ((double) (sslot.nvalues - 2 * n_skip));
@@ -2292,6 +2347,7 @@ eqjoinsel_inner(Oid opfuncoid,
22922347
* results", Technical Report 1018, Computer Science Dept., University
22932348
* of Wisconsin, Madison, March 1991 (available from ftp.cs.wisc.edu).
22942349
*/
2350+
LOCAL_FCINFO(fcinfo, 2);
22952351
FmgrInfo eqproc;
22962352
bool *hasmatch1;
22972353
bool *hasmatch2;
@@ -2310,6 +2366,18 @@ eqjoinsel_inner(Oid opfuncoid,
23102366
nmatches;
23112367

23122368
fmgr_info(opfuncoid, &eqproc);
2369+
2370+
/*
2371+
* Save a few cycles by setting up the fcinfo struct just once. Using
2372+
* FunctionCallInvoke directly also avoids failure if the eqproc
2373+
* returns NULL, though really equality functions should never do
2374+
* that.
2375+
*/
2376+
InitFunctionCallInfoData(*fcinfo, &eqproc, 2, sslot1->stacoll,
2377+
NULL, NULL);
2378+
fcinfo->args[0].isnull = false;
2379+
fcinfo->args[1].isnull = false;
2380+
23132381
hasmatch1 = (bool *) palloc0(sslot1->nvalues * sizeof(bool));
23142382
hasmatch2 = (bool *) palloc0(sslot2->nvalues * sizeof(bool));
23152383

@@ -2325,14 +2393,18 @@ eqjoinsel_inner(Oid opfuncoid,
23252393
{
23262394
int j;
23272395

2396+
fcinfo->args[0].value = sslot1->values[i];
2397+
23282398
for (j = 0; j < sslot2->nvalues; j++)
23292399
{
2400+
Datum fresult;
2401+
23302402
if (hasmatch2[j])
23312403
continue;
2332-
if (DatumGetBool(FunctionCall2Coll(&eqproc,
2333-
sslot1->stacoll,
2334-
sslot1->values[i],
2335-
sslot2->values[j])))
2404+
fcinfo->args[1].value = sslot2->values[j];
2405+
fcinfo->isnull = false;
2406+
fresult = FunctionCallInvoke(fcinfo);
2407+
if (!fcinfo->isnull && DatumGetBool(fresult))
23362408
{
23372409
hasmatch1[i] = hasmatch2[j] = true;
23382410
matchprodfreq += sslot1->numbers[i] * sslot2->numbers[j];
@@ -2502,6 +2574,7 @@ eqjoinsel_semi(Oid opfuncoid,
25022574
* lists. We still have to estimate for the remaining population, but
25032575
* in a skewed distribution this gives us a big leg up in accuracy.
25042576
*/
2577+
LOCAL_FCINFO(fcinfo, 2);
25052578
FmgrInfo eqproc;
25062579
bool *hasmatch1;
25072580
bool *hasmatch2;
@@ -2523,6 +2596,18 @@ eqjoinsel_semi(Oid opfuncoid,
25232596
clamped_nvalues2 = Min(sslot2->nvalues, nd2);
25242597

25252598
fmgr_info(opfuncoid, &eqproc);
2599+
2600+
/*
2601+
* Save a few cycles by setting up the fcinfo struct just once. Using
2602+
* FunctionCallInvoke directly also avoids failure if the eqproc
2603+
* returns NULL, though really equality functions should never do
2604+
* that.
2605+
*/
2606+
InitFunctionCallInfoData(*fcinfo, &eqproc, 2, sslot1->stacoll,
2607+
NULL, NULL);
2608+
fcinfo->args[0].isnull = false;
2609+
fcinfo->args[1].isnull = false;
2610+
25262611
hasmatch1 = (bool *) palloc0(sslot1->nvalues * sizeof(bool));
25272612
hasmatch2 = (bool *) palloc0(clamped_nvalues2 * sizeof(bool));
25282613

@@ -2537,14 +2622,18 @@ eqjoinsel_semi(Oid opfuncoid,
25372622
{
25382623
int j;
25392624

2625+
fcinfo->args[0].value = sslot1->values[i];
2626+
25402627
for (j = 0; j < clamped_nvalues2; j++)
25412628
{
2629+
Datum fresult;
2630+
25422631
if (hasmatch2[j])
25432632
continue;
2544-
if (DatumGetBool(FunctionCall2Coll(&eqproc,
2545-
sslot1->stacoll,
2546-
sslot1->values[i],
2547-
sslot2->values[j])))
2633+
fcinfo->args[1].value = sslot2->values[j];
2634+
fcinfo->isnull = false;
2635+
fresult = FunctionCallInvoke(fcinfo);
2636+
if (!fcinfo->isnull && DatumGetBool(fresult))
25482637
{
25492638
hasmatch1[i] = hasmatch2[j] = true;
25502639
nmatches++;
@@ -3687,8 +3776,9 @@ double
36873776
estimate_hashagg_tablesize(Path *path, const AggClauseCosts *agg_costs,
36883777
double dNumGroups)
36893778
{
3690-
Size hashentrysize = hash_agg_entry_size(
3691-
agg_costs->numAggs, path->pathtarget->width, agg_costs->transitionSpace);
3779+
Size hashentrysize = hash_agg_entry_size(agg_costs->numAggs,
3780+
path->pathtarget->width,
3781+
agg_costs->transitionSpace);
36923782

36933783
/*
36943784
* Note that this disregards the effect of fill-factor and growth policy

0 commit comments

Comments
 (0)