Re: Patch: Add parse_type Function

On Feb 12, 2024, at 12:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> "David E. Wheeler" <david@justatheory.com> writes:
>> [ v4-0001-Add-parse_type-SQL-function.patch ]
>
> It strikes me that this is basically to_regtype() with the additional
> option to return the typmod.  That leads to some questions:

Huh. I saw it more on a par with format_type(), at least in the output I expect.

> * Should we choose a name related to to_regtype?  I don't have any
> immediate suggestions, but since you didn't seem entirely set on
> parse_type, maybe it's worth thinking along those lines.  OTOH,
> to the extent that people would use this with format_type, maybe
> parse_type is fine.

Yeah, and the `to_*` functions return a single OID, not two fields.

> * Perhaps the type OID output argument should be declared regtype
> not plain OID?  It wouldn't make any difference for passing it to
> format_type, but in other use-cases perhaps regtype would be more
> readable.  It's not a deal-breaker either way of course, since
> you can cast oid to regtype or vice versa.

Sure. I used `oid` because that’s exactly what the argument to format_type() is called, so thought the parity there
moreappropriate. Maybe its argument should be renamed to regtype? 

> * Maybe the code should be in adt/regproc.c not format_type.c.

Happy to move it wherever makes the most sense.

> * Experience with to_regtype suggests strongly that people would
> prefer "return NULL" to failure for an unrecognized type name.

Could do, but as with to_regtype() there’s an issue with error handling when it descends into the SQL parser.

> Skimming the patch, I notice that the manual addition to
> builtins.h should be unnecessary: the pg_proc.dat entry
> should be enough to create an extern in fmgrprotos.h.

Confirmed, will remove in next patch.

> Also, I'm pretty sure that reformat_dat_file.pl will
> think your pg_proc.dat entry is overly verbose.  See
> https://www.postgresql.org/docs/devel/system-catalog-initial-data.html

There are a bunch of things it reformats:

❯ git diff --name-only
src/include/catalog/pg_aggregate.dat
src/include/catalog/pg_database.dat
src/include/catalog/pg_proc.dat
src/include/catalog/pg_type.dat
src/include/utils/builtins.h

And even in pg_proc.dat there are 13 blocks it reformats. I can submit with just my block formatted if you’d prefer.

> BTW, another way that this problem could be approached is to use
> to_regtype() as-is, with a separate function to obtain the typmod:
>
> select format_type(to_regtype('timestamp(4)'), to_regtypmod('timestamp(4)'));

Oh interesting.

> This is intellectually ugly, since it implies parsing the same
> typename string twice.  But on the other hand it avoids the notational
> pain and runtime overhead involved in using a record-returning
> function.  So I think it might be roughly a wash for performance.
> Question to think about is which way is easier to use.  I don't
> have an opinion particularly; just throwing the idea out there.

Honestly I haven’t cared for the fact that parse_type() returns a record; it makes it a bit clunky. But yeah, so does
thisto pass the same value to two function calls. 

Perhaps it makes sense to add to_regtypmod() as you suggest, but then also something like:

CREATE FUNCTION format_type_string(text) RETURNS TEXT AS $$
    SELECT format_type(to_regtype($1), to_regtypmod($1))
$$;

Best,

David