Fix confusion about the return rowtype of SQL-language procedures.

There is a very ancient hack in check_sql_fn_retval that allows a
single SELECT targetlist entry of composite type to be taken as
supplying all the output columns of a function returning composite.
(This is grotty and fundamentally ambiguous, but it's really hard
to do nested composite-returning functions without it.)

As far as I know, that doesn't cause any problems in ordinary
functions.  It's disastrous for procedures however.  All procedures
that have any output parameters are labeled with prorettype RECORD,
and the CALL code expects it will get back a record with one column
per output parameter, regardless of whether any of those parameters
is composite.  Doing something else leads to an assertion failure
or core dump.

This is simple enough to fix: we just need to not apply that rule
when considering procedures.  However, that requires adding another
argument to check_sql_fn_retval, which at least in principle might be
getting called by external callers.  Therefore, in the back branches
convert check_sql_fn_retval into an ABI-preserving wrapper around a
new function check_sql_fn_retval_ext.

Per report from Yahor Yuzefovich.  This has been broken since we
implemented procedures, so back-patch to all supported branches.

Discussion: https://postgr.es/m/CABz5gWHSjj2df6uG0NRiDhZ_Uz=Y8t0FJP-_SVSsRsnrQT76Gg@mail.gmail.com

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index c3e33e0f9c6fa7f5e0b1872f7b0d91b70ec3b7e9..ea026572f953441c7912d16b79bdb322db0e40c2 100644 (file)
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -945,9 +945,10 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
                        }
 
                        check_sql_fn_statements(querytree_list);
-                       (void) check_sql_fn_retval(funcoid, proc->prorettype,
-                                                                          querytree_list,
-                                                                          NULL, NULL);
+                       (void) check_sql_fn_retval_ext(funcoid, proc->prorettype,
+                                                                                  proc->prokind,
+                                                                                  querytree_list,
+                                                                                  NULL, NULL);
                }
 
                error_context_stack = sqlerrcontext.previous;

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index c7a3739dad78d452a98d092194f578398840c2ad..2e4d5d9f4541096ac82bd8b12a5607191671629f 100644 (file)
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -744,11 +744,12 @@ 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(foid,
-                                                                                          rettype,
-                                                                                          flat_query_list,
-                                                                                          NULL,
-                                                                                          &fcache->junkFilter);
+       fcache->returnsTuple = check_sql_fn_retval_ext(foid,
+                                                                                                  rettype,
+                                                                                                  procedureStruct->prokind,
+                                                                                                  flat_query_list,
+                                                                                                  NULL,
+                                                                                                  &fcache->junkFilter);
 
        if (fcache->returnsTuple)
        {
@@ -1590,6 +1591,20 @@ bool
 check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                                        bool *modifyTargetList,
                                        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);
