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

The following bug has been logged on the website:

Bug reference: 15572
Logged by: Ash Marath
Email address: makmarath(at)hotmail(dot)com
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
makmarath(at)hotmail(dot)com

On Fri, 4 Jan 2019 at 09:44, PG Bug reporting form
<noreply(at)postgresql(dot)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(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)com>
Sent: Thursday, January 3, 2019 11:45:16 PM
To: makmarath(at)hotmail(dot)com; pgsql-bugs(at)lists(dot)postgresql(dot)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(at)postgresql(dot)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(dot)g(dot)johnston(at)gmail(dot)com>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: makmarath(at)hotmail(dot)com; pgsql-bugs(at)lists(dot)postgresql(dot)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(dot)rowley(at)2ndquadrant(dot)com<mailto:david(dot)rowley(at)2ndquadrant(dot)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(dot)g(dot)johnston(at)gmail(dot)com>
Sent: Friday, January 4, 2019 1:10:05 AM
To: David Rowley
Cc: makmarath(at)hotmail(dot)com; pgsql-bugs(at)lists(dot)postgresql(dot)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(dot)rowley(at)2ndquadrant(dot)com<mailto:david(dot)rowley(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(dot)rowley(at)2ndquadrant(dot)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(at)sss(dot)pgh(dot)pa(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)zakirov(at)postgrespro(dot)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(at)paquier(dot)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(dot)rowley(at)2ndquadrant(dot)com> writes:
> On Tue, 12 Feb 2019 at 16:09, Michael Paquier <michael(at)paquier(dot)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

On Sun, Feb 17, 2019 at 05:31:43PM -0500, Tom Lane wrote:
> So I think targeting this for HEAD only is fine.

OK, thanks for helping me catching up, Tom and David!
--
Michael

On Sun, Feb 17, 2019 at 11:31 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > On Tue, 12 Feb 2019 at 16:09, Michael Paquier <michael(at)paquier(dot)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.

Extensions calling those functions with old true/false values probably
won't get any warning or error during compile. Is is something we
should worry about or is it enough to keep the same behavior in this
case?

@david: small typo, you removed a space in this chunk

- * LookupFuncName and let it make any error messages. Otherwise, we make
+ * LookupFuncNameand let it make any error messages. Otherwise, we make

Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> On Sun, Feb 17, 2019 at 11:31 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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.

> Extensions calling those functions with old true/false values probably
> won't get any warning or error during compile. Is is something we
> should worry about or is it enough to keep the same behavior in this
> case?

Yeah, I thought about that. We can avoid such problems by assigning
the enum values such that 0 and 1 correspond to the old behaviors.
I didn't look to see if the proposed patch does it like that right
now, but it should be an easy fix if not.

regards, tom lane

On Tue, Feb 19, 2019 at 5:46 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> >
> > Extensions calling those functions with old true/false values probably
> > won't get any warning or error during compile. Is is something we
> > should worry about or is it enough to keep the same behavior in this
> > case?
>
> Yeah, I thought about that. We can avoid such problems by assigning
> the enum values such that 0 and 1 correspond to the old behaviors.
> I didn't look to see if the proposed patch does it like that right
> now, but it should be an easy fix if not.

It does, I was just wondering whether that was a good enough solution.

Thinking more about it, I'm not sure if there's a general policy for
enums, but should we have an AssertArg() in LookupFuncName[WithArgs]
to check that a correct value was passed?

On Wed, 20 Feb 2019 at 05:00, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> @david: small typo, you removed a space in this chunk
>
> - * LookupFuncName and let it make any error messages. Otherwise, we make
> + * LookupFuncNameand let it make any error messages. Otherwise, we make

Thanks. Fixed in the attached.

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

On Wed, 20 Feb 2019 at 06:48, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Tue, Feb 19, 2019 at 5:46 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > >
> > > Extensions calling those functions with old true/false values probably
> > > won't get any warning or error during compile. Is is something we
> > > should worry about or is it enough to keep the same behavior in this
> > > case?
> >
> > Yeah, I thought about that. We can avoid such problems by assigning
> > the enum values such that 0 and 1 correspond to the old behaviors.
> > I didn't look to see if the proposed patch does it like that right
> > now, but it should be an easy fix if not.
>
> It does, I was just wondering whether that was a good enough solution.
>
> Thinking more about it, I'm not sure if there's a general policy for
> enums, but should we have an AssertArg() in LookupFuncName[WithArgs]
> to check that a correct value was passed?

I think since the original argument was a bool then it's pretty
unlikely that such an assert would ever catch anything, given 0 and 1
are both valid values for this enum type.

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

On Tue, Feb 19, 2019 at 9:04 PM David Rowley
<david(dot)rowley(at)2ndquadrant(dot)com> wrote:
>
> On Wed, 20 Feb 2019 at 06:48, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
> >
> > On Tue, Feb 19, 2019 at 5:46 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > >
> > > Julien Rouhaud <rjuju123(at)gmail(dot)com> writes:
> > > >
> > > > Extensions calling those functions with old true/false values probably
> > > > won't get any warning or error during compile. Is is something we
> > > > should worry about or is it enough to keep the same behavior in this
> > > > case?
> > >
> > > Yeah, I thought about that. We can avoid such problems by assigning
> > > the enum values such that 0 and 1 correspond to the old behaviors.
> > > I didn't look to see if the proposed patch does it like that right
> > > now, but it should be an easy fix if not.
> >
> > It does, I was just wondering whether that was a good enough solution.
> >
> > Thinking more about it, I'm not sure if there's a general policy for
> > enums, but should we have an AssertArg() in LookupFuncName[WithArgs]
> > to check that a correct value was passed?
>
> I think since the original argument was a bool then it's pretty
> unlikely that such an assert would ever catch anything, given 0 and 1
> are both valid values for this enum type.

Indeed. It looks all fine to me in v6, so I'm marking the patch as
ready for committer.

Thanks!

On Wed, 20 Feb 2019 at 09:20, Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> On Tue, Feb 19, 2019 at 9:04 PM David Rowley
> <david(dot)rowley(at)2ndquadrant(dot)com> wrote:
> > I think since the original argument was a bool then it's pretty
> > unlikely that such an assert would ever catch anything, given 0 and 1
> > are both valid values for this enum type.
>
> Indeed. It looks all fine to me in v6, so I'm marking the patch as
> ready for committer.

Great. Thanks for reviewing it.

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

On Wed, Feb 20, 2019 at 09:57:19AM +1300, David Rowley wrote:
> Great. Thanks for reviewing it.

You forgot to change a call of LookupFuncWithArgs() in
CreateTransform().

- address.objectId = LookupFuncWithArgs(objtype, castNode(ObjectWithArgs, object), missing_ok);
+ address.objectId = LookupFuncWithArgs(objtype, castNode(ObjectWithArgs, object),
+ missing_ok ? FUNCLOOKUP_ERRIFAMBIGUOUS :
+ FUNCLOOKUP_NORMAL);

LookupFuncWithArgs() calls itself LookupFuncName(), which may not use
the check type provided by the caller.. I think that the existing API
is already confusing enough, and this patch makes it a bit more
confusing by adding an extra error layer handling on top of it.
Wouldn't it be more simple from an error handling point of view to
move all the error handling into LookupFuncName() and let the caller
decide what kind of function type handling it expects from the start?
I think that the right call is to add the object type into the
arguments of LookupFuncName().
--
Michael

On Wed, 20 Feb 2019 at 18:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Wed, Feb 20, 2019 at 09:57:19AM +1300, David Rowley wrote:
> > Great. Thanks for reviewing it.
>
> You forgot to change a call of LookupFuncWithArgs() in
> CreateTransform().

Yikes, Arthur did mention that, but I somehow managed to stumble over
it when I checked. The attached fixes.

> - address.objectId = LookupFuncWithArgs(objtype, castNode(ObjectWithArgs, object), missing_ok);
> + address.objectId = LookupFuncWithArgs(objtype, castNode(ObjectWithArgs, object),
> + missing_ok ? FUNCLOOKUP_ERRIFAMBIGUOUS :
> + FUNCLOOKUP_NORMAL);
>
> LookupFuncWithArgs() calls itself LookupFuncName(), which may not use
> the check type provided by the caller.. I think that the existing API
> is already confusing enough, and this patch makes it a bit more
> confusing by adding an extra error layer handling on top of it.
> Wouldn't it be more simple from an error handling point of view to
> move all the error handling into LookupFuncName() and let the caller
> decide what kind of function type handling it expects from the start?
> I think that the right call is to add the object type into the
> arguments of LookupFuncName().

But there are plenty of callers that use LookupFuncName() directly. Do
you happen to know it's okay for all those to error out with the
additional error conditions that such a change would move into that
function? I certainly don't know that.

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

David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> On Wed, 20 Feb 2019 at 18:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> I think that the right call is to add the object type into the
>> arguments of LookupFuncName().

I'm not clear how that helps exactly?

> But there are plenty of callers that use LookupFuncName() directly. Do
> you happen to know it's okay for all those to error out with the
> additional error conditions that such a change would move into that
> function? I certainly don't know that.

The real problem here is that you've unilaterally decided that all callers
of get_object_address() need a particular behavior --- and not only that,
but a behavior that seems fairly surprising and unprincipled, given that
get_object_address's option is documented as "missing_ok" (something the
patch doesn't even bother to change). It's not very apparent to me why
function-related lookups should start behaving differently from other
lookups in that function, and it's sure not apparent that all callers of
get_object_address() are on board with it.

Should we be propagating that 3-way flag further up, to
get_object_address() callers? I dunno.

regards, tom lane

On Thu, 21 Feb 2019 at 09:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > On Wed, 20 Feb 2019 at 18:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >> I think that the right call is to add the object type into the
> >> arguments of LookupFuncName().
>
> I'm not clear how that helps exactly?
>
> > But there are plenty of callers that use LookupFuncName() directly. Do
> > you happen to know it's okay for all those to error out with the
> > additional error conditions that such a change would move into that
> > function? I certainly don't know that.
>
> The real problem here is that you've unilaterally decided that all callers
> of get_object_address() need a particular behavior --- and not only that,
> but a behavior that seems fairly surprising and unprincipled, given that
> get_object_address's option is documented as "missing_ok" (something the
> patch doesn't even bother to change). It's not very apparent to me why
> function-related lookups should start behaving differently from other
> lookups in that function, and it's sure not apparent that all callers of
> get_object_address() are on board with it.

I assume you're talking about:

* If the object is not found, an error is thrown, unless missing_ok is
* true. In this case, no lock is acquired, relp is set to NULL, and the
* returned address has objectId set to InvalidOid.

Well, I didn't update that comment because the code I've changed does
nothing different for the missing_ok case. The missing function error
is still raised or not raised correctly depending on the value of that
flag.

I understand your original gripe with the patch where I had changed
the meaning of noError to mean
"noError-Apart-From-If-Its-An-Ambiguous-Function", without much of any
documentation to mention that fact, but it seems to me that this time
around your confusing missing_ok with noError. To me noError means
don't raise an error, and missing_ok is intended for use with IF [NOT]
EXISTS... Yes, it might be getting used for something else, but since
we still raise an error when the function is missing when the flag is
set to false and don't when it's set to true. I fail to see why that
breaks the contract that's documented in the above comment. If you
think it does then please explain why.

> Should we be propagating that 3-way flag further up, to
> get_object_address() callers? I dunno.

I don't see why that's needed given what's mentioned above.

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

David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> On Thu, 21 Feb 2019 at 09:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The real problem here is that you've unilaterally decided that all callers
>> of get_object_address() need a particular behavior --- and not only that,
>> but a behavior that seems fairly surprising and unprincipled, given that
>> get_object_address's option is documented as "missing_ok" (something the
>> patch doesn't even bother to change).
>> ...
>> Should we be propagating that 3-way flag further up, to
>> get_object_address() callers? I dunno.

> I don't see why that's needed given what's mentioned above.

Well, if we're not going to propagate the option further up, then I think
we might as well do it like you originally suggested, ie leave the
signature of LookupFuncName alone and just document that the
ambiguous-function case is not covered by noError. As far as I can tell,
just about all the other callers besides get_object_address() have no
interest in this issue because they're not passing nargs == -1.
What's more, a lot of them look like this example in
findRangeSubtypeDiffFunction:

procOid = LookupFuncName(procname, 2, argList, FUNCLOOKUP_NOERROR);

if (!OidIsValid(procOid))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_FUNCTION),
errmsg("function %s does not exist",
func_signature_string(procname, 2, NIL, argList))));

