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 ? */
+ /* Check for default arguments */
if (nargs < pronargs)
{
int first_arg_with_default = pronargs - pronargdefaults;
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index c501a5e..0a34e81 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -445,9 +445,10 @@ ProcedureCreate(const char *procedureName,
}
/*
- * if there are named parameters, check names equality. Any change can break
- * exiting calls (when named parameters are used). Only IN and INOUT parameters are
- * checked.
+ * If there are named parameters, check to make sure the names
+ * have not been changed. Any change can break exiting calls
+ * when named parameters are used. Only IN and INOUT
+ * parameters are checked.
*/
prooldargnames = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
Anum_pg_proc_proargnames,
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index ccc6951..3f67607 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -3300,8 +3300,8 @@ simplify_function(Oid funcid, Oid result_type, int32 result_typmod,
}
/*
- * This function change order of any arg in arglist. When some arguments missing,
- * then it use defaults.
+ * This function changes the order of any arg in arglist. When some
+ * arguments are missing, then it uses the defaults.
*/
static List *
reorder_arguments(List *args, Oid result_type, HeapTuple func_tuple,
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a3d2d94..e86ce8a 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11332,7 +11332,7 @@ TableFuncTypeName(List *columns)
/*
* Append function parameter to function's parameter list. Reject
- * parameters with uniqueless param_name with same proargmode.
+ * parameters with non-unique param_name with same proargmode.
* It's enough for sql, plperl, plpython language, but not for
* plpgsql.
*
@@ -11363,7 +11363,7 @@ AppendFuncParameter(List *list, FunctionParameter *p, int location, base_yyscan_
if (p2->name != NULL && strcmp(p->name, p2->name) == 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("parameter has not unique name"),
+ errmsg("parameter has non-unique name"),
parser_errposition(location)));
}
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index e1c8a1c..5ea00c6 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -159,8 +159,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
if (strcmp(n->name, next_name) == 0)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("named argument has not unique name"),
- errhint("Verify your named parameters for ambiguous argument names."),
+ errmsg("named argument has non-unique name"),
+ errhint("Check your named parameters for ambiguous argument names."),
parser_errposition(pstate, exprLocation((Node *) lfirst(lc)))));
}
}
@@ -179,7 +179,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
}
}
- /* forgot argnames list of empty strings when positional notation */
+ /* forget argnames list of empty strings when positional notation */
if (notation == CALL_NOTATION_POSITIONAL)
fargnames = NIL;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index dab89ba..32620fe 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -326,10 +326,11 @@ typedef struct FuncExpr
/*
* NamedArgExpr - an named argument of function
*
- * It is used, when argument has name. When positional notation is used, then
- * args list doesn't contain any NamedArgExpr. Named notation means, so all
- * arguments in list are NamedArgExpr. Mixed notation means, so first n arguments
- * are Exprs and last m arguments are NamedArgExpr.
+ * Used when argument has name. When positional notation is used, then
+ * args list doesn't contain any NamedArgExpr. When named notation is
+ * used, all arguments in list are NamedArgExpr. When mixed notation
+ * is used, the first n arguments are Exprs and last m arguments are
+ * NamedArgExpr.
*/
typedef struct NamedArgExpr
{
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index cd437ca..dfac452 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -838,19 +838,19 @@ LINE 1: select * from testfoo();
drop function testfoo();
--fail, named parameter are not unique
create function testfoo(a int, a int) returns int as $$ select 1;$$ language sql;
-ERROR: parameter has not unique name
+ERROR: parameter has non-unique name
LINE 1: create function testfoo(a int, a int) returns int as $$ sele...
^
create function testfoo(int, out a int, out a int) returns int as $$ select 1;$$ language sql;
-ERROR: parameter has not unique name
+ERROR: parameter has non-unique name
LINE 1: create function testfoo(int, out a int, out a int) returns i...
^
create function testfoo(out a int, inout a int) returns int as $$ select 1;$$ language sql;
-ERROR: parameter has not unique name
+ERROR: parameter has non-unique name
LINE 1: create function testfoo(out a int, inout a int) returns int ...
^
create function testfoo(a int, inout a int) returns int as $$ select 1;$$ language sql;
-ERROR: parameter has not unique name
+ERROR: parameter has non-unique name
LINE 1: create function testfoo(a int, inout a int) returns int as $...
^
-- valid
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
