Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF

Tender Wang <tndrwang@gmail.com> writes:
>> The following bug has been logged on the website:
>> Bug reference:      18422
>> Logged by:          Alexander Lakhin
>> Email address:      exclusion@gmail.com
>> PostgreSQL version: 16.2
>> Operating system:   Ubuntu 22.04
>> Description:
>> 
>> The following query:
>> SELECT * FROM generate_series(1, 1),
>> COALESCE(row(1)) AS (a int, b int);
>> triggers an assertion failure:
>> TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
>> Line: 3054, PID: 151361

> I copy the ereport code in tupledesc_match() into expandRTE() if
> funcolcount is larger
> than natts. The attached patch looks like a workaround fix.

I think that's mostly a kluge.  It seems to me that the actual problem
is that 2ed8f9a01 missed some places that need fixing.  The policy
that that commit intended to institute is "if a RangeTblFunction has a
coldeflist, then the function return type is certainly RECORD, and we
don't need to try to dig the column specifications out of the function
expression" (which dodges the problem that the function expression
might not produce what we're expecting).  However, expandRTE didn't
get that memo, and would produce the wrong tupdesc in an example such
as this one.  IMO, it ought to produce the query-specified columns in
all cases, even when the function expression doesn't match.  Throwing
a premature error isn't better, especially if it only detects one
kind of mismatch.

I dug around looking at other callers of get_expr_result_type and
get_expr_result_tupdesc, and found a couple other places that aren't
actively buggy but can be made faster and more robust by adding
this same rule.

There is a remaining place where we're arguably misusing
get_expr_result_type, which is init_sexpr in execSRF.c.  However,
that will just result in postponing the eventual error, because
we'll still cross-check the function result tupdesc against the
query-expected one later.  So I think it's okay as-is; at least
it doesn't seem worth the trouble to refactor so that init_sexpr
would have access to the correct RangeTblFunction.

BTW, I wondered why the test cases we added for 2ed8f9a01 didn't
catch this, because they sure look superficially similar.  The
reason seems to be that we fail because this query produces a join
plan in which we invoke build_physical_tlist for the FunctionScan,
and that's what's calling expandRTE and hitting the assertion.
The older test cases get simplified sufficiently that there's no
join but just a FunctionScan, and that plan node will be required
to produce the query-specified tlist not a physical tlist, so we
accidentally avoid reaching the assertion.

So I end with the attached.

            regards, tom lane

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 4badb6ff58..fb768ad52c 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1854,6 +1854,10 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
     if (rtf->funccolcount != 1)
         return jtnode;            /* definitely composite */

+    /* If it has a coldeflist, it certainly returns RECORD */
+    if (rtf->funccolnames != NIL)
+        return jtnode;            /* must be a one-column RECORD type */
+
     functypclass = get_expr_result_type(rtf->funcexpr,
                                         &funcrettype,
                                         &tupdesc);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b50fe58d1c..59487cbd79 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4431,12 +4431,11 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
      * Can't simplify if it returns RECORD.  The immediate problem is that it
      * will be needing an expected tupdesc which we can't supply here.
      *
-     * In the case where it has OUT parameters, it could get by without an
-     * expected tupdesc, but we still have issues: get_expr_result_type()
-     * doesn't know how to extract type info from a RECORD constant, and in
-     * the case of a NULL function result there doesn't seem to be any clean
-     * way to fix that.  In view of the likelihood of there being still other
-     * gotchas, seems best to leave the function call unreduced.
+     * In the case where it has OUT parameters, we could build an expected
+     * tupdesc from those, but there may be other gotchas lurking.  In
+     * particular, if the function were to return NULL, we would produce a
+     * null constant with no remaining indication of which concrete record
+     * type it is.  For now, seems best to leave the function call unreduced.
      */
     if (funcform->prorettype == RECORDOID)
         return NULL;
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 7ca793a369..2f64eaf0e3 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2738,12 +2738,17 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
                 {
                     RangeTblFunction *rtfunc = (RangeTblFunction *) lfirst(lc);
                     TypeFuncClass functypclass;
-                    Oid            funcrettype;
-                    TupleDesc    tupdesc;
+                    Oid            funcrettype = InvalidOid;
+                    TupleDesc    tupdesc = NULL;
+
+                    /* If it has a coldeflist, it returns RECORD */
+                    if (rtfunc->funccolnames != NIL)
+                        functypclass = TYPEFUNC_RECORD;
+                    else
+                        functypclass = get_expr_result_type(rtfunc->funcexpr,
+                                                            &funcrettype,
+                                                            &tupdesc);

-                    functypclass = get_expr_result_type(rtfunc->funcexpr,
-                                                        &funcrettype,
-                                                        &tupdesc);
                     if (functypclass == TYPEFUNC_COMPOSITE ||
                         functypclass == TYPEFUNC_COMPOSITE_DOMAIN)
                     {
@@ -3369,6 +3374,10 @@ get_rte_attribute_is_dropped(RangeTblEntry *rte, AttrNumber attnum)
                     {
                         TupleDesc    tupdesc;

+                        /* If it has a coldeflist, it returns RECORD */
+                        if (rtfunc->funccolnames != NIL)
+                            return false;    /* can't have any dropped columns */
+
                         tupdesc = get_expr_result_tupdesc(rtfunc->funcexpr,
                                                           true);
                         if (tupdesc)
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 2bda957f43..397a8b35d6 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2498,3 +2498,6 @@ with a(b) as (values (row(1,2,3)))
 select * from a, coalesce(b) as c(d int, e int, f float);  -- fail
 ERROR:  function return row and query-specified return row do not match
 DETAIL:  Returned type integer at ordinal position 3, but query expects double precision.
+select * from int8_tbl, coalesce(row(1)) as (a int, b int);  -- fail
+ERROR:  function return row and query-specified return row do not match
+DETAIL:  Returned row contains 1 attribute, but query expects 2.
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 06d598c5e9..3c47c98e11 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -824,3 +824,4 @@ with a(b) as (values (row(1,2,3)))
 select * from a, coalesce(b) as c(d int, e int, f int, g int);  -- fail
 with a(b) as (values (row(1,2,3)))
 select * from a, coalesce(b) as c(d int, e int, f float);  -- fail
+select * from int8_tbl, coalesce(row(1)) as (a int, b int);  -- fail