so that if some day in the future FUNCLOOKUP_NOERROR could actually
suppress an ambiguous-function error here, the caller would proceed
to report an incorrect/misleading error message. It doesn't seem to
make much sense to allow callers to separately suppress or not
suppress ambiguous-function unless we also change the return
convention so that the callers can tell which case happened.
And that's looking a bit pointless, at least for now.

So, sorry for making you chase down this dead end, but it wasn't
obvious until now (to me anyway) that it was a dead end.

I did notice though that the patch fails to cover the same problem
right next door for procedures:

regression=# create procedure funcp(param1 text) language sql as 'select $1';
CREATE PROCEDURE
regression=# create procedure funcp(param1 int) language sql as 'select $1';
CREATE PROCEDURE
regression=# drop procedure funcp;
ERROR: could not find a procedure named "funcp"

This should surely be complaining about ambiguity, rather than giving
the same error text as if there were zero matches.

Possibly the same occurs for aggregates, though I'm not sure if that
code is reachable --- DROP AGGREGATE, at least, won't let you omit the
arguments.

I think the underlying cause of this is that LookupFuncWithArgs is in
the same situation I just complained for outside callers: it cannot tell
whether its noError request suppressed a not-found or ambiguous-function
case. Maybe the way to proceed here is to refactor within parse_func.c
so that there's an underlying function that returns an indicator of what
happened, and both LookupFuncName and LookupFuncWithArgs call it, each
with their own ideas about how to phrase the error reports. It's
certainly mighty ugly that LookupFuncWithArgs farms out the actual
error report in some cases and not others.

