BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name

On Fri, 4 Jan 2019 at 09:44, PG Bug reporting form
<noreply@postgresql.org> wrote:

Operating system: RDS (on Amazon)

You may want to talk to Amazon about this. However, since the same
behaviour exists in PostgreSQL too...

Run the command : DROP FUNCTION test.func1;

NOTE: This operation failed to execute the drop and reported the following
message

Message reported by PgAdmin4 & OmniDB:
---- start of message ------
function name "test.func1" is not unique
HINT: Specify the argument list to select the function
unambiguously.
---- end of message ------

Run the command : DROP FUNCTION IF EXISTS test.func1;

NOTE: This operation completed successfully without error and reported the
following message

Message reported by PgAdmin4 & OmniDB:
---- start of message ------
function admq.test1() does not exist, skipping
---- end of message ------
-----------------------------------------------------------------------------------------------------------
Proposed solution:
The operation in Step 2 should have failed with the same error as reported
in Step 1;

It's not really that clear to me that doing that would be any more
correct than the alternative. If we changed the behaviour of this then
someone might equally come along later and complain that they
specified "IF EXISTS" and got an error. Maintaining the status quo at
least has the benefit of not randomly changing the behaviour because
it didn't suit one particular use case. The patch to change the
behaviour is pretty trivial and amounts to removing a single line of
code:

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 4661fc4f62..a9912b0986 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2053,12 +2053,11 @@ LookupFuncName(List *funcname, int nargs,
const Oid *argtypes, bool noError)
                {
                        if (clist->next)
                        {
-                               if (!noError)
-                                       ereport(ERROR,
-
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
-
errmsg("function name \"%s\" is not unique",
-
 NameListToString(funcname)),
-
errhint("Specify the argument list to select the function
unambiguously.")));
+                               ereport(ERROR,
+
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
+
errmsg("function name \"%s\" is not unique",
+
NameListToString(funcname)),
+
errhint("Specify the argument list to select the function
unambiguously.")));
                        }
                        else
                                return clist->oid;

I just don't know if we'll have a better database by removing it.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thursday, January 3, 2019, David Rowley <david.rowley@2ndquadrant.com>
wrote:

If we changed the behaviour of this then
someone might equally come along later and complain that they
specified "IF EXISTS" and got an error.

I’m inclined to argue that the docs say you can only use the omitted-args
name if it is unique within the schema. Since the second case is using
that form in violation of that requirement reporting an error would match
the documentation.

IF EXISTS only applies when no functions exist; an error for ambiguity
doesn’t violate its promise; and likely even if we didn’t make it an error
something else will fail later on.

It is wrong for the drop function if exists command to translate/print the
omitted-args form of the name into a function with zero arguments; it
should not be looking explicitly for a zero-arg function as it is not the
same thing (as emphasized in the docs).

So, I vote for changing this in 12 but leaving prior versions as-is for
compatability as the harm doesn’t seem to be enough to risk breakage.
Might be worth a doc patch showing the second case for the back branches
(Head seems like it would be good as we are fixing the code to match the
documentation, IMO).

David J.

Your Concern is valid as well for "IF exists" complaint from users (its a possibility ):
Then I would propose:
1. Either word the return message identical to the drop command message(without the "If Exists") & successfully pass the command.
OR
2. Fail the execution since just using the function name without parameters returns ambiguous results for the Drop to continue.
OR
3. Drop all functions with that function name & successfully pass the command.

With your comment the 1st option looks as a better option.

Regards
Ash

A Marath.

________________________________
From: David Rowley <david.rowley@2ndquadrant.com>
Sent: Thursday, January 3, 2019 11:45:16 PM
To: makmarath@hotmail.com; pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name

On Fri, 4 Jan 2019 at 09:44, PG Bug reporting form
<noreply@postgresql.org> wrote:

Operating system: RDS (on Amazon)

You may want to talk to Amazon about this. However, since the same
behaviour exists in PostgreSQL too...

Run the command : DROP FUNCTION test.func1;

NOTE: This operation failed to execute the drop and reported the following
message

Message reported by PgAdmin4 & OmniDB:
---- start of message ------
function name "test.func1" is not unique
HINT: Specify the argument list to select the function
unambiguously.
---- end of message ------

Run the command : DROP FUNCTION IF EXISTS test.func1;

NOTE: This operation completed successfully without error and reported the
following message

Message reported by PgAdmin4 & OmniDB:
---- start of message ------
function admq.test1() does not exist, skipping
---- end of message ------
-----------------------------------------------------------------------------------------------------------
Proposed solution:
The operation in Step 2 should have failed with the same error as reported
in Step 1;

It's not really that clear to me that doing that would be any more
correct than the alternative. If we changed the behaviour of this then
someone might equally come along later and complain that they
specified "IF EXISTS" and got an error. Maintaining the status quo at
least has the benefit of not randomly changing the behaviour because
it didn't suit one particular use case. The patch to change the
behaviour is pretty trivial and amounts to removing a single line of
code:

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 4661fc4f62..a9912b0986 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2053,12 +2053,11 @@ LookupFuncName(List *funcname, int nargs,
const Oid *argtypes, bool noError)
                {
                        if (clist->next)
                        {
-                               if (!noError)
-                                       ereport(ERROR,
-
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
-
errmsg("function name \"%s\" is not unique",
-
 NameListToString(funcname)),
-
errhint("Specify the argument list to select the function
unambiguously.")));
+                               ereport(ERROR,
+
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
+
errmsg("function name \"%s\" is not unique",
+
NameListToString(funcname)),
+
errhint("Specify the argument list to select the function
unambiguously.")));
                        }
                        else
                                return clist->oid;

I just don't know if we'll have a better database by removing it.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

I second David's suggestion

A Marath.

________________________________
From: David G. Johnston <david.g.johnston@gmail.com>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: makmarath@hotmail.com; pgsql-bugs@lists.postgresql.org
Subject: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name

On Thursday, January 3, 2019, David Rowley <david.rowley@2ndquadrant.com<mailto:david.rowley@2ndquadrant.com>> wrote:
If we changed the behaviour of this then
someone might equally come along later and complain that they
specified "IF EXISTS" and got an error.

I’m inclined to argue that the docs say you can only use the omitted-args name if it is unique within the schema. Since the second case is using that form in violation of that requirement reporting an error would match the documentation.

IF EXISTS only applies when no functions exist; an error for ambiguity doesn’t violate its promise; and likely even if we didn’t make it an error something else will fail later on.

It is wrong for the drop function if exists command to translate/print the omitted-args form of the name into a function with zero arguments; it should not be looking explicitly for a zero-arg function as it is not the same thing (as emphasized in the docs).

So, I vote for changing this in 12 but leaving prior versions as-is for compatability as the harm doesn’t seem to be enough to risk breakage. Might be worth a doc patch showing the second case for the back branches (Head seems like it would be good as we are fixing the code to match the documentation, IMO).

David J.

I second David J. Suggestion.

To add to the possible list of solutions I also propose another solution and for better consistency between both the operation

Fix the error message reported by the "drop function without IF Exists" and make it similar to the "Drop.. If Exists".

If no parameters are passed by user then let the "DROP FUNCTION" routine only check for a function of that name which has no parameters => "func1()"

Ash

A Marath.

________________________________
From: David G. Johnston <david.g.johnston@gmail.com>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: makmarath@hotmail.com; pgsql-bugs@lists.postgresql.org
Subject: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name

On Thursday, January 3, 2019, David Rowley <david.rowley@2ndquadrant.com<mailto:david.rowley@2ndquadrant.com>> wrote:
If we changed the behaviour of this then
someone might equally come along later and complain that they
specified "IF EXISTS" and got an error.

I’m inclined to argue that the docs say you can only use the omitted-args name if it is unique within the schema. Since the second case is using that form in violation of that requirement reporting an error would match the documentation.

IF EXISTS only applies when no functions exist; an error for ambiguity doesn’t violate its promise; and likely even if we didn’t make it an error something else will fail later on.

It is wrong for the drop function if exists command to translate/print the omitted-args form of the name into a function with zero arguments; it should not be looking explicitly for a zero-arg function as it is not the same thing (as emphasized in the docs).

So, I vote for changing this in 12 but leaving prior versions as-is for compatability as the harm doesn’t seem to be enough to risk breakage. Might be worth a doc patch showing the second case for the back branches (Head seems like it would be good as we are fixing the code to match the documentation, IMO).

David J.

On 2019-Jan-04, David Rowley wrote:

It's not really that clear to me that doing that would be any more
correct than the alternative.

I think it would be. Specifying a function without params works only if
it's unambiguous; if ambiguity is possible, raise an error. On the
other hand, lack of IF EXISTS is supposed to raise an error if the
function doesn't exist; its presence means not the report that
particular error, but it doesn't mean to suppress other errors such as
the ambiguity one.

I'm not sure what's a good way to implement this, however. Maybe the
solution is to have LookupFuncName return InvalidOid when the function
name is ambiguous and let LookupFuncWithArgs report the error
appropriately. I think this behavior is weird:

/*
* When looking for a function or routine, we pass noError through to
* LookupFuncName and let it make any error messages. Otherwise, we make
* our own errors for the aggregate and procedure cases.
*/
oid = LookupFuncName(func->objname, func->args_unspecified ? -1 : argcount, argoids,
(objtype == OBJECT_FUNCTION || objtype == OBJECT_ROUTINE) ? noError : true);

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, 8 Jan 2019 at 03:54, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I'm not sure what's a good way to implement this, however. Maybe the
solution is to have LookupFuncName return InvalidOid when the function
name is ambiguous and let LookupFuncWithArgs report the error
appropriately. I think this behavior is weird:

/*
* When looking for a function or routine, we pass noError through to
* LookupFuncName and let it make any error messages. Otherwise, we make
* our own errors for the aggregate and procedure cases.
*/
oid = LookupFuncName(func->objname, func->args_unspecified ? -1 : argcount, argoids,
(objtype == OBJECT_FUNCTION || objtype == OBJECT_ROUTINE) ? noError : true);

Why can't we just remove the !noError check in the location where the
error is raised?

I had a look and I can't see any other callers that pass nargs as -1
and can pass noError as true. The only place I see is through
get_object_address() in RemoveObjects(). There's another possible call
in get_object_address_rv(), but there's only 1 call in the entire
source for that function and it passes missing_ok as false.

I ended up with the attached.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

Why can't we just remove the !noError check in the location where the
error is raised?

I don't like that a bit --- the point of noError is to prevent throwing
errors, and it doesn't seem like it should be LookupFuncName's business
to decide it's smarter than its callers. Maybe we need another flag
argument?

regards, tom lane

On Wed, 9 Jan 2019 at 13:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:

David Rowley <david.rowley@2ndquadrant.com> writes:

Why can't we just remove the !noError check in the location where the
error is raised?

I don't like that a bit --- the point of noError is to prevent throwing
errors, and it doesn't seem like it should be LookupFuncName's business
to decide it's smarter than its callers. Maybe we need another flag
argument?

Well, I guess you didn't have backpatching this in mind. The reason I
thought it was okay to hijack that flag was that the ambiguous error
was only raised when the function parameters were not defined. I
chased around and came to the conclusion this only happened during
DROP. Maybe that's a big assumption as it certainly might not help
future callers passing nargs as -1.

I've attached another version with a newly added flag.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, 16 Jan 2019 at 12:38, David Rowley <david.rowley@2ndquadrant.com> wrote:

I've attached another version with a newly added flag.

I've added this to the March commitfest.
https://commitfest.postgresql.org/22/1982/

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, 16 Jan 2019 at 12:38, David Rowley <david.rowley@2ndquadrant.com> wrote:

I've attached another version with a newly added flag.

Looks like I missed updating a call in pltcl.c. Thanks to the
commitfest bot for noticing.

Updated patch attached.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, 7 Feb 2019 at 01:17, David Rowley <david.rowley@2ndquadrant.com> wrote:

Updated patch attached.

Updated patch attached again. This time due to a newly added call to
LookupFuncName() in 1fb57af92.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

Updated patch attached again. This time due to a newly added call to
LookupFuncName() in 1fb57af92.

Hmm ... I'd not looked at this before, but now that I do, the new API
for LookupFuncName seems mighty confused, or at least confusingly
documented. It's not clear what the combinations of the flags actually
do, or why you'd want to use them.

I wonder whether you'd be better off replacing the two bools with an
enum, or something like that.

regards, tom lane

On Mon, 11 Feb 2019 at 11:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hmm ... I'd not looked at this before, but now that I do, the new API
for LookupFuncName seems mighty confused, or at least confusingly
documented. It's not clear what the combinations of the flags actually
do, or why you'd want to use them.

I wonder whether you'd be better off replacing the two bools with an
enum, or something like that.

Okay. Here's a modified patch with the enum.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 11, 2019 at 03:36:17PM +1300, David Rowley wrote:

Okay. Here's a modified patch with the enum.

FWIW, it makes me a bit uneasy to change this function signature in
back-branches if that's the intention as I suspect that it gets used
in extensions.. For HEAD that's fine of course.
--
Michael

Hello,

On 11.02.2019 05:36, David Rowley wrote:

On Mon, 11 Feb 2019 at 11:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wonder whether you'd be better off replacing the two bools with an
enum, or something like that.

Okay. Here's a modified patch with the enum.

There is a LookupFuncWithArgs() call within CreateTransform() where
`bool` is passed still:

tosqlfuncid = LookupFuncWithArgs(OBJECT_FUNCTION, stmt->tosql, *false*);

I had a look and I can't see any other callers that pass nargs as -1
and can pass noError as true. The only place I see is through
get_object_address() in RemoveObjects(). There's another possible call
in get_object_address_rv(), but there's only 1 call in the entire
source for that function and it passes missing_ok as false.

If nargs as -1 and noError as true can be passed only within
RemoveObjects() I wonder, could we just end up with a patch which raise
an error at every ambiguity? That is I mean the following patch:

diff --git a/src/backend/parser/parse_func.c 
b/src/backend/parser/parse_func.c
index 5222231b51..cce8f49f52 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2053,7 +2053,6 @@ LookupFuncName(List *funcname, int nargs, const 
Oid *argtypes, bool noError)
         {
             if (clist->next)
             {
-               if (!noError)
                     ereport(ERROR,
                             (errcode(ERRCODE_AMBIGUOUS_FUNCTION),
                              errmsg("function name \"%s\" is not unique",

But I may overlook something of course.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Fri, 15 Feb 2019 at 02:42, Arthur Zakirov <a.zakirov@postgrespro.ru> wrote:

If nargs as -1 and noError as true can be passed only within
RemoveObjects() I wonder, could we just end up with a patch which raise
an error at every ambiguity? That is I mean the following patch:

diff --git a/src/backend/parser/parse_func.c
b/src/backend/parser/parse_func.c
index 5222231b51..cce8f49f52 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2053,7 +2053,6 @@ LookupFuncName(List *funcname, int nargs, const
Oid *argtypes, bool noError)
{
if (clist->next)
{
-               if (!noError)
ereport(ERROR,
(errcode(ERRCODE_AMBIGUOUS_FUNCTION),
errmsg("function name \"%s\" is not unique",

But I may overlook something of course.

I had the same thoughts so I did that in the original patch, but see
Tom's comment which starts with "I don't like that a bit"

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, 12 Feb 2019 at 16:09, Michael Paquier <michael@paquier.xyz> wrote:

FWIW, it makes me a bit uneasy to change this function signature in
back-branches if that's the intention as I suspect that it gets used
in extensions.. For HEAD that's fine of course.

I wondered about this too and questioned Tom about it above. There
was no response.

I just assumed Tom didn't think it was worth fiddling with in back-branches.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

On Tue, 12 Feb 2019 at 16:09, Michael Paquier <michael@paquier.xyz> wrote:

FWIW, it makes me a bit uneasy to change this function signature in
back-branches if that's the intention as I suspect that it gets used
in extensions.. For HEAD that's fine of course.

I wondered about this too and questioned Tom about it above. There
was no response.

Sorry, I didn't realize you'd asked a question.

I just assumed Tom didn't think it was worth fiddling with in back-branches.

Yeah, exactly. Not only do I not feel a need to change this behavior
in the back branches, but the original patch is *also* an API change,
in that it changes the behavior of what appears to be a well-defined
boolean parameter. The fact that none of the call sites found in
core today would care doesn't change that; you'd still be risking
breaking extensions, and/or future back-patches.

So I think targeting this for HEAD only is fine.

regards, tom lane