Обсуждение: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters

The following bug has been logged on the website:

Bug reference:      18463
Logged by:          Drew Kimball
Email address:      drewk@cockroachlabs.com
PostgreSQL version: 16.3
Operating system:   macOS
Description:

Hello,

I believe there may be a bug related to stored procedures with
polymorphic-typed OUT parameters:

CREATE PROCEDURE p(INOUT x ANYELEMENT) LANGUAGE SQL AS $$ SELECT x; $$;
CALL p(1);

The above example results in an error message "cannot display a value of
type anyelement", but I would expect it to succeed and output "1". This also
reproduces with the following stored procedures:

CREATE PROCEDURE p(INOUT x ANYELEMENT) LANGUAGE SQL AS $$ SELECT 1; $$;
CREATE PROCEDURE p(x ANYELEMENT, OUT y ANYELEMENT) LANGUAGE SQL AS $$ SELECT
x; $$;
CREATE PROCEDURE p(x ANYARRAY, OUT y ANYELEMENT) LANGUAGE SQL AS $$ SELECT
x[1]; $$;

Interestingly, this doesn't seem to reproduce when the OUT param has type
ANYARRAY. The following example succeeds:

CREATE PROCEDURE p(INOUT x ANYARRAY) LANGUAGE SQL AS $$ SELECT x; $$;
CALL p(ARRAY[1, 2, 3]);

> On Tue, May 14, 2024 at 06:45:29AM +0000, PG Bug reporting form wrote:
> The following bug has been logged on the website:
>
> Bug reference:      18463
> Logged by:          Drew Kimball
> Email address:      drewk@cockroachlabs.com
> PostgreSQL version: 16.3
> Operating system:   macOS
> Description:
>
> Hello,
>
> I believe there may be a bug related to stored procedures with
> polymorphic-typed OUT parameters:
>
> CREATE PROCEDURE p(INOUT x ANYELEMENT) LANGUAGE SQL AS $$ SELECT x; $$;
> CALL p(1);
>
> The above example results in an error message "cannot display a value of
> type anyelement", but I would expect it to succeed and output "1". This also
> reproduces with the following stored procedures:
>
> CREATE PROCEDURE p(INOUT x ANYELEMENT) LANGUAGE SQL AS $$ SELECT 1; $$;
> CREATE PROCEDURE p(x ANYELEMENT, OUT y ANYELEMENT) LANGUAGE SQL AS $$ SELECT
> x; $$;
> CREATE PROCEDURE p(x ANYARRAY, OUT y ANYELEMENT) LANGUAGE SQL AS $$ SELECT
> x[1]; $$;
>
> Interestingly, this doesn't seem to reproduce when the OUT param has type
> ANYARRAY. The following example succeeds:
>
> CREATE PROCEDURE p(INOUT x ANYARRAY) LANGUAGE SQL AS $$ SELECT x; $$;
> CALL p(ARRAY[1, 2, 3]);

After looking at this I've got an impression this type of procedures
have to be disallowed in interpret_function_parameter_list. What happens
is that the procedure is created with INOUT anyelement argument and
return type record, because "procedures with output parameters always
return RECORD". I guess this contradicts the way how anyelement has to
be resolved, leading to this behaviour.

At the same time if we try to a function of the same type (INOUT
anyelement argument and returning record), we will get an error right
away:

    ERROR:  42P13: function result type must be anyelement because of
    OUT parameters

This is I think the behaviour that has to be enforced for procedures as
well. It works for anyarray only because of a side effect that
anyarray_out is allowed, due to some columns in pg_statistic having this
type.

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> On Tue, May 14, 2024 at 06:45:29AM +0000, PG Bug reporting form wrote:
>> CREATE PROCEDURE p(INOUT x ANYELEMENT) LANGUAGE SQL AS $$ SELECT x; $$;
>> CALL p(1);
>> The above example results in an error message "cannot display a value of
>> type anyelement", but I would expect it to succeed and output "1".

I agree that this is a bug.  There are comparable cases in our
regression tests that somehow manage to avoid hitting the bug, but
that looks purely accidental to me.

> After looking at this I've got an impression this type of procedures
> have to be disallowed in interpret_function_parameter_list.

No, it's just an oversight.  If you trace through it you will find
that the called procedure does all the right things and returns a
tuple containing the correct values.  The problem happens at the
very end, where we are trying to display that tuple using a tupdesc
that hasn't had the polymorphic types resolved.  That's clearly
possible, since we must have done it at least once already.

