Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist - Mailing list pgsql-hackers

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:
> [Moving to -hackers]
>
> On Sat, Feb 23, 2013 at 2:51 PM, Pavel Stehule <pavel.stehule@gmail.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.

ok

>
>> * -- 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.)

no

some people - like we in our company would to use this feature for
quiet and strict mode for plain text format too.

enabling this feature has zero overhead so there are no reason block
it anywhere.

>
> 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.

I'll send updated version in next months

Thank you for review

Regards

Pavel

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