Thread: 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
Attachment
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@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
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
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
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
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
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
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7f47a7c..1fead28 100644
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
*************** RestoreArchive(Archive *AHX)
*** 303,317 ****
/*
* Check for nonsensical option combinations.
*
- * NB: createDB+dropSchema is useless because if you're creating the DB,
- * there's no need to drop individual items in it. Moreover, if we tried
- * to do that then we'd issue the drops in the database initially
- * connected to, not the one we will create, which is very bad...
- */
- if (ropt->createDB && ropt->dropSchema)
- exit_horribly(modulename, "-C and -c are incompatible options\n");
-
- /*
* -C is not compatible with -1, because we can't create a database inside
* a transaction block.
*/
--- 303,308 ----
*************** RestoreArchive(Archive *AHX)
*** 456,462 ****
{
AH->currentTE = te;
! /* We want anything that's selected and has a dropStmt */
if (((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0) && te->dropStmt)
{
ahlog(AH, 1, "dropping %s %s\n", te->desc, te->tag);
--- 447,471 ----
{
AH->currentTE = te;
! /*
! * In createDB mode, issue a DROP *only* for the database as a
! * whole. Issuing drops against anything else would be wrong,
! * because at this point we're connected to the wrong database.
! * Conversely, if we're not in createDB mode, we'd better not
! * issue a DROP against the database at all.
! */
! if (ropt->createDB)
! {
! if (strcmp(te->desc, "DATABASE") != 0)
! continue;
! }
! else
! {
! if (strcmp(te->desc, "DATABASE") == 0)
! continue;
! }
!
! /* Otherwise, drop anything that's selected and has a dropStmt */
if (((te->reqs & (REQ_SCHEMA | REQ_DATA)) != 0) && te->dropStmt)
{
ahlog(AH, 1, "dropping %s %s\n", te->desc, te->tag);
*************** PrintTOCSummary(Archive *AHX, RestoreOpt
*** 938,946 ****
ahprintf(AH, ";\n;\n; Selected TOC Entries:\n;\n");
- /* We should print DATABASE entries whether or not -C was specified */
- ropt->createDB = 1;
-
curSection = SECTION_PRE_DATA;
for (te = AH->toc->next; te != AH->toc; te = te->next)
{
--- 947,952 ----
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