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

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

with the following stack trace:
...
#5  0x000055b935579366 in ExceptionalCondition
(conditionName=conditionName@entry=0x55b93569420d "count <= tupdesc->natts",
fileName=fileName@entry=0x55b9356941e6 "parse_relation.c",
lineNumber=lineNumber@entry=3054) at assert.c:66
#6  0x000055b9351841b5 in expandTupleDesc (tupdesc=0x55b9378c7660,
eref=0x55b9378c7358, count=2, offset=offset@entry=0,
rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0, location=-1,
include_dropped=true, colnames=0x0, 
    colvars=0x7fffaccae3e0) at parse_relation.c:3054
#7  0x000055b93518713a in expandRTE (rte=rte@entry=0x55b9377fc360,
rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0,
location=location@entry=-1, include_dropped=include_dropped@entry=true,
colnames=colnames@entry=0x0, 
    colvars=0x7fffaccae3e0) at parse_relation.c:2758
#8  0x000055b9353649b6 in build_physical_tlist
(root=root@entry=0x55b9378c8108, rel=rel@entry=0x55b9378c8cf0) at
plancat.c:1792
#9  0x000055b935330653 in create_scan_plan (root=root@entry=0x55b9378c8108,
best_path=best_path@entry=0x55b9378c9340, flags=<optimized out>,
flags@entry=0) at createplan.c:637
#10 0x000055b93532dd65 in create_plan_recurse
(root=root@entry=0x55b9378c8108, best_path=0x55b9378c9340,
flags=flags@entry=0) at createplan.c:411
#11 0x000055b93532f585 in create_nestloop_plan (root=0x55b9378c8108,
best_path=0x55b9377fc470) at createplan.c:4342
#12 0x000055b93532f6be in create_join_plan (root=root@entry=0x55b9378c8108,
best_path=best_path@entry=0x55b9377fc470) at createplan.c:1076
#13 0x000055b93532dd75 in create_plan_recurse
(root=root@entry=0x55b9378c8108, best_path=0x55b9377fc470,
flags=flags@entry=1) at createplan.c:416
#14 0x000055b93532e575 in create_plan (root=root@entry=0x55b9378c8108,
best_path=<optimized out>) at createplan.c:347
#15 0x000055b93533ff5b in standard_planner (parse=0x55b9377fbf78,
query_string=<optimized out>, cursorOptions=2048, boundParams=<optimized
out>) at planner.c:420
#16 0x000055b9353404f8 in planner (parse=parse@entry=0x55b9377fbf78,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n  COALESCE(row(1)) AS (a int, b int);",
cursorOptions=cursorOptions@entry=2048, 
    boundParams=boundParams@entry=0x0) at planner.c:281
#17 0x000055b93542c257 in pg_plan_query
(querytree=querytree@entry=0x55b9377fbf78,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n  COALESCE(row(1)) AS (a int, b int);", 
    cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:904
#18 0x000055b93542c30d in pg_plan_queries (querytrees=0x55b9378c80b8,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n  COALESCE(row(1)) AS (a int, b int);", 
    cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:996
#19 0x000055b93542c7f0 in exec_simple_query
(query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n  COALESCE(row(1)) AS (a int, b int);") at
postgres.c:1193

(gdb) frame 6

(gdb) p tupdesc->natts
$1 = 1

Reproduced on REL_12_STABLE .. master.

`git bisect` blames d57534740 (8a15b4178 on REL_12_STABLE).
(Thanks to Amit Langote for correcting my initial report where this issue
was hastily attributed to JSON_TABLE().)

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

Richard Guo <guofenglinux@gmail.com> writes:
> I wonder why the v2 patch does not apply this same rule in
> process_function_rte_ref(), by checking if the 'rtfunc' has a coldeflist
> before it calls get_expr_result_tupdesc.

I guess we could.  It won't matter because the following code will
reject RECORD in any case; but we could save a few cycles by not
calling get_expr_result_tupdesc there.

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Apr 10, 2024 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I guess we could.  It won't matter because the following code will
>> reject RECORD in any case; but we could save a few cycles by not
>> calling get_expr_result_tupdesc there.

> Indeed.  I think it would be better to add this same rule to
> process_function_rte_ref(), or at least write some comments there to
> explain why it is not necessary to check rtfunc->funccolnames while
> other places do.

Wilco.  Another thing I was considering, but didn't pull the trigger
on in the draft patch, was to introduce a funcapi.c function on the
order of
    get_expr_result_rtfunc(RangeTblFunction *rtfunc, ...)
which would encapsulate applying either BuildDescFromLists or
get_expr_result_type.  I didn't do it because I found that the
only places that would want to use it are the two that are correct
already; the places we still need to fix have short-cuts they can
take rather than building an intermediate tupdesc.  (The present
bug could be summarized as "the short-cuts still need to pay
attention to the coldeflist".)  But the advantage of doing this
anyway is that its header comment would be a natural place to
document this issue and policy.  Thoughts?

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Apr 10, 2024 at 10:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Wilco.  Another thing I was considering, but didn't pull the trigger
>> on in the draft patch, was to introduce a funcapi.c function on the
>> order of
>>     get_expr_result_rtfunc(RangeTblFunction *rtfunc, ...)
>> which would encapsulate applying either BuildDescFromLists or
>> get_expr_result_type.

> Do you think we can have a parameter in the new get_expr_result_rtfunc()
> function to indicate whether we want to build an intermediate tupdesc
> when we have a coldeflist?  Then we can set it to true in the two places
> that are correct already, and set it to false at the places we need to
> fix.  But I'm not sure if including such a new parameter would be an
> improvement or just make it worse.

I did think about that, but it seems mighty weird.  The semantics of
the flag would have to be something like "I want a tupdesc when the
result type is COMPOSITE, but not when it's RECORD", which seems
rather arbitrary.

Perhaps it'd be sufficient to add a note to the header comment of
get_expr_result_type warning about when not to use it.

            regards, tom lane

Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Apr 11, 2024 at 10:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I did think about that, but it seems mighty weird.  The semantics of
>> the flag would have to be something like "I want a tupdesc when the
>> result type is COMPOSITE, but not when it's RECORD", which seems
>> rather arbitrary.

> Indeed.

I did actually spend some time today trying to code that up, to see
what it would look like.  The attempt failed though, because there
are existing cases in which get_expr_result_type() returns
TYPEFUNC_RECORD, so we can't use that result code as a positive
indicator that "this RTE has a coldeflist".  In a green field
we could invent another TYPEFUNC_XXX code, but that's not a
back-patchable idea.  So the new function would need some independent
indicator that it saw a coldeflist, and at that point there's
basically nothing we're hiding from the callers.

>> Perhaps it'd be sufficient to add a note to the header comment of
>> get_expr_result_type warning about when not to use it.

> Works for me.

OK, barring other objections I'll push forward on that basis.

            regards, tom lane