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(at)cockroachlabs(dot)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(at)cockroachlabs(dot)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(at)gmail(dot)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

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

> 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(at)gmail(dot)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

Hello!

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:
Core was generated by `postgres: andrew postgres'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x000055b81c1d090d in memcpy (__len=18446744073709551615,
__src=0x55b81d000239, __dest=0x55b81cfefc94) at
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29
29 return __builtin___memcpy_chk (__dest, __src, __len,
(gdb) bt
#0 0x000055b81c1d090d in memcpy (__len=18446744073709551615,
__src=0x55b81d000239, __dest=0x55b81cfefc94) at
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29
#1 heap_tuple_untoast_attr (attr=0x55b81d000238) at tuptoaster.c:243
#2 0x000055b81c570b35 in pg_detoast_datum (datum=<optimized out>) at
fmgr.c:1742
#3 0x000055b81c4e5491 in numeric_out (fcinfo=<optimized out>) at
numeric.c:649
#4 0x000055b81c570728 in FunctionCall1Coll (arg1=<optimized out>,
collation=0, flinfo=0x55b81cf5d730) at fmgr.c:1140
#5 OutputFunctionCall (flinfo=0x55b81cf5d730, val=<optimized out>) at
fmgr.c:1577
#6 0x000055b81c190d5d in printtup (slot=0x55b81cf5d438,
self=0x55b81cf3b1a0) at printtup.c:434
#7 0x000055b81c45c1be in RunFromStore (portal=portal(at)entry=0x55b81cfa0d30,
direction=direction(at)entry=ForwardScanDirection, count=count(at)entry=0,
dest=0x55b81cf3b1a0) at pquery.c:1112
#8 0x000055b81c45c25b in PortalRunSelect (portal=0x55b81cfa0d30,
forward=<optimized out>, count=0, dest=<optimized out>) at pquery.c:934
#9 0x000055b81c45da3e in PortalRun (portal=portal(at)entry=0x55b81cfa0d30,
count=count(at)entry=9223372036854775807, isTopLevel=isTopLevel(at)entry=true,
run_once=run_once(at)entry=true, dest=dest(at)entry=0x55b81cf3b1a0,
altdest=altdest(at)entry=0x55b81cf3b1a0, completionTag=0x7ffd621f8850 "") at
pquery.c:779
#10 0x000055b81c4597a1 in exec_simple_query (query_string=0x55b81cf39f10
"CALL p(1.1, null);") at postgres.c:1214
#11 0x000055b81c45b5f2 in PostgresMain (argc=<optimized out>,
argv=argv(at)entry=0x55b81cf64e68, dbname=<optimized out>, username=<optimized
out>) at postgres.c:4297
#12 0x000055b81c3e71a7 in BackendRun (port=0x55b81cf5c140) at
postmaster.c:4517
#13 BackendStartup (port=0x55b81cf5c140) at postmaster.c:4200
#14 ServerLoop () at postmaster.c:1725
#15 0x000055b81c3e80c2 in PostmasterMain (argc=argc(at)entry=3,
argv=argv(at)entry=0x55b81cf346b0)
at postmaster.c:1398
#16 0x000055b81c18594d in main (argc=3, argv=0x55b81cf346b0) at main.c:228

This doesn't repoduce in 13+

Best regards, Andrew!

On Tue, Jun 4, 2024 at 9:16 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Dmitry Dolgov <9erthalion6(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)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