Re: Patch: Add parse_type Function

On Feb 10, 2024, at 20:52, Erik Wienhold <ewie(at)ewie(dot)name> wrote:
>
> Let me comment on some issues since I wrote the very first version of
> parse_type() on which David's patch is based.

Thanks Erik.

>> On 2024-02-01 01:00 +0100, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>> if you are wondering around other code deal with NULL, ErrorSaveContext.
>> NULL would be more correct?
>> `(void) parseTypeString(type, &typid, &typmod, NULL);`

Fixed.

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

Thanks, adopted that language.

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

I poked around and found some other examples, yes. I went with a single long line for errmsg following the example in adminpack.c[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.

Removed.

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

Left in place for now.

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

Fixed.

> 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 adopted some of your language here --- and fixed the spelling errors :-)

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

Done.

Best,

David
[1] https://github.com/postgres/postgres/blob/09eb633e1baa3b7cd7929f3cc77f9c46f63c20b1/contrib/adminpack/adminpack.c#L508-L511