Record returning function accept not matched columns declaration

On Thursday, February 29, 2024, PetSerAl <petseral@gmail.com> wrote:

postgres=# SHOW SERVER_VERSION;
server_version
----------------
16.2
(1 row)

postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, coalesce(b) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, nullif(b, null) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, unnest(array[b]) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, json_populate_record(b, null) as c(d int,e
int, f int); --expect OK
b | d | e | f
---------+---+---+---
(1,2,3) | 1 | 2 | 3
(1,2,3) | 1 | 2 | 3
(1,2,3) | 1 | 2 | 3
(1,2,3) | 1 | 2 | 3
(4 rows)

My concern with all of this is accepting the specification of column
definitions for functions that don’t return the record pseudo-type. This
seems like it should be a syntax error, or better documented how it should
behave. Specifically, for this bug report to make sense, one must assume
that the specification of a column definition composite on a non-record
result must match the actual structure that is seen. The syntax is not a
means to implement a cast of a row-typed value. The fact the last two
examples implement a cast is a bug. One that apparently doesn’t manifest
for set-returning functions in the from clause but does manifest for
non-SRF laterally applied functions.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Thursday, February 29, 2024, PetSerAl <petseral@gmail.com> wrote:

postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, coalesce(b) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, nullif(b, null) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, unnest(array[b]) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, json_populate_record(b, null) as c(d int,e
int, f int); --expect OK

My concern with all of this is accepting the specification of column
definitions for functions that don’t return the record pseudo-type.

Hm? These cases all *do* return record, because that's what a.b is
typed as.

It looks to me like we broke these edge cases in refactoring.
v11 gives me the expected errors:

regression=# with a(b) as (values (row(1,2,3)))
select * from a, nullif(b, null) as c(d int,e int);
ERROR: function return row and query-specified return row do not match
DETAIL: Returned row contains 3 attributes, but query expects 2.
regression=# with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int,e int);
ERROR: function return row and query-specified return row do not match
DETAIL: Returned row contains 3 attributes, but query expects 2.

v12 rejects the nullif example but not coalesce. v14 and up lets them
both by. I think the v14 change has to do with the function calls
having got flattened to constants, but I've not fingered the exact
culprit yet. I didn't look yet for the earlier behavioral change.

Even more amusing, since v14:

regression=# with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int,e int,f int, g int);
server closed the connection unexpectedly

I think that Assert is just wrong and can be removed, though;
it doesn't seem to be guarding anything of interest.

regards, tom lane

On Thu, Feb 29, 2024 at 1:11 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Thursday, February 29, 2024, PetSerAl <petseral@gmail.com> wrote:

postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, coalesce(b) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, nullif(b, null) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, unnest(array[b]) as c(d int,e int, f int)
postgres-# union all
postgres-# select * from a, json_populate_record(b, null) as c(d int,e
int, f int); --expect OK

My concern with all of this is accepting the specification of column
definitions for functions that don’t return the record pseudo-type.

Hm? These cases all *do* return record, because that's what a.b is
typed as.

I strongly dislike the seemingly overloaded terminology in this area.
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.

David J.

On Thu, Feb 29, 2024 at 2:31 PM David G. Johnston <
david.g.johnston@gmail.com> wrote:

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.

Hit send too soon...

I suppose this entire query form is basically a hack around the fact that
we have no syntax to directly assign names to the fields of an "anonymous
record type" literal.

with a(b) as (values (row(1,2,3)))
select (a.b).* from a;
ERROR: record type has not been registered

Though oddly this doesn't seem to be universal:

with a(b) as (values (row(1,2,3)))
select (row(c.*)).* from a, coalesce(a.b) as c (d int, e int, f int);
f1, f2, and f3 end up being the output field names...

David J.

postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, coalesce(b);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, coalesce(b);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, nullif(b, null);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, nullif(b, null);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, unnest(array[b]);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, unnest(array[b]);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3)))
postgres-# select * from a, json_populate_record(b, null);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, json_populate_record(b, null);
^
postgres=#

It seems PostgreSQL does not care about function being polymorphic,
but only about return type being "record". It explicitly require
column definition list in all this cases.

пт, 1 мар. 2024 г. в 00:32, David G. Johnston <david.g.johnston@gmail.com>:

I had this issue as well.

There is much inet talk about a bug in PGadmin in creating functions v cli, and how pgAdmin doesn't like some functions.

I had to change my function to this method to get out of the error.

The error wants you to have the columns in the function, not at SELECT statement.

at least this was my issue, your mileage may vary.

DROP FUNCTION IF EXISTS schema.function_name;

CREATE OR REPLACE FUNCTION schema.function_name;(
_typeahead character varying,
_rolename character varying,
_requiremisconductaccess character varying,
_test1 character varying,
_test2 character varying,
_test3 character varying)
RETURNS TABLE(col1 character varying, col2 character varying, col3 character varying)
LANGUAGE 'plpgsql'
COST 100
VOLATILE PARALLEL UNSAFE
ROWS 1000

