Re: Patch: Add parse_type Function

Hi David,
Thanks for the patch.

On 04.02.24 18:51, David E. Wheeler wrote:
> Hackers,
>
> Attached is a patch to add a new function, `parse_type()`. It parses a type string and returns a record with the
typidand typmod for the type, or raises an error if the type is invalid. It’s effectively a thin layer over the
parser’sparseTypeString() function.
 
>
> The purpose of this function is to allow uses to resolve any type name, including aliases, to its formal name as
renderedby, for example, pg_dump. An example:
 
>
> david=# WITH s(s) AS (
>     SELECT * FROM unnest(ARRAY[
>         'timestamp(4)',
>         'interval(0)',
>         'interval second(0)',
>         'timestamptz',
>         'timestamptz(6)',
>         'varchar',
>         'varchar(128)' 
>     ])         
> ),
> p(type, typid, typmod) AS (
>     SELECT s, ((parse_type(s)).*)
>       FROM s                     
> )           
> SELECT type, format_type(typid, typmod) FROM p;
>         type        |          format_type           
> --------------------+--------------------------------
>  timestamp(4)       | timestamp(4) without time zone
>  interval(0)        | interval(0)
>  interval second(0) | interval second(0)
>  timestamptz        | timestamp with time zone
>  timestamptz(6)     | timestamp(6) with time zone
>  varchar            | character varying
>  varchar(128)       | character varying(128)
> (7 rows)
>
>
> The use case leading to this patch was pgTAP column data type assertions. pgTAP traditionally required full type
namesfor testing data types, but recently added support for aliases. This broke for some types, because it was
difficultto extract the typmod correctly, and even when we did, it failed for types such as `interval second(0)`, where
`second(0)` is the typmod, and there was no sensible way to access the bit mask to generate the proper typmod to pass
to`format_type()`.
 
>
> Erik Wienhold wrote this function to solve the problem, and I volunteered to submit a patch.
>
> Potential issues and questions for this patch:
>
> * Is `parse_type()` the right name to use? I can see arguments for `parse_type_string()`, `pg_parse_type()`, and
`pg_parse_type_string()`.Whatever the consensus is works for me.
 
>
> * The function returns a record, which is a little fussy. I could see adding a `format_type_string()` function that
justcontains `SELECT format_type(p.typmod, p.typid) FROM parse_type($1) p` and returns the formatted type. But I think
itmight also be useful to have access to the typmod and typid directly via this method, since they’re already exposed
as`format_type()`’s arguments.
 
>
> * Is format_type.c the right home for the function? I put it there because I closely associate it with format_type(),
buthappy to move it to a more appropriate location.
 
>
> * I tried to link to `format_type` from the docs entry for `parse_type`, and added an ID for the former, but I’m not
sureit resolves.
 
>
> Best,
>
> David
>
The patch applies cleanly

Checking patch doc/src/sgml/func.sgml...
Checking patch src/backend/utils/adt/format_type.c...
Checking patch src/include/catalog/pg_proc.dat...
Checking patch src/include/utils/builtins.h...
Checking patch src/test/regress/expected/create_type.out...
Checking patch src/test/regress/sql/create_type.sql...
Applied patch doc/src/sgml/func.sgml cleanly.
Applied patch src/backend/utils/adt/format_type.c cleanly.
Applied patch src/include/catalog/pg_proc.dat cleanly.
Applied patch src/include/utils/builtins.h cleanly.
Applied patch src/test/regress/expected/create_type.out cleanly.
Applied patch src/test/regress/sql/create_type.sql cleanly.

The docs render just fine and all tests pass (check and check-world).

There are a few issues though:

1) The function causes a crash when called with a NULL parameter:

SELECT * FROM parse_type(NULL);
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
The connection to the server was lost. Attempting reset: Failed.

You have to check it in the beginning of function and either return an
error message or just NULL, e.g

    if (PG_ARGISNULL(0))
        PG_RETURN_NULL();

2) Add a function call with NULL in the regression tests

SELECT * FROM parse_type(NULL);

3) Add the expected behaviour of calling the function with NULL in the
docs (error message or null)

4) The name of the returned columns is repeated in the docs
> Returns a record with two fields, <parameter>typid</parameter> and
<parameter>typid</parameter>

-- 
Jim