Re: Issues for named/mixed function notation patch

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