From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
cc: Tender Wang <tndrwang@gmail.com>, Michael Paquier <michael@paquier.xyz>,
        exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to
 segfault
In-reply-to: 
 <CA+HiwqGZV2YWvso8B4bpPAEJDuEhDos3oUTxDTfr9kpX=nOAgA@mail.gmail.com>
References: <18657-1b90ccce2b16bdb8@postgresql.org>
 <Zw8Gk3xtrSmQh9mD@paquier.xyz>
 <CA+HiwqFpzUYXOqcJ4LdAoLjAxDJ-e4-mVe=4z9-jC5vhV0ZwEg@mail.gmail.com>
 <CAHewXNmSvzsSqqf6fG7OEkmB2aUuCa7SM4pfVrs9vBeCWZu7yQ@mail.gmail.com>
 <CA+HiwqGDZMwi6O9w64vtd0fFTeow4GnKfWraSaJFxC-y7mCm+w@mail.gmail.com>
 <CAHewXN=hHQepA9-JHaW9z-orPE-qA2UEFNvAvLpYZr6e1PGq2Q@mail.gmail.com>
 <CA+HiwqF0iihKd28ycs5BJcYprAWUBDqHv03vqLMteBid2YDCyA@mail.gmail.com>
 <CAHewXNnc35-OX=wA+MHkj+asYa0oezLwV5by=D00ga+EawN7Fg@mail.gmail.com>
 <CA+HiwqGJUv2Y=qCRO1kL-6cfZ79zAgag_zJ_=U4q-RaNUv9r2g@mail.gmail.com>
 <71060.1729088799@sss.pgh.pa.us>
 <CA+HiwqGZV2YWvso8B4bpPAEJDuEhDos3oUTxDTfr9kpX=nOAgA@mail.gmail.com>
Comments: In-reply-to Amit Langote <amitlangote09@gmail.com>
	message dated "Thu, 17 Oct 2024 09:38:04 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-ID: <508106.1729126612.1@sss.pgh.pa.us>
Content-Transfer-Encoding: 8bit
Date: Wed, 16 Oct 2024 20:56:52 -0400
Message-ID: <508107.1729126612@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/508107.1729126612%40sss.pgh.pa.us>
Precedence: bulk

Amit Langote <amitlangote09@gmail.com> writes:
> On Wed, Oct 16, 2024 at 11:26 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Would it be better for parse analysis to explicitly NULL out this
>> field once it's done looking at it?  Carrying unmaintained pieces
>> of an expression tree around seems both inefficient and prone to
>> future failures of this same ilk.  The further the raw_expr gets
>> out of step with current reality, the worse the hazards.

> It seems we can't do that in this case, because get_rule_expr() wants
> to read it.

I think you just increased my level of concern.  Aren't you saying
that get_rule_expr is likely to deliver utter garbage, because it
will be working from an expression that has not tracked
transformations made to other parts of the tree?

Also, I see that ruleutils is applying get_rule_expr to raw_expr,
which AFAICT means it's *not* raw-parser output, else that wouldn't
work at all.  Which means the field is badly named and incorrectly
documented.  But in particular it means you cannot just make the
tree-walking routines ignore it --- you *must* update it during
transformations such as subquery pullup, or you will have garbage
that will cause EXPLAIN to crash or at least produce wrong results.

In other words, the proposed patch is dangerously wrong.  I suspect at
this point that the true bug might be the opposite: somebody feeling
that they could dispense with updating raw_expr when they shouldn't.
In particular, I think "... is not evaluated by
eval_const_expressions_mutator()" is already a huge red flag.
There are transformations that eval_const_expressions does that
are not optional.

			regards, tom lane