From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: luy70@psu.edu
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18877: PostgreSQL triggers assertion failure
Date: Sat, 05 Apr 2025 10:47:28 -0400
Message-ID: <2459196.1743864448@sss.pgh.pa.us> (raw)
In-Reply-To: <202504051303.fhpbodkciqry@alvherre.pgsql>
References: <202504051303.fhpbodkciqry@alvherre.pgsql>
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=# 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=# \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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, alvherre@alvh.no-ip.org, luy70@psu.edu, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18877: PostgreSQL triggers assertion failure
In-Reply-To: <2459196.1743864448@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox