From: Tom Lane <tgl@sss.pgh.pa.us>
To: Dmitry Dolgov <9erthalion6@gmail.com>
cc: Michael Paquier <michael@paquier.xyz>, yarexeray@gmail.com,
        pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15940: json_populate_recordset fails with ERROR: record type
 has not been registered
In-reply-to: 
 <CA+q6zcVYyjDEh9hkKi3eG81TBghMRjVySNA4gAsQU0q4gJPbFQ@mail.gmail.com>
References: <15940-2ab76dc58ffb85b6@postgresql.org>
 <20190806073209.GI32256@paquier.xyz>
 <CA+q6zcVYyjDEh9hkKi3eG81TBghMRjVySNA4gAsQU0q4gJPbFQ@mail.gmail.com>
Comments: In-reply-to Dmitry Dolgov <9erthalion6@gmail.com>
	message dated "Tue, 06 Aug 2019 13:34:51 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <5153.1565120306.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Tue, 06 Aug 2019 15:38:26 -0400
Message-ID: <5154.1565120306@sss.pgh.pa.us>
Precedence: bulk

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> On Tue, Aug 6, 2019 at 9:32 AM Michael Paquier <michael@paquier.xyz> wro=
te:
>> 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=3D# SELECT * FROM json_populate_recordset(null::record, '[{"id_=
item":776}]');
ERROR:  a column definition list is required for functions returning "reco=
rd"

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=3D# 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