pgsql: Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/RO

Started by Tom Laneover 7 years ago4 messagescomitters
Jump to latest
#1Tom Lane
tgl@sss.pgh.pa.us

Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/ROUTINE.

These commands allow the argument type list to be omitted if there is
just one object that matches by name. However, if that syntax was
used with DROP IF EXISTS and there was more than one match, you got
a "function ... does not exist, skipping" notice message rather than a
truthful complaint about the ambiguity. This was basically due to
poor factorization and a rats-nest of logic, so refactor the relevant
lookup code to make it cleaner.

Note that this amounts to narrowing the scope of which sorts of
error conditions IF EXISTS will bypass. Per discussion, we only
intend it to skip no-such-object cases, not multiple-possible-matches
cases.

Per bug #15572 from Ash Marath. Although this definitely seems like
a bug, it's not clear that people would thank us for changing the
behavior in minor releases, so no back-patch.

David Rowley, reviewed by Julien Rouhaud and Pavel Stehule

Discussion: /messages/by-id/15572-ed1b9ed09503de8a@postgresql.org

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/bfb456c1b9656d5b717b84d833f62cf712b21726

Modified Files
--------------
src/backend/parser/parse_func.c | 396 ++++++++++++++++++---------
src/include/parser/parse_func.h | 4 +-
src/test/regress/expected/drop_if_exists.out | 29 ++
src/test/regress/sql/drop_if_exists.sql | 24 ++
4 files changed, 328 insertions(+), 125 deletions(-)

#2Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#1)
Re: pgsql: Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/RO

Hi,

On 2019-03-21 15:52:23 +0000, Tom Lane wrote:

Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/ROUTINE.

These commands allow the argument type list to be omitted if there is
just one object that matches by name. However, if that syntax was
used with DROP IF EXISTS and there was more than one match, you got
a "function ... does not exist, skipping" notice message rather than a
truthful complaint about the ambiguity. This was basically due to
poor factorization and a rats-nest of logic, so refactor the relevant
lookup code to make it cleaner.

Note that this amounts to narrowing the scope of which sorts of
error conditions IF EXISTS will bypass. Per discussion, we only
intend it to skip no-such-object cases, not multiple-possible-matches
cases.

Per bug #15572 from Ash Marath. Although this definitely seems like
a bug, it's not clear that people would thank us for changing the
behavior in minor releases, so no back-patch.

David Rowley, reviewed by Julien Rouhaud and Pavel Stehule

Discussion: /messages/by-id/15572-ed1b9ed09503de8a@postgresql.org

I now get:

/home/andres/src/postgresql/src/backend/parser/parse_func.c: In function ‘LookupFuncWithArgs’:
/home/andres/src/postgresql/src/backend/parser/parse_func.c:2285:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
switch (objtype)
^~~~~~
/home/andres/src/postgresql/src/backend/parser/parse_func.c:2336:4: note: here
case FUNCLOOKUP_AMBIGUOUS:
^~~~

which seems like a somewhat righteous complaint? I'd just add a break to
silence it (which can't be reached, because all paths ought to error
out).

Greetings,

Andres Freund

#3David Rowley
dgrowleyml@gmail.com
In reply to: Andres Freund (#2)
Re: pgsql: Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/RO

On Sat, 23 Mar 2019 at 15:33, Andres Freund <andres@anarazel.de> wrote:

I now get:

/home/andres/src/postgresql/src/backend/parser/parse_func.c: In function ‘LookupFuncWithArgs’:
/home/andres/src/postgresql/src/backend/parser/parse_func.c:2285:5: warning: this statement may fall through [-Wimplicit-fallthrough=]
switch (objtype)
^~~~~~
/home/andres/src/postgresql/src/backend/parser/parse_func.c:2336:4: note: here
case FUNCLOOKUP_AMBIGUOUS:
^~~~

which seems like a somewhat righteous complaint? I'd just add a break to
silence it (which can't be reached, because all paths ought to error
out).

hmm. Surprised your compiler can't see that's not possible. I guess we
just need a break on line 2335.

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

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: David Rowley (#3)
Re: pgsql: Improve error reporting for DROP FUNCTION/PROCEDURE/AGGREGATE/RO

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

On Sat, 23 Mar 2019 at 15:33, Andres Freund <andres@anarazel.de> wrote:

I now get:
/home/andres/src/postgresql/src/backend/parser/parse_func.c:2285:5: warning: this statement may fall through [-Wimplicit-fallthrough=]

hmm. Surprised your compiler can't see that's not possible. I guess we
just need a break on line 2335.

Yeah, this no doubt is connected to what I noticed in commit 41c912cad:

Testing with gcc 8.0.1 (Fedora 28's current version), I found that
I also had to put explicit "break"s after elog(ERROR) or ereport(ERROR);
apparently, for this purpose gcc doesn't recognize that those don't
return. That seems like possibly a gcc bug, but it's fine because
in most places we did that anyway; so this amounts to a visit from the
style police.

Will add the break shortly.

regards, tom lane