regards, tom lane

On Mon, 4 Mar 2019 at 09:14, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> > On Thu, 21 Feb 2019 at 09:36, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Should we be propagating that 3-way flag further up, to
> >> get_object_address() callers? I dunno.
>
> > I don't see why that's needed given what's mentioned above.
>
> Well, if we're not going to propagate the option further up, then I think
> we might as well do it like you originally suggested, ie leave the
> signature of LookupFuncName alone and just document that the
> ambiguous-function case is not covered by noError. As far as I can tell,
> just about all the other callers besides get_object_address() have no
> interest in this issue because they're not passing nargs == -1.

Okay.

> I think the underlying cause of this is that LookupFuncWithArgs is in
> the same situation I just complained for outside callers: it cannot tell
> whether its noError request suppressed a not-found or ambiguous-function
> case. Maybe the way to proceed here is to refactor within parse_func.c
> so that there's an underlying function that returns an indicator of what
> happened, and both LookupFuncName and LookupFuncWithArgs call it, each
> with their own ideas about how to phrase the error reports. It's
> certainly mighty ugly that LookupFuncWithArgs farms out the actual
> error report in some cases and not others.

I started working on this, but... damage control... I don't want to
take it too far without you having a glance at it first.

I've invented a new function by the name of LookupFuncNameInternal().
This attempts to find the function, but if it can't or the name is
ambiguous then it returns InvalidOid and sets an error code parameter.
I've made both LookupFuncName and LookupFuncWithArgs use this.

