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
        $ createdb test
        $ 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, Pennsylvania 19026

Thu, 17 Jan 2002 12:49:29 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Bug in pg_dump/restore -o
[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

Thu, 17 Jan 2002 13:25:30 -0500
Brent Verner <brent@rcfile.org>
Re: Bug in pg_dump/restore -o

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, Pennsylvania 19026

Thu, 17 Jan 2002 13:38:32 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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

Thu, 17 Jan 2002 17:27:05 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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 ----
Thu, 17 Jan 2002 18:43:25 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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;
  }
  
  /*
Thu, 17 Jan 2002 21:05:15 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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   |/

Fri, 18 Jan 2002 14:46:34 +1100
Philip Warner <pjw@rhyme.com.au>
Re: Bug in pg_dump/restore -o 
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   |/

Fri, 18 Jan 2002 14:50:11 +1100
Philip Warner <pjw@rhyme.com.au>
Re: Bug in pg_dump/restore -o 
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

Thu, 17 Jan 2002 23:08:14 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Thu, 17 Jan 2002 23:10:32 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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   |/

Fri, 18 Jan 2002 15:13:15 +1100
Philip Warner <pjw@rhyme.com.au>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Thu, 17 Jan 2002 23:14:03 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
> 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, Pennsylvania 19026

Thu, 17 Jan 2002 23:16:53 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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

Thu, 17 Jan 2002 23:24:29 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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

Thu, 17 Jan 2002 23:26:11 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Thu, 17 Jan 2002 23:30:43 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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

Thu, 17 Jan 2002 23:34:09 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Thu, 17 Jan 2002 23:36:32 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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   |/
Fri, 18 Jan 2002 17:28:21 +1100
Philip Warner <pjw@rhyme.com.au>
Re: Bug in pg_dump/restore -o
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, Pennsylvania 19026

Fri, 18 Jan 2002 01:31:49 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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   |/

Fri, 18 Jan 2002 17:42:51 +1100
Philip Warner <pjw@rhyme.com.au>
Re: Bug in pg_dump/restore -o
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

Fri, 18 Jan 2002 11:24:02 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Fri, 18 Jan 2002 12:02:31 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o
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

Fri, 18 Jan 2002 12:15:51 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: Bug in pg_dump/restore -o 
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, Pennsylvania 19026

Fri, 18 Jan 2002 14:15:14 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Bug in pg_dump/restore -o