Hi, On 1/2/24 01:00, jian he wrote: > On Mon, Dec 4, 2023 at 5:11 PM John Naylor <johncnaylo...@gmail.com> wrote: >> >> On Thu, Sep 28, 2023 at 12:22 AM Alexander Korotkov >> <aekorot...@gmail.com> wrote: >>> The one thing triggering my perfectionism is that the patch does two >>> syscache lookups instead of one. >> >> For an admin function used interactively, I'm not sure why that >> matters? Or do you see another use case? > > I did a minor refactor based on v1-0001. > I think pg_basetype should stay at "9.26.4. System Catalog Information > Functions". > So I placed it before pg_char_to_encoding. > Now functions listed on "Table 9.73. System Catalog Information > Functions" will look like alphabetical ordering. > I slightly changed the src/include/catalog/pg_proc.dat. > now it looks like very similar to pg_typeof > > src6=# \df pg_typeof > List of functions > Schema | Name | Result data type | Argument data types | Type > ------------+-----------+------------------+---------------------+------ > pg_catalog | pg_typeof | regtype | "any" | func > (1 row) > > src6=# \df pg_basetype > List of functions > Schema | Name | Result data type | Argument data types | Type > ------------+-------------+------------------+---------------------+------ > pg_catalog | pg_basetype | regtype | "any" | func > (1 row) > > v2-0001 is as is in the first email thread, 0002 is my changes based on > v2-0001.
I think the patch(es) look reasonable, so just a couple minor comments. 1) We already have pg_typeof() function, so maybe we should use a similar naming convention pg_basetypeof()? 2) I was going to suggest using "any" argument, just like pg_typeof, but I see 0002 patch already does that. Thanks! 3) I think the docs probably need some formatting - wrapping lines (to make it consistent with the nearby stuff) and similar stuff. Other than that it looks fine to me. It's a simple patch, so if we can agree on the naming I'll get it cleaned up and pushed. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
