Thread: mixed, named notation support
Hello I did some cleaning on this feature, and I hope so I solve some Tom's objections features: * PostgreSQL's specific syntax for named parameter: value AS name, * Doesn't change rules for defaults, * Get defaults for named, mixed notation in planner time. ToDo: enhance documentation (any volunteer?) regards Pavel Stehule CREATE FUNCTION fx(a int, b int, m int = 1, o int = 0) RETURNS int AS $$ SELECT ($1 + $2) * $3 + $4 $$ LANGUAGE SQL; -- positional notation SELECT fx(10,20); fx ---- 30 (1 row) SELECT fx(10,20,2,50); fx ----- 110 (1 row) -- named notation SELECT fx(20 as b, 10 as a); fx ---- 30 (1 row) SELECT fx(20 as b, 10 as a, 50 as o); fx ---- 80 (1 row) -- mixed notation SELECT fx(10,20, 10 as m); fx ----- 300 (1 row) SELECT fx(10,20, 50 as o, 2 as m); fx ----- 110 (1 row)
Attachment
--On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hello > > I did some cleaning on this feature, and I hope so I solve some Tom's > objections Attached is a cleaned up version of the patch, the one linked from the commit fest has some hunks failing to be applied to current CVS. This is mainly for reporting and backup purposes, i'm proceeding looking into the code deeper now. The patch passes make check and functionality check looks fine. Pavel, should I adjust the docs already (you've asked for volunteers...)? If yes, have you something specific in mind? -- Thanks Bernd
Attachment
2009/7/18 Bernd Helmle <mailings@oopsware.de>: > --On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule > <pavel.stehule@gmail.com> wrote: > >> Hello >> >> I did some cleaning on this feature, and I hope so I solve some Tom's >> objections > > Attached is a cleaned up version of the patch, the one linked from the > commit fest has some hunks failing to be applied to current CVS. This is > mainly for reporting and backup purposes, i'm proceeding looking into the > code deeper now. The patch passes make check and functionality check looks > fine. > > Pavel, should I adjust the docs already (you've asked for volunteers...)? If > yes, have you something specific in mind? sure, please. Doc needs mainly language correction - my English is bad. And maybe some others - from me is too short and too technical. Pavel > > -- > Thanks > > Bernd
--On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hello > > I did some cleaning on this feature, and I hope so I solve some Tom's > objections > > features: > * PostgreSQL's specific syntax for named parameter: value AS name, > * Doesn't change rules for defaults, > * Get defaults for named, mixed notation in planner time. > Pavel, consider the following function: CREATE OR REPLACE FUNCTION ftest(a int, b text) RETURNS RECORD LANGUAGE SQL AS $$ SELECT $1, $2 ; $$; #= SELECT ftest('blubb' AS b, 128 AS a); ERROR: function ftest(unknown, integer) does not exist at character 8 #= SELECT ftest(128 AS a, 'abcd' AS b); ftest ------------(128,abcd) (1 row) Isn't the first one supposed to work? -- Thanks Bernd
2009/7/23 Bernd Helmle <mailings@oopsware.de>: > --On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule > <pavel.stehule@gmail.com> wrote: > >> Hello >> >> I did some cleaning on this feature, and I hope so I solve some Tom's >> objections >> >> features: >> * PostgreSQL's specific syntax for named parameter: value AS name, >> * Doesn't change rules for defaults, >> * Get defaults for named, mixed notation in planner time. >> > > Pavel, consider the following function: > > CREATE OR REPLACE FUNCTION ftest(a int, b text) > RETURNS RECORD > LANGUAGE SQL > AS > $$ > SELECT $1, $2 ; > $$; > > #= SELECT ftest('blubb' AS b, 128 AS a); > ERROR: function ftest(unknown, integer) does not exist at character 8 > > #= SELECT ftest(128 AS a, 'abcd' AS b); > ftest > ------------ > (128,abcd) > (1 row) > > Isn't the first one supposed to work? it is probably bug. I'll look on it tomorrow. Pavel > > > -- > Thanks > > Bernd >
Hello, fixed patch attached + more regress tests. Regards Pavel Stehule 2009/7/23 Pavel Stehule <pavel.stehule@gmail.com>: > 2009/7/23 Bernd Helmle <mailings@oopsware.de>: >> --On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule >> <pavel.stehule@gmail.com> wrote: >> >>> Hello >>> >>> I did some cleaning on this feature, and I hope so I solve some Tom's >>> objections >>> >>> features: >>> * PostgreSQL's specific syntax for named parameter: value AS name, >>> * Doesn't change rules for defaults, >>> * Get defaults for named, mixed notation in planner time. >>> >> >> Pavel, consider the following function: >> >> CREATE OR REPLACE FUNCTION ftest(a int, b text) >> RETURNS RECORD >> LANGUAGE SQL >> AS >> $$ >> SELECT $1, $2 ; >> $$; >> >> #= SELECT ftest('blubb' AS b, 128 AS a); >> ERROR: function ftest(unknown, integer) does not exist at character 8 >> >> #= SELECT ftest(128 AS a, 'abcd' AS b); >> ftest >> ------------ >> (128,abcd) >> (1 row) >> >> Isn't the first one supposed to work? > > it is probably bug. I'll look on it tomorrow. > > Pavel > >> >> >> -- >> Thanks >> >> Bernd >> >
Attachment
Hi, I sending a little bit modified version - I removed my forgotten comment in gram.y Regards Pavel 2009/7/25 Pavel Stehule <pavel.stehule@gmail.com>: > Hello, > > fixed patch attached + more regress tests. > > Regards > Pavel Stehule > > > 2009/7/23 Pavel Stehule <pavel.stehule@gmail.com>: >> 2009/7/23 Bernd Helmle <mailings@oopsware.de>: >>> --On Donnerstag, März 05, 2009 08:41:28 +0100 Pavel Stehule >>> <pavel.stehule@gmail.com> wrote: >>> >>>> Hello >>>> >>>> I did some cleaning on this feature, and I hope so I solve some Tom's >>>> objections >>>> >>>> features: >>>> * PostgreSQL's specific syntax for named parameter: value AS name, >>>> * Doesn't change rules for defaults, >>>> * Get defaults for named, mixed notation in planner time. >>>> >>> >>> Pavel, consider the following function: >>> >>> CREATE OR REPLACE FUNCTION ftest(a int, b text) >>> RETURNS RECORD >>> LANGUAGE SQL >>> AS >>> $$ >>> SELECT $1, $2 ; >>> $$; >>> >>> #= SELECT ftest('blubb' AS b, 128 AS a); >>> ERROR: function ftest(unknown, integer) does not exist at character 8 >>> >>> #= SELECT ftest(128 AS a, 'abcd' AS b); >>> ftest >>> ------------ >>> (128,abcd) >>> (1 row) >>> >>> Isn't the first one supposed to work? >> >> it is probably bug. I'll look on it tomorrow. >> >> Pavel >> >>> >>> >>> -- >>> Thanks >>> >>> Bernd >>> >> >
Attachment
--On Sonntag, Juli 26, 2009 06:17:49 +0200 Pavel Stehule <pavel.stehule@gmail.com> wrote: > Hi, > > I sending a little bit modified version - I removed my forgotten > comment in gram.y Thanks, i'll look on it asap. -- Thanks Bernd
--On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle <mailings@oopsware.de> wrote: >> Hi, >> >> I sending a little bit modified version - I removed my forgotten >> comment in gram.y > > Thanks, i'll look on it asap. Looks good now. Here is a slightly edited reviewed patch version. I've edited the docs and fixed some spelling errors in a few error messages. It seems the patch mangled some source comments in namespace.c, i've rearranged them and tried to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing reviewing this but can't get on it again until sunday. Pavel, can you have a look at the docs part of this patch and watch out for any gotchas? -- Thanks Bernd
Attachment
2009/7/31 Bernd Helmle <mailings@oopsware.de>: > --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle > <mailings@oopsware.de> wrote: > >>> Hi, >>> >>> I sending a little bit modified version - I removed my forgotten >>> comment in gram.y >> >> Thanks, i'll look on it asap. > > Looks good now. > > Here is a slightly edited reviewed patch version. I've edited the docs and > fixed some spelling errors in a few error messages. It seems the patch > mangled some source comments in namespace.c, i've rearranged them and tried > to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing > reviewing this but can't get on it again until sunday. > > Pavel, can you have a look at the docs part of this patch and watch out for > any gotchas? > Today, I'll look on it. Pavel > -- > Thanks > > Bernd
2009/7/31 Bernd Helmle <mailings@oopsware.de>: > --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle > <mailings@oopsware.de> wrote: > >>> Hi, >>> >>> I sending a little bit modified version - I removed my forgotten >>> comment in gram.y >> >> Thanks, i'll look on it asap. > > Looks good now. > > Here is a slightly edited reviewed patch version. I've edited the docs and > fixed some spelling errors in a few error messages. It seems the patch > mangled some source comments in namespace.c, i've rearranged them and tried > to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing > reviewing this but can't get on it again until sunday. > > Pavel, can you have a look at the docs part of this patch and watch out for > any gotchas? > I looked there and I thing it's very good. I have only one idea about samples in docs. Mathematical function isn't too much readable. Maybe some string concation or returning record should be more readable. create or replace function foo(a varchar, b varchar, uppercase boolean = false) returns varchar as $$ select when uppercase then 'a=' || a ', b=' || b else upper('a='|| a ', b=' || b) end; $$ language sql immutable strict; select foo('Hello','World'); select foo('Hello', World', true); select foo('Hello' as a, 'World' as b); select foo('Hello', 'World', true as uppercase); select foo(true as uppercase, 'World' as b, 'Hello' as b); or some similar Thank You very much Pavel > -- > Thanks > > Bernd
On Fri, Jul 31, 2009 at 8:46 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote: > 2009/7/31 Bernd Helmle <mailings@oopsware.de>: >> --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle >> <mailings@oopsware.de> wrote: >> >>>> Hi, >>>> >>>> I sending a little bit modified version - I removed my forgotten >>>> comment in gram.y >>> >>> Thanks, i'll look on it asap. >> >> Looks good now. >> >> Here is a slightly edited reviewed patch version. I've edited the docs and >> fixed some spelling errors in a few error messages. It seems the patch >> mangled some source comments in namespace.c, i've rearranged them and tried >> to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing >> reviewing this but can't get on it again until sunday. >> >> Pavel, can you have a look at the docs part of this patch and watch out for >> any gotchas? >> > > I looked there and I thing it's very good. I have only one idea about > samples in docs. Mathematical function isn't too much readable. Maybe > some string concation or returning record should be more readable. > > create or replace function foo(a varchar, b varchar, uppercase boolean = false) > returns varchar as $$ > select when uppercase then 'a=' || a ', b=' || b > else upper('a=' || a ', b=' || b) end; > $$ language sql immutable strict; > > select foo('Hello','World'); > select foo('Hello', World', true); > select foo('Hello' as a, 'World' as b); > select foo('Hello', 'World', true as uppercase); > select foo(true as uppercase, 'World' as b, 'Hello' as b); > > or some similar > > Thank You very much > Pavel What's the current status of this patch? Is someone (either Pavel or Bernd) planning to update it further, or should it be marked Ready for Committer? Thanks, ...Robert
2009/8/2 Robert Haas <robertmhaas@gmail.com>: > On Fri, Jul 31, 2009 at 8:46 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote: >> 2009/7/31 Bernd Helmle <mailings@oopsware.de>: >>> --On Montag, Juli 27, 2009 15:24:12 +0200 Bernd Helmle >>> <mailings@oopsware.de> wrote: >>> >>>>> Hi, >>>>> >>>>> I sending a little bit modified version - I removed my forgotten >>>>> comment in gram.y >>>> >>>> Thanks, i'll look on it asap. >>> >>> Looks good now. >>> >>> Here is a slightly edited reviewed patch version. I've edited the docs and >>> fixed some spelling errors in a few error messages. It seems the patch >>> mangled some source comments in namespace.c, i've rearranged them and tried >>> to explain the purpose of VerifyCandidateNamedNotation(). I'm continuing >>> reviewing this but can't get on it again until sunday. >>> >>> Pavel, can you have a look at the docs part of this patch and watch out for >>> any gotchas? >>> >> >> I looked there and I thing it's very good. I have only one idea about >> samples in docs. Mathematical function isn't too much readable. Maybe >> some string concation or returning record should be more readable. >> >> create or replace function foo(a varchar, b varchar, uppercase boolean = false) >> returns varchar as $$ >> select when uppercase then 'a=' || a ', b=' || b >> else upper('a=' || a ', b=' || b) end; >> $$ language sql immutable strict; >> >> select foo('Hello','World'); >> select foo('Hello', World', true); >> select foo('Hello' as a, 'World' as b); >> select foo('Hello', 'World', true as uppercase); >> select foo(true as uppercase, 'World' as b, 'Hello' as b); >> >> or some similar >> >> Thank You very much >> Pavel > > What's the current status of this patch? Is someone (either Pavel or > Bernd) planning to update it further, or should it be marked Ready for > Committer? Hello Bernard wrote so will look on it today - I expect so there will be only cosmetic changes in doc. Pavel > > Thanks, > > ...Robert >
--On Sonntag, August 02, 2009 11:38:22 -0400 Robert Haas <robertmhaas@gmail.com> wrote: > What's the current status of this patch? Is someone (either Pavel or > Bernd) planning to update it further, or should it be marked Ready for > Committer? I will incorporate some additional docs adjustments per Pavels suggestion and will then mark it ready for committer review. Please note that there are actually two patches involved here: one is Pavels' patch the other is Steve Prentice' enhancement to allow named notation in plpgsql. -- Thanks Bernd
2009/8/3 Bernd Helmle <mailings@oopsware.de>: > --On Sonntag, August 02, 2009 11:38:22 -0400 Robert Haas > <robertmhaas@gmail.com> wrote: > >> What's the current status of this patch? Is someone (either Pavel or >> Bernd) planning to update it further, or should it be marked Ready for >> Committer? > > I will incorporate some additional docs adjustments per Pavels suggestion > and will then mark it ready for committer review. > > Please note that there are actually two patches involved here: one is > Pavels' patch the other is Steve Prentice' enhancement to allow named > notation in plpgsql. > I should to wait with Steve patch - I would to add main sql parser into plpgsql - than Steve's patch is unnecessary. But if there will be some problems, then we can use Steve's patch. It is simple - so there are not big problems with commit. > -- > Thanks > > Bernd >
On Aug 3, 2009, at 1:41 AM, Pavel Stehule wrote: > I should to wait with Steve patch - I would to add main sql parser > into plpgsql - than Steve's patch is unnecessary. But if there will be > some problems, then we can use Steve's patch. It is simple - so there > are not big problems with commit. I was hoping we could get the small patch into plpgsql during this commitfest. This makes plpgsql recognize 'AS' and not replace named parameter labels with the variable reference. I understand there is an effort underway to redo the plpgsql parser, but getting these two patches in together will allow people to start playing with plpgsql + named parameters at the end the of commitfest when the first alpha is released. (You can use named parameters + plpgsql without this patch, but not without some pretty serious limitations.) Without this patch, this will fail: create function create_user(alias text, display_name text) returns void as $$ BEGIN perform create_alias(alias AS alias); ... END $$ language plpgsql; This is a common pattern for many of the stored procedures we are porting and I'd imagine it's common elsewhere too. If the plpgsql parser patch lands, this patch won't be needed, but it's hard to predict when it will land. As an aside, this pattern really shows how confusing the AS syntax can be for named parameters. Which side is the label and which is the value? Thanks, -Steve
2009/8/3 Steve Prentice <prentice@cisco.com>: > On Aug 3, 2009, at 1:41 AM, Pavel Stehule wrote: > >> I should to wait with Steve patch - I would to add main sql parser >> into plpgsql - than Steve's patch is unnecessary. But if there will be >> some problems, then we can use Steve's patch. It is simple - so there >> are not big problems with commit. > > I was hoping we could get the small patch into plpgsql during this > commitfest. This makes plpgsql recognize 'AS' and not replace named > parameter labels with the variable reference. I understand there is an > effort underway to redo the plpgsql parser, but getting these two patches in > together will allow people to start playing with plpgsql + named parameters > at the end the of commitfest when the first alpha is released. (You can use > named parameters + plpgsql without this patch, but not without some pretty > serious limitations.) > > Without this patch, this will fail: > > create function create_user(alias text, display_name text) returns void as > $$ > BEGIN > perform create_alias(alias AS alias); > ... > END > $$ language plpgsql; > > This is a common pattern for many of the stored procedures we are porting > and I'd imagine it's common elsewhere too. If the plpgsql parser patch > lands, this patch won't be needed, but it's hard to predict when it will > land. > > As an aside, this pattern really shows how confusing the AS syntax can be > for named parameters. Which side is the label and which is the value? > ok - I don't though about it. My goal is integration main parser next commitfest, but it is true, so somebody would to play with named params now. Commiting of Steve's patch doesn't break anything. Pavel > Thanks, > -Steve >
On Mon, Aug 3, 2009 at 10:51 AM, Pavel Stehule<pavel.stehule@gmail.com> wrote: > 2009/8/3 Steve Prentice <prentice@cisco.com>: >> On Aug 3, 2009, at 1:41 AM, Pavel Stehule wrote: >> >>> I should to wait with Steve patch - I would to add main sql parser >>> into plpgsql - than Steve's patch is unnecessary. But if there will be >>> some problems, then we can use Steve's patch. It is simple - so there >>> are not big problems with commit. >> >> I was hoping we could get the small patch into plpgsql during this >> commitfest. This makes plpgsql recognize 'AS' and not replace named >> parameter labels with the variable reference. I understand there is an >> effort underway to redo the plpgsql parser, but getting these two patches in >> together will allow people to start playing with plpgsql + named parameters >> at the end the of commitfest when the first alpha is released. (You can use >> named parameters + plpgsql without this patch, but not without some pretty >> serious limitations.) >> >> Without this patch, this will fail: >> >> create function create_user(alias text, display_name text) returns void as >> $$ >> BEGIN >> perform create_alias(alias AS alias); >> ... >> END >> $$ language plpgsql; >> >> This is a common pattern for many of the stored procedures we are porting >> and I'd imagine it's common elsewhere too. If the plpgsql parser patch >> lands, this patch won't be needed, but it's hard to predict when it will >> land. >> >> As an aside, this pattern really shows how confusing the AS syntax can be >> for named parameters. Which side is the label and which is the value? >> > > ok - I don't though about it. My goal is integration main parser next > commitfest, but it is true, so somebody would to play with named > params now. Commiting of Steve's patch doesn't break anything. I'm a little confused here. We are 19 days into a 31 day CommitFest; you are almost three weeks too late to add a patch to the queue. Unless you can convince a friendly committer to pick this up out of sequence, I think the only patch that is under consideration here is the one that has been being discussed on this thread for the last 17 days. I sent several notes adding for all patches to be added to commitfest.postgresql.org prior to the start of CommitFest; AFAIK, this one was never added. You haven't actually specified which other patch of Steve's you're talking about anyway, but I'm guessing it's this one here: http://archives.postgresql.org/pgsql-hackers/2009-05/msg00801.php I don't think that patch would get applied even if HAD been added to the CommitFest page in time, see Tom's remarks here: http://archives.postgresql.org/pgsql-hackers/2009-05/msg00802.php Let's get back to focusing on the patch that is actually under consideration here. ...Robert
>> ok - I don't though about it. My goal is integration main parser next >> commitfest, but it is true, so somebody would to play with named >> params now. Commiting of Steve's patch doesn't break anything. > > I'm a little confused here. We are 19 days into a 31 day CommitFest; > you are almost three weeks too late to add a patch to the queue. > Unless you can convince a friendly committer to pick this up out of > sequence, I think the only patch that is under consideration here is > the one that has been being discussed on this thread for the last 17 > days. I sent several notes adding for all patches to be added to > commitfest.postgresql.org prior to the start of CommitFest; AFAIK, > this one was never added. > > You haven't actually specified which other patch of Steve's you're > talking about anyway, but I'm guessing it's this one here: > http://archives.postgresql.org/pgsql-hackers/2009-05/msg00801.php > > I don't think that patch would get applied even if HAD been added to > the CommitFest page in time, see Tom's remarks here: > http://archives.postgresql.org/pgsql-hackers/2009-05/msg00802.php > > Let's get back to focusing on the patch that is actually under > consideration here. > There are two patches - named and mixed parameters and protecting "AS" as keyword. First patch is pl independed, second patch is related to plpgsql. Both patches was in queue and Bernard did reviewing both I thing. Second patch fix some unwanted behave in plpgsql and it does on level that was possible on code base one month old. I hope, so we will be able to integrate main parser to core. I hope so this will be in next commitfest. If we doesn't release code after this commitfest, then I'll prefer wait for integration, but now, when we will release code, I thing, so for somebody should be nice to use fixed plpgsql. I thing so Steve fixed issues mentioned by Tom http://archives.postgresql.org/pgsql-hackers/2009-05/msg00804.php Integration of main parser to plpgsql will be very significant change and it needs separate patch. This solve some others big problems plpgsql and I would to solve it separately. I'll be glad to answer all questions. Pavel > ...Robert >
On Aug 3, 2009, at 9:38 AM, Robert Haas wrote: > I sent several notes adding for all patches to be added to > commitfest.postgresql.org prior to the start of CommitFest; AFAIK, > this one was never added. Hi Robert, The patch for plpgsql was added as a comment to Pavel's patch. I added it as a comment because it wouldn't make since to commit it or even review it separately. This was done on the wiki before the migration. Perhaps that was not the correct way to add it to the commitfest. If not, my apologies. -Steve
--On Montag, August 03, 2009 12:38:48 -0400 Robert Haas <robertmhaas@gmail.com> wrote: > I'm a little confused here. We are 19 days into a 31 day CommitFest; > you are almost three weeks too late to add a patch to the queue. > Unless you can convince a friendly committer to pick this up out of > sequence, I think the only patch that is under consideration here is > the one that has been being discussed on this thread for the last 17 > days. I sent several notes adding for all patches to be added to > commitfest.postgresql.org prior to the start of CommitFest; AFAIK, > this one was never added. Well, i already noted that in the "named and mixed notation" thread actually two patches were involved, see <http://archives.postgresql.org/pgsql-rrreviewers/2009-07/msg00016.php>. Since Steve's changes were so small, i considered it to be an "extension" to Pavels patch, without treating it separately to commit. In my opinion it wouldn't make sense to commit those changes to plpgsql _without_ having Pavels functionality. I have to admit that i saw Tom's complaint about making AS a special keyword in plpgsql, but decided to leave the decision wether we want to commit this or not up to a committer. I took the efforts to get the attached patches there in a reviewable state then instead. Please note that Steve's suggestion is linked into the commitfest since 2009-05-21, too. -- Thanks Bernd
--On Montag, August 03, 2009 12:18:13 -0700 Steve Prentice <prentice@cisco.com> wrote: > I added it as a comment because it wouldn't make since to commit it or > even review it separately. That was exactly i was understanding it. -- Thanks Bernd
Bernd Helmle <mailings@oopsware.de> wrote: > Please note that Steve's suggestion is linked into the commitfest > since 2009-05-21, too. Yeah: http://wiki.postgresql.org/index.php?title=CommitFest_2009-First&diff=6391&oldid=6250 -Kevin
--On Montag, August 03, 2009 12:38:48 -0400 Robert Haas <robertmhaas@gmail.com> wrote: > Let's get back to focusing on the patch that is actually under > consideration here. Status Report: I will finish documentation and review tomorrow and will mark this patch for committer review. -- Thanks Bernd
--On Montag, August 03, 2009 23:43:08 +0200 Bernd Helmle <mailings@oopsware.de> wrote: > Status Report: I will finish documentation and review tomorrow and will > mark this patch for committer review. Here's my latest reviewed version of Pavel's patch with adjusted documentation per latest discussion. While poking a little bit with simplify_function() I realized that this patch changes the behavior of VARIADIC functions a little bit. For example: CREATE OR REPLACE FUNCTION my_test(a IN text, txt VARIADIC text[]) RETURNS text AS $$ SELECT $2[1]; $$ LANGUAGE SQL; The following doesn't work in current 8.4: SELECT my_test('abcd', ARRAY['test', 'foo']); You need to use the VARIADIC keyword to match the second argument to text[]. However, if you are going to use named notation with the patch applied, the picture changes in HEAD: SELECT my_test('abcd' AS a, ARRAY['test', 'foo'] AS txt); my_test --------- test (1 row) This applies also when you reverse the argument order. I don't know wether this is intended, but its conflicting with what we have currently in the docs. It's also not clear to me wether we want this at all. -- Thanks Bernd
Attachment
2009/8/4 Bernd Helmle <mailings@oopsware.de>: > --On Montag, August 03, 2009 23:43:08 +0200 Bernd Helmle > <mailings@oopsware.de> wrote: > >> Status Report: I will finish documentation and review tomorrow and will >> mark this patch for committer review. > > Here's my latest reviewed version of Pavel's patch with adjusted > documentation per latest discussion. > > While poking a little bit with simplify_function() I realized that this > patch changes the behavior of VARIADIC functions a little bit. For example: > > CREATE OR REPLACE FUNCTION my_test(a IN text, txt VARIADIC text[]) > RETURNS text > AS > $$ > SELECT $2[1]; > $$ LANGUAGE SQL; > > The following doesn't work in current 8.4: > > SELECT my_test('abcd', ARRAY['test', 'foo']); > > You need to use the VARIADIC keyword to match the second argument to text[]. > However, if you are going to use named notation with the patch applied, the > picture changes in HEAD: > > SELECT my_test('abcd' AS a, ARRAY['test', 'foo'] AS txt); > my_test > --------- > test > (1 row) > > This applies also when you reverse the argument order. I don't know wether > this is intended, but its conflicting with what we have currently in the > docs. It's also not clear to me wether we want this at all. > Named notation has different algorithm for function detection then positional notation. There are not exist variadic parameters (because these parameters hasn't individual names). So only "packed" variadic parameter should be there, and this parameter have to be named - so keyword VARIADIC is optional. Thank You very much Pavel > -- > Thanks > > Bernd
--On 4. August 2009 20:22:05 +0200 Pavel Stehule <pavel.stehule@gmail.com> wrote: > Named notation has different algorithm for function detection then > positional notation. There are not exist variadic parameters (because > these parameters hasn't individual names). So only "packed" variadic > parameter should be there, and this parameter have to be named - so > keyword VARIADIC is optional. I wonder wether it wouldn't better to force positional notation for such functions then. I found it surprising that this works at all, but of course, someone else might enjoy this as a cool feature. To me, it feels strange and confusing that a function declared as VARIADIC suddenly accepts a "sloppy" argument only because you are using some other calling notation where others enforces you to use an additional keyword to match the function. At least, we need to document that both notations behaves different in this case. -- Thanks Bernd
2009/8/5 Bernd Helmle <mailings@oopsware.de>: > > > --On 4. August 2009 20:22:05 +0200 Pavel Stehule <pavel.stehule@gmail.com> > wrote: > >> Named notation has different algorithm for function detection then >> positional notation. There are not exist variadic parameters (because >> these parameters hasn't individual names). So only "packed" variadic >> parameter should be there, and this parameter have to be named - so >> keyword VARIADIC is optional. > > I wonder wether it wouldn't better to force positional notation for such > functions then. I found it surprising that this works at all, but of course, > someone else might enjoy this as a cool feature. To me, it feels strange and > confusing that a function declared as VARIADIC suddenly accepts a "sloppy" > argument only because you are using some other calling notation where others > enforces you to use an additional keyword to match the function. > it's little bit difficult - notation is known from code, and isn't possible change it.. > At least, we need to document that both notations behaves different in this > case. +1 some like - when notation or mixed notation are used, then variadic parameters are disabled besause individual variadic parameters hasn't name. You can put packed value to variadic parameter (when this parameter is named) without keyword VARIADIC (against to positional notation) - sample CREATE OR REPLACE FUNCTION foo(a varchar, variadic b varchar[]) RETURNS varchar AS $$ SELECT a || ' ' || array_to_string(ARRAY(unnest b),' ') $$ LANGUAGE sql; SELECT foo('Hello', 'World','again'); -- positional notation SELECT foo('Hello', VARIADIC ARRAY['World','again']) -- positional notation with packed variadic parameter SELECT foo('Hello', ARRAY['World','again'] AS b) -- mixed notation with named packed variadic parameter Pavel > > > > -- > Thanks > > Bernd >
--On Mittwoch, August 05, 2009 05:28:55 +0200 Pavel Stehule <pavel.stehule@gmail.com> wrote: >> At least, we need to document that both notations behaves different in >> this case. > > +1 Here again a patch version with updated documentation. I will stop reviewing this patch now and mark this ready for committer, so we have some time left to incorporate additional feedback. -- Thanks Bernd
Attachment
2009/8/5 Bernd Helmle <mailings@oopsware.de>: > --On Mittwoch, August 05, 2009 05:28:55 +0200 Pavel Stehule > <pavel.stehule@gmail.com> wrote: > >>> At least, we need to document that both notations behaves different in >>> this case. >> >> +1 > > Here again a patch version with updated documentation. I will stop reviewing > this patch now and mark this ready for committer, so we have some time left > to incorporate additional feedback. > > -- > Thanks > > Bernd my Thanks Pavel
Bernd Helmle <mailings@oopsware.de> writes: > Here again a patch version with updated documentation. I will stop > reviewing this patch now and mark this ready for committer, so we have some > time left to incorporate additional feedback. I'm starting to look at this now, and my very first reaction was "what in the world is a leaky list?". I'm not sure I like the data structure itself, but the terminology is certainly completely unhelpful. Can't you come up with something better than "continuous/leaky"? regards, tom lane
On Thu, Aug 6, 2009 at 7:10 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > Bernd Helmle <mailings@oopsware.de> writes: >> Here again a patch version with updated documentation. I will stop >> reviewing this patch now and mark this ready for committer, so we have some >> time left to incorporate additional feedback. > > I'm starting to look at this now, and my very first reaction was > "what in the world is a leaky list?". I'm not sure I like the > data structure itself, but the terminology is certainly completely > unhelpful. Can't you come up with something better than > "continuous/leaky"? Stepping back a bit, are we sure this is a feature we even want to support? It was already pointed out in the thread on "Parser's hook based on funccall" that SQL:201x may standardize => for this purpose. I realize that's a problem because of the possibility of a user-defined operator named =>, but aren't we usually reluctant to adopt syntax that is thought likely to be incompatible with current or future SQL standards? http://archives.postgresql.org/pgsql-hackers/2009-07/msg01715.php ...Robert
On Aug 6, 2009, at 7:12 PM, Robert Haas wrote: > On Thu, Aug 6, 2009 at 7:10 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Bernd Helmle <mailings@oopsware.de> writes: >>> Here again a patch version with updated documentation. I will stop >>> reviewing this patch now and mark this ready for committer, so we >>> have some >>> time left to incorporate additional feedback. >> >> I'm starting to look at this now, and my very first reaction was >> "what in the world is a leaky list?". I'm not sure I like the >> data structure itself, but the terminology is certainly completely >> unhelpful. Can't you come up with something better than >> "continuous/leaky"? > > Stepping back a bit, are we sure this is a feature we even want to > support? It was already pointed out in the thread on "Parser's hook > based on funccall" that SQL:201x may standardize => for this purpose. > I realize that's a problem because of the possibility of a > user-defined operator named =>, but aren't we usually reluctant to > adopt syntax that is thought likely to be incompatible with current or > future SQL standards? As a "newbie" to postgresql, I would hope this is a feature that will be supported in the not too distant future. If the standard seems to be moving in the direction of using 'name => value' as the syntax, it does seem like that would be the way we would want to go. If I remember correctly, the main argument for using "value AS name" was that it wouldn't conflict with current operators AND it would be the most likely way the standard body would go. (There was a long thread back in Dec 08 regarding the syntax that can be referenced if someone wants to read through all of them.) If it looks like the SQL standard will be going the direction of 'name => value', why would we go opposite that? Either way, I think Pavel has proven that it is easy to adjust his patch to support either syntax if a decision is made. -Steve
2009/8/7 Robert Haas <robertmhaas@gmail.com>: > On Thu, Aug 6, 2009 at 7:10 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Bernd Helmle <mailings@oopsware.de> writes: >>> Here again a patch version with updated documentation. I will stop >>> reviewing this patch now and mark this ready for committer, so we have some >>> time left to incorporate additional feedback. >> >> I'm starting to look at this now, and my very first reaction was >> "what in the world is a leaky list?". I'm not sure I like the >> data structure itself, but the terminology is certainly completely >> unhelpful. Can't you come up with something better than >> "continuous/leaky"? > > Stepping back a bit, are we sure this is a feature we even want to > support? It was already pointed out in the thread on "Parser's hook > based on funccall" that SQL:201x may standardize => for this purpose. > I realize that's a problem because of the possibility of a > user-defined operator named =>, but aren't we usually reluctant to > adopt syntax that is thought likely to be incompatible with current or > future SQL standards? > > http://archives.postgresql.org/pgsql-hackers/2009-07/msg01715.php > > ...Robert We should support both syntax - in future. I afraid so we will thing up nothing new now. The arguments against to '=>' are valid still. This syntax (with "AS") doesn't break anything in future. PostgreSQL could to support both (default with AS) and via GUC standard (like standard_conforming_strings) Pavel >
2009/8/7 Tom Lane <tgl@sss.pgh.pa.us>: > Bernd Helmle <mailings@oopsware.de> writes: >> Here again a patch version with updated documentation. I will stop >> reviewing this patch now and mark this ready for committer, so we have some >> time left to incorporate additional feedback. > > I'm starting to look at this now, and my very first reaction was > "what in the world is a leaky list?". I'm not sure I like the > data structure itself, but the terminology is certainly completely > unhelpful. Can't you come up with something better than > "continuous/leaky"? It's mean so there are some gaps in arg list and these gaps have to be filled from defaults. I am sorry, I am not native speaker, so in good names for identifiers are really bad. I am searching in dictionary - maybe incomplete or fragment, ... regards Pavel Stehule > > regards, tom lane >
--On Donnerstag, August 06, 2009 19:10:47 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm starting to look at this now, and my very first reaction was > "what in the world is a leaky list?". I'm not sure I like the > data structure itself, but the terminology is certainly completely > unhelpful. Hrm, i thought i put a comment in there, but it seems i forgot it, sorry. A leaky list seems to be a list to remember where to fill in DEFAULT argument values. Maybe arg_values with ARGS_NAMED_NOTATION and ARGS_POSITIONAL_NOTATION is a better notion. -- Thanks Bernd
Robert Haas <robertmhaas@gmail.com> writes: > Stepping back a bit, are we sure this is a feature we even want to > support? It was already pointed out in the thread on "Parser's hook > based on funccall" that SQL:201x may standardize => for this purpose. Absolutely no evidence has been presented that the SQL committee is even going to standardize something in this area, much less that they are likely to adopt => as the syntax. I think it would be completely out of character for them to do that --- their entire body of work over the past twenty years has been reflective of a COBOL-ish approach to syntax, ie use keywords not punctuation. Look at the built-in functions like SUBSTRING, POSITION, TREAT; or what they did to introduce window functions. There is definitely not enough evidence here to justify breaking existing applications, which is what introducing => would do. When and if there's a ratified standard using =>, it'll be time to break stuff. In the meantime we can do something with AS and be reasonably certain we haven't painted ourselves into a corner. regards, tom lane
On Fri, Aug 7, 2009 at 12:04 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > There is definitely not enough evidence here to justify breaking > existing applications, which is what introducing => would do. > When and if there's a ratified standard using =>, it'll be time > to break stuff. In the meantime we can do something with AS and > be reasonably certain we haven't painted ourselves into a corner. I wasn't proposing introducing => ; I thought we might want to hold off on doing anything at all. But I may be the only one, so never mind. ...Robert
Now that I've started to read this patch ... exactly what is the argument for allowing a "mixed" notation (some of the parameters named and some not)? ISTM that just serves to complicate both the patch and the user's-eye view, for no real benefit. Considering that we are worried about someday having to adjust to a SQL standard in this area, I think we ought to be as conservative as possible about what we introduce as user-visible features here. As an example, if they do go with "=>" as the parameter marker, mixed notation would become a seriously bad idea because it would be impossible to distinguish incidental use of => as an operator from mixed notation. regards, tom lane
--On 9. August 2009 12:27:53 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Now that I've started to read this patch ... exactly what is the > argument for allowing a "mixed" notation (some of the parameters named > and some not)? ISTM that just serves to complicate both the patch > and the user's-eye view, for no real benefit. Hmm, Oracle has started supporting it in recent versions, too. So one advantage would be at least some sort of compatibility for another favorite database. >From a user's point of view, i see one use case in calling functions with multiple default argument values, where only one of those value needs to be overwritten, e.g. SELECT foo(1, 100, 'this' AS one); SELECT foo(1, 102, 'other' AS two); SELECT foo(1, 100, 'another' AS three); where one, two, three are arguments with specific default values. -- Thanks Bernd
Bernd Helmle <mailings@oopsware.de> writes: > --On 9. August 2009 12:27:53 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Now that I've started to read this patch ... exactly what is the >> argument for allowing a "mixed" notation (some of the parameters named >> and some not)? ISTM that just serves to complicate both the patch >> and the user's-eye view, for no real benefit. > Hmm, Oracle has started supporting it in recent versions, too. So one > advantage would be at least some sort of compatibility for another favorite > database. Mph. Does Oracle adopt the same semantics for what a mixed call means? Because my next complaint was going to be that this definition was poorly chosen anyway --- it seems confusing, unintuitive, and restrictive. If the function is defined as having parameters (a,b,c) then what does this do: select foo(1, 2, 3 as b); and what's the argument for having it do that rather than something else? If the behavior isn't *exactly* like Oracle in corner cases like this, I think partial compatibility is worse than none. And in any case the point stands that the more behavior you invent here, the more likely you are to get blindsided by the eventual SQL standard. regards, tom lane
--On 9. August 2009 13:00:07 -0400 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Mph. Does Oracle adopt the same semantics for what a mixed call means? I had a look at the Oracle documentation while reviewing this patch, and i thought we are pretty close to what they do. Maybe Pavel can comment more on it. > Because my next complaint was going to be that this definition was > poorly chosen anyway --- it seems confusing, unintuitive, and > restrictive. If the function is defined as having parameters (a,b,c) > then what does this do: > > select foo(1, 2, 3 as b); > > and what's the argument for having it do that rather than something > else? Since b is ambiguous we error out (I don't know what Oracle does, but i would be surprised if they do anything different). -- Thanks Bernd
On Sun, Aug 9, 2009 at 12:27 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > Now that I've started to read this patch ... exactly what is the > argument for allowing a "mixed" notation (some of the parameters named > and some not)? ISTM that just serves to complicate both the patch > and the user's-eye view, for no real benefit. Wow, I can't imagine not supporting that. Doesn't every language that supports anything like named parameters also support a mix? LISP is the obvious example, but I think most major scripting languages (Perl, Ruby, etc.) support something akin to this through a trailing hash argument. C# apparently supports both for something called attribute classes (don't ask me what they are). Ada also supports both styles, and you can mix them. http://msdn.microsoft.com/en-us/library/aa664614(VS.71).aspx http://goanna.cs.rmit.edu.au/~dale/ada/aln/8_subprograms.html#RTFToC11 The case where this is really useful is when you have lots of parameters most of which don't need to be set to a non-default value most of the time, but a few of which always need to be specified. In C you might do something like this: int frobozz(int common1, int common2, struct frobozz_options *fopt); /* fopt == NULL for defaults */ void init_frobozz_options(struct frobozz_options *); ...but the named/mixed syntax is more compact, and certainly nice to have, even if I'm emphatically not in love with the syntax we're stuck with. ISTM that the Ada rule that all positional must proceed all named is a good one, and to resolve the problem you're talking about here it seems we should probably also make a rule that specifying a value for the same parameter more than once (either both position and named, or named twice) is an error. It would also be quite reasonable to impose a requirement that parameters can only be specified using named notation if it has been explicitly enabled for that particular parameter. In LISP, there are four kinds of parameters (required, optional, rest, keyword) and any particular parameter gets to be exactly one of those four things. The rule for resolving ambiguities may not be what you want, but it's well-defined and pretty reasonable. http://gigamonkeys.com/book/functions.html#mixing-different-parameter-types ...Robert
Robert Haas <robertmhaas@gmail.com> writes: > On Sun, Aug 9, 2009 at 12:27 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Now that I've started to read this patch ... exactly what is the >> argument for allowing a "mixed" notation (some of the parameters named >> and some not)? ISTM that just serves to complicate both the patch >> and the user's-eye view, for no real benefit. > Wow, I can't imagine not supporting that. Doesn't every language that > supports anything like named parameters also support a mix? I don't know if that's true, and I definitely don't know if they all handle corner cases identically. I think this patch is an exercise in guessing at what the SQL committee will eventually do, and as such, we should avoid like the plague making any guesses that carry significant risk of being semantically incompatible with what they eventually do. The risk/reward ratio isn't good enough. regards, tom lane
On Sun, Aug 9, 2009 at 2:17 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Sun, Aug 9, 2009 at 12:27 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >>> Now that I've started to read this patch ... exactly what is the >>> argument for allowing a "mixed" notation (some of the parameters named >>> and some not)? ISTM that just serves to complicate both the patch >>> and the user's-eye view, for no real benefit. > >> Wow, I can't imagine not supporting that. Doesn't every language that >> supports anything like named parameters also support a mix? > > I don't know if that's true, [...] I'm fairly sure it is. > and I definitely don't know if they all > handle corner cases identically. I'm 100% positive that they don't. > I think this patch is an exercise in > guessing at what the SQL committee will eventually do, and as such, we > should avoid like the plague making any guesses that carry significant > risk of being semantically incompatible with what they eventually do. > The risk/reward ratio isn't good enough. I completely agree; I would have chosen to boot the entire patch for exactly that reason. However, given that you don't seem to want to do that, I was just offering my thoughts on the various possible methods of eliminating that risk, since the one you're suggesting doesn't seem ideal to me. I thought that perhaps providing some examples of how other programming languages handle this problem might be helpful - in particular, I think the Lisp model, where each parameter must be EITHER named or positional, has a lot to recommend it. There is little doubt here that what gets committed here will be what you choose to commit. ...Robert
Robert Haas <robertmhaas@gmail.com> writes: > On Sun, Aug 9, 2009 at 2:17 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> I think this patch is an exercise in >> guessing at what the SQL committee will eventually do, and as such, we >> should avoid like the plague making any guesses that carry significant >> risk of being semantically incompatible with what they eventually do. >> The risk/reward ratio isn't good enough. > I completely agree; I would have chosen to boot the entire patch for > exactly that reason. Well, that's certainly still an available option. But people have been asking for this type of feature for a long time. I think we should be willing to include something along this line. What I don't want to do is include something that might be semantically incompatible with the eventual standard --- by which I mean accepting a call that the standard also accepts, but specifies doing something different than what we do. I'd prefer to omit inessential functionality rather than risk that. It might be that the patch does, or can be made to, throw error in any case that's even slightly questionable, while still allowing mixed cases that seem certain to have only one possible interpretation. But I'm not convinced that's where it's at today. regards, tom lane
Hello 2009/8/9 Tom Lane <tgl@sss.pgh.pa.us>: > Now that I've started to read this patch ... exactly what is the > argument for allowing a "mixed" notation (some of the parameters named > and some not)? ISTM that just serves to complicate both the patch > and the user's-eye view, for no real benefit. > consider function like foo(mandatory params without defaults, optional params with defaults) because you have to put all mandatory params, then names are needless. But some optional params you have to specify by names, because without names, you have to put full params set with respect to rule about using default params. CREATE OR REPLACE FUNCTION strtr(a varchar, uppercase boolean = false, lowercase boolean = false) RETURNS varchar AS $$ BEGIN IF uppercase and lowercase THEN RAISE EXCEPTION 'cannot use both modificator together' END IF; IF uppercase THEN RETURN upper(a); END IF; IF lowercase THEN RETURN lower(a); END IF; RETURN a; END IF; $$ LANGUAGE plpgsql IMMUTABLE STRICT; the advice is verbosity: SELECT strtr('some text',true, false); versus SELECT strtr('some text', true AS uppercase); or SELECT strtr('some text', true AS lowercase); With mixed notation is very clean border betwenn mandatory and optional params. I thing, so without mixed notation this patch hasn't any sense and I thing it's better to drop it. > Considering that we are worried about someday having to adjust to a > SQL standard in this area, I think we ought to be as conservative as > possible about what we introduce as user-visible features here. > As an example, if they do go with "=>" as the parameter marker, > mixed notation would become a seriously bad idea because it would be > impossible to distinguish incidental use of => as an operator from > mixed notation. I am sorry, I don't understand. When => should be some operator, then you cannot distinguish between named notation and positional notation too. Mixed notation doesn't play any role. foo(a => 10, b=>20) should be code in positional notation much like named notation. ??? How is difference? I thing so when some body use operator =>, then he have to break standard notation for some collision situation or for all situation. Syntax with AS is safe and should be enabled anywhere. We can simply detect situation where operator => exists and standard named parameters are allowed. I thing, so we are on safe side, because we should to support both syntax, and can disable temporary one ambiguous. regards Pavel Stehule > > regards, tom lane >
> >> Considering that we are worried about someday having to adjust to a >> SQL standard in this area, I think we ought to be as conservative as >> possible about what we introduce as user-visible features here. >> As an example, if they do go with "=>" as the parameter marker, >> mixed notation would become a seriously bad idea because it would be >> impossible to distinguish incidental use of => as an operator from >> mixed notation. > I thing, so ANSI will be in conformance with Oracle - so I'll try to check the possibility of the using => as any operator in Oracle regards Pavel Stehule
Robert Haas <robertmhaas@gmail.com> wrote: > On Sun, Aug 9, 2009 at 12:27 PM, Tom Lane<tgl@sss.pgh.pa.us> wrote: >> Now that I've started to read this patch ... exactly what is the >> argument for allowing a "mixed" notation (some of the parameters >> named and some not)? ISTM that just serves to complicate both the >> patch and the user's-eye view, for no real benefit. > > Wow, I can't imagine not supporting that. Doesn't every language > that supports anything like named parameters also support a mix? Sybase ASE and Microsoft SQL Server support mixed notation (and I think that goes back to their common version 1.0). If a parameter is specified more than once, it is an error. -Kevin