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.

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 44587a84bae24c3ac970c3487b72995338e46ef6..9f4229de600acb37edfa4d3d1acd25097a2e143b 100644 (file)
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -1172,6 +1172,7 @@ finalize_partialaggregate(AggState *aggstate,
                                                                                   pergroupstate->transValueIsNull,
                                                                                   pertrans->transtypeLen);
                        fcinfo->args[0].isnull = pergroupstate->transValueIsNull;
+                       fcinfo->isnull = false;
 
                        *resultVal = FunctionCallInvoke(fcinfo);
                        *resultIsNull = fcinfo->isnull;

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 11987c8f3ba5245b9586afba2565b7f85fb78f08..800107d4e72648fe4e54cd96fcc1288e8d88a94e 100644 (file)
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -3668,7 +3668,7 @@ array_eq(PG_FUNCTION_ARGS)
                        }
 
                        /*
-                        * Apply the operator to the element pair
+                        * Apply the operator to the element pair; treat NULL as false
                         */
                        locfcinfo->args[0].value = elt1;
                        locfcinfo->args[0].isnull = false;
@@ -3676,7 +3676,7 @@ array_eq(PG_FUNCTION_ARGS)
                        locfcinfo->args[1].isnull = false;
                        locfcinfo->isnull = false;
                        oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
-                       if (!oprresult)
+                       if (locfcinfo->isnull || !oprresult)
                        {
                                result = false;
                                break;
@@ -3841,9 +3841,11 @@ array_cmp(FunctionCallInfo fcinfo)
                locfcinfo->args[0].isnull = false;
                locfcinfo->args[1].value = elt2;
                locfcinfo->args[1].isnull = false;
-               locfcinfo->isnull = false;
                cmpresult = DatumGetInt32(FunctionCallInvoke(locfcinfo));
 
+               /* We don't expect comparison support functions to return null */
+               Assert(!locfcinfo->isnull);
+
                if (cmpresult == 0)
                        continue;                       /* equal */
 
@@ -3983,8 +3985,9 @@ hash_array(PG_FUNCTION_ARGS)
                        /* Apply the hash function */
                        locfcinfo->args[0].value = elt;
                        locfcinfo->args[0].isnull = false;
-                       locfcinfo->isnull = false;
                        elthash = DatumGetUInt32(FunctionCallInvoke(locfcinfo));
+                       /* We don't expect hash functions to return null */
+                       Assert(!locfcinfo->isnull);
                }
 
                /*
@@ -4074,6 +4077,8 @@ hash_array_extended(PG_FUNCTION_ARGS)
                        locfcinfo->args[1].value = Int64GetDatum(seed);
                        locfcinfo->args[1].isnull = false;
                        elthash = DatumGetUInt64(FunctionCallInvoke(locfcinfo));
+                       /* We don't expect hash functions to return null */
+                       Assert(!locfcinfo->isnull);
                }
 
                result = (result << 5) - result + elthash;
@@ -4207,7 +4212,7 @@ array_contain_compare(AnyArrayType *array1, AnyArrayType *array2, Oid collation,
                                continue;               /* can't match */
 
                        /*
-                        * Apply the operator to the element pair
+                        * Apply the operator to the element pair; treat NULL as false
                         */
                        locfcinfo->args[0].value = elt1;
                        locfcinfo->args[0].isnull = false;
@@ -4215,7 +4220,7 @@ array_contain_compare(AnyArrayType *array1, AnyArrayType *array2, Oid collation,
                        locfcinfo->args[1].isnull = false;
                        locfcinfo->isnull = false;
                        oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
-                       if (oprresult)
+                       if (!locfcinfo->isnull && oprresult)
                                break;
                }
 
@@ -6202,7 +6207,7 @@ array_replace_internal(ArrayType *array,
                        else
                        {
                                /*
-                                * Apply the operator to the element pair
+                                * Apply the operator to the element pair; treat NULL as false
                                 */
                                locfcinfo->args[0].value = elt;
                                locfcinfo->args[0].isnull = false;
@@ -6210,7 +6215,7 @@ array_replace_internal(ArrayType *array,
                                locfcinfo->args[1].isnull = false;
                                locfcinfo->isnull = false;
                                oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
-                               if (!oprresult)
+                               if (locfcinfo->isnull || !oprresult)
                                {
                                        /* no match, keep element */
                                        values[nresult] = elt;
@@ -6517,10 +6522,12 @@ width_bucket_array_fixed(Datum operand,
                locfcinfo->args[0].isnull = false;
                locfcinfo->args[1].value = fetch_att(ptr, typbyval, typlen);
                locfcinfo->args[1].isnull = false;
-               locfcinfo->isnull = false;
 
                cmpresult = DatumGetInt32(FunctionCallInvoke(locfcinfo));
 
+               /* We don't expect comparison support functions to return null */
+               Assert(!locfcinfo->isnull);
+
                if (cmpresult < 0)
                        right = mid;
                else
@@ -6577,6 +6584,9 @@ width_bucket_array_variable(Datum operand,
 
                cmpresult = DatumGetInt32(FunctionCallInvoke(locfcinfo));
 
+               /* We don't expect comparison support functions to return null */
+               Assert(!locfcinfo->isnull);
+
                if (cmpresult < 0)
                        right = mid;
                else

diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index 06ad83d7cae150afc540aaa4cac1e2b6c01a2e18..80cba2f4c261e308f05036583dd25a87b15bd05b 100644 (file)
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -966,9 +966,11 @@ record_cmp(FunctionCallInfo fcinfo)
                        locfcinfo->args[0].isnull = false;
                        locfcinfo->args[1].value = values2[i2];
                        locfcinfo->args[1].isnull = false;
-                       locfcinfo->isnull = false;
                        cmpresult = DatumGetInt32(FunctionCallInvoke(locfcinfo));
 
+                       /* We don't expect comparison support functions to return null */
+                       Assert(!locfcinfo->isnull);
+
                        if (cmpresult < 0)
                        {
                                /* arg1 is less than arg2 */
@@ -1200,9 +1202,8 @@ record_eq(PG_FUNCTION_ARGS)
                        locfcinfo->args[0].isnull = false;
                        locfcinfo->args[1].value = values2[i2];
                        locfcinfo->args[1].isnull = false;
-                       locfcinfo->isnull = false;
                        oprresult = DatumGetBool(FunctionCallInvoke(locfcinfo));
-                       if (!oprresult)
+                       if (locfcinfo->isnull || !oprresult)
                        {
                                result = false;
                                break;

diff --git a/src/include/fmgr.h b/src/include/fmgr.h
index a4249994b9219d636516afcf431d73d31dabc43a..d349510b7c76cb95cabb0623a01182929d556003 100644 (file)
--- a/src/include/fmgr.h
+++ b/src/include/fmgr.h
@@ -163,6 +163,11 @@ extern void fmgr_symbol(Oid functionId, char **mod, char **fn);
  * 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))