Re: [HACKERS] Issues for named/mixed function notation patch
2010/2/23 Bruce Momjian br...@momjian.us: Can someone work on a patch to implement the document changes suggested below? This patch is useless now. There are no this issue now, because we have integrated true SQL parser. Regards Pavel --- Jeff Davis wrote: On Tue, 2009-09-15 at 10:51 +0200, Pavel Stehule wrote: My renonc, please, try new patch. I forgot mark regproc.c file. I think the documentation around calling functions is disorganized: Variadic functions, functions with defaults, SRFs, out parameters, and polymorphism are all explained in 34.4, which is about SQL functions specifically. Overloading is in chapter 34 also, but not specifically in the SQL function section like the rest. Function calls themselves are only given 5 lines of explanation in 4.2.6, with no mention of things like the VARIADIC keyword. These complaints aren't about the patch, but we might want to consider some reorganization of those sections (probably a separate doc patch). The interaction with variadic functions appears to be misdocumented. From the code and tests, the VARIADIC keyword appears to be optional when using named notation, but required when using positional notation. But the documentation says: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. I'm still reviewing the code. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.us http://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Pavel Stehule wrote: 2010/2/23 Bruce Momjian br...@momjian.us: Can someone work on a patch to implement the document changes suggested below? This patch is useless now. There are no this issue now, because we have integrated true SQL parser. Great, thanks. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Tue, 2010-02-23 at 09:34 -0500, Bruce Momjian wrote: Pavel Stehule wrote: 2010/2/23 Bruce Momjian br...@momjian.us: Can someone work on a patch to implement the document changes suggested below? This patch is useless now. There are no this issue now, because we have integrated true SQL parser. Great, thanks. I believe a documentation issue still exists here. The section on calling functions (4.3) still says nothing about VARIADIC. Also, it's not 100% clear to me where function overloading should go, but perhaps it should be mentioned in that section as well. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2010/2/24 Jeff Davis pg...@j-davis.com: On Tue, 2010-02-23 at 09:34 -0500, Bruce Momjian wrote: Pavel Stehule wrote: 2010/2/23 Bruce Momjian br...@momjian.us: Can someone work on a patch to implement the document changes suggested below? This patch is useless now. There are no this issue now, because we have integrated true SQL parser. Great, thanks. I believe a documentation issue still exists here. The section on calling functions (4.3) still says nothing about VARIADIC. Also, it's not 100% clear to me where function overloading should go, but perhaps it should be mentioned in that section as well. please, can you write patch. For me it is mession impossible :) Pavel Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Can someone work on a patch to implement the document changes suggested below? --- Jeff Davis wrote: On Tue, 2009-09-15 at 10:51 +0200, Pavel Stehule wrote: My renonc, please, try new patch. I forgot mark regproc.c file. I think the documentation around calling functions is disorganized: Variadic functions, functions with defaults, SRFs, out parameters, and polymorphism are all explained in 34.4, which is about SQL functions specifically. Overloading is in chapter 34 also, but not specifically in the SQL function section like the rest. Function calls themselves are only given 5 lines of explanation in 4.2.6, with no mention of things like the VARIADIC keyword. These complaints aren't about the patch, but we might want to consider some reorganization of those sections (probably a separate doc patch). The interaction with variadic functions appears to be misdocumented. From the code and tests, the VARIADIC keyword appears to be optional when using named notation, but required when using positional notation. But the documentation says: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. I'm still reviewing the code. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Oct 7, 2009, at 7:41 PM, Tom Lane wrote: Pavel Stehule pavel.steh...@gmail.com writes: [ latest named-args patch ] Committed with a fair amount of corner-case cleanup and refactoring. Woot! Thanks for all the hard work getting this committed (Pavel, Bernd, Jeff, Tom and others)! I've been really looking forward to this feature. Still hoping a solution is found to the plpgsql parser issue. If not, I'll have to resubmit my rejected AS patch. :) -Steve -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Oct 7, 2009, at 9:00 PM, Steve Prentice wrote: Committed with a fair amount of corner-case cleanup and refactoring. Woot! Thanks for all the hard work getting this committed (Pavel, Bernd, Jeff, Tom and others)! I've been really looking forward to this feature. Still hoping a solution is found to the plpgsql parser issue. If not, I'll have to resubmit my rejected AS patch. :) +1 Thanks for getting this done. Now, does this just apply to PL/pgSQL? If so, what needs to happen for other PLs to support the feature? Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Thu, 2009-10-08 at 09:44 -0700, David E. Wheeler wrote: +1 Thanks for getting this done. Now, does this just apply to PL/pgSQL? If so, what needs to happen for other PLs to support the feature? It's just the call notation -- the function only needs to know what arguments it got for which parameters. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Oct 8, 2009, at 9:47 AM, Jeff Davis wrote: It's just the call notation -- the function only needs to know what arguments it got for which parameters. So they're still ordered? I'm thinking of PL/Perl here… David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
David E. Wheeler da...@kineticode.com writes: On Oct 8, 2009, at 9:47 AM, Jeff Davis wrote: It's just the call notation -- the function only needs to know what arguments it got for which parameters. So they're still ordered? I'm thinking of PL/Perl here It's PL-independent as far as I know --- if you find something where it doesn't work, that's a bug. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/8 David E. Wheeler da...@kineticode.com: On Oct 7, 2009, at 9:00 PM, Steve Prentice wrote: Committed with a fair amount of corner-case cleanup and refactoring. Woot! Thanks for all the hard work getting this committed (Pavel, Bernd, Jeff, Tom and others)! I've been really looking forward to this feature. Still hoping a solution is found to the plpgsql parser issue. If not, I'll have to resubmit my rejected AS patch. :) +1 Thanks for getting this done. Now, does this just apply to PL/pgSQL? If so, what needs to happen for other PLs to support the feature? For other PL is named notation transparent (like defaults). Problem is only with PL/pgSQL. I spend some time with integration main SQL parser to PL/pgSQL and this is little bit worse then I thougs. The code is more ugly - we have to swith between two lexers and problem is with $1.x elements. I hope, so I'll send patch to next commitfest. Then we can choise between Steve's patch or my patch. Pavel Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Pavel Stehule pavel.steh...@gmail.com writes: Sorry, I'm having trouble understanding what you're driving at here. I think we should just not allow named notation to be combined with VARIADIC, at least for a first version of this feature, either when defining a function or when calling one. Â We can consider relaxing that restriction at a later date if we can agree on what the semantics should be. This is maybe too strict. I thing, so safe version is allow variadic packed parameter with VARIADIC keyword as Jeff proposes. I'm working through this patch now, and I find myself not very satisfied on the question of variadic versus named arguments. What the latest patch actually does is: * completely ignores variadic functions when trying to match a call having any named arguments * does not throw an error for use of the VARIADIC keyword in a call together with named arguments Neither of these behaviors quite seem to me to satisfy the principle of least astonishment, and in combination they definitely do not. It seems to me that there is not anything wrong with using named arguments together with VARIADIC and getting a match to a variadic function. VARIADIC in the argument list essentially turns off the special behavior of variadic functions, and after that you might as well allow either named or positional matching. (I guess if you wanted to be really strict you'd insist that the VARIADIC keyword be attached to the specific named argument that matches the variadic parameter, but I don't mind being a bit lax there.) When VARIADIC is not specified, then I think that silently ignoring variadic functions for a named-argument call is probably reasonable. This can be argued by imagining that the function's implicit array element parameters do not have any names (the variadic array parameter might have a name, but the elements generated from it do not). Since these must be at the right end of the effective parameter list, and we only allow named arguments at the right of the call list, there is no way for the named arguments to match non-variadic named parameters and still have anything matching to the variadic array elements. Therefore a variadic function can never match such a call and ignoring it isn't surprising. Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/7 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: Sorry, I'm having trouble understanding what you're driving at here. I think we should just not allow named notation to be combined with VARIADIC, at least for a first version of this feature, either when defining a function or when calling one. We can consider relaxing that restriction at a later date if we can agree on what the semantics should be. This is maybe too strict. I thing, so safe version is allow variadic packed parameter with VARIADIC keyword as Jeff proposes. I'm working through this patch now, and I find myself not very satisfied on the question of variadic versus named arguments. What the latest patch actually does is: * completely ignores variadic functions when trying to match a call having any named arguments * does not throw an error for use of the VARIADIC keyword in a call together with named arguments Neither of these behaviors quite seem to me to satisfy the principle of least astonishment, and in combination they definitely do not. It seems to me that there is not anything wrong with using named arguments together with VARIADIC and getting a match to a variadic function. VARIADIC in the argument list essentially turns off the special behavior of variadic functions, and after that you might as well allow either named or positional matching. (I guess if you wanted to be really strict you'd insist that the VARIADIC keyword be attached to the specific named argument that matches the variadic parameter, but I don't mind being a bit lax there.) When VARIADIC is not specified, then I think that silently ignoring variadic functions for a named-argument call is probably reasonable. This can be argued by imagining that the function's implicit array element parameters do not have any names (the variadic array parameter might have a name, but the elements generated from it do not). Since these must be at the right end of the effective parameter list, and we only allow named arguments at the right of the call list, there is no way for the named arguments to match non-variadic named parameters and still have anything matching to the variadic array elements. Therefore a variadic function can never match such a call and ignoring it isn't surprising. It's same as my origin ideas, much better formulated. It is ok for me. Pavel Comments? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Wed, 2009-10-07 at 16:58 -0400, Tom Lane wrote: * completely ignores variadic functions when trying to match a call having any named arguments * does not throw an error for use of the VARIADIC keyword in a call together with named arguments Neither of these behaviors quite seem to me to satisfy the principle of least astonishment, and in combination they definitely do not. I agree that the combination is wrong, and we should either allow that call notation or throw a useful error message when it's attempted. It seems to me that there is not anything wrong with using named arguments together with VARIADIC and getting a match to a variadic function. The general feeling was that we should only support the most obvious call notations so we wouldn't have backwards compatibility problems if we tried to change it later. If we allow calling a variadic function using named notation, the VARIADIC keyword is not strictly necessary, but I think we should require it. It seems strange without it. Pavel indicated that there may be some implementation difficulty in requiring the VARIADIC keyword when calling a variadic function using named notation: http://archives.postgresql.org/pgsql-hackers/2009-09/msg01792.php and that just kind of pushed the idea from maybe that's OK to probably not a good idea right now. Robert Haas weighed in here: http://archives.postgresql.org/pgsql-hackers/2009-09/msg01732.php Its fine with me to allow it, assuming there's a reasonable way to implement it, and assuming that the VARIADIC keyword is required. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Jeff Davis pg...@j-davis.com writes: If we allow calling a variadic function using named notation, the VARIADIC keyword is not strictly necessary, but I think we should require it. It seems strange without it. Yeah. My first thought was to just remove the check in FuncnameGetCandidates, which would have the effect of matching with or without VARIADIC. It would be self-consistent but probably surprising. But it should just be a small tweak to match only with VARIADIC. Pavel indicated that there may be some implementation difficulty in requiring the VARIADIC keyword when calling a variadic function using named notation: I think what he was considering was the question of insisting that the VARIADIC keyword be attached to the named argument that actually matches the VARIADIC parameter. I think we could do it, but it might be a bit of a wart. I notice that right now, an unnecessary VARIADIC keyword in a regular positional call does not cause an error, it's just ignored --- so we're already being a bit lax with it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Wed, 2009-10-07 at 23:32 +0200, Pavel Stehule wrote: It's same as my origin ideas, much better formulated. It is ok for me. You indicated that there may be some implementation difficulty if the VARIADIC keyword is required for calling using named notation: http://archives.postgresql.org/pgsql-hackers/2009-09/msg01792.php Do you think it would be reasonable to implement? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Wed, 2009-10-07 at 17:45 -0400, Tom Lane wrote: I think what he was considering was the question of insisting that the VARIADIC keyword be attached to the named argument that actually matches the VARIADIC parameter. I think we could do it, but it might be a bit of a wart. I notice that right now, an unnecessary VARIADIC keyword in a regular positional call does not cause an error, it's just ignored --- so we're already being a bit lax with it. From a semantic standpoint, I lean towards requiring the VARIADIC keyword consistently between named and positional notation. It seems strange to me if we have a situation where changing the call: foo(a, b, VARIADIC c) to be more explicit by using named call notation: foo(a AS x, b AS y, VARIADIC c AS z) is less correct in the sense that the VARIADIC keyword goes from required to ignored. Also, requiring VARIADIC seems to guard us better against future changes, which seemed like a concern before. I don't have a strong opinion or a specific problem with making VARIADIC optional, so it's OK with me. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Jeff Davis pg...@j-davis.com writes: On Wed, 2009-10-07 at 17:45 -0400, Tom Lane wrote: I think what he was considering was the question of insisting that the VARIADIC keyword be attached to the named argument that actually matches the VARIADIC parameter. It seems strange to me if we have a situation where changing the call: foo(a, b, VARIADIC c) to be more explicit by using named call notation: foo(a AS x, b AS y, VARIADIC c AS z) is less correct in the sense that the VARIADIC keyword goes from required to ignored. No, that's not what I'm driving at. The small change that I've got in mind would require you to say VARIADIC, but it would allow the function to match both the above call and foo(a AS x, c AS z, VARIADIC b AS y) when really z is the variadic parameter in this case. I'm not sure if this would bother anyone or not. It seems impossible that a function could ever have more than one variadic parameter, so there's not really any ambiguity from maintaining the syntactic rule that the VARIADIC keyword is at the end even when the variadic argument isn't, but it might look a bit odd. What I *don't* want to do is fix this by allowing/requiring foo(a AS x, VARIADIC c AS z, b AS y) because it would be a bigger change in the grammar output structure than seems warranted. We could possibly have VARIADIC throw an error if the named argument that matches to the variadic parameter isn't the last one, but I'm not sure that that's important rather than just pedantry. People would probably tend to write it that way anyway. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/8 Tom Lane t...@sss.pgh.pa.us: Jeff Davis pg...@j-davis.com writes: On Wed, 2009-10-07 at 17:45 -0400, Tom Lane wrote: I think what he was considering was the question of insisting that the VARIADIC keyword be attached to the named argument that actually matches the VARIADIC parameter. It seems strange to me if we have a situation where changing the call: foo(a, b, VARIADIC c) to be more explicit by using named call notation: foo(a AS x, b AS y, VARIADIC c AS z) is less correct in the sense that the VARIADIC keyword goes from required to ignored. No, that's not what I'm driving at. The small change that I've got in mind would require you to say VARIADIC, but it would allow the function to match both the above call and foo(a AS x, c AS z, VARIADIC b AS y) when really z is the variadic parameter in this case. I'm not sure if this would bother anyone or not. It seems impossible that a function could ever have more than one variadic parameter, so there's not really any ambiguity from maintaining the syntactic rule that the VARIADIC keyword is at the end even when the variadic argument isn't, but it might look a bit odd. What I *don't* want to do is fix this by allowing/requiring foo(a AS x, VARIADIC c AS z, b AS y) because it would be a bigger change in the grammar output structure than seems warranted. We could possibly have VARIADIC throw an error if the named argument that matches to the variadic parameter isn't the last one, but I'm not sure that that's important rather than just pedantry. People would probably tend to write it that way anyway. It is just pedantry. Position in named notation isn't important, and there are no reason, why we VARIADIC should be last. Pavel regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/7 Jeff Davis pg...@j-davis.com: On Wed, 2009-10-07 at 23:32 +0200, Pavel Stehule wrote: It's same as my origin ideas, much better formulated. It is ok for me. You indicated that there may be some implementation difficulty if the VARIADIC keyword is required for calling using named notation: http://archives.postgresql.org/pgsql-hackers/2009-09/msg01792.php Do you think it would be reasonable to implement? I thing, so this is possible. But it needs some instructions more. I would not add some unnecessary checks. It needs one cycle over parameters more (and one array). * check if last variadic parameter isn't default * check if last variadic parameter has flag VARIADIC * check if there are not any other parameter with VARIADIC flag * some correction in gram.y (procedural code), that allows VARIADIC in any position when named notation is active. Pavel Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Wed, 2009-10-07 at 18:17 -0400, Tom Lane wrote: No, that's not what I'm driving at. The small change that I've got in mind would require you to say VARIADIC, but it would allow the function to match both the above call and foo(a AS x, c AS z, VARIADIC b AS y) when really z is the variadic parameter in this case. I'm not sure if this would bother anyone or not. It seems impossible that a function could ever have more than one variadic parameter, so there's not really any ambiguity from maintaining the syntactic rule that the VARIADIC keyword is at the end even when the variadic argument isn't, but it might look a bit odd. I'm worried about allowing such strange notation. Someone might have a new idea later that conflicts with it, and then we have a backwards-compatibility problem. What I *don't* want to do is fix this by allowing/requiring foo(a AS x, VARIADIC c AS z, b AS y) because it would be a bigger change in the grammar output structure than seems warranted. If it's the right thing to do (or might be the right thing to do), someone will want to do that later, and that would be incompatible with the: foo(a AS x, c AS z, VARIADIC b AS y) notation (where z is the variadic parameter). We could possibly have VARIADIC throw an error if the named argument that matches to the variadic parameter isn't the last one, but I'm not sure that that's important rather than just pedantry. I would prefer such a restriction if it's reasonable to do. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Wed, Oct 7, 2009 at 5:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: I think what he was considering was the question of insisting that the VARIADIC keyword be attached to the named argument that actually matches the VARIADIC parameter. I think we could do it, but it might be a bit of a wart. I notice that right now, an unnecessary VARIADIC keyword in a regular positional call does not cause an error, it's just ignored --- so we're already being a bit lax with it. I'd be more inclined to to tighten up the place where we're currently being lax than to treat additional situations in a similarly lax manner. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Jeff Davis pg...@j-davis.com writes: We could possibly have VARIADIC throw an error if the named argument that matches to the variadic parameter isn't the last one, but I'm not sure that that's important rather than just pedantry. I would prefer such a restriction if it's reasonable to do. [ pokes around ... ] It seems to only take a few more lines, so will do. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Pavel Stehule pavel.steh...@gmail.com writes: [ latest named-args patch ] Committed with a fair amount of corner-case cleanup and refactoring. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/8 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: [ latest named-args patch ] Committed with a fair amount of corner-case cleanup and refactoring. regards, tom lane Thank you Pavel Stehule -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Thu, 2009-10-01 at 17:56 +1000, Brendan Jurd wrote:
I've had a look through the documentation and cleaned up a few things.
Thanks, that is helpful. I have included that along with some other
comment updates in the attached patch.
Paval,
In ProcedureCreate(), you match the argument names to see if anything
changed (in which case you raise an error). The code for that looks more
complex than I expected because it keeps track of the two argument lists
using different array indexes (i and j). Is there a reason it you can't
just iterate through with something like:
if (p_oldargmodes[i] == PROARGMODE_OUT ||
p_oldargmodes[i] == PROARGMODE_TABLE)
continue;
if (strcmp(p_oldargnames[i], p_names[i]) != 0)
elog(ERROR, ...
I'm oversimplifying, of course, but I don't understand why you need both
i and j. Also, there is some unnecessarily verbose code like:
if (p_modes == NULL || (p_modes != NULL ...
In func_get_detail(), there is:
/* shouldn't happen, FuncnameGetCandidates messed up */
if (best_candidate-ndargs pform-pronargdefaults)
elog(ERROR, not enough default arguments);
Why is it only an error if ndargs is greater? Shouldn't they be equal?
/*
* Actually only first nargs field of best_candidate-args is valid,
* Now, we have to refresh last ndargs items.
*/
Can you explain the above comment?
Please review Brendan and my patches and combine them with your patch.
Regards,
Jeff Davis
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index 50c4128..542646d 100644
--- a/doc/src/sgml/syntax.sgml
+++ b/doc/src/sgml/syntax.sgml
@@ -1505,6 +1505,10 @@ sqrt(2)
The list of built-in functions is in xref linkend=functions.
Other functions can be added by the user.
/para
+
+ para
+ See xref linkend=extend for more details.
+ /para
/sect2
sect2 id=syntax-aggregates
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 9e8ccfa..1c06885 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -651,21 +651,19 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
int effective_nargs;
int pathpos = 0;
bool variadic;
- bool use_defaults = false; /* be compiler quiet */
+ bool use_defaults = false; /* make compiler quiet */
Oid va_elem_type;
FuncCandidateList newResult;
int *proargidxs = NULL;
- /* Try to attach names, when mixed or named notation is used. */
+ /* Try to attach names when mixed or named notation is used. */
if (argnames != NIL)
{
/*
- * Mixed or named notation
+ * Mixed or named notation
*
- * We would to disable an call of variadic function with named
- * or mixed notation, because it could be messy for users. We
- * would to allow only unique arg names, and this is useles for
- * variadic functions.
+ * Variadic functions can't be called using named or mixed
+ * notation.
*/
if (OidIsValid(procform-provariadic))
continue;
@@ -760,9 +758,12 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
newResult-nargs = effective_nargs;
/*
- * Wait with apply proargidxs on args. Detection ambigouos needs
- * consistent args (based on proargs). Store proargidxs for later
- * use.
+ * Save proargidxs in newResult. It's needed later to adjust
+ * the argument types to be the types corresponding to the
+ * named arguments (if any), and also to assign positions to
+ * any NamedArgExpr arguments after the best candidate is
+ * determined. The former could be done here, but we leave
+ * both for the caller to do.
*/
newResult-proargidxs = proargidxs;
memcpy(newResult-args, procform-proargtypes.values,
@@ -980,8 +981,9 @@ VerifyCandidateNamedNotation(HeapTuple proctup, int pronargs, int nargs, List *a
Assert(p_argnames != NULL);
/*
- * A number less or equal nargs means explicit arguments,
- */
+ * pronargs equal to nargs means explicit arguments (no defaults)
+ */
+
*proargidxs = palloc(nargs * sizeof(int));
for (i = 0; i pronargs; i++)
argfilling[i] = false;
@@ -1004,7 +1006,7 @@ VerifyCandidateNamedNotation(HeapTuple proctup, int pronargs, int nargs, List *a
continue;
if (p_argnames[i] strcmp(p_argnames[i], argname) == 0)
{
- /* protect us against duplicated entries from bad written mixed notation */
+ /* protect us against duplicated entries from badly written mixed notation */
if (argfilling[pp])
return false;
@@ -1035,9 +1037,13 @@ VerifyCandidateNamedNotation(HeapTuple proctup, int pronargs, int nargs, List *a
ap++;
}
+ /*
+ * This function is only called for named and mixed notation, and
+ * the last argument must be named in either case.
+ */
Assert(notation == CALL_NOTATION_NAMED);
- /* Check for default arguments ? */
+
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/2 Jeff Davis pg...@j-davis.com: On Thu, 2009-10-01 at 17:56 +1000, Brendan Jurd wrote: I've had a look through the documentation and cleaned up a few things. Thanks, that is helpful. I have included that along with some other comment updates in the attached patch. Paval, In ProcedureCreate(), you match the argument names to see if anything changed (in which case you raise an error). The code for that looks more complex than I expected because it keeps track of the two argument lists using different array indexes (i and j). Is there a reason it you can't just iterate through with something like: if (p_oldargmodes[i] == PROARGMODE_OUT || p_oldargmodes[i] == PROARGMODE_TABLE) continue; if (strcmp(p_oldargnames[i], p_names[i]) != 0) elog(ERROR, ... I testing visible interface from outside. from outside the following functions are same: foo1(in a1, in a2, in a3, out a1, out a2, out a3) foo2(in a1, out a1, in a2, out a2, in a3, out a3) so the used test is immune to this change. I'm oversimplifying, of course, but I don't understand why you need both i and j. Also, there is some unnecessarily verbose code like: if (p_modes == NULL || (p_modes != NULL ... when p_modes is null,then all arguments are input. So input parameter is when p_modes is null (all parameters are input) or is in, inout, variadic. In func_get_detail(), there is: /* shouldn't happen, FuncnameGetCandidates messed up */ if (best_candidate-ndargs pform-pronargdefaults) elog(ERROR, not enough default arguments); best_candidate-ndargs ~ use n default values, it have to be less or equal than declared defaults in pgproc. Why is it only an error if ndargs is greater? Shouldn't they be equal? ndargs == pronargdefaults is correct - it means, use all declared defaults. But, raise exception when you would to use more defaults than is declared. /* * Actually only first nargs field of best_candidate-args is valid, * Now, we have to refresh last ndargs items. */ Can you explain the above comment? this is not good formulation. It means, so in this moment we processed nargs fields, and we have to process others ndargs fields. But i named or mixed notation, the processed fields should not be first nargs fields (like positional notation). There should be a gap, that are filled by defaults. Gaps are identified via bitmap created on cycle above. In this cycle is processed positional parameters (with increasing i) and named parameters. Because positional parameters have to be front of named parameters, then we don't need increase i when process named p., Please review Brendan and my patches and combine them with your patch. yes, I'll go on this evening, thank you. Pavel Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Fri, 2009-10-02 at 16:06 +0200, Pavel Stehule wrote: see attachment, please Thank you, marked as ready for committer. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/9/30 Pavel Stehule pavel.steh...@gmail.com: So I dropped variadic functions from mixed/named notation and little bit modified documentation. Please, can some native English speaker look on documentation? Hi Pavel, I've had a look through the documentation and cleaned up a few things. I changed the chapter heading from Positional and named notation to Calling Functions. I felt that the original heading would not give a clear hint about the subject matter to anyone who wasn't already familiar with the terminology. The rest of my changes are rephrasing sentences, fixing spelling and grammar, and cleaning up indentation and wrapping. The code examples are not changed and I haven't meddled with the structure of the chapter. I hope you find this helpful. Cheers, BJ named-notation-doc-fixes.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/10/1 Brendan Jurd dire...@gmail.com: 2009/9/30 Pavel Stehule pavel.steh...@gmail.com: So I dropped variadic functions from mixed/named notation and little bit modified documentation. Please, can some native English speaker look on documentation? Hi Pavel, I've had a look through the documentation and cleaned up a few things. I changed the chapter heading from Positional and named notation to Calling Functions. I felt that the original heading would not give a clear hint about the subject matter to anyone who wasn't already familiar with the terminology. The rest of my changes are rephrasing sentences, fixing spelling and grammar, and cleaning up indentation and wrapping. The code examples are not changed and I haven't meddled with the structure of the chapter. I hope you find this helpful. yes, it's very helpful. I thing, so there is all what is important. thank you Pavel Cheers, BJ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Sorry, I'm having trouble understanding what you're driving at here. I think we should just not allow named notation to be combined with VARIADIC, at least for a first version of this feature, either when defining a function or when calling one. We can consider relaxing that restriction at a later date if we can agree on what the semantics should be. This is maybe too strict. I thing, so safe version is allow variadic packed parameter with VARIADIC keyword as Jeff proposes. I'll send actualised patch today. Pavel ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Mon, 2009-09-28 at 11:50 +0200, Pavel Stehule wrote: This is maybe too strict. I thing, so safe version is allow variadic packed parameter with VARIADIC keyword as Jeff proposes. The combination of variadic parameters and named call notation is somewhat strange, on second thought. Can you identify a use case? If not, then it should probably be blocked in this version of the patch. Even if it makes sense from a syntax standpoint, it might be confusing to users. Robert, did you have a specific concern in mind? Do you see a behavior there that we might want to change in the future? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/9/28 Jeff Davis pg...@j-davis.com: On Mon, 2009-09-28 at 11:50 +0200, Pavel Stehule wrote: This is maybe too strict. I thing, so safe version is allow variadic packed parameter with VARIADIC keyword as Jeff proposes. The combination of variadic parameters and named call notation is somewhat strange, on second thought. Can you identify a use case? I have not any use case now. Simply when I have a variadic function, then I would to allow call it with named notation. Some like create or replace foo (a int, variadic b int[]) ... SELECT foo(10 as int, variadic array[10,20] as b) If not, then it should probably be blocked in this version of the patch. Even if it makes sense from a syntax standpoint, it might be confusing to users. when I though about control, I found so syntax with mandatory VARIADIC is difficult implementable. So probably the most feasible solution for this moment is to discard a variadic functions from set of functions that are callable with named notation. So I thing we are in tune, and I am going to update patch. Regards Pavel Stehule Robert, did you have a specific concern in mind? Do you see a behavior there that we might want to change in the future? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Mon, 2009-09-28 at 18:23 +0200, Pavel Stehule wrote: when I though about control, I found so syntax with mandatory VARIADIC is difficult implementable. So probably the most feasible solution for this moment is to discard a variadic functions from set of functions that are callable with named notation. So I thing we are in tune, and I am going to update patch. Sounds good. I am looking at the code, and there's a part I don't understand: In FuncnameGetCandidates(): /* * Wait with apply proargidxs on args. Detection ambigouos needs * consistent args (based on proargs). Store proargidxs for later * use. */ newResult-proargidxs = proargidxs; But after calling FuncnameGetCandidates (the only place where fargnames is non-NIL), you immediately re-assign to best_candidate-args. What happens between those two places, and why can't it happen in FuncnameGetCandidates? Also, you should consistently pass NIL when you mean an empty list, but sometimes you pass NULL to FuncnameGetCandidates(). Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/9/28 Jeff Davis pg...@j-davis.com: On Mon, 2009-09-28 at 18:23 +0200, Pavel Stehule wrote: when I though about control, I found so syntax with mandatory VARIADIC is difficult implementable. So probably the most feasible solution for this moment is to discard a variadic functions from set of functions that are callable with named notation. So I thing we are in tune, and I am going to update patch. Sounds good. I am looking at the code, and there's a part I don't understand: In FuncnameGetCandidates(): /* * Wait with apply proargidxs on args. Detection ambigouos needs * consistent args (based on proargs). Store proargidxs for later * use. */ newResult-proargidxs = proargidxs; But after calling FuncnameGetCandidates (the only place where fargnames is non-NIL), you immediately re-assign to best_candidate-args. What happens between those two places, and why can't it happen in FuncnameGetCandidates? I am not sure - I have to look to code, but if I remember well, there are same arrays, with same values, but the field are different order. One is related to pgproc and second to real params. But I have to check code again. Also, you should consistently pass NIL when you mean an empty list, but sometimes you pass NULL to FuncnameGetCandidates(). It's bug, where is it? Regards Pavel Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Mon, 2009-09-28 at 19:26 +0200, Pavel Stehule wrote: Also, you should consistently pass NIL when you mean an empty list, but sometimes you pass NULL to FuncnameGetCandidates(). It's bug, where is it? In regproc.c. Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule pavel.steh...@gmail.com wrote: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. maybe we could to support variadic named parameters in future - then using VARIADIC keyword should be necessary - like foo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3) Pavel, This doesn't make sense to me, FWIW. I don't think we should allow parameters to be specified more than once. It's hard for me to imagine how that could be useful. I'm still reviewing the code. Jeff, When will you be able to post this review? Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/9/27 Robert Haas robertmh...@gmail.com: On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule pavel.steh...@gmail.com wrote: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. maybe we could to support variadic named parameters in future - then using VARIADIC keyword should be necessary - like foo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3) Pavel, This doesn't make sense to me, FWIW. I don't think we should allow parameters to be specified more than once. It's hard for me to imagine how that could be useful. ook I thing, so this should be little bit unclean too. I though why we need VARIADIC keyword mandatory for named notation. When we could specify only unique names, then we could use only one packed variadic parameter - and then VARIADIC keyword isn't necessary. Is this idea correct? I thing, so there are not problem ensure an using VARIADIC keyword in this context - but, personally I don't feel, so there it have to be. But I don't thing, so this is important (minimally for me) - I'll accept others opinions. Regards Pavel I'm still reviewing the code. Jeff, When will you be able to post this review? Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Sun, Sep 27, 2009 at 1:46 PM, Pavel Stehule pavel.steh...@gmail.com wrote: 2009/9/27 Robert Haas robertmh...@gmail.com: On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule pavel.steh...@gmail.com wrote: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. maybe we could to support variadic named parameters in future - then using VARIADIC keyword should be necessary - like foo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3) Pavel, This doesn't make sense to me, FWIW. I don't think we should allow parameters to be specified more than once. It's hard for me to imagine how that could be useful. ook I thing, so this should be little bit unclean too. I though why we need VARIADIC keyword mandatory for named notation. When we could specify only unique names, then we could use only one packed variadic parameter - and then VARIADIC keyword isn't necessary. Is this idea correct? I thing, so there are not problem ensure an using VARIADIC keyword in this context - but, personally I don't feel, so there it have to be. But I don't thing, so this is important (minimally for me) - I'll accept others opinions. Sorry, I'm having trouble understanding what you're driving at here. I think we should just not allow named notation to be combined with VARIADIC, at least for a first version of this feature, either when defining a function or when calling one. We can consider relaxing that restriction at a later date if we can agree on what the semantics should be. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Tue, 2009-09-15 at 10:51 +0200, Pavel Stehule wrote: My renonc, please, try new patch. I forgot mark regproc.c file. I think the documentation around calling functions is disorganized: Variadic functions, functions with defaults, SRFs, out parameters, and polymorphism are all explained in 34.4, which is about SQL functions specifically. Overloading is in chapter 34 also, but not specifically in the SQL function section like the rest. Function calls themselves are only given 5 lines of explanation in 4.2.6, with no mention of things like the VARIADIC keyword. These complaints aren't about the patch, but we might want to consider some reorganization of those sections (probably a separate doc patch). The interaction with variadic functions appears to be misdocumented. From the code and tests, the VARIADIC keyword appears to be optional when using named notation, but required when using positional notation. But the documentation says: However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. I'm still reviewing the code. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
However, a named variadic argument can only be called the way shown in the example above. The VARIADIC keyword must not be specified and a variadic notation of all arguments is not supported. To use variadic argument lists you must use positional notation instead. What is the intended behavior? I think we should always require VARIADIC to be specified regardless of using named notation. maybe we could to support variadic named parameters in future - then using VARIADIC keyword should be necessary - like foo(10 AS p1, 20 AS p1, 30 AS p3) is equalent of foo(VARIADIC ARRAY[10,20] AS p1, 30 AS p3) if we plan this feature, the VARIADIC keyword have to be mandatory. Regards Pavel Stehule I'm still reviewing the code. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Tue, Sep 15, 2009 at 4:51 AM, Pavel Stehule pavel.steh...@gmail.com wrote: Same problem. Build log attached. ...Robert My renonc, please, try new patch. I forgot mark regproc.c file. regards Pavel Stehule This one compiles for me. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Same problem. Build log attached. ...Robert My renonc, please, try new patch. I forgot mark regproc.c file. regards Pavel Stehule nm.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Hello Robert, 2009/9/14 Robert Haas robertmh...@gmail.com: On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I reworked patch to respect mentioned issues. - this patch still implement mixed notation - I am thing so this notation is really important. All others I respect. The behave is without change, fixed some bugs, enhanced regress tests. This does not compile. please, can you try this version? I hope so this in commitfest form too. I didn't do any changes, but it can be broken. I compiled attached patch today without problems. I have Federa 11. If you will have a problems still, please, send me log. Thank You Pavel ...Robert mnnotation.diff.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I reworked patch to respect mentioned issues. - this patch still implement mixed notation - I am thing so this notation is really important. All others I respect. The behave is without change, fixed some bugs, enhanced regress tests. This does not compile. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/9/14 Robert Haas robertmh...@gmail.com: On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule pavel.steh...@gmail.com wrote: I reworked patch to respect mentioned issues. - this patch still implement mixed notation - I am thing so this notation is really important. All others I respect. The behave is without change, fixed some bugs, enhanced regress tests. This does not compile. I'll recheck it today Pavel ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Hello,
I reworked patch to respect mentioned issues. - this patch still
implement mixed notation - I am thing so this notation is really
important. All others I respect. The behave is without change, fixed
some bugs, enhanced regress tests.
Sorry for delay.
Regards
Pavel Stehule
p.s. Bernard, please, can you look on this version?
2009/8/9 Tom Lane t...@sss.pgh.pa.us:
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.
1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want? Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.
5. I don't like anything about the leaky list representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.
Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).
* Most of the PG source code just writes short or long,
not short int. Actually I wonder whether int2 wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.
* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:
***
*** 10028,10034
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
--- 10056,10062
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
regards, tom lane
mnnotation.diff.gz
Description: GNU Zip compressed data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
2009/8/9 Tom Lane t...@sss.pgh.pa.us:
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.
1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
I disagree. I thing so people expect mainly mixed notation.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want? Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
I can't to imagine some recheck, so I prefer forbid CREATE OR REPLACE
FUNCTION for name change. We should to find some better solution
later. When we immutable names, then we have to have well RENAME
statement in plpgsql.
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
I thing, so duplicate parameter names is clean bug - minimally for
language like plpgsql. I can to imagine some use case in C or plperlu,
but now we have variadic params or arrays, so duplicate names should
be deprecated.
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.
5. I don't like anything about the leaky list representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.
I have to look on this - I newer did some changes in planner, so I
know nothing about it now.
Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).
ook
* Most of the PG source code just writes short or long,
not short int. Actually I wonder whether int2 wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.
ok
* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:
***
*** 10028,10034
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
--- 10056,10062
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
I am sorry, I'll be more careful
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
I spend week out of office, and actually I working on house, but I
hope so tomorrow
Re: [HACKERS] Issues for named/mixed function notation patch
2009/8/9 Tom Lane t...@sss.pgh.pa.us: Oh, another thing: the present restriction that all function parameters after the first one with a default must also have defaults is based on limitations of positional call notation. Does it make sense to relax that restriction once we allow named call notation, and if so what should we do exactly? (This could be addressed in a followup patch, it doesn't necessarily have to be dealt with immediately.) Yes, this rule should be useless. But with the remove of this rule, we have to modify algorithm for positional notation. It depends on this rule. regards Pavel Stehule regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
Oh, another thing: the present restriction that all function parameters after the first one with a default must also have defaults is based on limitations of positional call notation. Does it make sense to relax that restriction once we allow named call notation, and if so what should we do exactly? (This could be addressed in a followup patch, it doesn't necessarily have to be dealt with immediately.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Sun, Aug 9, 2009 at 7:30 PM, Tom Lanet...@sss.pgh.pa.us wrote: 1. As I already mentioned, I think the mixed-notation business is a bad idea. It's unintuitive, it's not especially useful, and it substantially increases our risk of being semantically incompatible with whatever the SQL committee might someday do in this area. I think we should disallow it until we see what they do. I gather that this might be an unpopular position though. It seems like we could safely allow the cases which are unambiguous. Namely where the call has a sequence of unnamed parameters followed by some named parameters all of which refer to parameters which come later. So foo(1,2,3 as x,4 as y) would be legal as long as x and y were not one of the first three arguments. That's a pretty common idiom when you have a function which does something normal to the first few arguments and then has a bunch of non-standard modes which can be optionally invoked. Take for example the perl DBI's connect method which takes a data source, username, authentication token, then a list of parameters which can be any of dozens of parameters (actually it takes a fifth argument which is a hashref -- but the point here is just that it's a common style, not that we should be copying perl's solution). The reason I'm saying this is safe is because there's just no other possible interpretation. As long as it only covers the unambiguous cases then there's no other meaning other implementations can define which this would conflict with. -- greg http://mit.edu/~gsstark/resume.pdf -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Sun, Aug 9, 2009 at 2:30 PM, Tom Lanet...@sss.pgh.pa.us wrote:
I've now read most of this patch, and I think there are some things that
need rework, and perhaps debate about what we want.
1. As I already mentioned, I think the mixed-notation business is a bad
idea. It's unintuitive, it's not especially useful, and it substantially
increases our risk of being semantically incompatible with whatever the
SQL committee might someday do in this area. I think we should disallow
it until we see what they do. I gather that this might be an unpopular
position though.
LOL. I already did my yelling and screaming on this point... though
it's all good-natured, in case that doesn't come through in the email.
2. It doesn't appear that any attention has been given to what happens
if CREATE OR REPLACE FUNCTION is used to change the parameter names of
an existing function. Since the post-analysis representation of parameter
lists is still entirely positional, the actual effect on a function call
in a stored view or rule is nil --- it will still call the same function
it did before, passing the parameters that were originally identified to
be passed. That might be considered good, but it's quite unlike what
will happen to function calls that are stored textually (eg, in plpgsql
functions). Is that what we want?
That sounds pretty dangerous. What if someone renames a parameter so
as to invert its sense, or something? (automatically_delete_all_files
becomes confirm_before_deleting, or somesuch)
Or should we consider that parameter
names are now part of the API of a function, and forbid CREATE OR REPLACE
FUNCTION from changing them? Or perhaps we should recheck parameter name
matching someplace after analysis, perhaps at default-expansion time?
I'm not sure what the right way to go with this is, but we have to
think about how it plays with function overloading - can I define two
identically-named functions with different sets of positional
parameters, and then resolve the function call based on which
parameters are specified?
3. In the same vein, CREATE FUNCTION doesn't disallow duplicate parameter
names, nor functions that have names for some but not all parameters.
The patch appears to cope with the latter case but not the former.
Should we disallow these things in CREATE FUNCTION, or make the patch
never match such functions, or what?
I think duplicate parameter names shouldn't be allowed.
4. No attention has been given to making ruleutils.c list out named or
mixed function calls correctly.
5. I don't like anything about the leaky list representation of
analyzed function arguments. Having null subexpressions in unexpected
places isn't a good idea --- it's likely to cause crashes in code that
isn't being really cautious. Moreover, if we did ultimately support
mixed notation, there's no way to list it out correctly on the basis
of this representation, because you can't tell which arguments were
named and which weren't. I think it would be better to keep the
ArgExprs in the transformed parameter list and have the planner
remove them (and reorder the arguments if needed) when it does
default-argument expansion. Depending on what you think about point
#2, the transformed ArgExprs might need to carry the argument number
instead of the argument name, but in any case they'd just be there
for named arguments. This approach probably will also reduce the
amount of change in the parser, since you won't have to separate
the names from the argument list and pass those all over the place
separately.
Some minor style issues:
* ArgExpr is confusingly named and incorrectly documented, since it's
only used for named arguments. Perhaps NamedArgExpr would be better.
Also, it'd probably be a good idea to include a location field in it
(pointing at the parameter name not the argument expression).
* Most of the PG source code just writes short or long,
not short int. Actually I wonder whether int2 wouldn't
be preferred anyway, since that's how the relevant pg_proc
columns are declared.
* The error messages aren't even approximately conformant to style guide.
* Please avoid useless whitespace changes, especially ones as
ill-considered as this:
***
*** 10028,10034
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
--- 10056,10062
;
! name: ColId
{ $$ = $1; };
database_name:
ColId
{ $$ = $1; };
I'm going to mark the patch Waiting on Author, since it's not close
to being committable until these issues are resolved.
Is it realistic to think that this will
Re: [HACKERS] Issues for named/mixed function notation patch
Robert Haas robertmh...@gmail.com writes: On Sun, Aug 9, 2009 at 2:30 PM, Tom Lanet...@sss.pgh.pa.us wrote: I'm going to mark the patch Waiting on Author, since it's not close to being committable until these issues are resolved. Is it realistic to think that this will be finished and committed this week? I didn't want to prejudge that question. We still have the rest of the week, and there's not that much else left to do, at least from my standpoint (some of the other committers still have stuff on their plates). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Sun, Aug 9, 2009 at 9:36 PM, Tom Lanet...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Sun, Aug 9, 2009 at 2:30 PM, Tom Lanet...@sss.pgh.pa.us wrote: I'm going to mark the patch Waiting on Author, since it's not close to being committable until these issues are resolved. Is it realistic to think that this will be finished and committed this week? I didn't want to prejudge that question. We still have the rest of the week, and there's not that much else left to do, at least from my standpoint (some of the other committers still have stuff on their plates). Fair point, my impatience is showing. Sorry. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issues for named/mixed function notation patch
On Mon, Aug 10, 2009 at 2:23 AM, Robert Haasrobertmh...@gmail.com wrote: 2. It doesn't appear that any attention has been given to what happens if CREATE OR REPLACE FUNCTION is used to change the parameter names of an existing function. Since the post-analysis representation of parameter lists is still entirely positional, the actual effect on a function call in a stored view or rule is nil --- it will still call the same function it did before, passing the parameters that were originally identified to be passed. That might be considered good, but it's quite unlike what will happen to function calls that are stored textually (eg, in plpgsql functions). Is that what we want? That sounds pretty dangerous. What if someone renames a parameter so as to invert its sense, or something? (automatically_delete_all_files becomes confirm_before_deleting, or somesuch) There's also the existing users using positional notation to consider. If all my callers are using positional notation then I might be kind of annoyed if I can't fix the parameter names of my functions because it would change the function signature. That would be a functionality regression for me. But on balance I don't see a better solution. If we allow people to change the parameter names and they're used for named arguments then it seems like the behaviour is not very clear and predictable no matter what resolution we pick. -- greg http://mit.edu/~gsstark/resume.pdf -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
