Re: Issues for named/mixed function notation patch

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