Thread: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
PG Bug reporting form
Date:
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]);
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Dmitry Dolgov
Date:
> 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.
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Dmitry Dolgov
Date:
> 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?
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Andrew Bille
Date:
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@entry=0x55b81cfa0d30, direction=direction@entry=ForwardScanDirection, count=count@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@entry=0x55b81cfa0d30, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x55b81cf3b1a0, altdest=altdest@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@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@entry=3, argv=argv@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!
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@entry=0x55b81cfa0d30, direction=direction@entry=ForwardScanDirection, count=count@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@entry=0x55b81cfa0d30, count=count@entry=9223372036854775807, isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true, dest=dest@entry=0x55b81cf3b1a0, altdest=altdest@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@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@entry=3, argv=argv@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@sss.pgh.pa.us> wrote:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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
Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
From
Tom Lane
Date:
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