Subject: Re: Patch: Add parse_type Function

From: "David E. Wheeler" <[email-masked]>
Date: Mon, 5 Feb 2024 10:14:00 -0500

On Feb 5, 2024, at 09:49, Jim Jones <[email-masked]> wrote:

> Yes, if the function was strict (which in the current design is not the
> case) there is no need to check for nulls.

Right, done, and cleaned up the redundant comments.

Best,

David

---

From: Jim Jones <[email-masked]>
Date: Mon, 5 Feb 2024 16:47:55 +0100

> Right, done, and cleaned up the redundant comments.

Nice.
The patch applies cleanly and it no longer crashes with NULL parameters.
Docs render fine and regression tests pass. I have nothing more to add.
Let's now wait for Pavel's review.

Thanks!

-- 
Jim

---

From: "David E. Wheeler" <[email-masked]>
Date: Mon, 5 Feb 2024 10:56:23 -0500

> Let's now wait for Pavel's review.

Much obliged!

D

---

From: jian he <[email-masked]>
Date: Sun, 11 Feb 2024 08:00:00 +0800

+ /*
+ * Parse type-name argument to obtain type OID and encoded typmod. We don't
+ * need to check for parseTypeString failure, but just let the error be
+ * raised. The 0 arg works both as the `Node *escontext` arg in Postgres 16
+ * and the `bool missing_ok` arg in 9.4-15.
+ */
+ (void) parseTypeString(type, &typid, &typmod, 0);
if you are wondering around other code deal with NULL, ErrorSaveContext.
NULL would be more correct?
`(void) parseTypeString(type, &typid, &typmod, NULL);`

also parseTypeString already explained the third argument, our
comments can be simplified as:
/*
* Parse type-name argument to obtain type OID and encoded typmod. We don't
* need to handle parseTypeString failure, just let the error be
* raised.
*/

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")));
`

`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.

+#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.

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

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.

The `parse_type()` function uses the underlying `parseTypeString()` C
function to parse a string representing a data type into a type ID and
typmod suitabld for passing to `format_type()`.

suitabld should be suitable.

+       <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)

---

From: Erik Wienhold <[email-masked]>
Date: Sun, 11 Feb 2024 02:52:56 +0100 (CET)

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-masked]> wrote:
> 

+1

> 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.

> 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.

> here `int4` should be `integer`?

+1

> 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.

> (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