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

Started by Dave Rolskyabout 13 years ago83 messageshackersbugs
Jump to latest
#1Dave Rolsky
autarch@urth.org
hackersbugs

The following bug has been logged on the website:

Bug reference: 7873
Logged by: Dave Rolsky
Email address: autarch@urth.org
PostgreSQL version: 9.2.3
Operating system: Linux
Description:

When you pass the --clean option to pg_restore it tries to drop tables
without checking if they exist. This results in lots of error output. If
you're running pg_restore via an automated process it's very hard to
distinguish between these "ok" errors and real errors.

It should be using "DROP TABLE IF EXISTS" and the equivalent for
constraints.

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#2Bruce Momjian
bruce@momjian.us
In reply to: Dave Rolsky (#1)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Wed, Feb 13, 2013 at 08:22:43PM +0000, autarch@urth.org wrote:

The following bug has been logged on the website:

Bug reference: 7873
Logged by: Dave Rolsky
Email address: autarch@urth.org
PostgreSQL version: 9.2.3
Operating system: Linux
Description:

When you pass the --clean option to pg_restore it tries to drop tables
without checking if they exist. This results in lots of error output. If
you're running pg_restore via an automated process it's very hard to
distinguish between these "ok" errors and real errors.

It should be using "DROP TABLE IF EXISTS" and the equivalent for
constraints.

Well, I think the question is whether you want error feedback for things
that don't exist. I don't really know the answer.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#3Dave Rolsky
autarch@urth.org
In reply to: Bruce Momjian (#2)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Fri, 15 Feb 2013, Bruce Momjian wrote:

On Wed, Feb 13, 2013 at 08:22:43PM +0000, autarch@urth.org wrote:

The following bug has been logged on the website:

Bug reference: 7873
Logged by: Dave Rolsky
Email address: autarch@urth.org
PostgreSQL version: 9.2.3
Operating system: Linux
Description:

When you pass the --clean option to pg_restore it tries to drop tables
without checking if they exist. This results in lots of error output. If
you're running pg_restore via an automated process it's very hard to
distinguish between these "ok" errors and real errors.

It should be using "DROP TABLE IF EXISTS" and the equivalent for
constraints.

Well, I think the question is whether you want error feedback for things
that don't exist. I don't really know the answer.

Fair enough. It should probably an option to add "if exists", at least. I
can't imagine I'm the only using this tool to ship database updates around
to different machines, some of which may not have new tables. I'd really
like to be able to know when the restore fails versus when it succeeds but
is noisy.

Cheers,

-dave

/*============================================================
http://VegGuide.org http://blog.urth.org
Your guide to all that's veg House Absolute(ly Pointless)
============================================================*/

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#4Bruce Momjian
bruce@momjian.us
In reply to: Dave Rolsky (#3)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Fri, Feb 15, 2013 at 04:06:12PM -0600, Dave Rolsky wrote:

On Fri, 15 Feb 2013, Bruce Momjian wrote:

On Wed, Feb 13, 2013 at 08:22:43PM +0000, autarch@urth.org wrote:

The following bug has been logged on the website:

Bug reference: 7873
Logged by: Dave Rolsky
Email address: autarch@urth.org
PostgreSQL version: 9.2.3
Operating system: Linux
Description:

When you pass the --clean option to pg_restore it tries to drop tables
without checking if they exist. This results in lots of error output. If
you're running pg_restore via an automated process it's very hard to
distinguish between these "ok" errors and real errors.

It should be using "DROP TABLE IF EXISTS" and the equivalent for
constraints.

Well, I think the question is whether you want error feedback for things
that don't exist. I don't really know the answer.

Fair enough. It should probably an option to add "if exists", at
least. I can't imagine I'm the only using this tool to ship database
updates around to different machines, some of which may not have new
tables. I'd really like to be able to know when the restore fails
versus when it succeeds but is noisy.

All I can say is I don't remember anyone asking for this in the past.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#4)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Feb 15, 2013 at 04:06:12PM -0600, Dave Rolsky wrote:

Fair enough. It should probably an option to add "if exists", at
least. I can't imagine I'm the only using this tool to ship database
updates around to different machines, some of which may not have new
tables. I'd really like to be able to know when the restore fails
versus when it succeeds but is noisy.

All I can say is I don't remember anyone asking for this in the past.

I think it has come up before. I wouldn't object to a pg_dump option to
add IF EXISTS to all the drop commands (though changing the default
behavior would be more controversial). Don't intend to spend my own
time on it though ...

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#6Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#5)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013/2/16 Tom Lane <tgl@sss.pgh.pa.us>:

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Feb 15, 2013 at 04:06:12PM -0600, Dave Rolsky wrote:

Fair enough. It should probably an option to add "if exists", at
least. I can't imagine I'm the only using this tool to ship database
updates around to different machines, some of which may not have new
tables. I'd really like to be able to know when the restore fails
versus when it succeeds but is noisy.

All I can say is I don't remember anyone asking for this in the past.

I think it has come up before. I wouldn't object to a pg_dump option to
add IF EXISTS to all the drop commands (though changing the default
behavior would be more controversial). Don't intend to spend my own
time on it though ...

we use this feature more than one year.

I'll send patch at Monday

Regards

Pavel Stehule

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#7Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#6)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

2013/2/16 Pavel Stehule <pavel.stehule@gmail.com>:

2013/2/16 Tom Lane <tgl@sss.pgh.pa.us>:

Bruce Momjian <bruce@momjian.us> writes:

On Fri, Feb 15, 2013 at 04:06:12PM -0600, Dave Rolsky wrote:

Fair enough. It should probably an option to add "if exists", at
least. I can't imagine I'm the only using this tool to ship database
updates around to different machines, some of which may not have new
tables. I'd really like to be able to know when the restore fails
versus when it succeeds but is noisy.

All I can say is I don't remember anyone asking for this in the past.

I think it has come up before. I wouldn't object to a pg_dump option to
add IF EXISTS to all the drop commands (though changing the default
behavior would be more controversial). Don't intend to spend my own
time on it though ...

we use this feature more than one year.

I'll send patch at Monday

here is patch, that we use about one year - originally for 9.1 - I did
port to 9.3

Regards

Pavel

Show quoted text

Regards

Pavel Stehule

regards, tom lane

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

Attachments:

conditional-drops.patchapplication/octet-stream; name=conditional-drops.patchDownload+130-102
#8Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#7)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Tue, Feb 19, 2013 at 6:00 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/2/16 Pavel Stehule <pavel.stehule@gmail.com>:

2013/2/16 Tom Lane <tgl@sss.pgh.pa.us>:

I think it has come up before. I wouldn't object to a pg_dump option to
add IF EXISTS to all the drop commands (though changing the default
behavior would be more controversial). Don't intend to spend my own
time on it though ...

FYI, it was proposed here:
/messages/by-id/507AD08C.5020603@dalibo.com

here is patch, that we use about one year - originally for 9.1 - I did
port to 9.3

dropdb and dropuser both support a similar option named --if-exists. I
suggest --if-exists instead of --conditional-drops for consistency.
I've only glanced at the patch, but if it makes no sense to use
--conditional-drops (or --if-exists, whatever it ends up being called)
without --clean, then attempting to do so should raise an error.

Josh

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#9Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#8)
hackersbugs
Re: BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

2013/2/21 Josh Kupershmidt <schmiddy@gmail.com>:

On Tue, Feb 19, 2013 at 6:00 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/2/16 Pavel Stehule <pavel.stehule@gmail.com>:

2013/2/16 Tom Lane <tgl@sss.pgh.pa.us>:

I think it has come up before. I wouldn't object to a pg_dump option to
add IF EXISTS to all the drop commands (though changing the default
behavior would be more controversial). Don't intend to spend my own
time on it though ...

FYI, it was proposed here:
/messages/by-id/507AD08C.5020603@dalibo.com

here is patch, that we use about one year - originally for 9.1 - I did
port to 9.3

dropdb and dropuser both support a similar option named --if-exists. I
suggest --if-exists instead of --conditional-drops for consistency.
I've only glanced at the patch, but if it makes no sense to use
--conditional-drops (or --if-exists, whatever it ends up being called)
without --clean, then attempting to do so should raise an error.

so

* --conditional-drops replaced by --if-exists
* -- additional check, available only with -c option
* fix bug with dump custom functions

Regards

Pavel

Show quoted text

Josh

Attachments:

conditional-drops.patchapplication/octet-stream; name=conditional-drops.patchDownload+183-147
#10Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#9)
hackersbugs
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.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.

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

--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

#11Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#10)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#11)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/3/8 Josh Kupershmidt <schmiddy@gmail.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.

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#12)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:

On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/3/8 Josh Kupershmidt <schmiddy@gmail.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'll see - please, stay tuned to 9.4 first commitfest

Regards

Pavel

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#13)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I'll see - please, stay tuned to 9.4 first commitfest

Hi Pavel,
Just a reminder, I didn't see this patch in the current commitfest. I
would be happy to spend some more time reviewing if you wish to pursue
the patch.

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#14)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:

On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I'll see - please, stay tuned to 9.4 first commitfest

Hi Pavel,
Just a reminder, I didn't see this patch in the current commitfest. I
would be happy to spend some more time reviewing if you wish to pursue
the patch.

Hello

yes, I hadn't free time for finalization of last patch. I hope so I
can do final version next month to next commitfest.

Regards and thank you

Pavel

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#16Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#12)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

2013/3/8 Josh Kupershmidt <schmiddy@gmail.com>:

On Fri, Mar 8, 2013 at 2:23 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

2013/3/8 Josh Kupershmidt <schmiddy@gmail.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@localhost ~]$ /usr/local/pgsql/bin/pg_dump --clean -t a
-Fc postgres > dump
[postgres@localhost ~]$ psql -c "drop table a"
DROP TABLE
[postgres@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@localhost ~]$ /usr/local/pgsql/bin/pg_dump --if-exist
--clean -t a -Fc postgres > dump
[postgres@localhost ~]$ psql -c "drop table a"
DROP TABLE
[postgres@localhost ~]$ pg_restore --clean -d postgres dump

So limit for plain format is not too strict

Regards

Pavel

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#10)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

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.

yes, I am thinking so it is probably best solution. Without it I
should to generate a DO statement with necessary conditions. :(

I'll prepare patch and proposal.

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.

we can raise a warning instead error ?

Regards

Pavel

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#14)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

remastered patch

still there is a issue with dependencies

Regards

Pavel Stehule

2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:

Show quoted text

On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I'll see - please, stay tuned to 9.4 first commitfest

Hi Pavel,
Just a reminder, I didn't see this patch in the current commitfest. I
would be happy to spend some more time reviewing if you wish to pursue
the patch.

Josh

Attachments:

conditional-drops.patchapplication/octet-stream; name=conditional-drops.patchDownload+142-41
#19Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#18)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

Hello

I am sending a patch that removes strict requirements for DROP IF
EXISTS statements. This behave is similar to our ALTER IF EXISTS
behave now.

postgres=# DROP CAST IF EXISTS (sss AS public.casttesttype);
NOTICE: types "sss" and "public.casttesttype" does not exist, skipping
DROP CAST
postgres=# DROP FUNCTION IF EXISTS public.pt_in_widget(point, widget);
NOTICE: function public.pt_in_widget(point,widget) does not exist, skipping
DROP FUNCTION
postgres=# DROP OPERATOR IF EXISTS public.<% (point, widget);
NOTICE: operator public.<% does not exist, skipping
DROP OPERATOR
postgres=# DROP TRIGGER test_trigger_exists ON no_such_table;
ERROR: relation "no_such_table" does not exist
postgres=# DROP TRIGGER IF EXISTS test_trigger_exists ON no_such_table;
NOTICE: trigger "test_trigger_exists" for table "no_such_table" does
not exist, skipping
DROP TRIGGER

This functionality is necessary for correct quite reload from dump
without possible warnings

Regards

Pavel

2013/7/2 Pavel Stehule <pavel.stehule@gmail.com>:

Show quoted text

Hello

remastered patch

still there is a issue with dependencies

Regards

Pavel Stehule

2013/6/17 Josh Kupershmidt <schmiddy@gmail.com>:

On Fri, Mar 8, 2013 at 11:58 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I'll see - please, stay tuned to 9.4 first commitfest

Hi Pavel,
Just a reminder, I didn't see this patch in the current commitfest. I
would be happy to spend some more time reviewing if you wish to pursue
the patch.

Josh

Attachments:

drop-if-exists-no-double-check.patchapplication/octet-stream; name=drop-if-exists-no-double-check.patchDownload+208-197
#20Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#19)
hackersbugs
Re: [BUGS] BUG #7873: pg_restore --clean tries to drop tables that don't exist

On Fri, Jul 5, 2013 at 12:16 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

I am sending a patch that removes strict requirements for DROP IF
EXISTS statements. This behave is similar to our ALTER IF EXISTS
behave now.

+1 for this idea. But this patch should be treated as a separate issue
from the use of IF EXISTS in pg_dump/pg_restore, right? If so, I
suggest starting a new thread about this patch to make reviewing
easier.

Josh

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Pavel Stehule
pavel.stehule@gmail.com
In reply to: Josh Kupershmidt (#20)
hackersbugs
#22Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#16)
hackersbugs
#23Josh Kupershmidt
schmiddy@gmail.com
In reply to: Pavel Stehule (#18)
hackersbugs
#24Satoshi Nagayasu
snaga@uptime.jp
In reply to: Pavel Stehule (#19)
hackersbugs
#25Pavel Stehule
pavel.stehule@gmail.com
In reply to: Satoshi Nagayasu (#24)
hackersbugs
#26Andrew Dunstan
andrew@dunslane.net
In reply to: Pavel Stehule (#25)
hackersbugs
#27Josh Kupershmidt
schmiddy@gmail.com
In reply to: Andrew Dunstan (#26)
hackersbugs
#28Andres Freund
andres@anarazel.de
In reply to: Andrew Dunstan (#26)
hackersbugs
#29Andrew Dunstan
andrew@dunslane.net
In reply to: Andres Freund (#28)
hackersbugs
#30Andres Freund
andres@anarazel.de
In reply to: Andrew Dunstan (#29)
hackersbugs
#31Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Andres Freund (#28)
hackersbugs
#32Andres Freund
andres@anarazel.de
In reply to: Tomas Vondra (#31)
hackersbugs
#33Josh Kupershmidt
schmiddy@gmail.com
In reply to: Josh Kupershmidt (#27)
hackersbugs
#34Robert Haas
robertmhaas@gmail.com
In reply to: Josh Kupershmidt (#33)
hackersbugs
#35Pavel Stehule
pavel.stehule@gmail.com
In reply to: Robert Haas (#34)
hackersbugs
#36Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#32)
hackersbugs
#37Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#36)
hackersbugs
#38Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#37)
hackersbugs
#39Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#38)
hackersbugs
#40Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#39)
hackersbugs
#41Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#40)
hackersbugs
#42Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#41)
hackersbugs
#43Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#42)
hackersbugs
#44Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#43)
hackersbugs
#45Pavel Stehule
pavel.stehule@gmail.com
In reply to: Andres Freund (#32)
hackersbugs
#46Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#44)
hackersbugs
#47Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#46)
hackersbugs
#48Pavel Stehule
pavel.stehule@gmail.com
In reply to: Pavel Stehule (#46)
hackersbugs
#49Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Pavel Stehule (#48)
hackersbugs
#50Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#49)
hackersbugs
#51Robert Haas
robertmhaas@gmail.com
In reply to: Dean Rasheed (#49)
hackersbugs
#52Pavel Stehule
pavel.stehule@gmail.com
In reply to: Robert Haas (#51)
hackersbugs
#53Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#52)
hackersbugs
#54Andres Freund
andres@anarazel.de
In reply to: Peter Eisentraut (#53)
hackersbugs
#55Pavel Stehule
pavel.stehule@gmail.com
In reply to: Peter Eisentraut (#53)
hackersbugs
#56Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#55)
hackersbugs
#57Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Peter Eisentraut (#56)
hackersbugs
#58Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dean Rasheed (#57)
hackersbugs
#59Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#58)
hackersbugs
#60Pavel Stehule
pavel.stehule@gmail.com
In reply to: Tom Lane (#58)
hackersbugs
#61Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#57)
hackersbugs
#62Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#61)
hackersbugs
#63Pavel Stehule
pavel.stehule@gmail.com
In reply to: Andres Freund (#28)
hackersbugs
#64Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Pavel Stehule (#63)
hackersbugs
#65Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#64)
hackersbugs
#66Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dean Rasheed (#64)
hackersbugs
#67Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Alvaro Herrera (#66)
hackersbugs
#68Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dean Rasheed (#67)
hackersbugs
#69Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#63)
hackersbugs
#70Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Pavel Stehule (#65)
hackersbugs
#71Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#70)
hackersbugs
#72Peter Eisentraut
peter_e@gmx.net
In reply to: Pavel Stehule (#71)
hackersbugs
#73Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Peter Eisentraut (#72)
hackersbugs
#74Pavel Stehule
pavel.stehule@gmail.com
In reply to: Peter Eisentraut (#72)
hackersbugs
#75Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#73)
hackersbugs
#76Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Pavel Stehule (#75)
hackersbugs
#77Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#76)
hackersbugs
#78Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Pavel Stehule (#77)
hackersbugs
#79Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#78)
hackersbugs
#80Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Dean Rasheed (#78)
hackersbugs
#81Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Alvaro Herrera (#80)
hackersbugs
#82Pavel Stehule
pavel.stehule@gmail.com
In reply to: Dean Rasheed (#81)
hackersbugs
#83Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Pavel Stehule (#82)
hackersbugs