Обсуждение: pg_restore -1 vs -C and -c
Hi!
As I think has been previously noted, using pg_restore with -1 (single
transaction) is fundamentally incompatible with -C (we can't CREATE
DATABASE inside a transaction) and often incompatible with -c (we don't
do DROP IF EXISTS, so if the objects don't exist the entire restore will
fail). It does work with -c *sometimes*.
It should be possible to make it compatible with -C by moving the CREATE
DATABASE command to outside of the transaction. I have only had a quick
look at the code wrt how much work this would be. One thing that hit me
quickly: do we support multiple CREATE DATABASE statements in a single
dump file? I think not, but the format seems to allow it? If not, it
should be "fairly simple" to do.
As for -c, the solution would be to issue DROP IF EXISTS statements. Is
there any particular reason why we don't?
Finally, attached is a patch that throws a nicer error message when -C
and -1 is used together, since they're fundamentally incompatible. This
one would also be backpatchable even if we fix the above.
Comments?
//Magnus
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
***************
*** 146,151 **** RestoreArchive(Archive *AHX, RestoreOptions *ropt)
--- 146,157 ----
*/
if (ropt->create && ropt->dropSchema)
die_horribly(AH, modulename, "-C and -c are incompatible options\n");
+ /*
+ * -1 is not compatible with -C, because we can't create a database
+ * inside a transaction block.
+ */
+ if (ropt->create && ropt->single_txn)
+ die_horribly(AH, modulename, "-C and -1 are incompatible options\n");
/*
* If we're using a DB connection, then connect it.
Magnus Hagander <magnus@hagander.net> writes:
> It should be possible to make it compatible with -C by moving the CREATE
> DATABASE command to outside of the transaction. I have only had a quick
> look at the code wrt how much work this would be. One thing that hit me
> quickly: do we support multiple CREATE DATABASE statements in a single
> dump file? I think not, but the format seems to allow it? If not, it
> should be "fairly simple" to do.
We don't, and a single transaction couldn't restore into multiple
databases anyway. So in principle there's no reason we shouldn't allow
it to do the CREATE DATABASE, switch into the new DB, and then start the
transaction.
However, one of the properties -1 is supposed to have is that any
failure aborts the whole restore; it's not immediately clear how to
preserve that with CREATE DATABASE issued separately.
Also you need to think about whether this might impact pg_dumpall.
> As for -c, the solution would be to issue DROP IF EXISTS statements. Is
> there any particular reason why we don't?
I think we did that to avoid damaging portability and backwards
compatibility of the dump files. The backwards compatibility argument
is pretty weak by now, but the "it's not standard SQL" argument still
has force.
regards, tom lane
On 12 jan 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.net> writes: >> It should be possible to make it compatible with -C by moving the >> CREATE >> DATABASE command to outside of the transaction. I have only had a >> quick >> look at the code wrt how much work this would be. One thing that >> hit me >> quickly: do we support multiple CREATE DATABASE statements in a >> single >> dump file? I think not, but the format seems to allow it? If not, it >> should be "fairly simple" to do. > > We don't, and a single transaction couldn't restore into multiple > databases anyway. So in principle there's no reason we shouldn't > allow > it to do the CREATE DATABASE, switch into the new DB, and then start > the > transaction. > > However, one of the properties -1 is supposed to have is that any > failure aborts the whole restore; it's not immediately clear how to > preserve that with CREATE DATABASE issued separately. Good point. Declare as incompatible it is, then :) it's not like it's hard do create the database before restoring. > Also you need to think about whether this might impact pg_dumpall. Dumps from pg_dumpall aren't restored through pg_restore... > >> As for -c, the solution would be to issue DROP IF EXISTS >> statements. Is >> there any particular reason why we don't? > > I think we did that to avoid damaging portability and backwards > compatibility of the dump files. The backwards compatibility argument > is pretty weak by now, but the "it's not standard SQL" argument still > has force. > IIRC the drop statements are generated by pg_restore and not stored in the archive. So we could do the if exists by default and have a switch to turn it off for a compatible dump, perhaps? /Magnus
Magnus Hagander <magnus@hagander.net> writes:
> On 12 jan 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> However, one of the properties -1 is supposed to have is that any
>> failure aborts the whole restore; it's not immediately clear how to
>> preserve that with CREATE DATABASE issued separately.
> Good point. Declare as incompatible it is, then :) it's not like it's
> hard do create the database before restoring.
Works for me.
>>> As for -c, the solution would be to issue DROP IF EXISTS
>>> statements. Is there any particular reason why we don't?
>>
>> I think we did that to avoid damaging portability and backwards
>> compatibility of the dump files. The backwards compatibility argument
>> is pretty weak by now, but the "it's not standard SQL" argument still
>> has force.
> IIRC the drop statements are generated by pg_restore and not stored in
> the archive. So we could do the if exists by default and have a switch
> to turn it off for a compatible dump, perhaps?
No, the text of the statements is in the archive; though it might not be
too painful to have pg_restore edit them to insert "IF EXISTS". You
don't need an extra switch, just do this if -1 is in use (and document
that that switch reduces the standard-ness of the output...)
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: >> On 12 jan 2009, at 17.46, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> However, one of the properties -1 is supposed to have is that any >>> failure aborts the whole restore; it's not immediately clear how to >>> preserve that with CREATE DATABASE issued separately. > >> Good point. Declare as incompatible it is, then :) it's not like it's >> hard do create the database before restoring. > > Works for me. ok, will do. >>>> As for -c, the solution would be to issue DROP IF EXISTS >>>> statements. Is there any particular reason why we don't? >>> I think we did that to avoid damaging portability and backwards >>> compatibility of the dump files. The backwards compatibility argument >>> is pretty weak by now, but the "it's not standard SQL" argument still >>> has force. > >> IIRC the drop statements are generated by pg_restore and not stored in >> the archive. So we could do the if exists by default and have a switch >> to turn it off for a compatible dump, perhaps? > > No, the text of the statements is in the archive; though it might not be > too painful to have pg_restore edit them to insert "IF EXISTS". You > don't need an extra switch, just do this if -1 is in use (and document > that that switch reduces the standard-ness of the output...) I still think we need a separate parameter. DROP IF EXISTS would also be very useful along with -e, I think... //Magnus
Tom Lane wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>>>> As for -c, the solution would be to issue DROP IF EXISTS
>>>> statements. Is there any particular reason why we don't?
>>> I think we did that to avoid damaging portability and backwards
>>> compatibility of the dump files. The backwards compatibility argument
>>> is pretty weak by now, but the "it's not standard SQL" argument still
>>> has force.
>
>> IIRC the drop statements are generated by pg_restore and not stored in
>> the archive. So we could do the if exists by default and have a switch
>> to turn it off for a compatible dump, perhaps?
>
> No, the text of the statements is in the archive; though it might not be
> too painful to have pg_restore edit them to insert "IF EXISTS". You
> don't need an extra switch, just do this if -1 is in use (and document
> that that switch reduces the standard-ness of the output...)
Something along the line of this?
(This is for the actual injection, I still haven't implemented
switch/decided when to actually include it, so this is not for
application yet - just for a comment on the general method..)
//Magnus
*** a/src/bin/pg_dump/pg_backup_archiver.c
--- b/src/bin/pg_dump/pg_backup_archiver.c
***************
*** 123,128 **** CloseArchive(Archive *AHX)
--- 123,144 ----
strerror(errno));
}
+ /*
+ * List all objects that can be DROPped that are made up of more
+ * than a single word.
+ */
+ static const char *multiword_drops[] = {
+ "FOREIGN DATA WRAPPER",
+ "OPERATOR CLASS",
+ "OPERATOR FAMILY",
+ "TEXT SEARCH CONFIGURATION",
+ "TEXT SEARCH DICTIONARY",
+ "TEXT SEARCH PARSER",
+ "TEXT SEARCH TEMPLATE",
+ "USER MAPPING",
+ NULL
+ };
+
/* Public */
void
RestoreArchive(Archive *AHX, RestoreOptions *ropt)
***************
*** 249,256 **** RestoreArchive(Archive *AHX, RestoreOptions *ropt)
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
! /* Drop it */
! ahprintf(AH, "%s", te->dropStmt);
}
}
--- 265,308 ----
/* Select owner and schema as necessary */
_becomeOwner(AH, te);
_selectOutputSchema(AH, te->namespace);
! /*
! * Figure out if it's something we can do DROP IF EXISTS on.
! * Check for "DROP " just to be sure.
! */
! if (strncmp(te->dropStmt, "DROP ", 5) == 0)
! {
! char *cp = te->dropStmt + 5;
! char *insertpoint = NULL;
! char *newstr = NULL;
! int i;
!
! /*
! * Assume that all objects can be DROP IF EXISTS:ed. However,
! * some have more than one word in them, so we need to figure
! * out exactly where to insert the IF EXISTS.
! */
! for (i = 0; multiword_drops[i] != NULL; i++)
! {
! if (strncmp(cp, multiword_drops[i], strlen(multiword_drops[i])) == 0)
! {
! insertpoint = cp + strlen(multiword_drops[i]);
! break;
! }
! }
! if (insertpoint == NULL)
! insertpoint = strchr(cp, ' ');
! if (insertpoint == NULL)
! die_horribly(AH,modulename,"malformatted DROP statement: %s", te->dropStmt);
!
! newstr = calloc(strlen(te->dropStmt) + 11, 1); /* "IF EXISTS " + terminator */
! strncpy(newstr, te->dropStmt, insertpoint - te->dropStmt);
! strcat(newstr, " IF EXISTS");
! strcat(newstr, insertpoint);
! ahprintf(AH, "%s", newstr);
! free(newstr);
! }
! else
! ahprintf(AH, "%s", te->dropStmt);
}
}