In my travels, I discovered something else that does not seem very great.

postgres=# create procedure abc(int) as $$ begin end; $$ language plpgsql;
CREATE PROCEDURE
postgres=# drop function if exists abc(int);
NOTICE: function abc(pg_catalog.int4) does not exist, skipping
DROP FUNCTION

I think it would be better to ERROR in that case. So with the attached
we now get:

postgres=# create procedure abc(int) as $$ begin end; $$ language plpgsql;
CREATE PROCEDURE
postgres=# drop function if exists abc(int);
ERROR: abc(integer) is not a function

I've also tried to have the error messages mention procedure when the
object is a procedure and function when its a function. It looks like
the previous code was calling LookupFuncName() with noError=true so it
could handle using "procedure" in the error messages itself, but it
failed to do that for an ambiguous procedure name. That should now be
fixed.

I also touched the too many function arguments case, but perhaps I
need to go further there and do something for aggregates. I've not
thought too hard about that.

I've not really read the patch back or done any polishing yet. Manual
testing done is minimal, and I didn't add tests for the new behaviour
change either. I can do more if the feedback is positive.

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

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

I read a discussion and I think so currently implemented behave (by last patch) is correct in all details.

I propose maybe more strongly comment fact so noError is applied only on "not found" event. In other cases, this flag is ignored and error is raised immediately there. I think so it is not good enough commented why.
This is significant change - in previous releases, noError was used like really noError, so should be commented more.

