Feedback-ID: ia2694551:Fastmail
Date: Sat, 5 Apr 2025 17:00:44 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: luy70@psu.edu, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18877: PostgreSQL triggers assertion failure
Message-ID: <202504051500.v6ekizm5fcv4@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <2459196.1743864448@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/202504051500.v6ekizm5fcv4%40alvherre.pgsql>
Precedence: bulk

On 2025-Apr-05, Tom Lane wrote:

> 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 ...")

Right.

> 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.

Oh, of course.  I failed to realize this at the time.

> 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.

Sounds reasonable, I can try to find better coding for this, but it
won't be soon.  I won't be sad if somebody else wants to do it, so if
you feel like it, please be my guest.

> 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.

Sounds reasonable.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Crear es tan difícil como ser libre" (Elsa Triolet)