AS $BODY$
BEGIN

-----Original Message-----
From: PetSerAl <petseral@gmail.com>
Sent: Thursday, February 29, 2024 2:02 PM
To: David G. Johnston <david.g.johnston@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs@lists.postgresql.org
Subject: [EXTERNAL] Re: Record returning function accept not matched columns declaration

postgres=# with a(b) as (values (row(1,2,3))) postgres-# select * from a, coalesce(b);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, coalesce(b);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3))) postgres-# select * from a, nullif(b, null);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, nullif(b, null);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3))) postgres-# select * from a, unnest(array[b]);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, unnest(array[b]);
^
postgres=#
postgres=# with a(b) as (values (row(1,2,3))) postgres-# select * from a, json_populate_record(b, null);
ERROR: a column definition list is required for functions returning "record"
LINE 2: select * from a, json_populate_record(b, null);
^
postgres=#

It seems PostgreSQL does not care about function being polymorphic, but only about return type being "record". It explicitly require column definition list in all this cases.

пт, 1 мар. 2024 г. в 00:32, David G. Johnston <david.g.johnston@gmail.com>:

"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

I wrote:

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.

Here's a draft patch that fixes it that way.

I'm having mixed feelings about whether to back-patch this. Somebody
might complain that we broke a working query in a minor release.
Assuming that the visible consequences are all pretty benign, as they
seem to be (noting that end-users won't see the assertion failure),
maybe the conservative course is to leave it unfixed in stable
branches. However, I'm not totally convinced that the consequences
are all benign, so there would be a risk on that side. Thoughts?

regards, tom lane

On Sat, Mar 2, 2024 at 1:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

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.

Here's a draft patch that fixes it that way.

I'm having mixed feelings about whether to back-patch this. Somebody
might complain that we broke a working query in a minor release.

context: in postgres 9.3.25, dbfiddle[1]https://dbfiddle.uk/4SrE-1JR
this query will fail:
`
with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int, e int);
`

+ * Note that if the function returns a named composite type, that may
+ * now contain more or different columns than it did when the plan was
+ * made.  For both that and the RECORD case, we need to check tuple
+ * compatibility.  ExecMakeTableFunctionResult handles some of this,
+ * and CheckVarSlotCompatibility provides a backstop.
  */

I think by ExecMakeTableFunctionResult you mean `mainly
ExecMakeTableFunctionResult's function: tupledesc_match`
since ExecMakeTableFunctionResult is quite long.

also looking around the code, `ExecMakeTableFunctionResult handles
some of this,`
actually is `ExecMakeTableFunctionResult handles most of this`?

So overall, I think `ExecMakeTableFunctionResult's inner function,
tupledesc_match handle most of this`
would be more accurate.

[1]: https://dbfiddle.uk/4SrE-1JR

jian he <jian.universality@gmail.com> writes:

On Sat, Mar 2, 2024 at 1:56 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I'm having mixed feelings about whether to back-patch this. Somebody
might complain that we broke a working query in a minor release.

context: in postgres 9.3.25, dbfiddle[1]
this query will fail:

with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int, e int);

Sure, but in v12 and up (i.e., the supported versions that we might
back-patch into) the query succeeds, and it might not be obvious
to someone that its behavior isn't as-intended.

Admittedly, this seems like a rather contrived case. In cases
where the record-returning function call isn't reducible to a
constant, you'll get the expected error. So maybe I'm worrying
over something that no one is depending on in practice.

I think by ExecMakeTableFunctionResult you mean `mainly
ExecMakeTableFunctionResult's function: tupledesc_match`
since ExecMakeTableFunctionResult is quite long.

Perhaps. I mentioned ExecMakeTableFunctionResult because that's
what's called directly from nodeFunctionscan.c. Referencing a
static function of another module is risky because it's pretty
unlikely the comment would get updated if refactoring in that
module makes it false.

regards, tom lane

"David G. Johnston" <david.g.johnston@gmail.com> writes:

with a(b) as (values (row(1,2,3)))
select (a.b).* from a;
ERROR: record type has not been registered

I looked into this case. The failure occurs when the parser tries
to expand the ".*" notation into separate output columns, as required
per SQL spec. All it has is a Var of type RECORD referencing the
VALUES RTE entry, so it has to fail.

In the specific example given here, you could imagine drilling down
into the VALUES and figuring out what concrete rowtype the RECORD
value will have at runtime, but I'm not excited about going there.
There are too many cases where it wouldn't work.

Note that as long as you don't need parse-time expansion of the
record, it works:

=# with a(b) as (values (row(1,2,3)))
select a.b from a;
b
---------
(1,2,3)
(1 row)

I pushed the patch for the original problem. After debating with
myself I concluded that back-patching it was probably less risky
than not back-patching, so I did that.

regards, tom lane