Re: Patch: Add parse_type Function

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