public inbox for [email protected]  

help / color / mirror / Atom feed

From: Erik Wienhold <[email protected]>
To: jian he <[email protected]>
To: David E. Wheeler <[email protected]>
Cc: Jim Jones <[email protected]>
Cc: Dagfinn Ilmari Mannsåker <[email protected]>
Cc: [email protected]
Cc: Pavel Stehule <[email protected]>
Subject: Re: Patch: Add parse_type Function
Date: Sun, 11 Feb 2024 02:52:56 +0100 (CET)
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxHS2ULcDvZJagS=RVo=Hqq_TC-jOG0euVdKvJ-DkcKtuw@mail.gmail.com>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CACJufxHS2ULcDvZJagS=RVo=Hqq_TC-jOG0euVdKvJ-DkcKtuw@mail.gmail.com>

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 <[email protected]> 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

view thread (6+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Patch: Add parse_type Function
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox