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: 
 <CA+HiwqGWJDVEscvUuXsUhFw6_=T+MbdbKVLb_NxtH4byJoMiJg@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>
 <508107.1729126612@sss.pgh.pa.us> <569213.1729138329@sss.pgh.pa.us>
 <CA+HiwqHpU4VARMpEih6O5xd+psvODEzutf8Rkx7m+sU8OPV-bA@mail.gmail.com>
 <711200.1729197017@sss.pgh.pa.us>
 <CA+HiwqHyYCb2XbyCwHjP+atoscATGeWrXmdcwEOLY0mGYL3UwA@mail.gmail.com>
 <CA+HiwqHPMcVQWB_9JEfWvdAww8P4QUntfww-bDT7_hUtkrNEjQ@mail.gmail.com>
 <CA+HiwqG05q!
 j0Z7m2KQk3m2hvA1JsbHfEKevYfWK68umK90beow@mail.gmail.com>
 <CA+HiwqGWJDVEscvUuXsUhFw6_=T+MbdbKVLb_NxtH4byJoMiJg@mail.gmail.com>
Comments: In-reply-to Amit Langote <amitlangote09@gmail.com>
	message dated "Fri, 18 Oct 2024 15:14:06 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <907537.1729272125.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 18 Oct 2024 13:22:05 -0400
Message-ID: <907538.1729272125@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/907538.1729272125%40sss.pgh.pa.us>
Precedence: bulk

Amit Langote <amitlangote09@gmail.com> writes:
> Another version which expands the comment of JsonValueExpr a little furt=
her.

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.

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 =3D eval_const_expressions_mutator((Node *) jve->formatt=
ed_expr,
                                                   context);
        if (formatted && IsA(formatted, Const))
            return formatted;
        newnode =3D makeNode(JsonValueExpr);
	newnode->formatted_expr =3D formatted;
	newnode->raw_expr =3D eval_const_expressions_mutator((Node *) jve->raw_ex=
pr,
                                                           context);
        newnode->format =3D copyObject(jve->format);
	return newnode;

(Untested, probably requires more casts than I wrote.)

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.

			regards, tom lane