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

On Fri, Oct 18, 2024 at 11:04 AM Amit Langote <amitlangote09@gmail.com> wrote:
> 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.

Here's a version with an updated commit message and the comments of
JsonValueExpr struct definition updated to describe raw_expr and
formatted_expr more clearly.

-- 
Thanks, Amit Langote