Thread: Bug in CVS pg_dump against 7.0.x
Hi, I know 7.0.x is pretty old, but I'm wondering if we should fix this to make it better for people upgrading. If you create a table like this in 7.0.x: CREATE TABLE address ( first_name character varying(50) DEFAULT 'asdf' NOT NULL, last_name character varying(50) NOT NULL, address character varying(50), tesing character varying[] ); The 7.5 pg_dump program will dump it like this: CREATE TABLE address ( first_name character varying(50) DEFAULT 'asdf' NOT NULL, last_name character varying(50) NOT NULL, address character varying(50), tesing _varchar ); I have attached a patch that should fix it. I haven't been able to actually test it since my dev machine and the 7.0 machine I have access to aren't connected - although it does compile. The fix is based on the 7.0 psql code. Chris ? src/bin/pg_dump/.deps ? src/bin/pg_dump/common.c.working ? src/bin/pg_dump/pg_dump ? src/bin/pg_dump/pg_dump.c.working ? src/bin/pg_dump/pg_dump.h.working ? src/bin/pg_dump/pg_dumpall ? src/bin/pg_dump/pg_restore Index: src/bin/pg_dump/pg_dump.c =================================================================== RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v retrieving revision 1.370 diff -c -r1.370 pg_dump.c *** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 --- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 *************** *** 7726,7733 **** --- 7726,7741 ---- myFormatType(const char *typname, int32 typmod) { char *result; + bool isarray = false; PQExpBuffer buf = createPQExpBuffer(); + /* Handle array types */ + if (typname[0] == '_') + { + isarray = true; + typname++; + } + /* Show lengths on bpchar and varchar */ if (!strcmp(typname, "bpchar")) { *************** *** 7770,7775 **** --- 7778,7787 ---- appendPQExpBuffer(buf, "\"char\""); else appendPQExpBuffer(buf, "%s", fmtId(typname)); + + /* Append array qualifier for array types */ + if (isarray) + appendPQExpBuffer(buf, "[]"); result = strdup(buf->data); destroyPQExpBuffer(buf);
Uh, not sure anyone would even see a 7.0.X release if we made it, and I question how many are using varying[]. Your patch is now in the archives, and we can point folks to it if they ask. --------------------------------------------------------------------------- Christopher Kings-Lynne wrote: > Hi, > > I know 7.0.x is pretty old, but I'm wondering if we should fix this to > make it better for people upgrading. > > If you create a table like this in 7.0.x: > > CREATE TABLE address ( > first_name character varying(50) DEFAULT 'asdf' NOT NULL, > last_name character varying(50) NOT NULL, > address character varying(50), > tesing character varying[] > ); > > The 7.5 pg_dump program will dump it like this: > > CREATE TABLE address ( > first_name character varying(50) DEFAULT 'asdf' NOT NULL, > last_name character varying(50) NOT NULL, > address character varying(50), > tesing _varchar > ); > > I have attached a patch that should fix it. I haven't been able to > actually test it since my dev machine and the 7.0 machine I have access > to aren't connected - although it does compile. The fix is based on the > 7.0 psql code. > > Chris > ? src/bin/pg_dump/.deps > ? src/bin/pg_dump/common.c.working > ? src/bin/pg_dump/pg_dump > ? src/bin/pg_dump/pg_dump.c.working > ? src/bin/pg_dump/pg_dump.h.working > ? src/bin/pg_dump/pg_dumpall > ? src/bin/pg_dump/pg_restore > Index: src/bin/pg_dump/pg_dump.c > =================================================================== > RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v > retrieving revision 1.370 > diff -c -r1.370 pg_dump.c > *** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 > --- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 > *************** > *** 7726,7733 **** > --- 7726,7741 ---- > myFormatType(const char *typname, int32 typmod) > { > char *result; > + bool isarray = false; > PQExpBuffer buf = createPQExpBuffer(); > > + /* Handle array types */ > + if (typname[0] == '_') > + { > + isarray = true; > + typname++; > + } > + > /* Show lengths on bpchar and varchar */ > if (!strcmp(typname, "bpchar")) > { > *************** > *** 7770,7775 **** > --- 7778,7787 ---- > appendPQExpBuffer(buf, "\"char\""); > else > appendPQExpBuffer(buf, "%s", fmtId(typname)); > + > + /* Append array qualifier for array types */ > + if (isarray) > + appendPQExpBuffer(buf, "[]"); > > result = strdup(buf->data); > destroyPQExpBuffer(buf); > > ---------------------------(end of broadcast)--------------------------- > TIP 5: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faqs/FAQ.html -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
No, the patch is against 7.5 CVS. It is a tiny fix that allows it to dump 7.0.x database backends correctly. I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) Chris Bruce Momjian wrote: > Uh, not sure anyone would even see a 7.0.X release if we made it, and I > question how many are using varying[]. Your patch is now in the > archives, and we can point folks to it if they ask. > > --------------------------------------------------------------------------- > > Christopher Kings-Lynne wrote: > >>Hi, >> >>I know 7.0.x is pretty old, but I'm wondering if we should fix this to >>make it better for people upgrading. >> >>If you create a table like this in 7.0.x: >> >>CREATE TABLE address ( >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, >> last_name character varying(50) NOT NULL, >> address character varying(50), >> tesing character varying[] >>); >> >>The 7.5 pg_dump program will dump it like this: >> >>CREATE TABLE address ( >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, >> last_name character varying(50) NOT NULL, >> address character varying(50), >> tesing _varchar >>); >> >>I have attached a patch that should fix it. I haven't been able to >>actually test it since my dev machine and the 7.0 machine I have access >>to aren't connected - although it does compile. The fix is based on the >>7.0 psql code. >> >>Chris > > >>? src/bin/pg_dump/.deps >>? src/bin/pg_dump/common.c.working >>? src/bin/pg_dump/pg_dump >>? src/bin/pg_dump/pg_dump.c.working >>? src/bin/pg_dump/pg_dump.h.working >>? src/bin/pg_dump/pg_dumpall >>? src/bin/pg_dump/pg_restore >>Index: src/bin/pg_dump/pg_dump.c >>=================================================================== >>RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v >>retrieving revision 1.370 >>diff -c -r1.370 pg_dump.c >>*** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 >>--- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 >>*************** >>*** 7726,7733 **** >>--- 7726,7741 ---- >> myFormatType(const char *typname, int32 typmod) >> { >> char *result; >>+ bool isarray = false; >> PQExpBuffer buf = createPQExpBuffer(); >> >>+ /* Handle array types */ >>+ if (typname[0] == '_') >>+ { >>+ isarray = true; >>+ typname++; >>+ } >>+ >> /* Show lengths on bpchar and varchar */ >> if (!strcmp(typname, "bpchar")) >> { >>*************** >>*** 7770,7775 **** >>--- 7778,7787 ---- >> appendPQExpBuffer(buf, "\"char\""); >> else >> appendPQExpBuffer(buf, "%s", fmtId(typname)); >>+ >>+ /* Append array qualifier for array types */ >>+ if (isarray) >>+ appendPQExpBuffer(buf, "[]"); >> >> result = strdup(buf->data); >> destroyPQExpBuffer(buf); > > >>---------------------------(end of broadcast)--------------------------- >>TIP 5: Have you checked our extensive FAQ? >> >> http://www.postgresql.org/docs/faqs/FAQ.html > >
Christopher Kings-Lynne wrote: > No, the patch is against 7.5 CVS. It is a tiny fix that allows it to > dump 7.0.x database backends correctly. > > I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) Oh, it for using a 7.5 dump on a 7.0 database? I didn't think that would even work. :-) Anyway, you say the fix is already in CVS so we are OK. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
>>I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) That previous fix was for a different issue... > Oh, it for using a 7.5 dump on a 7.0 database? I didn't think that > would even work. :-) Yes, it works fine. We still support pg_dumping all 7.x databases. > Anyway, you say the fix is already in CVS so we are OK. *cry* No, it's _not_ in CVS - that's what my patch is for! My patch is against 7.5CVS! Chris
Christopher Kings-Lynne wrote: > >>I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) > > That previous fix was for a different issue... > > > Oh, it for using a 7.5 dump on a 7.0 database? I didn't think that > > would even work. :-) > > Yes, it works fine. We still support pg_dumping all 7.x databases. > > > Anyway, you say the fix is already in CVS so we are OK. > > *cry* > > No, it's _not_ in CVS - that's what my patch is for! My patch is > against 7.5CVS! Duh, sorry. Got confused. I though you weren't the submitter, for some strange reason. Anyway, I see this in the code: >>+ /* Handle array types */ >>+ if (typname[0] == '_') >>+ { >>+ isarray = true; >>+ typname++; >>+ } Do we know that is always true? What is the issue that 7.0 needs this and newer released don't, and how are we sure this will not break some strange cases in post-7.0 releases? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
> Duh, sorry. Got confused. I though you weren't the submitter, for some > strange reason. Anyway, I see this in the code: > > >>>+ /* Handle array types */ >>>+ if (typname[0] == '_') >>>+ { >>>+ isarray = true; >>>+ typname++; >>>+ } > > > Do we know that is always true? What is the issue that 7.0 needs this > and newer released don't, and how are we sure this will not break some > strange cases in post-7.0 releases? It's not a problem, since that code path is only ever executed when dumping a 7.0 backend. It's in the MyFormatType function in pg_dump.c that is used whenever the backend doesn't have its own format_type function. Chris
Christopher Kings-Lynne <chriskl@familyhealth.com.au> writes: >> Do we know that is always true? What is the issue that 7.0 needs this >> and newer released don't, and how are we sure this will not break some >> strange cases in post-7.0 releases? > It's not a problem, since that code path is only ever executed when > dumping a 7.0 backend. It's in the MyFormatType function in pg_dump.c > that is used whenever the backend doesn't have its own format_type function. Right. The patch looked dangerous to me too, until I understood the context. As-is, I think it's fine -- indeed, I think we ought to backpatch it into 7.4 branch too. regards, tom lane
>>It's not a problem, since that code path is only ever executed when >>dumping a 7.0 backend. It's in the MyFormatType function in pg_dump.c >>that is used whenever the backend doesn't have its own format_type function. > > Right. The patch looked dangerous to me too, until I understood the > context. As-is, I think it's fine -- indeed, I think we ought to > backpatch it into 7.4 branch too. Strangely enough, the 7.0.x pg_dump.c does just dump arrays as bpchar_ type, but the 7.0.x psql utility displays them as character varying[] Chris
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it within the next 48 hours. --------------------------------------------------------------------------- Christopher Kings-Lynne wrote: > No, the patch is against 7.5 CVS. It is a tiny fix that allows it to > dump 7.0.x database backends correctly. > > I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) > > Chris > > Bruce Momjian wrote: > > > Uh, not sure anyone would even see a 7.0.X release if we made it, and I > > question how many are using varying[]. Your patch is now in the > > archives, and we can point folks to it if they ask. > > > > --------------------------------------------------------------------------- > > > > Christopher Kings-Lynne wrote: > > > >>Hi, > >> > >>I know 7.0.x is pretty old, but I'm wondering if we should fix this to > >>make it better for people upgrading. > >> > >>If you create a table like this in 7.0.x: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing character varying[] > >>); > >> > >>The 7.5 pg_dump program will dump it like this: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing _varchar > >>); > >> > >>I have attached a patch that should fix it. I haven't been able to > >>actually test it since my dev machine and the 7.0 machine I have access > >>to aren't connected - although it does compile. The fix is based on the > >>7.0 psql code. > >> > >>Chris > > > > > >>? src/bin/pg_dump/.deps > >>? src/bin/pg_dump/common.c.working > >>? src/bin/pg_dump/pg_dump > >>? src/bin/pg_dump/pg_dump.c.working > >>? src/bin/pg_dump/pg_dump.h.working > >>? src/bin/pg_dump/pg_dumpall > >>? src/bin/pg_dump/pg_restore > >>Index: src/bin/pg_dump/pg_dump.c > >>=================================================================== > >>RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v > >>retrieving revision 1.370 > >>diff -c -r1.370 pg_dump.c > >>*** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 > >>--- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 > >>*************** > >>*** 7726,7733 **** > >>--- 7726,7741 ---- > >> myFormatType(const char *typname, int32 typmod) > >> { > >> char *result; > >>+ bool isarray = false; > >> PQExpBuffer buf = createPQExpBuffer(); > >> > >>+ /* Handle array types */ > >>+ if (typname[0] == '_') > >>+ { > >>+ isarray = true; > >>+ typname++; > >>+ } > >>+ > >> /* Show lengths on bpchar and varchar */ > >> if (!strcmp(typname, "bpchar")) > >> { > >>*************** > >>*** 7770,7775 **** > >>--- 7778,7787 ---- > >> appendPQExpBuffer(buf, "\"char\""); > >> else > >> appendPQExpBuffer(buf, "%s", fmtId(typname)); > >>+ > >>+ /* Append array qualifier for array types */ > >>+ if (isarray) > >>+ appendPQExpBuffer(buf, "[]"); > >> > >> result = strdup(buf->data); > >> destroyPQExpBuffer(buf); > > > > > >>---------------------------(end of broadcast)--------------------------- > >>TIP 5: Have you checked our extensive FAQ? > >> > >> http://www.postgresql.org/docs/faqs/FAQ.html > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 7: don't forget to increase your free space map settings > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Patch applied. Thanks. --------------------------------------------------------------------------- Christopher Kings-Lynne wrote: > No, the patch is against 7.5 CVS. It is a tiny fix that allows it to > dump 7.0.x database backends correctly. > > I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) > > Chris > > Bruce Momjian wrote: > > > Uh, not sure anyone would even see a 7.0.X release if we made it, and I > > question how many are using varying[]. Your patch is now in the > > archives, and we can point folks to it if they ask. > > > > --------------------------------------------------------------------------- > > > > Christopher Kings-Lynne wrote: > > > >>Hi, > >> > >>I know 7.0.x is pretty old, but I'm wondering if we should fix this to > >>make it better for people upgrading. > >> > >>If you create a table like this in 7.0.x: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing character varying[] > >>); > >> > >>The 7.5 pg_dump program will dump it like this: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing _varchar > >>); > >> > >>I have attached a patch that should fix it. I haven't been able to > >>actually test it since my dev machine and the 7.0 machine I have access > >>to aren't connected - although it does compile. The fix is based on the > >>7.0 psql code. > >> > >>Chris > > > > > >>? src/bin/pg_dump/.deps > >>? src/bin/pg_dump/common.c.working > >>? src/bin/pg_dump/pg_dump > >>? src/bin/pg_dump/pg_dump.c.working > >>? src/bin/pg_dump/pg_dump.h.working > >>? src/bin/pg_dump/pg_dumpall > >>? src/bin/pg_dump/pg_restore > >>Index: src/bin/pg_dump/pg_dump.c > >>=================================================================== > >>RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v > >>retrieving revision 1.370 > >>diff -c -r1.370 pg_dump.c > >>*** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 > >>--- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 > >>*************** > >>*** 7726,7733 **** > >>--- 7726,7741 ---- > >> myFormatType(const char *typname, int32 typmod) > >> { > >> char *result; > >>+ bool isarray = false; > >> PQExpBuffer buf = createPQExpBuffer(); > >> > >>+ /* Handle array types */ > >>+ if (typname[0] == '_') > >>+ { > >>+ isarray = true; > >>+ typname++; > >>+ } > >>+ > >> /* Show lengths on bpchar and varchar */ > >> if (!strcmp(typname, "bpchar")) > >> { > >>*************** > >>*** 7770,7775 **** > >>--- 7778,7787 ---- > >> appendPQExpBuffer(buf, "\"char\""); > >> else > >> appendPQExpBuffer(buf, "%s", fmtId(typname)); > >>+ > >>+ /* Append array qualifier for array types */ > >>+ if (isarray) > >>+ appendPQExpBuffer(buf, "[]"); > >> > >> result = strdup(buf->data); > >> destroyPQExpBuffer(buf); > > > > > >>---------------------------(end of broadcast)--------------------------- > >>TIP 5: Have you checked our extensive FAQ? > >> > >> http://www.postgresql.org/docs/faqs/FAQ.html > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 7: don't forget to increase your free space map settings > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Backpatched to 7.4.X. --------------------------------------------------------------------------- Christopher Kings-Lynne wrote: > No, the patch is against 7.5 CVS. It is a tiny fix that allows it to > dump 7.0.x database backends correctly. > > I submitted a fix for 7.5 dumping 7.0 previously and it was accepted :) > > Chris > > Bruce Momjian wrote: > > > Uh, not sure anyone would even see a 7.0.X release if we made it, and I > > question how many are using varying[]. Your patch is now in the > > archives, and we can point folks to it if they ask. > > > > --------------------------------------------------------------------------- > > > > Christopher Kings-Lynne wrote: > > > >>Hi, > >> > >>I know 7.0.x is pretty old, but I'm wondering if we should fix this to > >>make it better for people upgrading. > >> > >>If you create a table like this in 7.0.x: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing character varying[] > >>); > >> > >>The 7.5 pg_dump program will dump it like this: > >> > >>CREATE TABLE address ( > >> first_name character varying(50) DEFAULT 'asdf' NOT NULL, > >> last_name character varying(50) NOT NULL, > >> address character varying(50), > >> tesing _varchar > >>); > >> > >>I have attached a patch that should fix it. I haven't been able to > >>actually test it since my dev machine and the 7.0 machine I have access > >>to aren't connected - although it does compile. The fix is based on the > >>7.0 psql code. > >> > >>Chris > > > > > >>? src/bin/pg_dump/.deps > >>? src/bin/pg_dump/common.c.working > >>? src/bin/pg_dump/pg_dump > >>? src/bin/pg_dump/pg_dump.c.working > >>? src/bin/pg_dump/pg_dump.h.working > >>? src/bin/pg_dump/pg_dumpall > >>? src/bin/pg_dump/pg_restore > >>Index: src/bin/pg_dump/pg_dump.c > >>=================================================================== > >>RCS file: /projects/cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v > >>retrieving revision 1.370 > >>diff -c -r1.370 pg_dump.c > >>*** src/bin/pg_dump/pg_dump.c 24 Mar 2004 03:06:08 -0000 1.370 > >>--- src/bin/pg_dump/pg_dump.c 16 May 2004 14:42:55 -0000 > >>*************** > >>*** 7726,7733 **** > >>--- 7726,7741 ---- > >> myFormatType(const char *typname, int32 typmod) > >> { > >> char *result; > >>+ bool isarray = false; > >> PQExpBuffer buf = createPQExpBuffer(); > >> > >>+ /* Handle array types */ > >>+ if (typname[0] == '_') > >>+ { > >>+ isarray = true; > >>+ typname++; > >>+ } > >>+ > >> /* Show lengths on bpchar and varchar */ > >> if (!strcmp(typname, "bpchar")) > >> { > >>*************** > >>*** 7770,7775 **** > >>--- 7778,7787 ---- > >> appendPQExpBuffer(buf, "\"char\""); > >> else > >> appendPQExpBuffer(buf, "%s", fmtId(typname)); > >>+ > >>+ /* Append array qualifier for array types */ > >>+ if (isarray) > >>+ appendPQExpBuffer(buf, "[]"); > >> > >> result = strdup(buf->data); > >> destroyPQExpBuffer(buf); > > > > > >>---------------------------(end of broadcast)--------------------------- > >>TIP 5: Have you checked our extensive FAQ? > >> > >> http://www.postgresql.org/docs/faqs/FAQ.html > > > > > > ---------------------------(end of broadcast)--------------------------- > TIP 7: don't forget to increase your free space map settings > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073