Re: BUG #18657: Using JSON_OBJECTAGG with volatile function leads to segfault

On Fri, Oct 18, 2024 at 5:30 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Oct 17, 2024 at 1:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 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.
>
> > This or actually I'm tempted to simply revert the whole thing
> > (b6e1157e7d3) as an ill-considered refactoring, because I am not able
> > to convince myself that calling ExecAggPlainTransByVal() twice, via
> > both raw_expr and formatted_expr, is always safe.
>
> Not following the concern here?  As far as nodeAgg is concerned,
> they'd be two independent aggregates.

Ok, thanks for clarifying that.  I can see that both instances of the
Aggref contain identical values in this case and only the latter gets
*used*.

> In any case, whatever we do in master, you can't "simply revert"
> b6e1157e7 in released branches.  It changed the way JsonValueExpr is
> represented in stored rules, and you don't get to undo that midstream.
> (The commit should have included a catversion bump, in fact.)

While I think I understand the general rule around when to bump the
catversion, I’m not entirely sure how this particular change or its
reversion would break anything.  get_rule_expr() doesn’t, and didn’t
previously, inspect formatted_expr, which is where the content will
change.  That said, I will err on the side of following the general
rule here. :-)

> I do think that reverting the ExecInitExprRec and
> eval_const_expressions_mutator changes would be good,

What was there in eval_const_expressions_mutator() before b6e1157e was
there because only raw_expr contained the actual expression and
formatted_expr only a wrapper for coercion, but that's not true after
b6e1157e.  So I don't think we can revert
eval_const_expressions_mutator() changes.  The expression tree in
raw_expr does get the eval_const_expressions_mutator() treatment via
ece_generic_processing() at the bottom.

> but we can't undo the parser changes, at least not in v16/v17.

Ok, understood.

I think we can apply the attached down to v16.


--
Thanks, Amit Langote