Re: Patch: Add parse_type Function

2024-03-20 Thread David E. Wheeler
On Mar 20, 2024, at 17:23, Tom Lane wrote: > Pushed with some editorialization. Mostly, I whacked the > documentation around pretty heavily: we have a convention for what > examples in function descriptions should look like, and this wasn't > it. Not entirely your fault, since some nearby

Re: Patch: Add parse_type Function

2024-03-20 Thread Tom Lane
"David E. Wheeler" writes: > Thanks, fixed in the attached patch. Pushed with some editorialization. Mostly, I whacked the documentation around pretty heavily: we have a convention for what examples in function descriptions should look like, and this wasn't it. Not entirely your fault, since

Re: Patch: Add parse_type Function

2024-03-11 Thread Erik Wienhold
On 2024-03-09 02:42 +0100, David E. Wheeler wrote: > On Mar 7, 2024, at 23:39, Erik Wienhold wrote: > > > I think you need to swap the examples. The text mentions the error case > > first and the NULL case second. > > 臘‍♂️ > > Thanks, fixed in the attached patch. Thanks, LGTM. -- Erik

Re: Patch: Add parse_type Function

2024-03-08 Thread David E. Wheeler
On Mar 7, 2024, at 23:39, Erik Wienhold wrote: > I think you need to swap the examples. The text mentions the error case > first and the NULL case second. 臘‍♂️ Thanks, fixed in the attached patch. David v11-0001-Add-to_regtypemod-SQL-function.patch Description: Binary data

Re: Patch: Add parse_type Function

2024-03-07 Thread Erik Wienhold
Hi David, On 2024-03-08 02:37 +0100, David E. Wheeler wrote: > I’ve rebased the patch and, in an attempt to clarify this behavior, > added a couple of examples to the docs for to_regtype. Updated patch > attached. On your latest addition: > +). Failure to extract a valid potential > +

Re: Patch: Add parse_type Function

2024-03-07 Thread David E. Wheeler
Hello Hackers, On Feb 25, 2024, at 13:00, David E. Wheeler wrote: >> 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=#

Re: Patch: Add parse_type Function

2024-02-25 Thread David E. Wheeler
On Feb 24, 2024, at 19:11, Jim Jones wrote: >> What’s the protocol for marking a patch ready for committer? > > I guess after the review of the last assigned reviewer Oh, I didn’t realize someone was assigned. :-) > The fact that a completely invalid type returns NULL .. > > SELECT

Re: Patch: Add parse_type Function

2024-02-24 Thread Jim Jones
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

Re: Patch: Add parse_type Function

2024-02-24 Thread David E. Wheeler
On Feb 21, 2024, at 19:13, David E. Wheeler wrote: > Thanks. Anyone else? Or is it ready for committer? What’s the protocol for marking a patch ready for committer? Thanks, David

Re: Patch: Add parse_type Function

2024-02-21 Thread David E. Wheeler
On Feb 21, 2024, at 17:19, Erik Wienhold wrote: > Thanks David! LGTM. Thanks. Anyone else? Or is it ready for committer? Best, David

Re: Patch: Add parse_type Function

2024-02-21 Thread Erik Wienhold
On 2024-02-21 22:49 +0100, David E. Wheeler wrote: > On Feb 21, 2024, at 16:43, Erik Wienhold wrote: > > > The docs still state that to_regtypemod() has a named parameter, which > > is not the case per pg_proc.dat. > > Bah, I cleaned it up before but somehow put it back. Thanks for the > catch.

Re: Patch: Add parse_type Function

2024-02-21 Thread David E. Wheeler
On Feb 21, 2024, at 16:43, Erik Wienhold wrote: > The docs still state that to_regtypemod() has a named parameter, which > is not the case per pg_proc.dat. Bah, I cleaned it up before but somehow put it back. Thanks for the catch. Fixed. Best, David

Re: Patch: Add parse_type Function

2024-02-21 Thread Erik Wienhold
On 2024-02-21 17:51 +0100, David E. Wheeler wrote: > On Feb 21, 2024, at 11:18, Erik Wienhold wrote: > > > Thanks. But it's an applefile again, not a patch :P > > Let’s try that again. Thanks. > +to_regtypemod ( type > text ) The docs still state that to_regtypemod() has a named

