From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Tender Wang <tndrwang@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: exclusion@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault
Date: Wed, 16 Oct 2024 20:56:52 -0400
Message-ID: <508107.1729126612@sss.pgh.pa.us> (raw)
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>
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
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, amitlangote09@gmail.com, tndrwang@gmail.com, 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: <508107.1729126612@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