I believe the fault lies with CallStmtResultDesc(), which invokes
build_function_result_tupdesc_t() on the pg_proc tuple and thinks
it's done.  However, build_function_result_tupdesc_t clearly says

 * Note that this does not handle resolution of polymorphic types;
 * that is deliberate.

The other caller that needs to think about this is
internal_get_result_type, and behold it does some fooling about
with resolve_polymorphic_tupdesc.  So that's what's missing here.

It looks like we'd have to teach resolve_polymorphic_tupdesc how
to get argument types out of a CallExpr, so that does not lead
to an entirely trivial fix, but it's surely possible.

Maybe it'd be better to not try to use build_function_result_tupdesc_t
here at all.  It looks to me like the output argument list in the
CallStmt is already fully polymorphically resolved, so we could just
build a tupdesc based on that and probably save a lot of work.

            regards, tom lane

I wrote:
> It looks like we'd have to teach resolve_polymorphic_tupdesc how
> to get argument types out of a CallExpr, so that does not lead
> to an entirely trivial fix, but it's surely possible.

> Maybe it'd be better to not try to use build_function_result_tupdesc_t
> here at all.  It looks to me like the output argument list in the
> CallStmt is already fully polymorphically resolved, so we could just
> build a tupdesc based on that and probably save a lot of work.

Some experimentation showed that we need to return the correct
output column names in this tupdesc, so continuing to use
build_function_result_tupdesc_t seems like the easiest path for that.
However, stmt->outargs does hold nodes of the correct resolved data
types, so overwriting the atttypid's from that produces a nicely
small patch, as attached.

            regards, tom lane

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 9cf3fe8275..6593fd7d81 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -52,6 +52,7 @@
 #include "executor/functions.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
 #include "parser/analyze.h"
 #include "parser/parse_coerce.h"
@@ -2364,5 +2365,32 @@ CallStmtResultDesc(CallStmt *stmt)

     ReleaseSysCache(tuple);

+    /*
+     * The result of build_function_result_tupdesc_t has the right column
+     * names, but it just has the declared output argument types, which is the
+     * wrong thing in polymorphic cases.  Get the correct types by examining
+     * stmt->outargs.  We intentionally keep the atttypmod as -1 and the
+     * attcollation as the type's default, since that's always the appropriate
+     * thing for function outputs; there's no point in considering any
+     * additional info available from outargs.  Note that tupdesc is null if
+     * there are no outargs.
+     */
+    if (tupdesc)
+    {
+        Assert(tupdesc->natts == list_length(stmt->outargs));
+        for (int i = 0; i < tupdesc->natts; i++)
+        {
+            Form_pg_attribute att = TupleDescAttr(tupdesc, i);
+            Node       *outarg = (Node *) list_nth(stmt->outargs, i);
+
+            TupleDescInitEntry(tupdesc,
+                               i + 1,
+                               NameStr(att->attname),
+                               exprType(outarg),
+                               -1,
+                               0);
+        }
+    }
+
     return tupdesc;
 }
diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index ab16416c1e..17235fca91 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -409,6 +409,40 @@ END
 $$;
 NOTICE:  a: <NULL>, b: {30,7}
 NOTICE:  _a: 37, _b: 30, _c: 7
