Thread: Bug in pg_dump/restore -o
I am seeing a bug in CVS in pg_dump/pg_restore with -o:
$ dropdb test $ createdb test $ pg_dump -Fc -b -o test > /bjm/out $ dropdb test $
createdbtest $ pg_restore -v -d test /bjm/out pg_restore: connecting to database for restore
pg_restore:creating <Init> Max OID pg_restore: [archiver (db)] could not execute query: no result from server
pg_restore: *** aborted because of error
It fails even on an empty database. It appears to be the setting of the
max oid:CREATE TEMPORARY TABLE pgdump_oid (dummy int4);COPY pgdump_oid WITH OIDS FROM stdin;16612 0\.DROP TABLE
pgdump_oid;
It seems the -Fc format is somehow misinterpreting this. I am
researching this now but if someone has a clue, I could use it.
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
[2002-01-17 12:49] Bruce Momjian said:
| It seems the -Fc format is somehow misinterpreting this. I am
| researching this now but if someone has a clue, I could use it.
Breakpoint 1, ExecuteSqlCommandBuf (AH=0x8056758, qryv=0x80614d0, bufLen=121) at pg_backup_db.c:653
653 for (pos = 0; pos < (eos - qry); pos++)
(gdb) next
655 appendPQExpBufferChar(AH->sqlBuf, qry[pos]);
(gdb)
658 switch (AH->sqlparse.state)
(gdb) print *AH->sqlBuf
$9 = { data = 0x8056a10 "--\n-- Selected TOC Entries:\n--\n--\n-- TOC Entry ID 2 (OID 0)\n--\n-- Name: Max OID Type:
<Init>Owner: \n-- Data Pos: 0 (Length 0)\n--\n\nC", len = 132, maxlen = 256}
(gdb) cont
Continuing.
pg_restore: [archiver (db)] could not execute query: no result from server
pg_restore: *** aborted because of error
Notice the 'C' at the end of the AH->sqlBuf->data. Looks like a
bad count somewhere. I won't have time to dig any more til after
work to(day|nignt).
hth. brent
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
I think I see the cause. I created a simple table and then generated
the dump. I found this that the normal table had its COPY data in
binary format while the max oid dump was pure ASCII. My guess is that
the max oid dump code isn't calling the right routine to dump its data.
---------------------------------------------------------------------------
Brent Verner wrote:
> [2002-01-17 12:49] Bruce Momjian said:
>
> | It seems the -Fc format is somehow misinterpreting this. I am
> | researching this now but if someone has a clue, I could use it.
>
> Breakpoint 1, ExecuteSqlCommandBuf (AH=0x8056758, qryv=0x80614d0, bufLen=121)
> at pg_backup_db.c:653
> 653 for (pos = 0; pos < (eos - qry); pos++)
> (gdb) next
> 655 appendPQExpBufferChar(AH->sqlBuf, qry[pos]);
> (gdb)
> 658 switch (AH->sqlparse.state)
> (gdb) print *AH->sqlBuf
> $9 = {
> data = 0x8056a10 "--\n-- Selected TOC Entries:\n--\n--\n-- TOC Entry ID 2 (OID 0)\n--\n-- Name: Max OID Type:
<Init>Owner: \n-- Data Pos: 0 (Length 0)\n--\n\nC", len = 132, maxlen = 256}
> (gdb) cont
> Continuing.
> pg_restore: [archiver (db)] could not execute query: no result from server
> pg_restore: *** aborted because of error
>
> Notice the 'C' at the end of the AH->sqlBuf->data. Looks like a
> bad count somewhere. I won't have time to dig any more til after
> work to(day|nignt).
>
> hth.
> brent
>
> --
> "Develop your talent, man, and leave the world something. Records are
> really gifts from people. To think that an artist would love you enough
> to share his music with anyone is a beautiful thing." -- Duane Allman
>
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I think I see the cause. I created a simple table and then generated
> the dump. I found this that the normal table had its COPY data in
> binary format while the max oid dump was pure ASCII. My guess is that
> the max oid dump code isn't calling the right routine to dump its data.
Indeed, the max oid dump code doesn't look like it's been clued at all
about interoperating with the new pg_dump output code. It can't just
intermix commands and data into one string and expect pg_restore to cope.
Compare what happens in dumpClasses/dumpClasses_nodumpData to what's
being done in setMaxOid.
Philip, you're probably the best-qualified to fix this, but if you don't
have time today then I can work on it. I don't want to delay RC1 for
this...
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I think I see the cause. I created a simple table and then generated > > the dump. I found this that the normal table had its COPY data in > > binary format while the max oid dump was pure ASCII. My guess is that > > the max oid dump code isn't calling the right routine to dump its data. > > Indeed, the max oid dump code doesn't look like it's been clued at all > about interoperating with the new pg_dump output code. It can't just > intermix commands and data into one string and expect pg_restore to cope. > Compare what happens in dumpClasses/dumpClasses_nodumpData to what's > being done in setMaxOid. > > Philip, you're probably the best-qualified to fix this, but if you don't > have time today then I can work on it. I don't want to delay RC1 for > this... I am told this was broken in 7.1 too. Attached is a diff that shows a possible direction for a solution. What I did was instead of doing the COPY myself, I am keeping the temp table I just created and passing that table name to the standard dump routines to do the COPY. One problem is that there is no TableInfo for the table, but I can call getTables() with the temp table name. Also, it is a temp table and I am not sure if it is going to like that. Anyway, this is not completed code but just a possible solution. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/bin/pg_dump/pg_dump.c =================================================================== RCS file: /cvsroot/pgsql/src/bin/pg_dump/pg_dump.c,v retrieving revision 1.237 diff -c -r1.237 pg_dump.c *** src/bin/pg_dump/pg_dump.c 2002/01/11 23:21:55 1.237 --- src/bin/pg_dump/pg_dump.c 2002/01/17 23:35:46 *************** *** 4545,4551 **** setMaxOid(Archive *fout) { PGresult *res; - Oid max_oid; char sql[1024]; res = PQexec(g_conn, "CREATE TEMPORARY TABLE pgdump_oid (dummy int4)"); --- 4542,4547 ---- *************** *** 4563,4575 **** write_msg(NULL, "could not insert into pgdump_oid table: %s", PQerrorMessage(g_conn)); exit_nicely(); } - max_oid = PQoidValue(res); - if (max_oid == 0) - { - write_msg(NULL, "inserted invalid oid\n"); - exit_nicely(); - } PQclear(res); res = PQexec(g_conn, "DROP TABLE pgdump_oid;"); if (!res || PQresultStatus(res) != PGRES_COMMAND_OK) --- 4559,4577 ---- write_msg(NULL, "could not insert into pgdump_oid table: %s", PQerrorMessage(g_conn)); exit_nicely(); } PQclear(res); + strcpy(sql, "COPY pgdump_oid WITH OIDS FROM stdin;\n"); + + dumpCtx = (DumpContext *) malloc(sizeof(DumpContext)); + dumpCtx->tblinfo = (TableInfo *) tblinfo; + dumpCtx->tblidx = 0; + dumpCtx->oids = true; + + dumpFn = dumpClasses_nodumpData; + + ArchiveEntry(fout, "0", "Max OID", "<Init>", NULL, sql, "", "", "", + dumpCtx, dumpFn); + res = PQexec(g_conn, "DROP TABLE pgdump_oid;"); if (!res || PQresultStatus(res) != PGRES_COMMAND_OK) *************** *** 4578,4594 **** exit_nicely(); } PQclear(res); - if (g_verbose) - write_msg(NULL, "maximum system oid is %u\n", max_oid); - snprintf(sql, 1024, - "CREATE TEMPORARY TABLE pgdump_oid (dummy int4);\n" - "COPY pgdump_oid WITH OIDS FROM stdin;\n" - "%u\t0\n" - "\\.\n" - "DROP TABLE pgdump_oid;\n", - max_oid); - - ArchiveEntry(fout, "0", "Max OID", "<Init>", NULL, sql, "", "", "", NULL, NULL); } /* --- 4580,4585 ----
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I am told this was broken in 7.1 too.
Good point. That probably means it's not a "must fix" for 7.2, but
still, it'd be nice to have a solution.
Attached is a diff that seems to actually work in all three pgdump modes
(script output, -Fc, -Ft). A disadvantage of this approach is that
there doesn't seem to be any clean way to issue a DROP for the temp
table, so it will persist in the target database until we finish the
psql script or pg_restore run (or at least until we have to do a
\connect). This would create a problem if the name pgdump_oid
conflicted with any actual table name being restored. We could choose
some more bizarre name to reduce the probability of such a conflict.
A potentially more serious problem is that if the archiving code chooses
to issue other operations between the schema restore and data restore
for the temp table, we might do a \connect and lose the temp table.
Come to think of it, a data-only restore request won't work either.
I had originally tried a single data-only archive entry with copy
command CREATE... COPY. This would avoid the latter two problems.
However, while it seemed to work okay in -Fc mode, -Ft blew up:
$ pg_dump -Ft -b -o test >/tmp/dump.tar
pg_dump: [tar archiver] bad COPY statement - could not find "copy" in string "cr
eate temporary table pgdump_oid (dummy int4);
copy pgdump_oid with oids from stdin;
Philip, is this a fundamental problem, or do we just need to make
the tar archiver a little smarter about looking at the COPY strings?
regards, tom lane
*** src/bin/pg_dump/pg_dump.c.orig Fri Jan 11 18:21:55 2002
--- src/bin/pg_dump/pg_dump.c Thu Jan 17 20:51:50 2002
***************
*** 89,94 ****
--- 89,95 ----
static Oid findLastBuiltinOid_V71(const char *);
static Oid findLastBuiltinOid_V70(void);
static void setMaxOid(Archive *fout);
+ static int setMaxOid_dumper(Archive *fout, char *oid, void *dctxv);
static void AddAcl(char *aclbuf, const char *keyword);
static char *GetPrivileges(Archive *AH, const char *s);
***************
*** 4538,4552 ****
/*
* setMaxOid -
! * find the maximum oid and generate a COPY statement to set it
! */
static void
setMaxOid(Archive *fout)
{
PGresult *res;
Oid max_oid;
- char sql[1024];
res = PQexec(g_conn, "CREATE TEMPORARY TABLE pgdump_oid (dummy int4)");
if (!res ||
--- 4539,4568 ----
/*
* setMaxOid -
! * find the maximum oid and generate commands to reproduce it in the target
! */
static void
setMaxOid(Archive *fout)
{
+ ArchiveEntry(fout, "0", "Max OID",
+ "<Init>", NULL,
+ "CREATE TEMPORARY TABLE pgdump_oid (dummy int4);\n",
+ "", "", "",
+ NULL, NULL);
+
+ ArchiveEntry(fout, "0", "Max OID",
+ "<Init Data>", NULL, "", "",
+ "COPY pgdump_oid WITH OIDS FROM stdin;\n",
+ "",
+ setMaxOid_dumper, NULL);
+ }
+
+ static int
+ setMaxOid_dumper(Archive *fout, char *oid, void *dctxv)
+ {
PGresult *res;
Oid max_oid;
res = PQexec(g_conn, "CREATE TEMPORARY TABLE pgdump_oid (dummy int4)");
if (!res ||
***************
*** 4578,4594 ****
exit_nicely();
}
PQclear(res);
if (g_verbose)
write_msg(NULL, "maximum system oid is %u\n", max_oid);
- snprintf(sql, 1024,
- "CREATE TEMPORARY TABLE pgdump_oid (dummy int4);\n"
- "COPY pgdump_oid WITH OIDS FROM stdin;\n"
- "%u\t0\n"
- "\\.\n"
- "DROP TABLE pgdump_oid;\n",
- max_oid);
! ArchiveEntry(fout, "0", "Max OID", "<Init>", NULL, sql, "", "", "", NULL, NULL);
}
/*
--- 4594,4609 ----
exit_nicely();
}
PQclear(res);
+
if (g_verbose)
write_msg(NULL, "maximum system oid is %u\n", max_oid);
! archprintf(fout,
! "%u\t0\n"
! "\\.\n",
! max_oid);
!
! return 1;
}
/*
At 17:27 17/01/02 -0500, Tom Lane wrote: > >Philip, you're probably the best-qualified to fix this, but if you don't >have time today then I can work on it. I don't want to delay RC1 for >this... > I'm around now; do you still want me to look into this? >A potentially more serious problem is that if the archiving code chooses >to issue other operations between the schema restore and data restore >for the temp table, we might do a \connect and lose the temp table. >Come to think of it, a data-only restore request won't work either. This leads me to the question: when should the OID restoration be performed - in the SCHEMA or DATA phase? ISTM that it is part of the data, and should be performed after data restoration. But perhaps I misunderstand what it is for. >Philip, is this a fundamental problem, or do we just need to make >the tar archiver a little smarter about looking at the COPY strings? I need to look into this; there should be no difference. >pg_dump: [tar archiver] bad COPY statement - could not find "copy" in string "cr >eate temporary table pgdump_oid (dummy int4); >copy pgdump_oid with oids from stdin; I'm not sure, from your patch code, how it got the CREATE and COPY statements in the one string - did you by any chance use an defective dump file from a previous patch attempt? Bye for now, Philip. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
Philip Warner <pjw@rhyme.com.au> writes:
> I'm around now; do you still want me to look into this?
Yup.
>> pg_dump: [tar archiver] bad COPY statement - could not find "copy" in
> string "cr
>> eate temporary table pgdump_oid (dummy int4);
>> copy pgdump_oid with oids from stdin;
> I'm not sure, from your patch code, how it got the CREATE and COPY
> statements in the one string - did you by any chance use an defective dump
> file from a previous patch attempt?
Sorry that I wasn't clearer. This message did not come from the given
patch. What I had tried to do was issue a single ArchiveEntry call with
the CREATE TABLE/COPY both in the copyStmt string of the one archive
entry. I don't have that code handy anymore, but it was approximately
ArchiveEntry(fout, "0", "Max OID", "<Init>", NULL, "", "", "CREATE TEMPORARY TABLE
pgdump_oid(dummy int4);\n" "COPY pgdump_oid WITH OIDS FROM stdin;\n", "",
setMaxOid_dumper,NULL);
in setMaxOid, and the same code as given in the patch for
setMaxOid_dumper.
> This leads me to the question: when should the OID restoration be performed
> - in the SCHEMA or DATA phase? ISTM that it is part of the data, and should
> be performed after data restoration. But perhaps I misunderstand what it is
> for.
Well, that's an interesting question. I thought it was data too, when I
was making these patches. But I just got off the horn from a long
conversation with Bruce, who put in this max-oid-setting code to begin
with, and what he says is that the original intention was to force up
the OID counter *before* loading any schema information *or* data. The
idea being that if you believe that OIDs are unique across your whole
database, then you want the OIDs assigned to tables, sequences, rules,
etc etc to be disjoint from those assigned to your user data rows.
Since we can't directly restore the original OIDs of these constructs,
the best we can do is ensure they will be assigned OIDs beyond the ones
being loaded into data rows.
Of course this whole concept collapses in the face of OID wraparound,
not to mention the current idea that we should change to generating OIDs
from per-table counters instead of globally. So one possible answer is
to say "forget it, we don't guarantee that anymore" and just rip out
the setMaxOid code entirely. But since we don't have per-table OID
counters yet, that seems a bit premature to both me and Bruce.
If we do want to try to maintain the original concept, it seems that the
OID-setting code needs to be emitted in a way that would cause it to be
executed *first* and in *all* restore modes: schema only, data only, or
schema+data. Not sure if that's even possible given the current design
of the archiver, but you'd know better than I.
>>A potentially more serious problem is that if the archiving code chooses
>>to issue other operations between the schema restore and data restore
>>for the temp table, we might do a \connect and lose the temp table.
>Why is this a problem - I presume I don't understand the OID allocation stuff.
CREATE TEMP TABLE pgdump_oids (...);
\connect - otheruser
CREATE TABLE someothertable;
...
\connect - postgres
COPY pgdump_oids FROM stdin;
ERROR: pgdump_oids doesn't exist
Since \connect starts a fresh connection, the temp table will disappear.
AFAICS, the only way to work around this is to be certain that no other
archive TOC entries will be emitted between the schema and data for
pgdump_oids (if these are separate TOC entries). I'd prefer a solution
in which they are only one TOC entry, since that seems less likely to
break in the future...
regards, tom lane
At 21:05 17/01/02 -0500, Tom Lane wrote: > >A potentially more serious problem is that if the archiving code chooses >to issue other operations between the schema restore and data restore >for the temp table, we might do a \connect and lose the temp table. Why is this a problem - I presume I don't understand the OID allocation stuff. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
Philip Warner wrote: > At 17:27 17/01/02 -0500, Tom Lane wrote: > > > >Philip, you're probably the best-qualified to fix this, but if you don't > >have time today then I can work on it. I don't want to delay RC1 for > >this... > > > > I'm around now; do you still want me to look into this? Yep. :-) > >A potentially more serious problem is that if the archiving code chooses > >to issue other operations between the schema restore and data restore > >for the temp table, we might do a \connect and lose the temp table. > >Come to think of it, a data-only restore request won't work either. > > This leads me to the question: when should the OID restoration be performed > - in the SCHEMA or DATA phase? ISTM that it is part of the data, and should > be performed after data restoration. But perhaps I misunderstand what it is > for. I believe it should be performed only as part of a data dump. Saving the oid during as part of the schema makes no sense because the schema dump doesn't have any reference to oid's. > >Philip, is this a fundamental problem, or do we just need to make > >the tar archiver a little smarter about looking at the COPY strings? > > I need to look into this; there should be no difference. OK. > >pg_dump: [tar archiver] bad COPY statement - could not find "copy" in > string "cr > >eate temporary table pgdump_oid (dummy int4); > >copy pgdump_oid with oids from stdin; > > I'm not sure, from your patch code, how it got the CREATE and COPY > statements in the one string - did you by any chance use an defective dump > file from a previous patch attempt? Well, the problem is that the -Fc format has the CREATE/COPY in the dump file, but the COPY data is non-compressed, e.g. 12323<tab>0. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Philip Warner wrote: > At 21:05 17/01/02 -0500, Tom Lane wrote: > > > >A potentially more serious problem is that if the archiving code chooses > >to issue other operations between the schema restore and data restore > >for the temp table, we might do a \connect and lose the temp table. > > Why is this a problem - I presume I don't understand the OID allocation stuff. The problem is that if you split the creation of the temp table from the actual COPY that loads the data, and there is a reconnection to the server in between, the temp table is automatically deleted, causing the COPY to fail. The trick with this oid setting is the temp table COPY should happen _before_ the schema is created. This way, the pg_class.oid of the newly created schema doesn't match any of the loaded oid's. Now, you can say that the oid counter could wrap around, and that is true, but until we go with the pg_class.oid/table.oid 8-byte unique oid, I would like to keep the oid unique if possible. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
At 23:08 17/01/02 -0500, Tom Lane wrote: >AFAICS, the only way to work around this is to be certain that no other >archive TOC entries will be emitted between the schema and data for >pgdump_oids (if these are separate TOC entries). I'd prefer a solution >in which they are only one TOC entry, since that seems less likely to >break in the future... A sigle TOC entry, with both CREATE and COPY run at the start no matter what kind of dump/restore, seems like the solution. I'll look into it. ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
> If we do want to try to maintain the original concept, it seems that the > OID-setting code needs to be emitted in a way that would cause it to be > executed *first* and in *all* restore modes: schema only, data only, or > schema+data. Not sure if that's even possible given the current design > of the archiver, but you'd know better than I. My only comment here is that I don't see a need to set the oid counter for a schema-only restore because there are no oid's in the schema dump anyway. Now, if you then do a data-only restore, you would have a problem with the oid counter but it seems counter-intuitive for the schema-only restore to set the oid counter. Comments, Tom? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> Given a pg_dump archive containing OIDs, I would expect a schema-only
>> pg_restore followed by a data-only pg_restore to produce the same end
>> result as a schema+data restore, no?
> That is the big question, if they are doing a schema-only restore, will
> then then do a data-only restore, or will they not. My guess is that
> they will not or they would have just restored the whole thing.
> The downside of setting the oid counter on schema-only is that you have
> set the counter much higher than they may have wanted, especially if
> they are doing the schema-only restore to somehow get the counter down
> again. The downside of _not_ setting the oid counter on schema-only is
> that they may have duplicate oids between system and user tables. That
> seems less of a risk than the former, and much less likely to happen.
Good points. So I guess you are saying it would be okay to treat the
setMaxOid TOC item as data, and have it appear only in schema+data or
data-only restores. In that case, back to plan A.
regards, tom lane
Philip Warner <pjw@rhyme.com.au> writes:
> A sigle TOC entry, with both CREATE and COPY run at the start no matter
> what kind of dump/restore, seems like the solution. I'll look into it.
Okay. Keep in mind that given where we are in the release cycle, any
patch applied now has got to be minimal and easily checked. If what
you are thinking of involves any extensive changes, it'd probably be
better to let the problem go till 7.3, rather than risk introducing
worse breakage in 7.2. Especially since this problem isn't that major
in the big scheme of things (if it were, it'd have been found sooner).
One stopgap answer Bruce and I discussed on the phone was to temporarily
add a couple lines of code to pg_dump to reject the combination of -o
with -Fc or -Ft. That would prevent people from shooting themselves in
the foot in 7.2 until a proper solution can be devised for 7.3.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > My only comment here is that I don't see a need to set the oid counter > > for a schema-only restore because there are no oid's in the schema dump > > anyway. > > Given a pg_dump archive containing OIDs, I would expect a schema-only > pg_restore followed by a data-only pg_restore to produce the same end > result as a schema+data restore, no? That is the big question, if they are doing a schema-only restore, will then then do a data-only restore, or will they not. My guess is that they will not or they would have just restored the whole thing. The downside of setting the oid counter on schema-only is that you have set the counter much higher than they may have wanted, especially if they are doing the schema-only restore to somehow get the counter down again. The downside of _not_ setting the oid counter on schema-only is that they may have duplicate oids between system and user tables. That seems less of a risk than the former, and much less likely to happen. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> My only comment here is that I don't see a need to set the oid counter
> for a schema-only restore because there are no oid's in the schema dump
> anyway.
Given a pg_dump archive containing OIDs, I would expect a schema-only
pg_restore followed by a data-only pg_restore to produce the same end
result as a schema+data restore, no?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom Lane wrote: > >> Given a pg_dump archive containing OIDs, I would expect a schema-only > >> pg_restore followed by a data-only pg_restore to produce the same end > >> result as a schema+data restore, no? > > > That is the big question, if they are doing a schema-only restore, will > > then then do a data-only restore, or will they not. My guess is that > > they will not or they would have just restored the whole thing. > > > The downside of setting the oid counter on schema-only is that you have > > set the counter much higher than they may have wanted, especially if > > they are doing the schema-only restore to somehow get the counter down > > again. The downside of _not_ setting the oid counter on schema-only is > > that they may have duplicate oids between system and user tables. That > > seems less of a risk than the former, and much less likely to happen. > > Good points. So I guess you are saying it would be okay to treat the > setMaxOid TOC item as data, and have it appear only in schema+data or > data-only restores. In that case, back to plan A. Yes, that was my proposal. I was consindering a case where the load the schema just to populate a fresh database that is to be used by the application. In such cases, setting the oid makes little sense. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Philip Warner wrote: > At 23:36 17/01/02 -0500, Bruce Momjian wrote: > > > >Yes, that was my proposal. I was consindering a case where the load the > >schema just to populate a fresh database that is to be used by the > >application. In such cases, setting the oid makes little sense. > > > > Here's a patch that seems to do the trick. It may seem large, but it is > primarily a reorganization of existing code in pg_backup_db.c to make the > code a little clearer and to facilitate the swapping around of some loops. > > Let me know how you go... I hate to say it but I think this is too risky for 7.2. We have a problem that needs fixing, but seeing as it was broken in 7.1.X as well, and we are just now realizing it, I think the best bet would be to put in some code to throw an error for invalid combinations of -F and -o and keep this for 7.3. I will wait to see what others say. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
At 23:36 17/01/02 -0500, Bruce Momjian wrote:
>
>Yes, that was my proposal. I was consindering a case where the load the
>schema just to populate a fresh database that is to be used by the
>application. In such cases, setting the oid makes little sense.
>
Here's a patch that seems to do the trick. It may seem large, but it is
primarily a reorganization of existing code in pg_backup_db.c to make the
code a little clearer and to facilitate the swapping around of some loops.
Let me know how you go...
Bye for now,
Philip.
----------------------------------------------------------------
Philip Warner | __---_____
Albatross Consulting Pty. Ltd. |----/ - \
(A.B.N. 75 008 659 498) | /(@) ______---_
Tel: (+61) 0500 83 82 81 | _________ \
Fax: (+61) 0500 83 82 82 | ___________ |
Http://www.rhyme.com.au | / \|
| --________--
PGP key available upon request, | /
and from pgp5.ai.mit.edu:11371 |/
Attachment
At 01:31 18/01/02 -0500, Bruce Momjian wrote: > >I hate to say it but I think this is too risky for 7.2. ... >...and keep this for 7.3. > >I will wait to see what others say. Try 7.2.1. If you compare the patched Vs unpatched file, you will see that they are not substantially different (despite the size of the patch). But I don't have any attachment to this going in 7.2 (but it should be in 7.2.1). ---------------------------------------------------------------- Philip Warner | __---_____ Albatross Consulting Pty. Ltd. |----/ - \ (A.B.N. 75 008 659 498) | /(@) ______---_ Tel: (+61) 0500 83 82 81 | _________ \ Fax: (+61) 0500 83 82 82 | ___________ | Http://www.rhyme.com.au | / \| | --________-- PGP key available upon request, | / and from pgp5.ai.mit.edu:11371 |/
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Philip Warner wrote:
>> Here's a patch that seems to do the trick. It may seem large, but it is
>> primarily a reorganization of existing code in pg_backup_db.c to make the
>> code a little clearer and to facilitate the swapping around of some loops.
> I hate to say it but I think this is too risky for 7.2.
It does seem a rather large diff ... however, if I understand it right,
what Philip has done here is to hack pg_restore so that it will do the
right thing with an unmodified 7.1 dump file. This is much superior to
hacking pg_dump to change the emitted dump data. Weren't you telling me
that we might need a solution for someone who had already done pg_dump
-o -Ft and then wiped their 7.1 database?
I think we should scrutinize this as carefully as we can, but apply it
if at all possible.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Philip Warner wrote: > >> Here's a patch that seems to do the trick. It may seem large, but it is > >> primarily a reorganization of existing code in pg_backup_db.c to make the > >> code a little clearer and to facilitate the swapping around of some loops. > > > I hate to say it but I think this is too risky for 7.2. > > It does seem a rather large diff ... however, if I understand it right, > what Philip has done here is to hack pg_restore so that it will do the > right thing with an unmodified 7.1 dump file. This is much superior to > hacking pg_dump to change the emitted dump data. Weren't you telling me > that we might need a solution for someone who had already done pg_dump > -o -Ft and then wiped their 7.1 database? > > I think we should scrutinize this as carefully as we can, but apply it > if at all possible. Interesting solution. I defer to your opinion on this. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> I think we should scrutinize this as carefully as we can, but apply it
>> if at all possible.
> Interesting solution. I defer to your opinion on this.
Okay, I've applied it ;-). I studied the code carefully and also
exercised as many combinations as I could think of. I think it's okay.
You might want to run pg_indent on pg_backup_db.c however ...
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> I think we should scrutinize this as carefully as we can, but apply it > >> if at all possible. > > > Interesting solution. I defer to your opinion on this. > > Okay, I've applied it ;-). I studied the code carefully and also > exercised as many combinations as I could think of. I think it's okay. > > You might want to run pg_indent on pg_backup_db.c however ... Done. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026