From: Tom Lane <tgl@sss.pgh.pa.us>
To: "David E. Wheeler" <david@justatheory.com>
cc: Erik Wienhold <ewie@ewie.name>, jian he <jian.universality@gmail.com>,
        Jim Jones <jim.jones@uni-muenster.de>,
        =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>,
        pgsql-hackers@lists.postgresql.org,
        Pavel Stehule <pavel.stehule@gmail.com>
Subject: Re: Patch: Add parse_type Function
In-reply-to: <F445E541-BAFB-4E29-B5BF-ECD1A014E952@justatheory.com>
References: <DF2324CA-2673-4ABE-B382-26B5770B6AA3@justatheory.com> <c1f10523-1d0c-4bf2-b45d-1b903a9315d2@uni-muenster.de> <87il33kmmn.fsf@wibble.ilmari.org> <dbed3b98-3d86-41e4-bded-b15016f122dc@uni-muenster.de> <505981B3-2572-41E0-896C-964DDF7048A9@justatheory.com> <CACJufxHS2ULcDvZJagS=RVo=Hqq_TC-jOG0euVdKvJ-DkcKtuw@mail.gmail.com> <1317430675.73641.1707616376477@office.mailbox.org> <F445E541-BAFB-4E29-B5BF-ECD1A014E952@justatheory.com>
Comments: In-reply-to "David E. Wheeler" <david@justatheory.com>
	message dated "Mon, 12 Feb 2024 11:37:46 -0500"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2245609.1707760389.1@sss.pgh.pa.us>
Date: Mon, 12 Feb 2024 12:53:09 -0500
Message-ID: <2245610.1707760389@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/2245610.1707760389%40sss.pgh.pa.us>
Precedence: bulk

"David E. Wheeler" <david@justatheory.com> writes:
> [ v4-0001-Add-parse_type-SQL-function.patch ]

It strikes me that this is basically to_regtype() with the additional
option to return the typmod.  That leads to some questions:

* Should we choose a name related to to_regtype?  I don't have any
immediate suggestions, but since you didn't seem entirely set on
parse_type, maybe it's worth thinking along those lines.  OTOH,
to the extent that people would use this with format_type, maybe
parse_type is fine.

* Perhaps the type OID output argument should be declared regtype
not plain OID?  It wouldn't make any difference for passing it to
format_type, but in other use-cases perhaps regtype would be more
readable.  It's not a deal-breaker either way of course, since
you can cast oid to regtype or vice versa.

* Maybe the code should be in adt/regproc.c not format_type.c.

* Experience with to_regtype suggests strongly that people would
prefer "return NULL" to failure for an unrecognized type name.


Skimming the patch, I notice that the manual addition to
builtins.h should be unnecessary: the pg_proc.dat entry
should be enough to create an extern in fmgrprotos.h.
Also, I'm pretty sure that reformat_dat_file.pl will
think your pg_proc.dat entry is overly verbose.  See
https://www.postgresql.org/docs/devel/system-catalog-initial-data.html

			regards, tom lane