From: | "David E(dot) Wheeler" <david(at)justatheory(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Erik Wienhold <ewie(at)ewie(dot)name>, jian he <jian(dot)universality(at)gmail(dot)com>, 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-12 20:55:58 |
Message-ID: | AD4F58ED-A927-4AED-9854-87B8F3610D43@justatheory.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Feb 12, 2024, at 12:53 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "David E. Wheeler" <david(at)justatheory(dot)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 more appropriate. 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 this to 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
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2024-02-12 20:59:15 | Re: Fix a typo in pg_rotate_logfile |
Previous Message | Nathan Bossart | 2024-02-12 20:55:07 | Re: Popcount optimization using AVX512 |