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

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
isignored 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.stehule@gmail.com> wrote:
> I propose maybe more strongly comment fact so noError is applied only on "not found" event. In other cases, this flag
isignored 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.rowley@2ndquadrant.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@sss.pgh.pa.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