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: <508107.1729126612@sss.pgh.pa.us>
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>
 <508107.1729126612@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 16 Oct 2024 20:56:52 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <569212.1729138329.1@sss.pgh.pa.us>
Date: Thu, 17 Oct 2024 00:12:09 -0400
Message-ID: <569213.1729138329@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/569213.1729138329%40sss.pgh.pa.us>
Precedence: bulk

I wrote:
> 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.

To expand on that: the proximate cause of the bug is that
ExecBuildAggTrans is expecting that expression initialization found
an Aggref for every aggtransno index.  In the plan as emitted by
the planner, there are two json_object_agg Aggref nodes, one in
the JsonValueExpr's raw_expr and the other in the formatted_expr.
preprocess_aggrefs() assigned aggno 0 and aggtransno 0 to the
first one, aggno 1 and aggtransno 1 to the second.  As already
noted upthread, the core of the problem is that ExecInitExprRec's
T_JsonValueExpr case is not recursing into raw_expr, so that
only the second Aggref gets found and linked into the parent
AggState, thus leaving a hole in the per-transno array.

The proposed patch fixes that indirectly by lobotomizing
expression_tree_walker so that preprocess_aggrefs doesn't
find the raw_expr's Aggref either.  However, the side effects
of that will be spectacularly unpleasant, because there are
approximately zero other callers of expression_tree_walker
that will be pleased with it.

In the short term, I suspect the only workable fix is to undo the
optimization of having ExecInitExprRec not recurse into both raw_expr
and formatted_expr.  In the longer run, I'd look hard at why the
tree needs to have two copies of that subexpression at all.
At least in this example, it appears that the only difference is
a type coercion expression wrapped around the formatted_expr, and
there are surely better ways to handle that.

BTW, the reason that using a volatile function matters is that
that stops find_compatible_agg from deciding that the two identical
Aggrefs can be given the same aggno and aggtransno, which masks
the bug in another way.  That also means that the planner thinks
the two Aggrefs represent distinct computations.  At minimum that
messes up our estimates of aggregate evaluation costs, and there
might be more subtle semantic consequences.

I remain concerned about whether it's really okay to skip raw_expr
in eval_const_expressions_mutator, too.

			regards, tom lane