Bug in -c CLI option of pg_dump/pg_restore

Hi,

One of my colleagues, Jehan-Guillaume de Rorthais, found a weird
behaviour of the "-c" command line option in the pg_restore tool while
doing a training. Here is the following steps he followed:

createdb foo
<adds a few objets in foo>
pg_dump -Fc foo > foo.dump
createdb bar
pg_restore -c -d bar foo.dump

bar contains the same objects as foo (nothing unusual here), but... foo
is no longer present. Actually, if you use the "-c" command line option,
you get a "DROP DATABASE" statement. To me, it feels like a quite
terrible bug.

It's quite easy to reproduce. Just create a database, and use pg_dump
with the "-c" option:

createdb foo
pg_dump -s -c foo | grep DATABASE

and you end up with this:

DROP DATABASE foo;

I tried from 8.3 till 9.2, and only 9.2 has this behaviour.

You'll find attached a patch that fixes this issue. Another colleague,
Gilles Darold, tried it in every possible way, and it works. I'm not
sure the test I added makes it a very good patch, but it fixes the bug.

Regards.

--
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com

On Sat, 2012-10-13 at 16:47 +0200, Guillaume Lelarge wrote:
> Hi,
>
> One of my colleagues, Jehan-Guillaume de Rorthais, found a weird
> behaviour of the "-c" command line option in the pg_restore tool while
> doing a training. Here is the following steps he followed:
>
> createdb foo
> <adds a few objets in foo>
> pg_dump -Fc foo > foo.dump
> createdb bar
> pg_restore -c -d bar foo.dump
>
> bar contains the same objects as foo (nothing unusual here), but... foo
> is no longer present. Actually, if you use the "-c" command line option,
> you get a "DROP DATABASE" statement. To me, it feels like a quite
> terrible bug.
>
> It's quite easy to reproduce. Just create a database, and use pg_dump
> with the "-c" option:
>
> createdb foo
> pg_dump -s -c foo | grep DATABASE
>
> and you end up with this:
>
> DROP DATABASE foo;
>
> I tried from 8.3 till 9.2, and only 9.2 has this behaviour.
>
> You'll find attached a patch that fixes this issue. Another colleague,
> Gilles Darold, tried it in every possible way, and it works. I'm not
> sure the test I added makes it a very good patch, but it fixes the bug.
>

Any comments on this?

--
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com

On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
<guillaume(at)lelarge(dot)info> wrote:
> Any comments on this?

I'm not sure I'd want to back-patch this, since it is a behavior
change, but I do think it's probably a good idea to change it for 9.3.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas escribió:
> On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
> <guillaume(at)lelarge(dot)info> wrote:
> > Any comments on this?
>
> I'm not sure I'd want to back-patch this, since it is a behavior
> change, but I do think it's probably a good idea to change it for 9.3.

Hm, but the bug is said to happen only in 9.2, so if we don't backpatch
we would leave 9.2 alone exhibiting this behavior.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, 2012-10-18 at 12:19 -0300, Alvaro Herrera wrote:
> Robert Haas escribió:
> > On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
> > <guillaume(at)lelarge(dot)info> wrote:
> > > Any comments on this?
> >
> > I'm not sure I'd want to back-patch this, since it is a behavior
> > change, but I do think it's probably a good idea to change it for 9.3.
>
> Hm, but the bug is said to happen only in 9.2, so if we don't backpatch
> we would leave 9.2 alone exhibiting this behavior.
>

Yeah, Alvarro got it right. The behaviour changed in 9.2. This patch
needs to be applied on 9.2 and master, nothing else. If the patch is
good enough though...

--
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com

On Thu, Oct 18, 2012 at 11:19 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Robert Haas escribió:
>> On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
>> <guillaume(at)lelarge(dot)info> wrote:
>> > Any comments on this?
>>
>> I'm not sure I'd want to back-patch this, since it is a behavior
>> change, but I do think it's probably a good idea to change it for 9.3.
>
> Hm, but the bug is said to happen only in 9.2, so if we don't backpatch
> we would leave 9.2 alone exhibiting this behavior.

Oh, yeah. I missed that. But then shouldn't we start by identifying
which commit broke it before we decide on a fix?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Oct 18, 2012 at 11:19 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> Hm, but the bug is said to happen only in 9.2, so if we don't backpatch
>> we would leave 9.2 alone exhibiting this behavior.

> Oh, yeah. I missed that. But then shouldn't we start by identifying
> which commit broke it before we decide on a fix?

It looks like I broke this in commit
4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
_tocEntryRequired():

- /* Ignore DATABASE entry unless we should create it */
- if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
- return 0;

and transferred the responsibility to restore_toc_entry():

+ /*
+ * Ignore DATABASE entry unless we should create it. We must check this
+ * here, not in _tocEntryRequired, because the createDB option should
+ * not affect emitting a DATABASE entry to an archive file.
+ */
+ if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
+ reqs = 0;

That was a good change for the reason stated in the comment ... but
I missed the fact that the old coding was also suppressing emission of a
DROP DATABASE command in -c mode. I concur with Guillaume that this is
a bug --- in fact, I got burnt by there being a DROP DATABASE command in
the tar-format special script a few weeks ago, but I supposed that that
was a tar-specific issue of long standing, and didn't pursue it further
at the time.

I think Guillaume's fix is basically OK except it would be better if it
looked more like the code added to restore_toc_entry(). Will adjust and
commit.

regards, tom lane

I wrote:
> It looks like I broke this in commit
> 4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
> _tocEntryRequired():

> - /* Ignore DATABASE entry unless we should create it */
> - if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
> - return 0;

Actually, on closer look, this change provides the foundation needed to
do more than just fix this bug. We can also make the combination
"pg_dump -C -c" work sanely, which it never has before. I propose that
we fix this with the attached patch (plus probably some documentation
changes, though I've not looked yet to see what the docs say about it).
With this fix, the output for "-C -c" looks like

DROP DATABASE regression;
CREATE DATABASE regression WITH ...
ALTER DATABASE regression OWNER ...
\connect regression
... etc ...

which seems to me to be just about exactly what one would expect.

The patch also gets rid of a kluge in PrintTOCSummary, which was needed
because of the old coding in _tocEntryRequired(), but no longer is.

regards, tom lane

On Sat, 2012-10-20 at 14:28 -0400, Tom Lane wrote:
> I wrote:
> > It looks like I broke this in commit
> > 4317e0246c645f60c39e6572644cff1cb03b4c65, because I removed this from
> > _tocEntryRequired():
>
> > - /* Ignore DATABASE entry unless we should create it */
> > - if (!ropt->createDB && strcmp(te->desc, "DATABASE") == 0)
> > - return 0;
>
> Actually, on closer look, this change provides the foundation needed to
> do more than just fix this bug. We can also make the combination
> "pg_dump -C -c" work sanely, which it never has before. I propose that
> we fix this with the attached patch (plus probably some documentation
> changes, though I've not looked yet to see what the docs say about it).
> With this fix, the output for "-C -c" looks like
>
> DROP DATABASE regression;
> CREATE DATABASE regression WITH ...
> ALTER DATABASE regression OWNER ...
> \connect regression
> ... etc ...
>
> which seems to me to be just about exactly what one would expect.
>
> The patch also gets rid of a kluge in PrintTOCSummary, which was needed
> because of the old coding in _tocEntryRequired(), but no longer is.
>

Thanks a lot.

--
Guillaume
http://blog.guillaume.lelarge.info
http://www.dalibo.com