From 0d0a0e4c253eb0f5220f9fe8296cdeac0daae662 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Apr 2020 14:23:42 -0400 Subject: [PATCH] Fix minor violations of FunctionCallInvoke usage protocol. Working on commit 1c455078b led me to check through FunctionCallInvoke call sites to see if every one was being honest about (a) making sure that fcinfo.isnull is initially false, and (b) checking its state after the call. Sure enough, I found some violations. The main one is that finalize_partialaggregate re-used serialfn_fcinfo without resetting isnull, even though it clearly intends to cater for serialfns that return NULL. There would only be an issue with a non-strict serialfn, since it's unlikely that a serialfn would return NULL for non-null input. We have no non-strict serialfns in core, and there may be none in the wild either, which would account for the lack of complaints. Still, it's clearly wrong, so back-patch that fix to 9.6 where finalize_partialaggregate was introduced. Also, arrayfuncs.c and rowtypes.c contained various callers that were not bothering to check for result nulls. While what's being called is a comparison or hash function that probably *shouldn't* return null, that's a lousy excuse for not having any check at all. There are existing places that just Assert(!fcinfo->isnull) in comparable situations, so I added that to the places that were calling btree comparison or hash support functions. In the places calling boolean-returning equality functions, it's quite cheap to have them treat isnull as FALSE, so make those places do that. Also remove some "locfcinfo->isnull = false" assignments that are unnecessary given the assumption that no previous call returned null. These changes seem like mostly neatnik-ism or debugging support, so I didn't back-patch. --- src/backend/executor/nodeAgg.c | 1 + src/include/fmgr.h | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index ea4cfde33f1..1dcd8e19ed2 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -1670,6 +1670,7 @@ finalize_partialaggregate(AggState *aggstate, pergroupstate->transValueIsNull, pertrans->transtypeLen); fcinfo->argnull[0] = pergroupstate->transValueIsNull; + fcinfo->isnull = false; *resultVal = FunctionCallInvoke(fcinfo); *resultIsNull = fcinfo->isnull; diff --git a/src/include/fmgr.h b/src/include/fmgr.h index 0216965bfc1..5c614e79fe9 100644 --- a/src/include/fmgr.h +++ b/src/include/fmgr.h @@ -133,6 +133,11 @@ extern void fmgr_info_copy(FmgrInfo *dstinfo, FmgrInfo *srcinfo, * caller must still check fcinfo->isnull! Also, if function is strict, * it is caller's responsibility to verify that no null arguments are present * before calling. + * + * Some code performs multiple calls without redoing InitFunctionCallInfoData, + * possibly altering the argument values. This is okay, but be sure to reset + * the fcinfo->isnull flag before each call, since callees are permitted to + * assume that starts out false. */ #define FunctionCallInvoke(fcinfo) ((* (fcinfo)->flinfo->fn_addr) (fcinfo))