From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Amit Langote <amitlangote09@gmail.com>,
        jian he <jian.universality@gmail.com>,
        Peter Eisentraut <peter@eisentraut.org>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add more SQL/JSON constructor functions
In-reply-to: <202406291824.reofujy7xdj3@alvherre.pgsql>
References: <202406291824.reofujy7xdj3@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Sat, 29 Jun 2024 20:24:27 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2232737.1719687375.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sat, 29 Jun 2024 14:56:15 -0400
Message-ID: <2232738.1719687375@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2232738.1719687375%40sss.pgh.pa.us>
Precedence: bulk

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> +	/*
>> +	 * For domains, consider the base type's typmod to decide whether to =
setup
>> +	 * an implicit or explicit cast.
>> +	 */
>> +	if (get_typtype(returning->typid) =3D=3D TYPTYPE_DOMAIN)
>> +		(void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);

> TBH I'm not super clear on why we decide on explicit or implicit cast
> based on presence of a typmod.  Why isn't it better to always use an
> implicit one?

Hmm ... there are a bunch of existing places that seem to have similar
logic, but they are all in new-ish SQL/JSON functionality, and I would
not be surprised if they are all wrong.  parse_coerce.c is quite
opinionated about what a domain's typtypmod means (see comments in
coerce_type() for instance); see also the logic in coerce_to_domain:

     * If the domain applies a typmod to its base type, build the appropri=
ate
     * coercion step.  Mark it implicit for display purposes, because we d=
on't
     * want it shown separately by ruleutils.c; but the isExplicit flag pa=
ssed
     * to the conversion function depends on the manner in which the domai=
n
     * coercion is invoked, so that the semantics of implicit and explicit
     * coercion differ.  (Is that really the behavior we want?)

I don't think that this SQL/JSON behavior quite matches that.

While I'm bitching ... this coding style is bogus anyway:

>> +	if (get_typtype(returning->typid) =3D=3D TYPTYPE_DOMAIN)
>> +		(void) getBaseTypeAndTypmod(returning->typid, &baseTypmod);

because it results in two syscache lookups not one.  You are supposed
to apply getBaseTypeAndTypmod unconditionally, as is done everywhere
except in the SQL/JSON logic.  I am also wondering how it can possibly
be sensible to throw away the function result of getBaseTypeAndTypmod
in this context.

			regards, tom lane