Fix hints on CREATE PROCEDURE errors
When testing out CREATE PROCEDURE with 11 beta 2, I noticed, the hints
in the errors reference DROP FUNCTION, which doesn't work for
procedures. DROP ROUTINE works for both functions and procedures, so
this patch should work for both.
Please CC me when responding as I don't currently subscribe to the
list.
Thanks,
Jeremy
From 1e4880e77b2b11917021e9d46756264013bd87d5 Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@jeremyevans.net>
Date: Mon, 6 Aug 2018 11:01:00 -0700
Subject: [PATCH] Have hint use DROP ROUTINE instead of DROP FUNCTION
The current code's hint is misleading for procedures:
CREATE OR REPLACE PROCEDURE a(in int)
LANGUAGE SQL
AS $$
SELECT NULL;
$$;
# CREATE PROCEDURE
CREATE OR REPLACE PROCEDURE a(inout int)
LANGUAGE SQL
AS $$
SELECT NULL;
$$;
# ERROR: cannot change return type of existing function
# HINT: Use DROP FUNCTION a(integer) first.
DROP FUNCTION a(integer);
# ERROR: a(integer) is not a function
DROP ROUTINE a(integer);
# DROP ROUTINE
DROP ROUTINE should work for both functions and procedures.
---
src/backend/catalog/pg_proc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 9b4015d0d4..84ee8d686d 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -409,7 +409,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup))))); /*
@@ -434,7 +434,7 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
errdetail("Row type defined by OUT parameters is different."),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
}@@ -477,7 +477,7 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change name of input parameter \"%s\"",
old_arg_names[j]),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
}
}
@@ -501,7 +501,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot remove parameter defaults from existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup))))); proargdefaults = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
@@ -527,7 +527,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change data type of existing parameter default value"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
newlc = lnext(newlc);
}
--
2.17.1
Hi.
On 2018/08/07 3:32, Jeremy Evans wrote:
When testing out CREATE PROCEDURE with 11 beta 2, I noticed, the hints
in the errors reference DROP FUNCTION, which doesn't work for
procedures.
Good catch.
DROP ROUTINE works for both functions and procedures, so
this patch should work for both.
Not sure if we should fix it the way your patch does, but it seems you
forgot to include changes to the expected output of some of the regression
tests affected by this patch. Attached updated patch includes those.
Thanks,
Amit
Attachments:
drop-routine-in-HINT.patchtext/plain; charset=UTF-8; name=drop-routine-in-HINT.patchDownload
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 9b4015d0d4..84ee8d686d 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -409,7 +409,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
/*
@@ -434,7 +434,7 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
errdetail("Row type defined by OUT parameters is different."),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
}
@@ -477,7 +477,7 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change name of input parameter \"%s\"",
old_arg_names[j]),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
}
}
@@ -501,7 +501,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot remove parameter defaults from existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
proargdefaults = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
@@ -527,7 +527,7 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change data type of existing parameter default value"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP ROUTINE %s first.",
format_procedure(HeapTupleGetOid(oldtup)))));
newlc = lnext(newlc);
}
diff --git a/src/test/regress/expected/polymorphism.out b/src/test/regress/expected/polymorphism.out
index 67e70c8c14..ec88621dc3 100644
--- a/src/test/regress/expected/polymorphism.out
+++ b/src/test/regress/expected/polymorphism.out
@@ -1081,7 +1081,7 @@ select dfunc(10,20);
create or replace function dfunc(a variadic int[]) returns int as
$$ select array_upper($1, 1) $$ language sql;
ERROR: cannot remove parameter defaults from existing function
-HINT: Use DROP FUNCTION dfunc(integer[]) first.
+HINT: Use DROP ROUTINE dfunc(integer[]) first.
\df dfunc
List of functions
Schema | Name | Result data type | Argument data types | Type
@@ -1294,13 +1294,13 @@ returns record as $$
select $1, $2;
$$ language sql;
ERROR: cannot change name of input parameter "c"
-HINT: Use DROP FUNCTION dfunc(character varying,numeric) first.
+HINT: Use DROP ROUTINE dfunc(character varying,numeric) first.
create or replace function dfunc(a varchar = 'def a', out _a varchar, numeric = NULL, out _c numeric)
returns record as $$
select $1, $2;
$$ language sql;
ERROR: cannot change name of input parameter "c"
-HINT: Use DROP FUNCTION dfunc(character varying,numeric) first.
+HINT: Use DROP ROUTINE dfunc(character varying,numeric) first.
drop function dfunc(varchar, numeric);
--fail, named parameters are not unique
create function testpolym(a int, a int) returns int as $$ select 1;$$ language sql;
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 34ca0ef890..a870f9843b 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -1433,7 +1433,7 @@ CREATE OR REPLACE FUNCTION rngfunc(in f1 int, out f2 int, out f3 text)
RETURNS record
AS 'select $1+1' LANGUAGE sql;
ERROR: cannot change return type of existing function
-HINT: Use DROP FUNCTION rngfunc(integer) first.
+HINT: Use DROP ROUTINE rngfunc(integer) first.
CREATE OR REPLACE FUNCTION rngfuncr(in f1 int, out f2 int, out text)
AS $$select $1-1, $1::text || 'z'$$ LANGUAGE sql;
SELECT f1, rngfuncr(f1) FROM int4_tbl;
@@ -1515,7 +1515,7 @@ SELECT * FROM dup('xyz'::text);
CREATE OR REPLACE FUNCTION dup (inout f2 anyelement, out f3 anyarray)
AS 'select $1, array[$1,$1]' LANGUAGE sql;
ERROR: cannot change name of input parameter "f1"
-HINT: Use DROP FUNCTION dup(anyelement) first.
+HINT: Use DROP ROUTINE dup(anyelement) first.
DROP FUNCTION dup(anyelement);
-- equivalent behavior, though different name exposed for input arg
CREATE OR REPLACE FUNCTION dup (inout f2 anyelement, out f3 anyarray)
On 06/08/2018 20:32, Jeremy Evans wrote:
The current code's hint is misleading for procedures:
CREATE OR REPLACE PROCEDURE a(in int)
LANGUAGE SQL
AS $$
SELECT NULL;
$$;
# CREATE PROCEDURECREATE OR REPLACE PROCEDURE a(inout int)
LANGUAGE SQL
AS $$
SELECT NULL;
$$;
# ERROR: cannot change return type of existing function
# HINT: Use DROP FUNCTION a(integer) first.
Yes, the hint should be changed. But I also think the error message
should be changed to be more appropriate to the procedure situation
(where is the return type?). Attached patch does both. Unlike your
patch, I kept the "DROP FUNCTION" message for the function case. It
might be too confusing otherwise. Thoughts?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU.patchtext/plain; charset=UTF-8; name=0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU.patch; x-mac-creator=0; x-mac-type=0Download
From 36f00184dcc7597f69466a6e9aa2db6eee1a6ef6 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter_e@gmx.net>
Date: Wed, 8 Aug 2018 20:39:26 +0200
Subject: [PATCH] Improve error messages for CREATE OR REPLACE PROCEDURE
Change the hint to recommend DROP PROCEDURE instead of FUNCTION. Also
make the error message when changing the return type more specific to
the case of procedures.
Reported-by: Jeremy Evans <code@jeremyevans.net>
---
src/backend/catalog/pg_proc.c | 26 ++++++++++++++++++++------
1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 9b4015d0d4..654ee943be 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -375,6 +375,7 @@ ProcedureCreate(const char *procedureName,
Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup);
Datum proargnames;
bool isnull;
+ const char *prokind_keyword;
if (!replace)
ereport(ERROR,
@@ -400,16 +401,25 @@ ProcedureCreate(const char *procedureName,
errdetail("\"%s\" is a window function.", procedureName) :
0)));
+ prokind_keyword = (prokind == PROKIND_PROCEDURE ? "PROCEDURE" : "FUNCTION");
+
/*
* Not okay to change the return type of the existing proc, since
* existing rules, views, etc may depend on the return type.
+ *
+ * In case of a procedure, a changing return type means that whether
+ * the procedure has output parameters was changed. Since there is no
+ * user visible return type, we produce a more specific error message.
*/
if (returnType != oldproc->prorettype ||
returnsSet != oldproc->proretset)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("cannot change return type of existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ prokind == PROKIND_PROCEDURE
+ ? errmsg("cannot change whether a procedure has output parameters")
+ : errmsg("cannot change return type of existing function"),
+ errhint("Use DROP %s %s first.",
+ prokind_keyword,
format_procedure(HeapTupleGetOid(oldtup)))));
/*
@@ -434,7 +444,8 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
errdetail("Row type defined by OUT parameters is different."),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP %s %s first.",
+ prokind_keyword,
format_procedure(HeapTupleGetOid(oldtup)))));
}
@@ -477,7 +488,8 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change name of input parameter \"%s\"",
old_arg_names[j]),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP %s %s first.",
+ prokind_keyword,
format_procedure(HeapTupleGetOid(oldtup)))));
}
}
@@ -501,7 +513,8 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot remove parameter defaults from existing function"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP %s %s first.",
+ prokind_keyword,
format_procedure(HeapTupleGetOid(oldtup)))));
proargdefaults = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
@@ -527,7 +540,8 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change data type of existing parameter default value"),
- errhint("Use DROP FUNCTION %s first.",
+ errhint("Use DROP %s %s first.",
+ prokind_keyword,
format_procedure(HeapTupleGetOid(oldtup)))));
newlc = lnext(newlc);
}
--
2.18.0
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Yes, the hint should be changed. But I also think the error message
should be changed to be more appropriate to the procedure situation
(where is the return type?). Attached patch does both. Unlike your
patch, I kept the "DROP FUNCTION" message for the function case. It
might be too confusing otherwise. Thoughts?
I'm not a translator, but if I were, stuff like "Use DROP %s %s first."
would probably confuse me. IMO it's too close to assembling a message
out of parts, even if it's true that neither %s needs translation.
I think you'd be better off with
isprocedure ? errhint("Use DROP PROCEDURE %s first.", ...)
: errhint("Use DROP FUNCTION %s first.", ...)
Or if that seems too carpal-tunnel-inducing, maybe a workable compromise
is
dropcmd = (prokind == PROKIND_PROCEDURE ? "DROP PROCEDURE" : "DROP FUNCTION");
/* translator: first %s is DROP FUNCTION or DROP PROCEDURE */
errhint("Use %s %s first.", dropcmd, ...)
Looks reasonable other than that quibble.
regards, tom lane
On Aug 8, 2018, at 3:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
Yes, the hint should be changed. But I also think the error message
should be changed to be more appropriate to the procedure situation
(where is the return type?). Attached patch does both. Unlike your
patch, I kept the "DROP FUNCTION" message for the function case. It
might be too confusing otherwise. Thoughts?I'm not a translator, but if I were, stuff like "Use DROP %s %s first."
would probably confuse me. IMO it's too close to assembling a message
out of parts, even if it's true that neither %s needs translation.
I think you'd be better off withisprocedure ? errhint("Use DROP PROCEDURE %s first.", ...)
: errhint("Use DROP FUNCTION %s first.", ...)Or if that seems too carpal-tunnel-inducing, maybe a workable compromise
isdropcmd = (prokind == PROKIND_PROCEDURE ? "DROP PROCEDURE" : "DROP FUNCTION");
/* translator: first %s is DROP FUNCTION or DROP PROCEDURE */
errhint("Use %s %s first.", dropcmd, ...)Looks reasonable other than that quibble.
To help move this along, I went ahead and applied Tom’s first suggestion
to the patch. I tested the various scenarios and it seemed to work.
Jonathan
Attachments:
0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU-v2.patchapplication/octet-stream; name=0001-Improve-error-messages-for-CREATE-OR-REPLACE-PROCEDU-v2.patch; x-unix-mode=0644Download
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 654ee943be..87d4850c11 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -375,7 +375,7 @@ ProcedureCreate(const char *procedureName,
Form_pg_proc oldproc = (Form_pg_proc) GETSTRUCT(oldtup);
Datum proargnames;
bool isnull;
- const char *prokind_keyword;
+ bool isprocedure;
if (!replace)
ereport(ERROR,
@@ -401,7 +401,8 @@ ProcedureCreate(const char *procedureName,
errdetail("\"%s\" is a window function.", procedureName) :
0)));
- prokind_keyword = (prokind == PROKIND_PROCEDURE ? "PROCEDURE" : "FUNCTION");
+ /* Store if this is a procedure/function to help with errors/hints */
+ isprocedure = (prokind == PROKIND_PROCEDURE);
/*
* Not okay to change the return type of the existing proc, since
@@ -415,12 +416,14 @@ ProcedureCreate(const char *procedureName,
returnsSet != oldproc->proretset)
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- prokind == PROKIND_PROCEDURE
- ? errmsg("cannot change whether a procedure has output parameters")
- : errmsg("cannot change return type of existing function"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errmsg("cannot change whether a procedure has output parameters") :
+ errmsg("cannot change return type of existing function"),
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
/*
* If it returns RECORD, check for possible change of record type
@@ -444,9 +447,11 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change return type of existing function"),
errdetail("Row type defined by OUT parameters is different."),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
}
/*
@@ -488,9 +493,11 @@ ProcedureCreate(const char *procedureName,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change name of input parameter \"%s\"",
old_arg_names[j]),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
}
}
@@ -513,9 +520,11 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot remove parameter defaults from existing function"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
proargdefaults = SysCacheGetAttr(PROCNAMEARGSNSP, oldtup,
Anum_pg_proc_proargdefaults,
@@ -540,9 +549,11 @@ ProcedureCreate(const char *procedureName,
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
errmsg("cannot change data type of existing parameter default value"),
- errhint("Use DROP %s %s first.",
- prokind_keyword,
- format_procedure(HeapTupleGetOid(oldtup)))));
+ isprocedure ?
+ errhint("Use DROP PROCEDURE %s first.",
+ format_procedure(HeapTupleGetOid(oldtup))) :
+ errhint("Use DROP FUNCTION %s first.",
+ format_procedure(HeapTupleGetOid(oldtup)))));
newlc = lnext(newlc);
}
}
This has been committed.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services