Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Rewrite --section option to decouple it from --schema-only/--data-only.

  1. Bug in -c CLI option of pg_dump/pg_restore

    Guillaume Lelarge <guillaume@lelarge.info> — 2012-10-13T14:47:06Z

    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

  2. Re: Bug in -c CLI option of pg_dump/pg_restore

    Guillaume Lelarge <guillaume@lelarge.info> — 2012-10-16T14:31:50Z

    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

  3. Re: Bug in -c CLI option of pg_dump/pg_restore

    Robert Haas <robertmhaas@gmail.com> — 2012-10-18T15:09:53Z

    On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
<guillaume@lelarge.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

  4. Re: Bug in -c CLI option of pg_dump/pg_restore

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2012-10-18T15:19:31Z

    Robert Haas escribió:
> On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
> <guillaume@lelarge.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

  5. Re: Bug in -c CLI option of pg_dump/pg_restore

    Guillaume Lelarge <guillaume@lelarge.info> — 2012-10-18T17:08:53Z

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

  6. Re: Bug in -c CLI option of pg_dump/pg_restore

    Robert Haas <robertmhaas@gmail.com> — 2012-10-19T18:30:53Z

    On Thu, Oct 18, 2012 at 11:19 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas escribió:
>> On Tue, Oct 16, 2012 at 10:31 AM, Guillaume Lelarge
>> <guillaume@lelarge.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

  7. Re: Bug in -c CLI option of pg_dump/pg_restore

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-20T18:06:42Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Oct 18, 2012 at 11:19 AM, Alvaro Herrera
> <alvherre@2ndquadrant.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

  8. Re: Bug in -c CLI option of pg_dump/pg_restore

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-10-20T18:28:30Z

    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

  9. Re: Bug in -c CLI option of pg_dump/pg_restore

    Guillaume Lelarge <guillaume@lelarge.info> — 2012-10-20T21:41:15Z

    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