+-- polymorphic OUT arguments
+CREATE PROCEDURE test_proc12(a anyelement, OUT b anyelement, OUT c anyarray)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'a: %', a;
+  b := a;
+  c := array[a];
+END;
+$$;
+DO $$
+DECLARE _a int; _b int; _c int[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+NOTICE:  a: 10
+NOTICE:  _a: 10, _b: 10, _c: {10}
+DO $$
+DECLARE _a int; _b int; _c text[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);  -- error
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+ERROR:  procedure test_proc12(integer, integer, text[]) does not exist
+LINE 1: CALL test_proc12(_a, _b, _c)
+             ^
+HINT:  No procedure matches the given name and argument types. You might need to add explicit type casts.
+QUERY:  CALL test_proc12(_a, _b, _c)
+CONTEXT:  PL/pgSQL function inline_code_block line 5 at CALL
 -- transition variable assignment
 TRUNCATE test1;
 CREATE FUNCTION triggerfunc1() RETURNS trigger
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index 8efc8e823a..869d021a07 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -375,6 +375,36 @@ BEGIN
 END
 $$;

+-- polymorphic OUT arguments
+
+CREATE PROCEDURE test_proc12(a anyelement, OUT b anyelement, OUT c anyarray)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'a: %', a;
+  b := a;
+  c := array[a];
+END;
+$$;
+
+DO $$
+DECLARE _a int; _b int; _c int[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+
+DO $$
+DECLARE _a int; _b int; _c text[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);  -- error
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+

 -- transition variable assignment

diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 6ab09d7ec8..8a32fd960c 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -193,6 +193,40 @@ AS $$
 SELECT NULL::int;
 $$;
 CALL ptest6(1, 2);
+CREATE PROCEDURE ptest6a(inout a anyelement, out b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, $1;
+$$;
+CALL ptest6a(1, null);
+ a | b
+---+---
+ 1 | 1
+(1 row)
+
+CALL ptest6a(1.1, null);
+  a  |  b
+-----+-----
+ 1.1 | 1.1
+(1 row)
+
+CREATE PROCEDURE ptest6b(a anyelement, out b anyelement, out c anyarray)
+LANGUAGE SQL
+AS $$
+SELECT $1, array[$1];
+$$;
+CALL ptest6b(1, null, null);
+ b |  c
+---+-----
+ 1 | {1}
+(1 row)
+
+CALL ptest6b(1.1, null, null);
+  b  |   c
+-----+-------
+ 1.1 | {1.1}
+(1 row)
+
 -- collation assignment
 CREATE PROCEDURE ptest7(a text, b text)
 LANGUAGE SQL
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 012cdf3628..b10cf71ca4 100644
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -131,6 +131,24 @@ $$;

 CALL ptest6(1, 2);

+CREATE PROCEDURE ptest6a(inout a anyelement, out b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, $1;
+$$;
+
+CALL ptest6a(1, null);
+CALL ptest6a(1.1, null);
+
+CREATE PROCEDURE ptest6b(a anyelement, out b anyelement, out c anyarray)
+LANGUAGE SQL
+AS $$
+SELECT $1, array[$1];
+$$;
+
+CALL ptest6b(1, null, null);
+CALL ptest6b(1.1, null, null);
+

 -- collation assignment

I wrote:
> Some experimentation showed that we need to return the correct
> output column names in this tupdesc, so continuing to use
> build_function_result_tupdesc_t seems like the easiest path for that.
> However, stmt->outargs does hold nodes of the correct resolved data
> types, so overwriting the atttypid's from that produces a nicely
> small patch, as attached.

Bleah ... that works fine back to v14, but in 12 and 13 it falls
down because there's no outargs list in CallStmt.  We can look at
stmt->funcexpr->args instead, but (a) we have to rediscover which
elements of that are output args, and (b) that list hasn't been
run through expand_function_arguments, so we have to do that
an extra time.

(b) is pretty annoying, since the work is 100% duplicative of
what's about to happen in ExecuteCallStmt.  I thought briefly
about moving the expand_function_arguments call from execution to
transformCallStmt, the way it's done in v14 and later.  I'm afraid
to do that though, because it seems just barely possible that there's
third-party code that knows that that list hasn't been expanded in
these old branches.  I fear we just have to eat the additional
cycles.  They're not that much compared to what will happen to run
a user-defined procedure, but still, bleah.

So I end with the attached modification for 12/13.

            regards, tom lane

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index c24a85dc8c..6fa414c5fa 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -55,6 +55,7 @@
 #include "executor/executor.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "nodes/nodeFuncs.h"
 #include "optimizer/optimizer.h"
 #include "parser/parse_coerce.h"
 #include "parser/parse_collate.h"
@@ -2327,6 +2328,78 @@ CallStmtResultDesc(CallStmt *stmt)

     tupdesc = build_function_result_tupdesc_t(tuple);

+    /*
+     * The result of build_function_result_tupdesc_t has the right column
+     * names, but it just has the declared output argument types, which is the
+     * wrong thing in polymorphic cases.  Get the correct types by examining
+     * the procedure's resolved argument expressions.  We intentionally keep
+     * the atttypmod as -1 and the attcollation as the type's default, since
+     * that's always the appropriate thing for function outputs; there's no
+     * point in considering any additional info available from outargs.  Note
+     * that tupdesc is null if there are no outargs.
+     */
+    if (tupdesc)
+    {
+        Datum        proargmodes;
+        bool        isnull;
+        ArrayType  *arr;
+        char       *argmodes;
+        int            nargs,
+                    noutargs;
+        ListCell   *lc;
+
+        /*
+         * Expand named arguments, defaults, etc.  We do not want to scribble
+         * on the passed-in CallStmt parse tree, so first flat-copy fexpr,
+         * allowing us to replace its args field.  (Note that
+         * expand_function_arguments will not modify any of the passed-in data
+         * structure.)
+         */
+        {
+            FuncExpr   *nexpr = makeNode(FuncExpr);
+
+            memcpy(nexpr, fexpr, sizeof(FuncExpr));
+            fexpr = nexpr;
+        }
+
+        fexpr->args = expand_function_arguments(fexpr->args,
+                                                fexpr->funcresulttype,
+                                                tuple);
+
+        /*
+         * If we're here, build_function_result_tupdesc_t already validated
+         * that the procedure has non-null proargmodes that is the right kind
+         * of array, so it seems unnecessary to check again.
+         */
+        proargmodes = SysCacheGetAttr(PROCOID, tuple,
+                                      Anum_pg_proc_proargmodes,
+                                      &isnull);
+        Assert(!isnull);
+        arr = DatumGetArrayTypeP(proargmodes);    /* ensure not toasted */
+        argmodes = (char *) ARR_DATA_PTR(arr);
+
+        nargs = noutargs = 0;
+        foreach(lc, fexpr->args)
+        {
+            Node       *arg = (Node *) lfirst(lc);
+            Form_pg_attribute att = TupleDescAttr(tupdesc, noutargs);
+            char        argmode = argmodes[nargs++];
+
+            /* ignore non-out arguments */
+            if (argmode == PROARGMODE_IN ||
+                argmode == PROARGMODE_VARIADIC)
+                continue;
+
+            TupleDescInitEntry(tupdesc,
+                               ++noutargs,
+                               NameStr(att->attname),
+                               exprType(arg),
+                               -1,
+                               0);
+        }
+        Assert(tupdesc->natts == noutargs);
+    }
+
     ReleaseSysCache(tuple);

     return tupdesc;
diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index f92e108366..83d5973135 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -302,6 +302,40 @@ END
 $$;
 ERROR:  procedure parameter "c" is an output parameter but corresponding argument is not writable
 CONTEXT:  PL/pgSQL function inline_code_block line 5 at CALL
+-- polymorphic OUT arguments
+CREATE PROCEDURE test_proc12(a anyelement, INOUT b anyelement, INOUT c anyarray)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'a: %', a;
+  b := a;
+  c := array[a];
+END;
+$$;
+DO $$
+DECLARE _a int; _b int; _c int[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+NOTICE:  a: 10
+NOTICE:  _a: 10, _b: 10, _c: {10}
+DO $$
+DECLARE _a int; _b int; _c text[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);  -- error
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+ERROR:  procedure test_proc12(integer, integer, text[]) does not exist
+LINE 1: CALL test_proc12(_a, _b, _c)
+             ^
+HINT:  No procedure matches the given name and argument types. You might need to add explicit type casts.
+QUERY:  CALL test_proc12(_a, _b, _c)
+CONTEXT:  PL/pgSQL function inline_code_block line 5 at CALL
 -- transition variable assignment
 TRUNCATE test1;
 CREATE FUNCTION triggerfunc1() RETURNS trigger
diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index d3194e44c0..6825f59dbc 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -278,6 +278,36 @@ BEGIN
 END
 $$;

+-- polymorphic OUT arguments
+
+CREATE PROCEDURE test_proc12(a anyelement, INOUT b anyelement, INOUT c anyarray)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'a: %', a;
+  b := a;
+  c := array[a];
+END;
+$$;
+
+DO $$
+DECLARE _a int; _b int; _c int[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+
+DO $$
+DECLARE _a int; _b int; _c text[];
+BEGIN
+  _a := 10;
+  CALL test_proc12(_a, _b, _c);  -- error
+  RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
+END
+$$;
+

 -- transition variable assignment

diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 215abcc8c7..8bf5f9a362 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -151,6 +151,40 @@ AS $$
 SELECT NULL::int;
 $$;
 CALL ptest6(1, 2);
+CREATE PROCEDURE ptest6a(inout a anyelement, inout b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, $1;
+$$;
+CALL ptest6a(1, null);
+ a | b
+---+---
+ 1 | 1
+(1 row)
+
+CALL ptest6a(1.1, null);
+  a  |  b
+-----+-----
+ 1.1 | 1.1
+(1 row)
+
+CREATE PROCEDURE ptest6b(a anyelement, inout b anyelement, inout c anyarray)
+LANGUAGE SQL
+AS $$
+SELECT $1, array[$1];
+$$;
+CALL ptest6b(1, null, null);
+ b |  c
+---+-----
+ 1 | {1}
+(1 row)
+
+CALL ptest6b(1.1, null, null);
+  b  |   c
+-----+-------
+ 1.1 | {1.1}
+(1 row)
+
 -- collation assignment
 CREATE PROCEDURE ptest7(a text, b text)
 LANGUAGE SQL
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 127120278c..ec32656c15 100644
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -109,6 +109,24 @@ $$;

 CALL ptest6(1, 2);

+CREATE PROCEDURE ptest6a(inout a anyelement, inout b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, $1;
+$$;
+
+CALL ptest6a(1, null);
+CALL ptest6a(1.1, null);
+
+CREATE PROCEDURE ptest6b(a anyelement, inout b anyelement, inout c anyarray)
+LANGUAGE SQL
+AS $$
+SELECT $1, array[$1];
+$$;
+
+CALL ptest6b(1, null, null);
+CALL ptest6b(1.1, null, null);
+

 -- collation assignment

> On Tue, May 14, 2024 at 08:00:31PM -0400, Tom Lane wrote:
> I wrote:
> > Some experimentation showed that we need to return the correct
> > output column names in this tupdesc, so continuing to use
> > build_function_result_tupdesc_t seems like the easiest path for that.
> > However, stmt->outargs does hold nodes of the correct resolved data
> > types, so overwriting the atttypid's from that produces a nicely
> > small patch, as attached.
>
> Bleah ... that works fine back to v14, but in 12 and 13 it falls
> down because there's no outargs list in CallStmt.  We can look at
> stmt->funcexpr->args instead, but (a) we have to rediscover which
> elements of that are output args, and (b) that list hasn't been
> run through expand_function_arguments, so we have to do that
> an extra time.
>
> (b) is pretty annoying, since the work is 100% duplicative of
> what's about to happen in ExecuteCallStmt.  I thought briefly
> about moving the expand_function_arguments call from execution to
> transformCallStmt, the way it's done in v14 and later.  I'm afraid
> to do that though, because it seems just barely possible that there's
> third-party code that knows that that list hasn't been expanded in
> these old branches.  I fear we just have to eat the additional
> cycles.  They're not that much compared to what will happen to run
> a user-defined procedure, but still, bleah.
>
> So I end with the attached modification for 12/13.

TIL, thanks. The patches look good to me, I've verified on both master
and 13 that it fixes the problem.

I'm now curious why it is different for functions, when creating one
with an INOUT ANYELEMENT argument and record return type will error out.
Disabling the corresponding ereport check in CreateFunction seems to
produce a function that works in the similar way as the procedure in
this thread. Are those type of functions incorrect in some way?

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> I'm now curious why it is different for functions, when creating one
> with an INOUT ANYELEMENT argument and record return type will error out.
> Disabling the corresponding ereport check in CreateFunction seems to
> produce a function that works in the similar way as the procedure in
> this thread. Are those type of functions incorrect in some way?

With procedures, there's no explicit RETURNS clause; we just
automatically fill RECORD into prorettype because (a) we gotta
put something and (b) that's the right thing anyway if there's
multiple OUT parameters.  Arguably it's not wrong for a single
output parameter, either, since CALL will return a tuple in
that case too.  I think it might've been better to put VOID
for the case of zero output parameters, since CALL doesn't
return a zero-column tuple in that case.  But that ship's
sailed, and it's not worth quibbling about.

We do this differently for functions: if there's exactly one
output parameter, that is the function result, so prorettype
has to match.  If we were to allow RETURNS RECORD with a
single output parameter, I think that'd have to mean that
we return a one-column tuple containing that parameter value.
That's not implemented, and I have doubts that it'd be useful.
It'd certainly be a bit inefficient.

            regards, tom lane

Andrew Bille <andrewbille@gmail.com> writes:
> After 70ffb27b in REL_12 following script
> CREATE OR REPLACE PROCEDURE p(inout a anyelement, inout b anyelement)
> LANGUAGE SQL
> AS $$
>   SELECT $1, 1;
> $$;
> CALL p(1.1, null);
> crash server with backtrace:

Thanks for the report!  It's fine in v13 and later, so I must have
missed something while back-patching.  Will look.

            regards, tom lane

Andrew Bille <andrewbille@gmail.com> writes:
> After 70ffb27b in REL_12 following script
> CREATE OR REPLACE PROCEDURE p(inout a anyelement, inout b anyelement)
> LANGUAGE SQL
> AS $$
>   SELECT $1, 1;
> $$;
> CALL p(1.1, null);
> crash server with backtrace:

So the problem here is that in v12, check_sql_fn_retval() fails to
resolve the polymorphic output types and then just throws up its hands
and assumes the check will be made at runtime.  I think that's true
for ordinary functions returning RECORD, but it doesn't happen in
CALL.  What needs to happen is for check_sql_fn_retval to resolve
those types and then notice that the SELECT output doesn't match.

In v13 and later, this was fixed by 913bbd88d ("Improve the handling
of result type coercions in SQL functions"), which not only did the
polymorphism stuff correctly but would also insert a cast from int
to numeric to allow this case to succeed.  I thought then, and still
think, that that was too big a behavior change to risk back-patching.
So the best we can hope for in v12 is that this example throws an
error cleanly.

Fortunately that doesn't seem too painful --- with a little bit of
local rejiggering, we can use get_call_result_type instead of
get_func_result_type, and that will resolve the arguments correctly.
So that leads me to the attached.

Even though there's no bug in >= v13, I'm slightly tempted to put
the new test cases into the later branches too.  If we'd had a test
like this we'd have noticed the problem ...

            regards, tom lane

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 2e4d5d9f45..4ec1be83e3 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -154,7 +154,7 @@ static Node *sql_fn_resolve_param_name(SQLFunctionParseInfoPtr pinfo,
 static List *init_execution_state(List *queryTree_list,
                                   SQLFunctionCachePtr fcache,
                                   bool lazyEvalOK);
-static void init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK);
+static void init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK);
 static void postquel_start(execution_state *es, SQLFunctionCachePtr fcache);
 static bool postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache);
 static void postquel_end(execution_state *es);
@@ -166,6 +166,12 @@ static Datum postquel_get_single_result(TupleTableSlot *slot,
                                         MemoryContext resultcontext);
 static void sql_exec_error_callback(void *arg);
 static void ShutdownSQLFunction(Datum arg);
+static bool check_sql_fn_retval_ext2(Oid func_id,
+                                     FunctionCallInfo fcinfo,
+                                     Oid rettype, char prokind,
+                                     List *queryTreeList,
+                                     bool *modifyTargetList,
+                                     JunkFilter **junkFilter);
 static void sqlfunction_startup(DestReceiver *self, int operation, TupleDesc typeinfo);
 static bool sqlfunction_receive(TupleTableSlot *slot, DestReceiver *self);
 static void sqlfunction_shutdown(DestReceiver *self);
@@ -591,8 +597,9 @@ init_execution_state(List *queryTree_list,
  * Initialize the SQLFunctionCache for a SQL function
  */
 static void
-init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
+init_sql_fcache(FunctionCallInfo fcinfo, Oid collation, bool lazyEvalOK)
 {
+    FmgrInfo   *finfo = fcinfo->flinfo;
     Oid            foid = finfo->fn_oid;
     MemoryContext fcontext;
     MemoryContext oldcontext;
@@ -744,12 +751,13 @@ init_sql_fcache(FmgrInfo *finfo, Oid collation, bool lazyEvalOK)
      * coerce the returned rowtype to the desired form (unless the result type
      * is VOID, in which case there's nothing to coerce to).
      */
-    fcache->returnsTuple = check_sql_fn_retval_ext(foid,
-                                                   rettype,
-                                                   procedureStruct->prokind,
-                                                   flat_query_list,
-                                                   NULL,
-                                                   &fcache->junkFilter);
+    fcache->returnsTuple = check_sql_fn_retval_ext2(foid,
+                                                    fcinfo,
+                                                    rettype,
+                                                    procedureStruct->prokind,
+                                                    flat_query_list,
+                                                    NULL,
+                                                    &fcache->junkFilter);

     if (fcache->returnsTuple)
     {
@@ -1066,7 +1074,7 @@ fmgr_sql(PG_FUNCTION_ARGS)

     if (fcache == NULL)
     {
-        init_sql_fcache(fcinfo->flinfo, PG_GET_COLLATION(), lazyEvalOK);
+        init_sql_fcache(fcinfo, PG_GET_COLLATION(), lazyEvalOK);
         fcache = (SQLFunctionCachePtr) fcinfo->flinfo->fn_extra;
     }

@@ -1593,11 +1601,11 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                     JunkFilter **junkFilter)
 {
     /* Wrapper function to preserve ABI compatibility in released branches */
-    return check_sql_fn_retval_ext(func_id, rettype,
-                                   PROKIND_FUNCTION,
-                                   queryTreeList,
-                                   modifyTargetList,
-                                   junkFilter);
+    return check_sql_fn_retval_ext2(func_id, NULL, rettype,
+                                    PROKIND_FUNCTION,
+                                    queryTreeList,
+                                    modifyTargetList,
+                                    junkFilter);
 }

 bool
@@ -1605,6 +1613,22 @@ check_sql_fn_retval_ext(Oid func_id, Oid rettype, char prokind,
                         List *queryTreeList,
                         bool *modifyTargetList,
                         JunkFilter **junkFilter)
+{
+    /* Wrapper function to preserve ABI compatibility in released branches */
+    return check_sql_fn_retval_ext2(func_id, NULL, rettype,
+                                    prokind,
+                                    queryTreeList,
+                                    modifyTargetList,
+                                    junkFilter);
+}
+
+static bool
+check_sql_fn_retval_ext2(Oid func_id,
+                         FunctionCallInfo fcinfo,
+                         Oid rettype, char prokind,
+                         List *queryTreeList,
+                         bool *modifyTargetList,
+                         JunkFilter **junkFilter)
 {
     Query       *parse;
     List      **tlist_ptr;
@@ -1750,6 +1774,7 @@ check_sql_fn_retval_ext(Oid func_id, Oid rettype, char prokind,
          * result type, so there is no way to produce a domain-over-composite
          * result except by computing it as an explicit single-column result.
          */
+        TypeFuncClass tfclass;
         TupleDesc    tupdesc;
         int            tupnatts;    /* physical number of columns in tuple */
         int            tuplogcols; /* # of nondeleted columns in tuple */
@@ -1806,10 +1831,19 @@ check_sql_fn_retval_ext(Oid func_id, Oid rettype, char prokind,
         }

         /*
-         * Is the rowtype fixed, or determined only at runtime?  (Note we
+         * Identify the output rowtype, resolving polymorphism if possible
+         * (that is, if we were passed an fcinfo).
+         */
+        if (fcinfo)
+            tfclass = get_call_result_type(fcinfo, NULL, &tupdesc);
+        else
+            tfclass = get_func_result_type(func_id, NULL, &tupdesc);
+
+        /*
+         * Is the rowtype known, or determined only at runtime?  (Note we
          * cannot see TYPEFUNC_COMPOSITE_DOMAIN here.)
          */
-        if (get_func_result_type(func_id, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+        if (tfclass != TYPEFUNC_COMPOSITE)
         {
             /*
              * Assume we are returning the whole tuple. Crosschecking against
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 4758744071..2f646a02b5 100644
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -185,6 +185,21 @@ CALL ptest6b(1.1, null, null);
  1.1 | {1.1}
 (1 row)

+CREATE PROCEDURE ptest6c(inout a anyelement, inout b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, 1;
+$$;
+CALL ptest6c(1, null);
+ a | b
+---+---
+ 1 | 1
+(1 row)
+
+CALL ptest6c(1.1, null);  -- fails before v13
+ERROR:  return type mismatch in function declared to return record
+DETAIL:  Final statement returns integer instead of numeric at column 2.
+CONTEXT:  SQL function "ptest6c" during startup
 -- collation assignment
 CREATE PROCEDURE ptest7(a text, b text)
 LANGUAGE SQL
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 2dfa0ac2fd..0ba98fe240 100644
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -127,6 +127,15 @@ $$;
 CALL ptest6b(1, null, null);
 CALL ptest6b(1.1, null, null);

+CREATE PROCEDURE ptest6c(inout a anyelement, inout b anyelement)
+LANGUAGE SQL
+AS $$
+SELECT $1, 1;
+$$;
+
+CALL ptest6c(1, null);
+CALL ptest6c(1.1, null);  -- fails before v13
+

 -- collation assignment