Re: Issues for named/mixed function notation patch

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

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

On Sun, Aug 9, 2009 at 7:30 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)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

On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)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 be finished and committed this week?

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)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

On Sun, Aug 9, 2009 at 9:36 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Sun, Aug 9, 2009 at 2:30 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)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

On Mon, Aug 10, 2009 at 2:23 AM, Robert Haas<robertmhaas(at)gmail(dot)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

2009/8/9 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 will have time for fixing these issues.

>                        regards, tom lane
>

thank you
Pavel Stehule

2009/8/9 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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
>

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 <tgl(at)sss(dot)pgh(dot)pa(dot)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
>

On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2009/9/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

Hello Robert,

2009/9/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

On Mon, Sep 14, 2009 at 6:09 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hello Robert,
>
> 2009/9/14 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Aug 24, 2009 at 3:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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.

Same problem. Build log attached.

...Robert

>
> Same problem.  Build log attached.
>
> ...Robert
>

My renonc, please, try new patch. I forgot mark regproc.c file.

regards
Pavel Stehule

On Tue, Sep 15, 2009 at 4:51 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

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

> "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
>
>

On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2009/9/27 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

On Sun, Sep 27, 2009 at 1:46 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 2009/9/27 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Sep 27, 2009 at 12:37 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

> 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
>

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

2009/9/28 Jeff Davis <pgsql(at)j-davis(dot)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
>
>

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

2009/9/28 Jeff Davis <pgsql(at)j-davis(dot)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
>
>
>

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

2009/9/28 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 2009/9/28 Jeff Davis <pgsql(at)j-davis(dot)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;

proargidxs is used more times in func_get_detail function

a) for reordering pgproc->args to actual params order
b) for numbering (filling) NamedArgExpr->position based on known best candidate

>
>>
>> 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?

I fixed it

So I dropped variadic functions from mixed/named notation and little
bit modified documentation. Please, can some native English speaker
look on documentation?

Patch attached

Pavel

>
> Regards
> Pavel
>
>>
>> Regards,
>>        Jeff Davis
>>
>>
>>
>

2009/9/30 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2009/10/1 Brendan Jurd <direvus(at)gmail(dot)com>:
> 2009/9/30 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

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

2009/10/2 Jeff Davis <pgsql(at)j-davis(dot)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
>

>
> Please review Brendan and my patches and combine them with your patch.
>

see attachment, please

regards
pavel

> Regards,
>        Jeff Davis
>

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

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

2009/10/7 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>

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

Jeff Davis <pgsql(at)j-davis(dot)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

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

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

Jeff Davis <pgsql(at)j-davis(dot)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

2009/10/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Jeff Davis <pgsql(at)j-davis(dot)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
>

2009/10/7 Jeff Davis <pgsql(at)j-davis(dot)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
>
>

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

On Wed, Oct 7, 2009 at 5:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Jeff Davis <pgsql(at)j-davis(dot)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

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> [ latest named-args patch ]

Committed with a fair amount of corner-case cleanup and refactoring.

regards, tom lane

2009/10/8 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> [ latest named-args patch ]
>
> Committed with a fair amount of corner-case cleanup and refactoring.
>
>                        regards, tom lane
>

Thank you

Pavel Stehule

On Oct 7, 2009, at 7:41 PM, Tom Lane wrote:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

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

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

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

"David E. Wheeler" <david(at)kineticode(dot)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

2009/10/8 David E. Wheeler <david(at)kineticode(dot)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
>

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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)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. +

2010/2/23 Bruce Momjian <bruce(at)momjian(dot)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(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
> --
>  Bruce Momjian  <bruce(at)momjian(dot)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. +
>

Pavel Stehule wrote:
> 2010/2/23 Bruce Momjian <bruce(at)momjian(dot)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 <bruce(at)momjian(dot)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. +

On Tue, 2010-02-23 at 09:34 -0500, Bruce Momjian wrote:
> Pavel Stehule wrote:
> > 2010/2/23 Bruce Momjian <bruce(at)momjian(dot)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

2010/2/24 Jeff Davis <pgsql(at)j-davis(dot)com>:
> On Tue, 2010-02-23 at 09:34 -0500, Bruce Momjian wrote:
>> Pavel Stehule wrote:
>> > 2010/2/23 Bruce Momjian <bruce(at)momjian(dot)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
>
>