Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

[Moving to -hackers]

On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> so
>
> * --conditional-drops replaced by --if-exists

Thanks for the fixes, I played around with the patch a bit. I was sort
of expecting this example to work (after setting up the regression
database with `make installcheck`)

pg_dump --clean --if-exists -Fp -d regression --file=regression.sql
createdb test
psql -v ON_ERROR_STOP=1 --single-transaction -d test -f regression.sql

But it fails, first at:
...
DROP TRIGGER IF EXISTS tsvectorupdate ON public.test_tsvector;
ERROR: relation "public.test_tsvector" does not exist

This seems like a shortcoming of DROP TRIGGER ... IF EXISTS, and it
looks like DROP RULE ... IF EXISTS has the same problem. I recall DROP
... IF EXISTS being fixed recently for not to error out if the schema
specified for the object does not exist, and ISTM the same arguments
could be made in favor of fixing DROP TRIGGER/TABLE ... IF EXISTS not
to error out if the table doesn't exist.

Working further through the dump of the regression database, these
also present problems for --clean --if-exists dumps:

DROP CAST IF EXISTS (text AS public.casttesttype);
ERROR: type "public.casttesttype" does not exist

DROP OPERATOR IF EXISTS public.<% (point, widget);
ERROR: type "widget" does not exist

DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget);
ERROR: type "widget" does not exist

I'm not sure whether DROP CAST/OPERATOR/FUNCTION IF EXISTS should be
more tolerant of nonexistent types, of if the mess could perhaps be
avoided by dump reordering.

Note, this usability problem affects unpatched head as well:

pg_dump -Fc -d regression --file=regression.dump
pg_restore --clean -1 -d regression regression.dump
...
pg_restore: [archiver (db)] could not execute query: ERROR: type
"widget" does not exist
Command was: DROP FUNCTION public.widget_out(widget);

(The use here is a little different than the first example above, but
I would still expect this case to work.) The above problems with IF
EXISTS aren't really a problem of the patch per se, but IMO it would
be nice to straighten all the issues out together for 9.4.

> * -- additional check, available only with -c option

Cool. I think it would also be useful to check that --clean may only
be used with --format=p to avoid any confusion there. (This issue
could be addressed in a separate patch if you'd rather not lard this
patch.)

Some comments on the changes:

1. There is at least one IF EXISTS check missing from pg_dump.c, see
for example this statement from a dump of the regression database with
--if-exists:

ALTER TABLE public.nv_child_2010 DROP CONSTRAINT nv_child_2010_d_check;

2. Shouldn't pg_restore get --if-exists as well?

3.
+ printf(_(" --if-exists don't report error if
cleaned object doesn't exist\n"));

This help output bleeds just over our de facto 80-character limit.
Also contractions seem to be avoided elsewhere. It's a little hard to
squeeze a decent explanation into one line, but perhaps:

Use IF EXISTS when dropping objects

would be better. The sgml changes could use some wordsmithing and
grammar fixes. I could clean these up for you in a later version if
you'd like.

4. There seem to be spurious whitespace changes to the function
prototype and declaration for _printTocEntry.

That's all I've had time for so far...

Josh