Horton

Thread

Commits

  1. Fix handling of R/W expanded datums that are passed to SQL functions.

  1. Possible bug (or at least unexpected behavior)

    Adam Mackler <adam@mackler.email> — 2022-08-07T20:06:21Z

    Hi, forgive me if I should be posting this somewhere else.  I asked the following question on stackoverflow, and the first response suggests a possible bug:

https://stackoverflow.com/questions/73261240/recursive-sql-function-returning-array-has-extra-elements-when-self-invocation-u#73261240

Briefly, given the following function:

    CREATE FUNCTION runs(input int[], output int[] DEFAULT '{}')
    RETURNS int[] AS $$
      SELECT
        CASE WHEN cardinality(input) = 0 THEN output
        ELSE runs(input[2:],
                  array_append(output, CASE
                    WHEN input[1] = 0 THEN 0
                    ELSE output[cardinality(output)] + input[1]
                  END)
                 )
        END
    $$ LANGUAGE SQL;

I expect the following invocation to return an array with the same number of elements as the passed-in argument array:

    # select runs('{0,1,1,1,1,0,-1,-1,-1,0}');
                      runs
    ----------------------------------------
     {0,1,2,3,4,5,6,0,0,0,-1,-2,-3,-4,-5,0}
    (1 row)

which it does not with PostgreSQL version 14.4.  If it not a bug, then I would be extremely interested in why it's returning an array with more elements than the input array has.

Thanks in advance,
--
Adam Mackler

  2. Re: Possible bug (or at least unexpected behavior)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-07T23:31:41Z

    Adam Mackler <adam@mackler.email> writes:
> Briefly, given the following function:

>     CREATE FUNCTION runs(input int[], output int[] DEFAULT '{}')
>     RETURNS int[] AS $$
>       SELECT
>         CASE WHEN cardinality(input) = 0 THEN output
>         ELSE runs(input[2:],
>                   array_append(output, CASE
>                     WHEN input[1] = 0 THEN 0
>                     ELSE output[cardinality(output)] + input[1]
>                   END)
>                  )
>         END
>     $$ LANGUAGE SQL;

> I expect the following invocation to return an array with the same number of elements as the passed-in argument array:

>     # select runs('{0,1,1,1,1,0,-1,-1,-1,0}');
>                       runs
>     ----------------------------------------
>      {0,1,2,3,4,5,6,0,0,0,-1,-2,-3,-4,-5,0}
>     (1 row)

Yeah, there's a bug in here somewhere.  If you transpose the logic
into plpgsql, it behaves fine:

    CREATE FUNCTION runs_p(input int[], output int[] DEFAULT '{}')
    RETURNS int[] AS $$
    begin
      return
        CASE WHEN cardinality(input) = 0 THEN output
        ELSE runs_p(input[2:],
                  array_append(output, CASE
                    WHEN input[1] = 0 THEN 0
                    ELSE output[cardinality(output)] + input[1]
                  END)
                 )
        END;
    end
    $$ LANGUAGE plpgsql;

so that might do as a workaround.  It looks like memory management
in SQL functions is not coping well with expanded arrays, but I'm
not quite sure where it's going off the rails.

			regards, tom lane

  3. Re: Possible bug (or at least unexpected behavior)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-09T21:10:57Z

    I wrote:
> ... It looks like memory management
> in SQL functions is not coping well with expanded arrays, but I'm
> not quite sure where it's going off the rails.

It doesn't seem to be a memory management problem, but rather that
SQL functions are being careless with arguments that are read/write
expanded Datums.  A function that is passed such an argument is
allowed to modify it in-place, and array_append does so to reduce
the expense of repeatedly appending to an array value.  If you
have two array_append's referencing the same parameter in a SQL
function, and that parameter is passed in as a R/W datum, you
get the wrong answer: the second array_append will see the effects
of the first one.  Also, if the SQL function does array_append
first and array_cardinality second, array_cardinality reports the
wrong result.

Now, it doesn't look like your example function does either of those
things, but it turns out that it does after function inlining.  The
planner effectively flattens out one level of the recursion, creating
a plan in which we do have these hazards.

The only practical fix I can see is to force SQL function parameter
values to read-only.  We could do better if we knew which parameters
are actually multiply referenced in the function, but we don't have
the infrastructure needed to detect that.  I'm not convinced that
it'd be appropriate to expend a lot of effort here --- non-inlined
execution of a SQL function is a pretty low-performance situation in
any case.  So I've just gone for the simplest possible fix in the
attached.

			regards, tom lane