Re: Patch: Add parse_type Function - Mailing list pgsql-hackers

From Jim Jones
Subject Re: Patch: Add parse_type Function
Date
Msg-id 39614e00-9f0e-4b17-9a85-9b3078daa11e@uni-muenster.de
Whole thread Raw
In response to Re: Patch: Add parse_type Function  ("David E. Wheeler" <david@justatheory.com>)
Responses Re: Patch: Add parse_type Function
List pgsql-hackers

On 24.02.24 14:46, David E. Wheeler wrote:
> What’s the protocol for marking a patch ready for committer?
I guess after the review of the last assigned reviewer


v9 applies cleanly, all tests pass and documentation builds correctly.

Just a very small observation:

The fact that a completely invalid type returns NULL ..

SELECT to_regtypemod('foo');
 to_regtypemod
---------------
              
(1 row)


.. but a "partially" valid one returns an error might be confusing

postgres=# SELECT to_regtypemod('timestamp(-4)');
ERROR:  syntax error at or near "-"
LINE 1: SELECT to_regtypemod('timestamp(-4)');
                  ^
CONTEXT:  invalid type name "timestamp(-4)"

postgres=# SELECT to_regtypemod('text(-4)');
ERROR:  type modifier is not allowed for type "text"


This behaviour is mentioned in the documentation, so I'd say it is ok.

+        <xref linkend="datatype-oid"/>). Failure to extract a valid
potential
+        type name results in an error; however, if the extracted name
is not
+        known to the system, this function will return
<literal>NULL</literal>.

I would personally prefer either NULL or an error in both cases, but I
can totally live with the current design.

OTOH, format_type returns "???" and it seems to be fine, so don't take
this comment too seriously :)

SELECT format_type(-1,-1);
 format_type
-------------
 ???
(1 row)


Other than that, LGTM.

Thanks David!

Best,
Jim

-- 
Jim




pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Optimize planner memory consumption for huge arrays
Next
From: Etsuro Fujita
Date:
Subject: Re: ExecAppendAsyncEventWait() in REL_14_STABLE can corrupt PG_exception_stack