Re: Patch: Add parse_type Function

From: jian he <jian(dot)universality(at)gmail(dot)com>
To: "David E(dot) Wheeler" <david(at)justatheory(dot)com>
Cc: Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Subject: Re: Patch: Add parse_type Function
Date: 2024-02-11 00:00:00
Message-ID: CACJufxHS2ULcDvZJagS=RVo=Hqq_TC-jOG0euVdKvJ-DkcKtuw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2024-02-11 00:03:44 Re: Sequence Access Methods, round two
Previous Message Noah Misch 2024-02-10 23:52:38 Re: Popcount optimization using AVX512