+}
+
+bool
+check_sql_fn_retval_ext(Oid func_id, Oid rettype, char prokind,
+                                               List *queryTreeList,
+                                               bool *modifyTargetList,
+                                               JunkFilter **junkFilter)
 {
        Query      *parse;
        List      **tlist_ptr;
@@ -1608,7 +1623,7 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
 
        /*
         * If it's declared to return VOID, we don't care what's in the function.
-        * (This takes care of the procedure case, as well.)
+        * (This takes care of procedures with no output parameters, as well.)
         */
        if (rettype == VOIDOID)
                return false;
@@ -1753,8 +1768,13 @@ check_sql_fn_retval(Oid func_id, Oid rettype, List *queryTreeList,
                 * will succeed for any composite restype.  For the moment we rely on
                 * runtime type checking to catch any discrepancy, but it'd be nice to
                 * do better at parse time.
+                *
+                * We must *not* do this for a procedure, however.  Procedures with
+                * output parameter(s) have rettype RECORD, and the CALL code expects
+                * to get results corresponding to the list of output parameters, even
+                * when there's just one parameter that's composite.
                 */
-               if (tlistlen == 1)
+               if (tlistlen == 1 && prokind != PROKIND_PROCEDURE)
                {
                        TargetEntry *tle = (TargetEntry *) linitial(tlist);
 

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 519dbd0dcbccc651b25feeb5b229e48e0c1839b9..b2a7f6609a0129951d301d3e700ca69e9f42c416 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4624,8 +4624,9 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid,
         * Note: we do not try this until we have verified that no rewriting was
         * needed; that's probably not important, but let's be careful.
         */
-       if (check_sql_fn_retval(funcid, result_type, list_make1(querytree),
-                                                       &modifyTargetList, NULL))
+       if (check_sql_fn_retval_ext(funcid, result_type, funcform->prokind,
+                                                               list_make1(querytree),
+                                                               &modifyTargetList, NULL))
                goto fail;                              /* reject whole-tuple-result cases */
 
        /* Now we can grab the tlist expression */
@@ -5149,9 +5150,10 @@ inline_set_returning_function(PlannerInfo *root, RangeTblEntry *rte)
         * check_sql_fn_retval, we deliberately exclude domains over composite
         * here.)
         */
-       if (!check_sql_fn_retval(func_oid, fexpr->funcresulttype,
-                                                        querytree_list,
-                                                        &modifyTargetList, NULL) &&
+       if (!check_sql_fn_retval_ext(func_oid, fexpr->funcresulttype,
+                                                                funcform->prokind,
+                                                                querytree_list,
+                                                                &modifyTargetList, NULL) &&
                (get_typtype(fexpr->funcresulttype) == TYPTYPE_COMPOSITE ||
                 fexpr->funcresulttype == RECORDOID))
                goto fail;                              /* reject not-whole-tuple-result cases */

diff --git a/src/include/executor/functions.h b/src/include/executor/functions.h
index 99131bfadb4a0d4aff981ff0d6a122b0cc684f32..24701bfb38303fb9c87f0ffa05f6586b308d2b1f 100644 (file)
--- a/src/include/executor/functions.h
+++ b/src/include/executor/functions.h
@@ -36,6 +36,12 @@ extern bool check_sql_fn_retval(Oid func_id, Oid rettype,
                                                                bool *modifyTargetList,
                                                                JunkFilter **junkFilter);
 
+extern bool check_sql_fn_retval_ext(Oid func_id, Oid rettype,
+                                                                       char prokind,
+                                                                       List *queryTreeList,
+                                                                       bool *modifyTargetList,
+                                                                       JunkFilter **junkFilter);
+
 extern DestReceiver *CreateSQLFunctionDestReceiver(void);
 
 #endif                                                 /* FUNCTIONS_H */

diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
index 211a42cefa039e70fc3e63289f724505081f7758..13e3470f42fe86a9705eee03c1b1a91c44df4e00 100644 (file)
--- a/src/test/regress/expected/create_procedure.out
+++ b/src/test/regress/expected/create_procedure.out
@@ -106,7 +106,19 @@ CALL ptest4a(a, b);  -- error, not supported
 $$;
 ERROR:  calling procedures with output arguments is not supported in SQL functions
 CONTEXT:  SQL function "ptest4b"
-DROP PROCEDURE ptest4a;
+-- we used to get confused by a single output argument that is composite
+CREATE PROCEDURE ptest4c(INOUT comp int8_tbl)
+LANGUAGE SQL
+AS $$
+SELECT ROW(1, 2)::int8_tbl;
+$$;
+CALL ptest4c(NULL);
+ comp  
+-------
+ (1,2)
+(1 row)
+
+DROP PROCEDURE ptest4a, ptest4c;
 -- named and default parameters
 CREATE OR REPLACE PROCEDURE ptest5(a int, b text, c int default 100)
 LANGUAGE SQL

diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
index 89b96d580ffa472b3dfc705e36c100c16eb08429..7414090372983069eb8ed9c6ef4bef93cd335258 100644 (file)
--- a/src/test/regress/sql/create_procedure.sql
+++ b/src/test/regress/sql/create_procedure.sql
@@ -68,7 +68,16 @@ AS $$
 CALL ptest4a(a, b);  -- error, not supported
 $$;
 
-DROP PROCEDURE ptest4a;
+-- we used to get confused by a single output argument that is composite
+CREATE PROCEDURE ptest4c(INOUT comp int8_tbl)
+LANGUAGE SQL
+AS $$
+SELECT ROW(1, 2)::int8_tbl;
+$$;
+
+CALL ptest4c(NULL);
+
+DROP PROCEDURE ptest4a, ptest4c;
 
 
 -- named and default parameters