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

On Tue, Jul 2, 2013 at 5:39 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:

> 2013/3/8 Josh Kupershmidt <schmiddy(at)gmail(dot)com>:
>> On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>>> 2013/3/8 Josh Kupershmidt <schmiddy(at)gmail(dot)com>:
>>
>>>> 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.
>>
>> I'm not sure I understand what you're getting at, but maybe my
>> proposal wasn't so clear. Right now, you can specify --clean along
>> with -Fc to pg_dump, and pg_dump will not complain even though this
>> combination is nonsense. I am proposing that pg_dump error out in this
>> case, i.e.
>>
>> $ pg_dump -Fc --file=test.dump --clean -d test
>> pg_dump: option --clean only valid with plain format dump
>>
>> Although this lack of an error a (IMO) misfeature of existing pg_dump,
>> so if you'd rather leave this issue aside for your patch, that is
>> fine.
>>
>
> I tested last patch and I am thinking so this patch has sense for
> custom format too
>
> [postgres(at)localhost ~]$ /usr/local/pgsql/bin/pg_dump --clean -t a
> -Fc postgres > dump
> [postgres(at)localhost ~]$ psql -c "drop table a"
> DROP TABLE
> [postgres(at)localhost ~]$ pg_restore --clean -d postgres dump
> pg_restore: [archiver (db)] Error while PROCESSING TOC:
> pg_restore: [archiver (db)] Error from TOC entry 171; 1259 16462 TABLE
> a postgres
> pg_restore: [archiver (db)] could not execute query: ERROR: table "a"
> does not exist
> Command was: DROP TABLE public.a;
>
> WARNING: errors ignored on restore: 1
>
> [postgres(at)localhost ~]$ /usr/local/pgsql/bin/pg_dump --if-exist
> --clean -t a -Fc postgres > dump
> [postgres(at)localhost ~]$ psql -c "drop table a"
> DROP TABLE
> [postgres(at)localhost ~]$ pg_restore --clean -d postgres dump
>
> So limit for plain format is not too strict

I'm not sure I understand what you're proposing here, but for the
record I really don't think it's a good idea to encourage the use of
pg_dump -Fc --clean ...

Right now, we don't error out on this combination of command-line
options, but the --clean is effectively a no-op; you have to tell
pg_restore to use --clean. IMO, this is basically how it should be:
pg_dump, at least in custom-format, is preserving the contents of your
database with the understanding that you will use pg_restore wish to
restore from this dump in a variety of possible ways. Putting
restrictions like --clean into the custom-format dump file just makes
that dump file less useful overall.

Although it's still not clear to me why the examples you showed above
used *both* `pg_dump -Fc --clean ...` and `pg_restore --clean ...`
together. Surely the user should be specifying this preference in only
one place?

Josh