Regress tests are enough.
The patch is possible to apply without problems and compile without warnings

The new status of this patch is: Ready for Committer

Thanks for reviewing this.

On Wed, 20 Mar 2019 at 04:31, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I propose maybe more strongly comment fact so noError is applied only on "not found" event. In other cases, this flag is ignored and error is raised immediately there. I think so it is not good enough commented why.
> This is significant change - in previous releases, noError was used like really noError, so should be commented more.

I've made a change to the comments in LookupFuncWithArgs() to make
this more clear. I also ended up renaming noError to missing_ok.
Hopefully this backs up the comments and reduces the chances of
surprises.

> Regress tests are enough.
> The patch is possible to apply without problems and compile without warnings

Thanks. I also fixed a bug that caused an Assert failure when
performing DROP ROUTINE ambiguous_name; test added for that case too.

> The new status of this patch is: Ready for Committer

Great!

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

David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> writes:
> [ drop_func_if_not_exists_fix_v9.patch ]

Pushed with mostly-cosmetic adjustments.

I noticed a couple of loose ends that are somewhat outside the scope
of the bug report, but maybe are worth considering now:

1. There's some inconsistency in the wording of the error messages in
these routines, eg

errmsg("%s is not a function",
vs
errmsg("%s is not a procedure",
vs
errmsg("function %s is not an aggregate",

Also there's
errmsg("function name \"%s\" is not unique",
where elsewhere in parse_func.c, we find
errmsg("function %s is not unique",

You didn't touch this and I didn't either, but maybe we should try to
make these consistent?

2. Consider

regression=# CREATE FUNCTION ambig(int) returns int as $$ select $1; $$ language sql;
CREATE FUNCTION
regression=# CREATE PROCEDURE ambig() as $$ begin end; $$ language plpgsql;
CREATE PROCEDURE
regression=# DROP PROCEDURE ambig;
ERROR: procedure name "ambig" is not unique
HINT: Specify the argument list to select the procedure unambiguously.

Arguably, because I said "drop procedure", there's no ambiguity here;
but we don't account for objtype while doing the lookup.

I'm inclined to leave point 2 alone, because we haven't had complaints
about it, and because I'm not sure we could make it behave in a clean
way given the historical ambiguity about what OBJECT_FUNCTION should
match. But perhaps it's worth discussing.

regards, tom lane

On Fri, 22 Mar 2019 at 05:04, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Pushed with mostly-cosmetic adjustments.

Thanks for pushing this.

> I noticed a couple of loose ends that are somewhat outside the scope
> of the bug report, but maybe are worth considering now:
>
> 1. There's some inconsistency in the wording of the error messages in
> these routines, eg
>
> errmsg("%s is not a function",
> vs
> errmsg("%s is not a procedure",
> vs
> errmsg("function %s is not an aggregate",
>
> Also there's
> errmsg("function name \"%s\" is not unique",
> where elsewhere in parse_func.c, we find
> errmsg("function %s is not unique",
>
> You didn't touch this and I didn't either, but maybe we should try to
> make these consistent?

I think aligning those is a good idea. I had just been wondering to
myself last night about how much binary space is taken up by needless
additional string constants that could be normalised a bit.
Translators might be happy if we did that.

> 2. Consider
>
> regression=# CREATE FUNCTION ambig(int) returns int as $$ select $1; $$ language sql;
> CREATE FUNCTION
> regression=# CREATE PROCEDURE ambig() as $$ begin end; $$ language plpgsql;
> CREATE PROCEDURE
> regression=# DROP PROCEDURE ambig;
> ERROR: procedure name "ambig" is not unique
> HINT: Specify the argument list to select the procedure unambiguously.
>
> Arguably, because I said "drop procedure", there's no ambiguity here;
> but we don't account for objtype while doing the lookup.

Yeah. I went with reporting the objtype that was specified in a
command. I stayed well clear of allowing overlapping names between
procedures and functions. It would be hard to put that back if we
ever discovered a reason we shouldn't have done it.

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