From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: luy70@psu.edu, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18877: PostgreSQL triggers assertion failure
In-reply-to: <202504051303.fhpbodkciqry@alvherre.pgsql>
References: <202504051303.fhpbodkciqry@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Sat, 05 Apr 2025 15:03:08 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2459195.1743864448.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sat, 05 Apr 2025 10:47:28 -0400
Message-ID: <2459196.1743864448@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2459196.1743864448%40sss.pgh.pa.us>
Precedence: bulk

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2025-Apr-04, Tom Lane wrote:
>> Is it really necessary to transform the subquery twice just to
>> produce this error?

> Ugh, yeah, this is really dumb.  This quick-and-dirty patch (not final
> form) gets us halfway there, by checking the targetlist after
> transforming the query.  I think this is the behavior we want, although
> it's a bit scary that we have so few test cases for this.

Yeah, I spent some time looking at this and came to similar
conclusions: it's not easy to produce the same error without
a significant investment of work.  (For example, determining
the targetlist length pre-transformation seems impractical
because it could be "SELECT * FROM ...")

However, after looking around there is more not to like about this
code, as demonstrated by this example from the regression tests:

regression=3D# CREATE VIEW json_array_subquery_view AS
regression-# SELECT JSON_ARRAY(SELECT i FROM (VALUES (1), (2), (NULL), (4)=
) foo(i) RETURNING jsonb);
CREATE VIEW
regression=3D# \sv json_array_subquery_view
CREATE OR REPLACE VIEW public.json_array_subquery_view AS
 SELECT ( SELECT JSON_ARRAYAGG(q.* RETURNING jsonb) AS "json_arrayagg"
           FROM ( SELECT foo.i
                   FROM ( VALUES (1), (2), (NULL::integer), (4)) foo(i)) q=
(a)) AS "json_array"

That is, we are exposing the implementation of json_array() in
a way that makes it impossible to change.  If we ever discover
that json_array() isn't exactly equivalent to this json_arrayagg()
invocation, we'll be in deep trouble.  We've expended a lot of
sweat in the past to avoid exposing implementations this way,
eg 40c24bfef, fb32748e3.

So what we should be doing is building a parse-analysis result
that deparses into something that looks like the input; probably,
a JsonArrayQueryConstructor node with an analyzed EXPR_SUBLINK
SubLink below it.  Then we can make this tlist-length check against
the analyzed SubLink, removing the problem of premature errors that
are not spelled the way we want.

We could still transform to a json_arrayagg call for execution,
but it would have to be done during rewriting or planning.
Or maybe it'd be easier to just write executor support that
shares code somehow with json_arrayagg.

Anyway, that idea is far too invasive to be back-patchable,
and IMO it's too late to consider even getting it into v18.
So what I'm thinking is we should just apply the copyObject
hack for now, and resolve to reconsider this code later.

			regards, tom lane