Re: BUG #18463: Possible bug in stored procedures with polymorphic OUT parameters

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