BUG #15940: json_populate_recordset fails with ERROR: record type has not been registered

On Tue, Aug 06, 2019 at 06:41:34AM +0000, PG Bug reporting form wrote:

Following query works fine in previous freebsd versions

SELECT
id_item
FROM json_populate_recordset(null::record, '[{"id_item":776}]')
AS
(
id_item int
);

This visibly is a regression between 11 and 10, and one bisect later
here is the culprit:
commit: 37a795a60b4f4b1def11c615525ec5e0e9449e05
author: Tom Lane <tgl@sss.pgh.pa.us>
date: Thu, 26 Oct 2017 13:47:45 -0400
Support domains over composite types.
--
Michael

On Tue, Aug 6, 2019 at 9:32 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Aug 06, 2019 at 06:41:34AM +0000, PG Bug reporting form wrote:

Following query works fine in previous freebsd versions

SELECT
id_item
FROM json_populate_recordset(null::record, '[{"id_item":776}]')
AS
(
id_item int
);

This visibly is a regression between 11 and 10, and one bisect later
here is the culprit:
commit: 37a795a60b4f4b1def11c615525ec5e0e9449e05
author: Tom Lane <tgl@sss.pgh.pa.us>
date: Thu, 26 Oct 2017 13:47:45 -0400
Support domains over composite types.

Looks like there are tests for such behaviour with exactly this error, is there
a chance it was a feature? But anyway before this change there was an
invocation of get_call_result_type in the branch

if (have_record_arg && PG_ARGISNULL(0))

Adding similar stuff to the current implementation make this error to disappear
for me and passes tests (except those where we actually expect this error):

--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -3647,7 +3647,19 @@ populate_recordset_worker(FunctionCallInfo
fcinfo, const char *funcname,
         }
     }
     else
+    {
+        TupleDesc    tupdesc;
+
          rec = NULL;
+        if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+            ereport(ERROR,
+                    (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                     errmsg("function returning record called in context "
+                            "that cannot accept type record")));
+
+        cache->c.io.composite.base_typid = tupdesc->tdtypeid;
+        cache->c.io.composite.base_typmod = tupdesc->tdtypmod;
+    }

/* if the json is null send back an empty set */
if (PG_ARGISNULL(json_arg_num))

Dmitry Dolgov <9erthalion6@gmail.com> writes:

On Tue, Aug 6, 2019 at 9:32 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Aug 06, 2019 at 06:41:34AM +0000, PG Bug reporting form wrote:

Following query works fine in previous freebsd versions

SELECT
id_item
FROM json_populate_recordset(null::record, '[{"id_item":776}]')
AS
(
id_item int
);

Looks like there are tests for such behaviour with exactly this error,
is there a chance it was a feature?

Yeah, my first reaction to this bug report was "didn't we fix this
already?" --- it's real close to some other behaviors we fixed in
that code.

In an ideal world we'd decide that this query is wrong and we should
not support it. The point of json_populate_recordset is to take the
result rowtype from its first argument; if you want to take the result
rowtype from the call context, you should be using json_to_recordset.
I really don't like semantics as squishy as "we'll take the result
type from the first argument except if it's exactly a null of type
RECORD, and then we'll look somewhere else for the type". Yeah, it
worked that way before v11, but IMO that was a bug.

Now, it's surely true that if we are going to take that attitude we
ought to throw some better error about it than "record type has not been
registered"; that's my fault for not thinking harder about the UX.
(I think that there are some related cases where < v11 already
threw that error, and it seemed sufficient to keep on doing so.)

However, the bigger part of the UX problem is that if you leave off
the AS, you get

regression=# SELECT * FROM json_populate_recordset(null::record, '[{"id_item":776}]');
ERROR: a column definition list is required for functions returning "record"

which is a bit misleading; the parser is seeing that the resolved
polymorphic result type of json_populate_recordset is just "record"
and complaining about that. Ideally it would counsel that you should
use json_to_recordset plus an AS clause, but I don't see any very sane
way to do that. If people do what the error tells them to, and it
still doesn't work, they're not being unreasonable to complain.

Maybe we have to stay bug-compatible with the old behavior just
because we can't generate a reasonable error report. But ugh.

A related issue that it'd be nice to have a better answer for
is "what happens if the two type-info sources conflict?"
For example,

