Issues for named/mixed function notation patch

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: Bernd Helmle <mailings(at)oopsware(dot)de>, Robert Haas <robertmhaas(at)gmail(dot)com>, pgsql-hackers(at)postgreSQL(dot)org
Subject: Issues for named/mixed function notation patch
Date: 2009-08-09 18:30:29
Message-ID: 15635.1249842629@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2009-08-09 18:43:38 Re: hot standby - merged up to CVS HEAD
Previous Message Robert Haas 2009-08-09 18:29:44 Re: Review: Patch for contains/overlap of polygons