Date: Sun, 11 Feb 2024 02:52:56 +0100 (CET)
From: Erik Wienhold <ewie@ewie.name>
To: jian he <jian.universality@gmail.com>,
	"David E. Wheeler" <david@justatheory.com>
Cc: 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>
Message-ID: <1317430675.73641.1707616376477@office.mailbox.org>
In-Reply-To: <CACJufxHS2ULcDvZJagS=RVo=Hqq_TC-jOG0euVdKvJ-DkcKtuw@mail.gmail.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>
Subject: Re: Patch: Add parse_type Function
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Importance: Normal
Archived-At: <https://www.postgresql.org/message-id/1317430675.73641.1707616376477%40office.mailbox.org>
Precedence: bulk

Let me comment on some issues since I wrote the very first version of
parse_type() on which David's patch is based.

> On 2024-02-01 01:00 +0100, jian he <jian.universality@gmail.com> wrote:
> 
> cosmetic issue. Looking around other error handling code, the format
> should be something like:
> `
> if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
>     ereport(ERROR,
>                   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                     errmsg("function returning record called in"
>                                  "context that cannot accept type record")));
> `

+1

> `PG_FUNCTION_INFO_V1(parse_type);`
> is unnecessary?
> based on the error information:  https://cirrus-ci.com/task/5899457502380032
> not sure why it only fails on windows.

Yeah, it's not necessary for internal functions per [1].  It's a
leftover from when this function was part of the pgtap extension.

> +#define PARSE_TYPE_STRING_COLS 2 /* Returns two columns. */
> +#undef PARSE_TYPE_STRING_COLS
> Are these necessary?
> given that the comments already say return the OID and type modifier.

Not sure what the convention here is but I found this in a couple of
places, maybe even a tutorial on writing C functions.  See
`git grep '_COLS\s\+[1-9]'` for those instances.  It's in line with my
habit of avoiding magic constants.

> +        ( <parameter>typid</parameter> <type>oid</type>,
> +          <parameter>typmod</parameter> <type>int4</type> )
> here `int4` should be `integer`?

+1

> commit message:
> `Also accounts for data typs that require the SQL grammar to be parsed:`
> except the typo issue, this sentence is still hard for me to understand.

Yes, the sentence is rather handwavy.  What is meant here is that this
function also resolves types whose typmod is determined by the SQL
parser or some step after that.  There are types whose typmod is
whatever value is found inside the parenthesis, e.g. bit(13) has typmod
13.  Our first idea before coming up with that function was to do some
string manipulation and extract the typmod from inside the parenthesis.
But we soon found out that other typmods don't translate one to one,
e.g.  varchar(13) has typmod 17.  The interval type is also special
because the typmod is some bitmask encoding of e.g. 'second(0)'.  Hence
the mention of the SQL grammar.

> +       <para>
> +        Parses a string representing an SQL type declaration as used in a
> +        <command>CREATE TABLE</command> statement, optionally schema-qualified.
> +        Returns a record with two fields, <parameter>typid</parameter> and
> +        <parameter>typmod</parameter>, representing the OID and
> modifier for the
> +        type. These correspond to the parameters to pass to the
> +        <link linkend="format_type"><function>format_type</function>
> function.</link>
> +       </para>
> 
> can be simplified:
> +       <para>
> +        Parses a string representing an SQL data type, optionally
> schema-qualified.
> +        Returns a record with two fields, <parameter>typid</parameter> and
> +        <parameter>typmod</parameter>, representing the OID and
> modifier for the
> +        type. These correspond to the parameters to pass to the
> +        <link linkend="format_type"><function>format_type</function>
> function.</link>
> +       </para>
> (I don't have a strong opinion though)

Sounds better.  The CREATE TABLE reference is superfluous.

[1] https://www.postgresql.org/docs/current/xfunc-c.html#XFUNC-C-V1-CALL-CONV

-- 
Erik