Re: Patch: Add parse_type Function

2024-02-21 Thread David E. Wheeler
On Feb 21, 2024, at 11:18, Erik Wienhold wrote: > Thanks. But it's an applefile again, not a patch :P WTF. I still have that feature disabled. Oh, I think I deleted the file after dragged it into Mail but before sending, because it’s empty everywhere I look. 臘‍♂️ Let’s try that again.

Re: Patch: Add parse_type Function

2024-02-21 Thread Erik Wienhold
On 2024-02-21 16:14 +0100, David E. Wheeler wrote: > > V8 attached. Thanks. But it's an applefile again, not a patch :P -- Erik

Re: Patch: Add parse_type Function

2024-02-21 Thread David E. Wheeler
On Feb 20, 2024, at 21:09, Erik Wienhold wrote: > The references are printed as "???" right now. Can be fixed with > xreflabel="format_type" and xreflabel="to_regtype" on those > elements. Thanks. > The docs show parameter name "type" but pg_proc.data does not define > proargnames. So the

Re: Patch: Add parse_type Function

2024-02-20 Thread Erik Wienhold
On 2024-02-20 15:44 +0100, David E. Wheeler wrote: > I’ve tweaked the comments and their order in v7, attached. > > This goes back to the discussion of the error raising of > to_regtype[1], so I’ve incorporated the patch from that thread into > this patch, and set up the docs for to_regtypemod()

Re: Patch: Add parse_type Function

2024-02-20 Thread David E. Wheeler
On Feb 20, 2024, at 01:30, jian he wrote: > the second hint `-- grammar error expected` seems to contradict with > the results? Quite right, thank you, that’s actually a trapped error. I’ve tweaked the comments and their order in v7, attached. This goes back to the discussion of the error

Re: Patch: Add parse_type Function

2024-02-19 Thread jian he
On Tue, Feb 20, 2024 at 11:06 AM David E. Wheeler wrote: > > LOL, I missed that, just wrote it myself in the last hour. :-) v6 attached. > +SELECT to_regtypemod('interval nonesuch'); -- grammar error expected +ERROR: syntax error at or near "nonesuch" +LINE 1: SELECT to_regtypemod('interval

Re: Patch: Add parse_type Function

2024-02-19 Thread David E. Wheeler
On Feb 19, 2024, at 21:58, Erik Wienhold wrote: > See the patch I wrote for my benchmarks. But it's pretty easy anyway to > cut down parse_type() ;) LOL, I missed that, just wrote it myself in the last hour. :-) v6 attached. > But you don't actually need reformat_type() in pgTAP. You can

Re: Patch: Add parse_type Function

2024-02-19 Thread Erik Wienhold
On 2024-02-19 23:59 +0100, David E. Wheeler wrote: > On Feb 19, 2024, at 15:47, Tom Lane wrote: > > >> 1. Add a to_regtypmod() for those who just want the typemod. > > > > Seems like there's a good case for doing that. > > I’ll work on that. See the patch I wrote for my benchmarks. But it's

Re: Patch: Add parse_type Function

2024-02-19 Thread David E. Wheeler
On Feb 19, 2024, at 15:47, Tom Lane wrote: >> 1. Add a to_regtypmod() for those who just want the typemod. > > Seems like there's a good case for doing that. I’ll work on that. > I'm less thrilled about that, mainly because I can't think of > a good name for it ("format_type_string" is

Re: Patch: Add parse_type Function

2024-02-19 Thread Tom Lane
I wrote: > I'm less thrilled about that, mainly because I can't think of > a good name for it ("format_type_string" is certainly not that). After some time ruminating, a couple of possibilities occurred to me: reformat_type(text) canonical_type_name(text) > Is the use-case for

Re: Patch: Add parse_type Function

2024-02-19 Thread Tom Lane
"David E. Wheeler" writes: > The only way I can think of to avoid that is to: > 1. Add a to_regtypmod() for those who just want the typemod. Seems like there's a good case for doing that. > 2. Add a format_type_string() function that returns a string, the equivalent > of this function but in

Re: Patch: Add parse_type Function

2024-02-19 Thread David E. Wheeler
On Feb 18, 2024, at 15:55, Erik Wienhold wrote: >> The overhead of parse_type_and_format can be related to higher planning >> time. PL/pgSQL can assign composite without usage FROM clause. > > Thanks, didn't know that this makes a difference. In that case both > variants are on par.

Re: Patch: Add parse_type Function

2024-02-18 Thread Erik Wienhold
On 2024-02-18 20:00 +0100, Pavel Stehule wrote: > The overhead of parse_type_and_format can be related to higher planning > time. PL/pgSQL can assign composite without usage FROM clause. Thanks, didn't know that this makes a difference. In that case both variants are on par. BEGIN;

Re: Patch: Add parse_type Function

2024-02-18 Thread Pavel Stehule
Hi ne 18. 2. 2024 v 19:50 odesílatel Erik Wienhold napsal: > On 2024-02-12 19:20 +0100, Tom Lane wrote: > > I wrote: > > > It strikes me that this is basically to_regtype() with the additional > > > option to return the typmod. That leads to some questions: > > > > BTW, another way that this

Re: Patch: Add parse_type Function

2024-02-18 Thread Erik Wienhold
On 2024-02-12 19:20 +0100, Tom Lane wrote: > I wrote: > > It strikes me that this is basically to_regtype() with the additional > > option to return the typmod. That leads to some questions: > > BTW, another way that this problem could be approached is to use > to_regtype() as-is, with a

Re: Patch: Add parse_type Function

2024-02-13 Thread David E. Wheeler
On Feb 12, 2024, at 15:55, David E. Wheeler wrote: > Happy to move it wherever makes the most sense. Update with a bunch of the suggested changes. Some questions still open from the previous post, though. Best, David v5-0001-Add-parse_type-SQL-function.patch Description: Binary data

Re: Patch: Add parse_type Function

2024-02-12 Thread David E. Wheeler
On Feb 12, 2024, at 12:53 PM, Tom Lane wrote: > "David E. Wheeler" 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

Re: Patch: Add parse_type Function

2024-02-12 Thread Pavel Stehule
po 12. 2. 2024 v 19:20 odesílatel Tom Lane napsal: > I wrote: > > It strikes me that this is basically to_regtype() with the additional > > option to return the typmod. That leads to some questions: > > BTW, another way that this problem could be approached is to use > to_regtype() as-is, with

Re: Patch: Add parse_type Function

2024-02-12 Thread Tom Lane
I wrote: > It strikes me that this is basically to_regtype() with the additional > option to return the typmod. That leads to some questions: 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

Re: Patch: Add parse_type Function

2024-02-12 Thread Tom Lane
"David E. Wheeler" 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: * Should we choose a name related to to_regtype? I don't have any immediate suggestions,

Re: Patch: Add parse_type Function

2024-02-12 Thread David E. Wheeler
On Feb 10, 2024, at 20:52, Erik Wienhold wrote: > > Let me comment on some issues since I wrote the very first version of > parse_type() on which David's patch is based. Thanks Erik. >> On 2024-02-01 01:00 +0100, jian he wrote: >> if you are wondering around other code deal with NULL,

Re: Patch: Add parse_type Function

2024-02-10 Thread Erik Wienhold
Let me comment on some issues since I wrote the very first version of parse_type() on which David's patch is based. > On 2024-02-01 01:00 +0100, jian he wrote: > > cosmetic issue. Looking around other error handling code, the format > should be something like: > ` > if

Re: Patch: Add parse_type Function

2024-02-10 Thread jian he
+ /* + * Parse type-name argument to obtain type OID and encoded typmod. We don't + * need to check for parseTypeString failure, but just let the error be + * raised. The 0 arg works both as the `Node *escontext` arg in Postgres 16 + * and the `bool missing_ok` arg in 9.4-15. + */ + (void)

Re: Patch: Add parse_type Function

2024-02-05 Thread David E. Wheeler
On Feb 5, 2024, at 10:47 AM, Jim Jones wrote: > The patch applies cleanly and it no longer crashes with NULL parameters. > Docs render fine and regression tests pass. I have nothing more to add. > Let's now wait for Pavel's review. Much obliged! D

Re: Patch: Add parse_type Function

2024-02-05 Thread Jim Jones
On 05.02.24 16:14, David E. Wheeler wrote: > Right, done, and cleaned up the redundant comments. > > Best, > > David Nice. The patch applies cleanly and it no longer crashes with NULL parameters. Docs render fine and regression tests pass. I have nothing more to add. Let's now wait for Pavel's

Re: Patch: Add parse_type Function

2024-02-05 Thread David E. Wheeler
On Feb 5, 2024, at 09:49, Jim Jones wrote: > Yes, if the function was strict (which in the current design is not the > case) there is no need to check for nulls. Right, done, and cleaned up the redundant comments. Best, David v3-0001-Add-parse_type-SQL-function.patch Description: Binary

Re: Patch: Add parse_type Function

2024-02-05 Thread Jim Jones
On 05.02.24 15:32, Dagfinn Ilmari Mannsåker wrote: > Once the function is declared strict, I don't think either of these is > necessary: function strictness is tested elsewhere, and it's the default > behaviour. The only functions that explicitly say they return NULL on > NULL inputs are

Re: Patch: Add parse_type Function

2024-02-05 Thread Jim Jones
On 05.02.24 15:10, David E. Wheeler wrote: > Excellent, thank you very much! Updated patch attached. > > Best, > > David v2 no longer crashes with a null parameter. docs and regress tests were updated accordingly. patch no longer applies cleanly (tiny little indentation issue):

Re: Patch: Add parse_type Function

2024-02-05 Thread Dagfinn Ilmari Mannsåker
Jim Jones writes: > 1) The function causes a crash when called with a NULL parameter: > > SELECT * FROM parse_type(NULL); > server closed the connection unexpectedly >     This probably means the server terminated abnormally >     before or while processing the request. > The connection

Re: Patch: Add parse_type Function

2024-02-05 Thread David E. Wheeler
On Feb 5, 2024, at 08:02, Jim Jones wrote: > There are a few issues though: Excellent, thank you very much! Updated patch attached. Best, David v2-0001-Add-parse_type-SQL-function.patch Description: Binary data

Re: Patch: Add parse_type Function

2024-02-05 Thread Jim Jones
Hi David, Thanks for the patch. On 04.02.24 18:51, David E. Wheeler wrote: > Hackers, > > Attached is a patch to add a new function, `parse_type()`. It parses a type > string and returns a record with the typid and typmod for the type, or raises > an error if the type is invalid. It’s

Re: Patch: Add parse_type Function

2024-02-04 Thread David E. Wheeler
On Feb 4, 2024, at 13:52, Pavel Stehule wrote: > not yet, but I'll do it Nice, thank you. I put it into the Commitfest. https://commitfest.postgresql.org/47/4807/ Best, David

Re: Patch: Add parse_type Function

2024-02-04 Thread Pavel Stehule
ne 4. 2. 2024 v 19:30 odesílatel David E. Wheeler napsal: > On Feb 4, 2024, at 13:02, Pavel Stehule wrote: > > > I thinks so proposed functionality can be useful > > Great, thank you! > > Is that a review? :-) > not yet, but I'll do it Pavel > > D > > >

Re: Patch: Add parse_type Function

2024-02-04 Thread David E. Wheeler
On Feb 4, 2024, at 13:02, Pavel Stehule wrote: > I thinks so proposed functionality can be useful Great, thank you! Is that a review? :-) D

Re: Patch: Add parse_type Function

2024-02-04 Thread Pavel Stehule
Hi ne 4. 2. 2024 v 18:51 odesílatel David E. Wheeler napsal: > Hackers, > > Attached is a patch to add a new function, `parse_type()`. It parses a > type string and returns a record with the typid and typmod for the type, or > raises an error if the type is invalid. It’s effectively a thin

Patch: Add parse_type Function

2024-02-04 Thread David E. Wheeler
Hackers, Attached is a patch to add a new function, `parse_type()`. It parses a type string and returns a record with the typid and typmod for the type, or raises an error if the type is invalid. It’s effectively a thin layer over the parser’s parseTypeString() function. The purpose of this