Re: [PATCHES] patch adding new regexp functions
On Fri, 2007-02-09 at 16:33 -0800, Jeremy Drake wrote:
> Here is a new version of the patch which eliminates the doing_srf stuff.
* C89 require constant-sized stack allocated arrays, so the coding in
perform_regexp_matches() is non-portable.
* I'm not clear about the control flow in regexp_matches() and
regexp_split(). Presumably it's not possible for the call_cntr to
actually exceed max_calls, so the error message in these cases should be
elog(ERROR), not ereport (the former is for "shouldn't happen" bug
scenarios, the latter is for user-facing errors). Can you describe the
logic here (e.g. via comments) a bit more?
* The logic in regexp_split (incremented_offset, first_match, etc.) is
pretty ugly and hard to follow. The "if" condition on line 1037 is
particularly objectionable. Again, ISTM there should be a cleaner way to
do this.
* Try to keep lines to 80 characters or fewer. If the code is wandering
off the right side of the screen all the time, that might be a hint that
it needs simplification.
Attached is a cleaned up version of your patch -- various improvements
throughout, but mostly cosmetic stuff. Do you want to take a look at the
above?
-Neil
Index: doc/src/sgml/func.sgml
===
RCS file: /home/neilc/postgres/cvs_root/pgsql/doc/src/sgml/func.sgml,v
retrieving revision 1.357
diff -c -p -r1.357 func.sgml
*** doc/src/sgml/func.sgml 1 Feb 2007 00:28:16 - 1.357
--- doc/src/sgml/func.sgml 10 Feb 2007 03:20:14 -
***
*** 1446,1451
--- 1446,1464
+regexp_matches(string text, pattern text [,flags text])
+text[] or setof record (if flags are given)
+
+ Return all capture groups resulting from matching POSIX regular
+ expression against the string. See
+ for more information on pattern
+ matching.
+
+regexp_matches('foobarbequebaz', '(bar)(beque)')
+{bar,beque}
+
+
+
regexp_replace(string text, pattern text, replacement text [,flags text])
text
***
*** 1458,1463
--- 1471,1488
+regexp_split(string text, pattern text [,flags text])
+setof text
+
+ Splits string using POSIX regular expression as
+ the delimiter. See for more
+ information on pattern matching.
+
+regexp_split('hello world', E'\\s+')
+helloworld (2 rows)
+
+
+
repeat(string text, number int)
text
Repeat string the specified
*** cast(-44 as bit(12))
regexp_replace
+
+ regexp_matches
+
+
+ regexp_split
+
string SIMILAR TO pattern ESCAPE escape-character
*** substring('foobar' from 'o(.)b') i specifies case-insensitive
matching, while flag g specifies replacement of each matching
! substring rather than only the first one.
--- 3143,3152
string containing zero or more single-letter flags that change the
function's behavior. Flag i specifies case-insensitive
matching, while flag g specifies replacement of each matching
! substring rather than only the first one. Other supported flags are
! m, n, p, w and
! x, whose meanings correspond to those shown in
! .
*** regexp_replace('foobarbaz', 'b(..)', E'X
*** 3127,3132
--- 3161,3341
+
+ The regexp_matches function returns all of the capture
+ groups resulting from matching POSIX regular expression patterns.
+ It has the syntax
+ regexp_matches(string, pattern,
+ , flags ).
+ If there is no match to the pattern, the function returns NULL.
+ If there is a match, all of the capture groups in the pattern
+ are returned in a text[]. If the flags are not given,
+ this text[] is the only returned value. If the
+ flags are given, the function returns the following values:
+
+
+ Columns Returned from regexp_matches
+
+
+
+ Column Name
+ Column Type
+ Description
+
+
+
+
+
+ prematch
+ text
+ NULL unless the r flag is given, else all of the string prior to the match
+
+
+
+ fullmatch
+ text
+ The entire match of the regular expression
+
+
+
+ matches
+ text[]
+ Contents of all of the capture groups in the regular expression, or an empty array if there were none
+
+
+
+ postmatch
+ text
+ NULL unless the r flag is given, else all of the string after the match
+
+
+
+
+
+
+ The flags parameter is an optional text
+ string containing zero or more single-letter flags that change the
+ fu
Re: [PATCHES] patch adding new regexp functions
On Fri, 2007-02-09 at 01:08 -0800, Jeremy Drake wrote: > Yeah, I try to do that, but this one just barely passed my personal > compression threshold. Guess I should raise my threshold :) No, don't pay any attention to me, I'm just lazy :) > Here is a new version of the patch which fixes up the documentation a > little (should have read it over again before posting). The "doing_srf" business is rather tortured, and seems an invitation for bugs. ISTM there should be a cleaner way to implement this. For example, would it be possible to put all the common logic into one or more additional functions, and then have SRF vs. non-SRF cases that call into those functions after doing the appropriate initialization? -Neil ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [HACKERS] [PATCHES] plpgsql, return can contains any expression
Pavel Stehule wrote: > >OK, where are we on this patch? > > without changes. This task have to do anybody who better know PostgreSQL > cache system than me. How about you submit a version without any caching, but which works correctly; and we worry about optimizations later? I can update and send simple version. Regards Pavel Stehule _ Najdete si svou lasku a nove pratele na Match.com. http://www.msn.cz/ ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Feature: POSIX Shared memory support, round 2
That is strange, because the majority of the comments are new. Much of the code and comments are reused from the SysV code because, you know, this is an enhancement. The comments that are left serve a purpose. In PGSharedMemoryCreate, this implementation avoids the need to tell if live backends are attached to an existing segment, since exisiting segments are not reattached to--the old segments are cleared when the live orphan backends die. I would love to hear some specific, less sweeping, comments about how the code is actually written and functions. Otherwise, I'll try to refactor this and return once again. Thank you, Chris Marcellino On Feb 9, 2007, at 6:40 AM, Tom Lane wrote: Chris Marcellino <[EMAIL PROTECTED]> writes: Here is a new patch that uses the POSIX api's. It encodes the canonical path (see 'man realpath') of the database's data directory into the shared memory segment name using an strong hash function to make it fit in the shared memory segment name under all cases, without risk of key collision. I find this patch utterly unreadable, because of your cavalier disregard for making the comments match the truth. You have copied-and- pasted the original SysV code and fixed some small fraction of the comments, and I cannot tell which ones still reflect reality --- but I can tell that a lot of them don't. Also, I don't see where this implements any sort of detection of live backends attached to an existing segment, so I don't think you have responded to that objection. Magnus' idea for Windows was to use a segment set up to automatically go away as soon as the last attacher died, but AFAICT that isn't how this works. regards, tom lane ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] \prompt for psql
On 2/9/07, Alvaro Herrera <[EMAIL PROTECTED]> wrote: Gurjeet Singh wrote: > On 2/9/07, Alvaro Herrera <[EMAIL PROTECTED]> wrote: > > > >At least it'd help those poor people trying to do conditional COMMIT or > >ROLLBACK based on the transaction status. > > > The user doesn't need to check the status of the transaction if he just > needs to end it. Just fire the END command and it'll take care of whether to > COMMIT or ROLLBACK the transaction: Hmm, right. Maybe I was thinking in savepoints, which Merlin Moncure is so fond of complaining of ;-) I'm still looking for a use for them. When I find one, I'll give you a heads up. (just so you know, with savepoints came exception blocks, one of my all time favorite features). anyways, instead of complaining, consider it more of 'hopeful nudging' :-) merlin ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] \prompt for psql
Gurjeet Singh wrote: > On 2/9/07, Alvaro Herrera <[EMAIL PROTECTED]> wrote: > > > >At least it'd help those poor people trying to do conditional COMMIT or > >ROLLBACK based on the transaction status. > > > The user doesn't need to check the status of the transaction if he just > needs to end it. Just fire the END command and it'll take care of whether to > COMMIT or ROLLBACK the transaction: Hmm, right. Maybe I was thinking in savepoints, which Merlin Moncure is so fond of complaining of ;-) -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] [PATCHES] plpgsql, return can contains any expression
Pavel Stehule wrote: > >OK, where are we on this patch? > > without changes. This task have to do anybody who better know PostgreSQL > cache system than me. How about you submit a version without any caching, but which works correctly; and we worry about optimizations later? > >--- > > > >Pavel Stehule wrote: > >> > >> > >> > > >> >"Pavel Stehule" <[EMAIL PROTECTED]> writes: > >> > >> This patch doesn't seem to cope with cases where the supplied tuple > >has > >> > >> the wrong number of columns, and it doesn't look like it's being > >> >careful > >> > >> about dropped columns either. Also, that's a mighty > >bizarre-looking > >> > >> choice of cache memory context in coerce_to_tuple ... but then > >again, > >> > >> why are you bothering with a cache at all for temporary arrays? > >> > > >> > > I am sorry, Tom. But I don't understand. I can check number of > >columns, > >> > > ofcourse and I'll do it. What cache for temporary arrays do you > >mean? > >> > > >> >I thought that making coerce_to_tuple depend on estate->err_func was > >> >pretty bizarre, and that there was no need for any "cache" at all for > >> >arrays that need only live as long as the function runs. All you are > >> >saving here is a palloc/pfree cycle, which is not worth the > >obscurantism > >> >and risk of bugs (are you sure natts can never change?). > >> > >> No, cache there is ugly. But I don't have idea about more efective > >> implementation of it :-(. First version of this patch was more clean. > >and > >> little bit slow. This cache save 10%. > >> > >> > > >> >BTW, if you want this patch to make it into 8.2, it needs to be fixed > >> >and resubmitted *very* soon. > >> > >> I understand, but I am not able work on it in next four days. And I need > >> help with it from Neil. It will be for 8.3. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] Feature: POSIX Shared memory support, round 2
Chris Marcellino <[EMAIL PROTECTED]> writes: > Here is a new patch that uses the POSIX api's. It encodes the > canonical path (see 'man realpath') of the database's data directory > into the shared memory segment name using an strong hash function to > make it fit in the shared memory segment name under all cases, > without risk of key collision. I find this patch utterly unreadable, because of your cavalier disregard for making the comments match the truth. You have copied-and-pasted the original SysV code and fixed some small fraction of the comments, and I cannot tell which ones still reflect reality --- but I can tell that a lot of them don't. Also, I don't see where this implements any sort of detection of live backends attached to an existing segment, so I don't think you have responded to that objection. Magnus' idea for Windows was to use a segment set up to automatically go away as soon as the last attacher died, but AFAICT that isn't how this works. regards, tom lane ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] better support of out parameters in plperl
I'll look on it
From: Bruce Momjian <[EMAIL PROTECTED]>
To: Andrew Dunstan <[EMAIL PROTECTED]>
CC: Pavel Stehule <[EMAIL PROTECTED]>,
pgsql-patches@postgresql.org, [EMAIL PROTECTED]
Subject: Re: [PATCHES] better support of out parameters in plperl
Date: Thu, 8 Feb 2007 18:09:18 -0500 (EST)
This patch has been rejected based on comments just made by Andrew
Dunstan. If the author wants to revisit that, please reply and we can
discuss the issues.
---
Andrew Dunstan wrote:
>
>
> I wrote:
> > Pavel Stehule wrote:
> >> Hello,
> >>
> >> I send two small patches. First does conversion from perl to
> >> postgresql array in OUT parameters. Second patch allow hash form
> >> output from procedures with one OUT argument.
> >>
> >
> > I will try to review these in the next 2 weeks unless someone beats me
> > to it.
> >
> >
>
> I have reviewed this lightly, as committed by Bruce, and have some
> concerns. Unfortunately, the deathof my main workstation has cost me
> much of the time I intended to use for a more thorough review, so there
> may well be more issues than are outlined here.
>
> First, it is completely undocumented.
>
> Second, this comment is at best confusing:
>
> /* if value is ref on array do to pg string array conversion */
>
>
> Third, it appears to assume that we will have names for all OUT params.
But names are optional, as I understand it. Arguably, we should be treating
the returns positionally, and thus return an arrayref when there are OYT
params, not a hashref, and ignore the names - after all, all perl function
args are nameless, in fact, even if you use a naming convention to refer to
them.
>
> Fourth, I don't understand the change: "allow hash form output from
procedures with one OUT argument." That seems very non-orthogonal, and I
can't see any good reason for it.
>
> Lastly, if you look at the expected output as committed,it appears to
have been prepared without being actually examined, for example:
>
>
> CREATE OR REPLACE FUNCTION test05(OUT a varchar) AS $$
> return {a=>'ahoj'};
> $$ LANGUAGE plperl;
> SELECT '05' AS i,a FROM test05();
> i |a
> +-
> 05 | HASH(0x8558f9c)
> (1 row)
>
>
> what???
>
> And now that I look I see every buildfarm box broken on PLCheck. That's
no surprise at all.
>
>
> The conversation regarding these features appears only to have started
on July 28th, which was probably much too late given some of the issues.
Unless we can solve these issues very fast I would be inclined to say this
should be tabled for 8.3. I think this is a fairly good illustration of the
danger of springing a feature, largely undiscussed, on the community just
about freeze time.
>
> cheers
>
> andrew
--
Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?
http://www.postgresql.org/docs/faq
_
Chcete sdilet sve obrazky a hudbu s prateli? http://messenger.msn.cz/
---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] \prompt for psql
On 2/9/07, Alvaro Herrera <[EMAIL PROTECTED]> wrote: At least it'd help those poor people trying to do conditional COMMIT or ROLLBACK based on the transaction status. The user doesn't need to check the status of the transaction if he just needs to end it. Just fire the END command and it'll take care of whether to COMMIT or ROLLBACK the transaction: edb=# begin; BEGIN edb=# select count(*) from pg_class; count --- 280 (1 row) edb=# select count(*) from pg_class_xyz; ERROR: relation "pg_class_xyz" does not exist edb=# end; ROLLBACK edb=# Regards, -- [EMAIL PROTECTED] [EMAIL PROTECTED] gmail | hotmail | yahoo }.com
Re: [PATCHES] \prompt for psql
Joshua D. Drake wrote: > Alvaro Herrera wrote: >> Joshua D. Drake wrote: >>> Peter Eisentraut wrote: Magnus Hagander wrote: > That also requires a "reasonable shell", which all platforms don't > have... I think doing any sort of reasonable scripting around psql requires a reasonable shell. Or next someone will suggest implementing loops and conditionals in psql. >>> ... Now that you mention it :) >>> >>> psql is a shell. It is the postgresql shell. I don't see any problem >>> with continuing to extend the postgresql shell to a more functional >>> platform that is independent. >> At least it'd help those poor people trying to do conditional COMMIT or >> ROLLBACK based on the transaction status. Maybe it's not such a bad >> idea. >> >> On the other hand, seeing how the history line numbers patch is still >> nowhere to be seen, I don't think we should be expecting you to send a >> patch ... ;-) > > No one would except it from me, I would just embed python ;) > or perhaps accept > Sincerely, > > Joshua D. Drake > > > -- === The PostgreSQL Company: Command Prompt, Inc. === Sales/Support: +1.503.667.4564 || 24x7/Emergency: +1.800.492.2240 Providing the most comprehensive PostgreSQL solutions since 1997 http://www.commandprompt.com/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate PostgreSQL Replication: http://www.commandprompt.com/products/ ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [HACKERS] [PATCHES] plpgsql, return can contains any expression
OK, where are we on this patch? without changes. This task have to do anybody who better know PostgreSQL cache system than me. Regards Pavel --- Pavel Stehule wrote: > > > > > >"Pavel Stehule" <[EMAIL PROTECTED]> writes: > > >> This patch doesn't seem to cope with cases where the supplied tuple has > > >> the wrong number of columns, and it doesn't look like it's being > >careful > > >> about dropped columns either. Also, that's a mighty bizarre-looking > > >> choice of cache memory context in coerce_to_tuple ... but then again, > > >> why are you bothering with a cache at all for temporary arrays? > > > > > I am sorry, Tom. But I don't understand. I can check number of columns, > > > ofcourse and I'll do it. What cache for temporary arrays do you mean? > > > >I thought that making coerce_to_tuple depend on estate->err_func was > >pretty bizarre, and that there was no need for any "cache" at all for > >arrays that need only live as long as the function runs. All you are > >saving here is a palloc/pfree cycle, which is not worth the obscurantism > >and risk of bugs (are you sure natts can never change?). > > No, cache there is ugly. But I don't have idea about more efective > implementation of it :-(. First version of this patch was more clean. and > little bit slow. This cache save 10%. > > > > >BTW, if you want this patch to make it into 8.2, it needs to be fixed > >and resubmitted *very* soon. > > I understand, but I am not able work on it in next four days. And I need > help with it from Neil. It will be for 8.3. > > Thank you > Pavel > > _ > Emotikony a pozadi programu MSN Messenger ozivi vasi konverzaci. > http://messenger.msn.cz/ > > > ---(end of broadcast)--- > TIP 5: don't forget to increase your free space map settings -- Bruce Momjian <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate _ Najdete si svou lasku a nove pratele na Match.com. http://www.msn.cz/ ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
