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

On Sat, Oct 19, 2024 at 2:22 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > Another version which expands the comment of JsonValueExpr a little further.

Thanks for the review.

> I still don't like this approach to eval_const_expressions one bit.
> It's undocumented and it pessimizes as many cases as it optimizes.
> Sure, in the case where we can fold formatted_expr to a constant,
> we avoid processing raw_expr at all --- but if we fail to do that,
> we end by recursively simplifying formatted_expr twice (and raw_expr
> once).  That's not a win.

Ah, I hadn't considered that.  Good point.

> I think it should look more like
>
>         /*
>          * If we can fold formatted_expr to a constant, we can elide
>          * the JsonValueExpr altogether.  Otherwise we must process
>          * raw_expr too.  But JsonFormat is a flat node and requires
>          * no simplification, only copying.
>          */
>         formatted = eval_const_expressions_mutator((Node *) jve->formatted_expr,
>                                                    context);
>         if (formatted && IsA(formatted, Const))
>             return formatted;
>         newnode = makeNode(JsonValueExpr);
>         newnode->formatted_expr = formatted;
>         newnode->raw_expr = eval_const_expressions_mutator((Node *) jve->raw_expr,
>                                                            context);
>         newnode->format = copyObject(jve->format);
>         return newnode;

I decided to use makeJsonValueExpr() here.

> Also, in primnodes.h, personally I'd write
>
> +       Expr       *raw_expr;           /* user-specified expression */
>
> "raw" is a misnomer here, and even if we're not going to rename the
> field, we're not helping anyone by using that word in the comment.

Good point.

Updated patch attached.

-- 
Thanks, Amit Langote