From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: drewk@cockroachlabs.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
Date: Tue, 14 May 2024 20:00:31 -0400
Message-ID: <1631943.1715731231@sss.pgh.pa.us> (raw)
In-Reply-To: <1341220.1715715337@sss.pgh.pa.us>
References: <18463-f8cd77e12564d8a2@postgresql.org>
<20240514151843.yvq3wlt7n2kechv5@ddolgov.remote.csb>
<1124184.1715706026@sss.pgh.pa.us>
<1341220.1715715337@sss.pgh.pa.us>
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
Attachments:fix-polymorphic-procedure-outputs-v13.patch (text/x-diff, 6.6K) downloaddiff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.cindex 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.outindex 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.sqlindex 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.outindex 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.sqlindex 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
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, 9erthalion6@gmail.com, drewk@cockroachlabs.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters
In-Reply-To: <1631943.1715731231@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox