Re: Record returning function accept not matched columns declaration - Mailing list pgsql-bugs

"David G. Johnston" <david.g.johnston@gmail.com> writes:
> I strongly dislike the seemingly overloaded terminology in this area.

Fair.  By "returns RECORD" I meant that it actually is declared as
returning that pseudotype, rather than a named composite type.

> Namely I was trying to distinguish these two example function signatures.

> json_populate_record ( base anyelement, from_json json ) → anyelement
> jsonb_to_record ( jsonb ) → record

> Polymorphic functions do not require a column definition list.  The
> non-polymorphic function signature does require the column definition
> list.  That we accept a column definition list in the polymorphic case is
> settled code but seems like it led to this bug.

No, I don't think that has much to do with it; note that the
json_populate_record case is one of the ones *not* misbehaving here.
Also note that what we've got here is that the actual input type
for json_populate_record is RECORD, and therefore so is its resolved
output type, and that means you need a coldeflist.

I bisected to see where the behavior changed, and arrived at

Author: Tom Lane <tgl@sss.pgh.pa.us>
Branch: master Release: REL_16_BR [d57534740] 2022-10-16 19:18:08 -0400
Branch: REL_15_STABLE Release: REL_15_1 [d4abb0bc5] 2022-10-16 19:18:08 -0400
Branch: REL_14_STABLE Release: REL_14_6 [8122160ff] 2022-10-16 19:18:08 -0400

    Fix EXPLAIN of SEARCH BREADTH FIRST with a constant initial value.

which surprised me more than a little.  What that patch did was merely
to teach get_expr_result_type() how to extract a tuple descriptor out
of a RECORD-type constant (again, using RECORD in the specific sense
above).  That affects this case because the WITH has been flattened
sufficiently that the function-in-FROM expresssion is just a RECORD
constant, as you can see with EXPLAIN:

regression=# explain verbose with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int,e int,f int);
                      QUERY PLAN                       
-------------------------------------------------------
 Function Scan on c  (cost=0.00..0.01 rows=1 width=44)
   Output: '(1,2,3)'::record, c.d, c.e, c.f
   Function Call: '(1,2,3)'::record
(3 rows)

(If you mark the WITH as MATERIALIZED, the bug goes away because
things aren't flattened so much.)

I believe the problem here is that ExecInitFunctionScan has its
priorities wrong.  It consults the coldeflist (rtfunc->funccolnames
etc) only if get_expr_result_type says TYPEFUNC_RECORD, which it
would have done before this commit but not after.  When the answer
is TYPEFUNC_COMPOSITE we believe the tupdesc extracted from the
expression.  Which essentially means that the later check for
"tupdesc returned by expression matches what the query expects"
is comparing that tupdesc to itself, so it always succeeds.

I think we just need to flip things around so that we build the
expected tupdesc from coldeflist if it's present, and only if not do
we examine the expression.  The cases this might fail to catch should
all have been handled at parse time in addRangeTableEntryForFunction,
so we don't have to check again.

This is a bit scary because it seems plausible that other callers
of get_expr_result_type might've made the same mistake.  I can only
find one other place in core that is doing the same sort of thing:
inline_set_returning_function looks like it has a related bug.
I'm worried about third-party callers though.

Note that reverting d57534740 wouldn't be much of a fix, because
there were already cases where get_expr_result_type could return
TYPEFUNC_COMPOSITE from a nominally-RECORD expression.  Also,
we later back-patched that, meaning that (I think) there are
related hazards in all active branches.  The specific example
given here only fails in branches that will flatten WITH queries,
but I bet you can break it further back with (for example) an
inline-able function that expands to a RowExpr.

I still haven't looked into why the older branches behave differently
for nullif() than coalesce().  Most likely the answer is not very
interesting but just devolves to whether eval_const_expressions
knew how to fold those things to constants.

            regards, tom lane