regression=# SELECT
id_item
FROM json_populate_recordset(row(1,2), '[{"id_item":776}]')
AS
(
id_item int
);
ERROR: function return row and query-specified return row do not match
DETAIL: Returned row contains 2 attributes, but query expects 1.

This is clearly pilot error, but it could be pretty confusing.

Thoughts?

regards, tom lane

On Tue, Aug 6, 2019 at 2:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Dmitry Dolgov <9erthalion6@gmail.com> writes:

On Tue, Aug 6, 2019 at 9:32 AM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Aug 06, 2019 at 06:41:34AM +0000, PG Bug reporting form wrote:

Following query works fine in previous freebsd versions

SELECT
id_item
FROM json_populate_recordset(null::record, '[{"id_item":776}]')
AS
(
id_item int
);

Looks like there are tests for such behaviour with exactly this error,
is there a chance it was a feature?

Yeah, my first reaction to this bug report was "didn't we fix this
already?" --- it's real close to some other behaviors we fixed in
that code.

In an ideal world we'd decide that this query is wrong and we should
not support it. The point of json_populate_recordset is to take the
result rowtype from its first argument; if you want to take the result
rowtype from the call context, you should be using json_to_recordset.
I really don't like semantics as squishy as "we'll take the result
type from the first argument except if it's exactly a null of type
RECORD, and then we'll look somewhere else for the type". Yeah, it
worked that way before v11, but IMO that was a bug.

Now, it's surely true that if we are going to take that attitude we
ought to throw some better error about it than "record type has not been
registered"; that's my fault for not thinking harder about the UX.
(I think that there are some related cases where < v11 already
threw that error, and it seemed sufficient to keep on doing so.)

still does.

postgres=# select version();
version
─────────────────────────────────────────────────────────────────────────────────────────────────────────
PostgreSQL 11.1 on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5
20150623 (Red Hat 4.8.5-28), 64-bit
(1 row)

postgres=# select (row(1,2,3)).*;
ERROR: record type has not been registered

For posterity I agree that OP was essentially exploiting undefined, or
at least poorly defined, behavior. For my money, I'd advise using
this function for cases where you don't want to use an in place type,
just a column list:

postgres=# SELECT * from json_to_recordset('[{"id_item":776}]') as
(id_item int);
id_item
─────────
776

merlin

On Tue, Aug 06, 2019 at 02:53:45PM -0500, Merlin Moncure wrote:

For posterity I agree that OP was essentially exploiting undefined, or
at least poorly defined, behavior. For my money, I'd advise using
this function for cases where you don't want to use an in place type,
just a column list:

If I were to change something here, that would be this error string
exposed to the user. With the current error, there is no real way
that the user knows what is wrong and why he/she should not do that.
Could it be possible to add some recommendation for example on top of
an error "cannot do that because blah"?
--
Michael

On Thu, Aug 8, 2019 at 9:35 PM Michael Paquier <michael@paquier.xyz> wrote:

On Tue, Aug 06, 2019 at 02:53:45PM -0500, Merlin Moncure wrote:

For posterity I agree that OP was essentially exploiting undefined, or
at least poorly defined, behavior. For my money, I'd advise using
this function for cases where you don't want to use an in place type,
just a column list:

If I were to change something here, that would be this error string
exposed to the user. With the current error, there is no real way
that the user knows what is wrong and why he/she should not do that.
Could it be possible to add some recommendation for example on top of
an error "cannot do that because blah"?

Good question. Maybe something like, "ERROR: Unable to expand generic
rowtype. HINT: Try using explicit cast on rowtype"

merlin

Michael Paquier <michael@paquier.xyz> writes:

On Tue, Aug 06, 2019 at 02:53:45PM -0500, Merlin Moncure wrote:

For posterity I agree that OP was essentially exploiting undefined, or
at least poorly defined, behavior. For my money, I'd advise using
this function for cases where you don't want to use an in place type,
just a column list:

If I were to change something here, that would be this error string
exposed to the user. With the current error, there is no real way
that the user knows what is wrong and why he/she should not do that.
Could it be possible to add some recommendation for example on top of
an error "cannot do that because blah"?

I concluded that we'd better restore the former behavior, or people
will complain that this is a regression. We can however do better
than the "record type has not been registered" error, at least for
the case where the error is being thrown at runtime. I went with

ERROR: could not determine row type for result of json_populate_record
HINT: Provide a non-null record argument, or call the function in the FROM clause using a column definition list.

regards, tom lane