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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

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

apt.postgresql.org Repository Update
The following bug has been logged on the website:

Bug reference:      15572
Logged by:          Ash Marath
Email address:      [hidden email]
PostgreSQL version: 10.5
Operating system:   RDS (on Amazon)
Description:        

Scenario:
DB has 2 functions with same name.
DB: testDB
Schema: test
Function 1: test.func1(param1 text, param2 text)
Function 2: test.func1(param1 text)
---------------------------------
Issue: Misleading message reported by "DROP FUNCTION" command with the above
scenario

Step 1:
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 ------
--------------------------------------------------------------------------------------------------------
Step 2:
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;

Thanks
Ash Marath
[hidden email]

Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Fri, 4 Jan 2019 at 09:44, PG Bug reporting form
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David G Johnston
On Thursday, January 3, 2019, David Rowley <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

a Marath
In reply to this post by David Rowley-3


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 <[hidden email]>
Sent: Thursday, January 3, 2019 11:45:16 PM
To: [hidden email]; [hidden email]
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
<[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

a Marath
In reply to this post by David G Johnston
I second David's suggestion

A Marath.


From: David G. Johnston <[hidden email]>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: [hidden email]; [hidden email]
Subject: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name
 
On Thursday, January 3, 2019, David Rowley <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

a Marath
In reply to this post by David G Johnston
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 <[hidden email]>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: [hidden email]; [hidden email]
Subject: BUG #15572: Misleading message reported by "Drop function operation" on DB with functions having same name
 
On Thursday, January 3, 2019, David Rowley <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

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

Alvaro Herrera-9
In reply to this post by David Rowley-3
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

Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Tue, 8 Jan 2019 at 03:54, Alvaro Herrera <[hidden email]> 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

drop_func_if_not_exists_fix.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
David Rowley <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Wed, 9 Jan 2019 at 13:36, Tom Lane <[hidden email]> wrote:
>
> David Rowley <[hidden email]> 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

drop_func_if_not_exists_fix_v2.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Wed, 16 Jan 2019 at 12:38, David Rowley <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
In reply to this post by David Rowley-3
On Wed, 16 Jan 2019 at 12:38, David Rowley <[hidden email]> 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

drop_func_if_not_exists_fix_v3.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Thu, 7 Feb 2019 at 01:17, David Rowley <[hidden email]> 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

drop_func_if_not_exists_fix_v4.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Lane-2
David Rowley <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David Rowley-3
On Mon, 11 Feb 2019 at 11:39, Tom Lane <[hidden email]> 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

drop_func_if_not_exists_fix_v5.patch (34K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

a.zakirov
In reply to this post by David Rowley-3
Hello,

On 11.02.2019 05:36, David Rowley wrote:
> On Mon, 11 Feb 2019 at 11:39, Tom Lane <[hidden email]> 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

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