Strict functions with variadic "any" argument bug - Mailing list pgsql-hackers

Hello, hackers!

A few days ago Karina Litskevich noticed that strict function with 
variadic argument containing a few values, only one of which was NULL, 
returned NULL without evaluating the function itself. It didn't match 
with documented behaviour:

"If a function is declared STRICT with a VARIADIC argument, the 
strictness check tests that the variadic array as a whole is non-null. 
The function will still be called if the array has null elements."

After some digging it turned out that since VARIADIC "any" argument 
contents are not (can not?) transformed into single array (due to being 
possibly of different types), the corresponding parameters are checked 
in evaluate_function individually. Thus the following check

    /*
     * If the function is strict and has a constant-NULL input, it will 
never
     * be called at all, so we can replace the call by a NULL constant, 
even
     * if there are other inputs that aren't constant, and even if the
     * function is not otherwise immutable.
     */
    if (funcform->proisstrict && has_null_input)
        return (Expr *) makeNullConst(result_type, result_typmod,
                                      result_collid);
Passes successfully.

To see this bug in action it is enough to create a few functions with 
variadic arguments (i used a small contrib to pack it neatly):
=====================
/* contrib/test/test--1.0.sql */

-- complain if script is sourced in psql, rather than via CREATE 
EXTENSION
\echo Use "CREATE EXTENSION test" to load this file. \quit

CREATE FUNCTION test_variadic_any(VARIADIC "any")
RETURNS text
AS 'MODULE_PATHNAME'
LANGUAGE C;

CREATE FUNCTION test_variadic_any_strict(VARIADIC "any")
RETURNS text
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT;

CREATE FUNCTION test_variadic_int_strict(VARIADIC b int[])
RETURNS text
AS 'MODULE_PATHNAME'
LANGUAGE C STRICT;

CREATE FUNCTION test_variadic_int(VARIADIC b int[])
RETURNS text
AS 'MODULE_PATHNAME'
LANGUAGE C;

=================
Functions (it is enough to just have them to return something not-NULL, 
just wanted to see what extract_variadic_args will return:

PG_FUNCTION_INFO_V1(test_variadic_any);
Datum
test_variadic_any(PG_FUNCTION_ARGS)
{
    int nargs;
    Datum *args;
    bool *nulls;
    Oid *types;

    nargs = extract_variadic_args(fcinfo, 0, true, &args, &types, &nulls);

    if (nargs < 0)
        PG_RETURN_TEXT_P(cstring_to_text("is_null"));

    PG_RETURN_TEXT_P(cstring_to_text("OK"));
}

PG_FUNCTION_INFO_V1(test_variadic_any_strict);
Datum
test_variadic_any_strict(PG_FUNCTION_ARGS)
{
    int nargs;
    Datum *args;
    bool *nulls;
    Oid *types;

    nargs = extract_variadic_args(fcinfo, 0, true, &args, &types, &nulls);

    if (nargs < 0)
        PG_RETURN_TEXT_P(cstring_to_text("is_null"));

    PG_RETURN_TEXT_P(cstring_to_text("OK"));
}

PG_FUNCTION_INFO_V1(test_variadic_int);
Datum
test_variadic_int(PG_FUNCTION_ARGS)
{
    int nargs;
    Datum *args;
    bool *nulls;
    Oid *types;

    nargs = extract_variadic_args(fcinfo, 0, true, &args, &types, &nulls);

    if (nargs < 0)
        PG_RETURN_TEXT_P(cstring_to_text("is_null"));

    PG_RETURN_TEXT_P(cstring_to_text("OK"));
}

PG_FUNCTION_INFO_V1(test_variadic_int_strict);
Datum
test_variadic_int_strict(PG_FUNCTION_ARGS)
{
    int nargs;
    Datum *args;
    bool *nulls;
    Oid *types;

    nargs = extract_variadic_args(fcinfo, 0, true, &args, &types, &nulls);

    if (nargs < 0)
        PG_RETURN_TEXT_P(cstring_to_text("is_null"));

    PG_RETURN_TEXT_P(cstring_to_text("OK"));
}

=================
Output:

postgres=# select test_variadic_int_strict(1, 2, NULL);
  test_variadic_int_strict
--------------------------
  OK
(1 row)

postgres=# select test_variadic_any_strict(1, 2, NULL);
  test_variadic_any_strict
--------------------------

(1 row)

=================
Debugger showed that for these tests in evaluate_function for VARIADIC 
"any" case while funcform->pronargs = 1, actual amount of args is three, 
whereas for VARIADIC int[] funcform->pronargs = 1 and args list consists 
from one argument (array of int).

Attached is a small patch which partially fixes this place in 
evaluate_function by using funcform->provariadic and difference in args 
list length and funcform->pronargs to improve this check to work at 
least a little more correctly.

This version is not distinguishing between VARIADIC NULL::int[] and NULL 
for
VARIADIC "any" case - extract_variadic_args (by the way, it seems that 
the comment to this function
  is not correct for VARIADIC "any" case, since the variadic array is not 
constructed... The code itself works correctly with VARIADIC "any".) 
thinks that the second is array from one (NULL) element, while the first 
is real NULL (probably right):
=================
--no patch

postgres=# select test_variadic_any(NULL);
  test_variadic_any
-------------------
  OK
(1 row)

postgres=# select test_variadic_any(VARIADIC NULL::int[]);
  test_variadic_any
-------------------
  is_null
(1 row)

postgres=# select test_variadic_int(NULL);
  test_variadic_int
-------------------
  OK
(1 row)

postgres=# select test_variadic_int(VARIADIC NULL::int[]);
  test_variadic_int
-------------------
  is_null
(1 row)

In evaluate_function these cases look pretty similar. Probably should 
check consttype of elements of args list, since for simple NULL it will 
be "unknown", and for VARIADIC NULL::int[] (or any other type) it will 
be this type. Not sure, how to do it correctly.

The second and the bigger issue is that even with fixing 
evaluate_function the result is still the same, since ExecInterpExpr 
does the same check as evaluate_function:


postgres=# select test_variadic_any_strict(1, 2, NULL);
  test_variadic_any_strict
--------------------------

(1 row)
============================

        /* strict function call with more than two arguments */
        EEO_CASE(EEOP_FUNCEXPR_STRICT)
        {
            FunctionCallInfo fcinfo = op->d.func.fcinfo_data;
            NullableDatum *args = fcinfo->args;
            int            nargs = op->d.func.nargs;
            Datum        d;

            Assert(nargs > 2);

            /* strict function, so check for NULL args */
            for (int argno = 0; argno < nargs; argno++)
            {
                if (args[argno].isnull)
                {
                    *op->resnull = true;
                    goto strictfail;
                }
            }
            fcinfo->isnull = false;
            d = op->d.func.fn_addr(fcinfo);
            *op->resvalue = d;
            *op->resnull = fcinfo->isnull;

(And nargs here is three, get_fn_expr_variadic returns false, so no sign 
(or I don't see one) that we are actually working with VARIADIC "any" 
argument).

I am not quite sure where to go from here, since in ExecInterpExpr there 
is no (obvious for me) way to know if we are working with VARIADIC "any" 
argument. If not fixed (or not considered a bug), at least this 
behaviour should probably be mentioned in documentation for CREATE 
FUNCTION.

Thoughts?

With best regards,
Svetlana Derevyanko

Postgres Professional: http://postgrespro.com