Bug in CVS pg_dump against 7.0.x

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:

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