ALTER TYPE DROP + composite-typed col vs. pg_upgrade

Started by Noah Mischover 14 years ago3 messages
#1Noah Misch
noah@leadboat.com
1 attachment(s)

As originally noted here:
http://archives.postgresql.org/message-id/20110329215043.GA11023@tornado.gateway.2wire.net

Previous version of patch proposed here:
http://archives.postgresql.org/message-id/20110418235041.GB2769@tornado.leadboat.com

This was a side issue to that thread, and its primary issue is now resolved.
Here's a fresh thread to finish this other bug.

Now that we have ALTER TYPE DROP ATTRIBUTE, pg_dump --binary-upgrade must, for
the sake of composite-typed columns, preserve the dropped-column configuration
of stand-alone composite types. Here's a test case:

create type t as (x int, y int);
create table has_a (tcol t);
insert into has_a values ('(1,2)');
table has_a; -- (1,2)
alter type t drop attribute y cascade, add attribute z int cascade;
table has_a; -- (1,)
table has_a; -- after pg_upgrade: (1,2)

Apparently I did not fully test the last version after merging it with upstream
changes, because it did not work. Sorry for that. This version updates the
queries correctly and adds a test case. A regular "make check" passes the new
test case with or without the rest of this patch. However, a comparison of
regression database dumps before and after a pg_upgrade will reveal the problem
given this new test case. See, for example, Peter's recent patch to have the
contrib/pg_upgrade "make check" do this.

Thanks,
nm

Attachments:

tt2v3-pgupgrade-composite-col.patchtext/plain; charset=us-asciiDownload
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index afc7fd7..13ba7dd 100644
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***************
*** 7937,7942 **** static void
--- 7937,7943 ----
  dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
  {
  	PQExpBuffer q = createPQExpBuffer();
+ 	PQExpBuffer dropped = createPQExpBuffer();
  	PQExpBuffer delq = createPQExpBuffer();
  	PQExpBuffer labelq = createPQExpBuffer();
  	PQExpBuffer query = createPQExpBuffer();
***************
*** 7944,7952 **** dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
--- 7945,7957 ----
  	int			ntups;
  	int			i_attname;
  	int			i_atttypdefn;
+ 	int			i_attlen;
+ 	int			i_attalign;
+ 	int			i_attisdropped;
  	int			i_attcollation;
  	int			i_typrelid;
  	int			i;
+ 	int			actual_atts;
  
  	/* Set proper schema search path so type references list correctly */
  	selectSourceSchema(tyinfo->dobj.namespace->dobj.name);
***************
*** 7958,7990 **** dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
  		 * attcollation is new in 9.1.	Since we only want to dump COLLATE
  		 * clauses for attributes whose collation is different from their
  		 * type's default, we use a CASE here to suppress uninteresting
! 		 * attcollations cheaply.
  		 */
  		appendPQExpBuffer(query, "SELECT a.attname, "
  						  "pg_catalog.format_type(a.atttypid, a.atttypmod) AS atttypdefn, "
  						  "CASE WHEN a.attcollation <> at.typcollation "
  						  "THEN a.attcollation ELSE 0 END AS attcollation, "
  						  "ct.typrelid "
! 						  "FROM pg_catalog.pg_type ct, pg_catalog.pg_attribute a, "
! 						  "pg_catalog.pg_type at "
  						  "WHERE ct.oid = '%u'::pg_catalog.oid "
- 						  "AND a.attrelid = ct.typrelid "
- 						  "AND a.atttypid = at.oid "
- 						  "AND NOT a.attisdropped "
  						  "ORDER BY a.attnum ",
  						  tyinfo->dobj.catId.oid);
  	}
  	else
  	{
! 		/* We assume here that remoteVersion must be at least 70300 */
  		appendPQExpBuffer(query, "SELECT a.attname, "
  						  "pg_catalog.format_type(a.atttypid, a.atttypmod) AS atttypdefn, "
  						  "0 AS attcollation, "
  						  "ct.typrelid "
  						  "FROM pg_catalog.pg_type ct, pg_catalog.pg_attribute a "
  						  "WHERE ct.oid = '%u'::pg_catalog.oid "
  						  "AND a.attrelid = ct.typrelid "
- 						  "AND NOT a.attisdropped "
  						  "ORDER BY a.attnum ",
  						  tyinfo->dobj.catId.oid);
  	}
--- 7963,7999 ----
  		 * attcollation is new in 9.1.	Since we only want to dump COLLATE
  		 * clauses for attributes whose collation is different from their
  		 * type's default, we use a CASE here to suppress uninteresting
! 		 * attcollations cheaply.  atttypid will be 0 for dropped columns;
! 		 * collation does not matter for those.
  		 */
  		appendPQExpBuffer(query, "SELECT a.attname, "
  						  "pg_catalog.format_type(a.atttypid, a.atttypmod) AS atttypdefn, "
+ 						  "a.attlen, a.attalign, a.attisdropped, "
  						  "CASE WHEN a.attcollation <> at.typcollation "
  						  "THEN a.attcollation ELSE 0 END AS attcollation, "
  						  "ct.typrelid "
! 						  "FROM pg_catalog.pg_type ct "
! 						  "JOIN pg_catalog.pg_attribute a ON a.attrelid = ct.typrelid "
! 						  "LEFT JOIN pg_catalog.pg_type at ON at.oid = a.atttypid "
  						  "WHERE ct.oid = '%u'::pg_catalog.oid "
  						  "ORDER BY a.attnum ",
  						  tyinfo->dobj.catId.oid);
  	}
  	else
  	{
! 		/*
! 		 * We assume here that remoteVersion must be at least 70300.  Since
! 		 * ALTER TYPE could not drop columns until 9.1, attisdropped should
! 		 * always be false.
! 		 */
  		appendPQExpBuffer(query, "SELECT a.attname, "
  						  "pg_catalog.format_type(a.atttypid, a.atttypmod) AS atttypdefn, "
+ 						  "a.attlen, a.attalign, a.attisdropped, "
  						  "0 AS attcollation, "
  						  "ct.typrelid "
  						  "FROM pg_catalog.pg_type ct, pg_catalog.pg_attribute a "
  						  "WHERE ct.oid = '%u'::pg_catalog.oid "
  						  "AND a.attrelid = ct.typrelid "
  						  "ORDER BY a.attnum ",
  						  tyinfo->dobj.catId.oid);
  	}
***************
*** 7996,8001 **** dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
--- 8005,8013 ----
  
  	i_attname = PQfnumber(res, "attname");
  	i_atttypdefn = PQfnumber(res, "atttypdefn");
+ 	i_attlen = PQfnumber(res, "attlen");
+ 	i_attalign = PQfnumber(res, "attalign");
+ 	i_attisdropped = PQfnumber(res, "attisdropped");
  	i_attcollation = PQfnumber(res, "attcollation");
  	i_typrelid = PQfnumber(res, "typrelid");
  
***************
*** 8010,8047 **** dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
  	appendPQExpBuffer(q, "CREATE TYPE %s AS (",
  					  fmtId(tyinfo->dobj.name));
  
  	for (i = 0; i < ntups; i++)
  	{
  		char	   *attname;
  		char	   *atttypdefn;
  		Oid			attcollation;
  
  		attname = PQgetvalue(res, i, i_attname);
  		atttypdefn = PQgetvalue(res, i, i_atttypdefn);
  		attcollation = atooid(PQgetvalue(res, i, i_attcollation));
  
! 		appendPQExpBuffer(q, "\n\t%s %s", fmtId(attname), atttypdefn);
  
! 		/* Add collation if not default for the column type */
! 		if (OidIsValid(attcollation))
  		{
! 			CollInfo   *coll;
  
! 			coll = findCollationByOid(attcollation);
! 			if (coll)
  			{
! 				/* always schema-qualify, don't try to be smart */
! 				appendPQExpBuffer(q, " COLLATE %s.",
! 								  fmtId(coll->dobj.namespace->dobj.name));
! 				appendPQExpBuffer(q, "%s",
! 								  fmtId(coll->dobj.name));
  			}
  		}
! 
! 		if (i < ntups - 1)
! 			appendPQExpBuffer(q, ",");
  	}
  	appendPQExpBuffer(q, "\n);\n");
  
  	/*
  	 * DROP must be fully qualified in case same name appears in pg_catalog
--- 8022,8096 ----
  	appendPQExpBuffer(q, "CREATE TYPE %s AS (",
  					  fmtId(tyinfo->dobj.name));
  
+ 	actual_atts = 0;
  	for (i = 0; i < ntups; i++)
  	{
  		char	   *attname;
  		char	   *atttypdefn;
+ 		char	   *attlen;
+ 		char	   *attalign;
+ 		bool		attisdropped;
  		Oid			attcollation;
  
  		attname = PQgetvalue(res, i, i_attname);
  		atttypdefn = PQgetvalue(res, i, i_atttypdefn);
+ 		attlen = PQgetvalue(res, i, i_attlen);
+ 		attalign = PQgetvalue(res, i, i_attalign);
+ 		attisdropped = (PQgetvalue(res, i, i_attisdropped)[0] == 't');
  		attcollation = atooid(PQgetvalue(res, i, i_attcollation));
  
! 		if (attisdropped && !binary_upgrade)
! 			continue;
  
! 		/* Format properly if not first attr */
! 		if (actual_atts++ > 0)
! 			appendPQExpBuffer(q, ",");
! 		appendPQExpBuffer(q, "\n\t");
! 
! 		if (!attisdropped)
  		{
! 			appendPQExpBuffer(q, "%s %s", fmtId(attname), atttypdefn);
  
! 			/* Add collation if not default for the column type */
! 			if (OidIsValid(attcollation))
  			{
! 				CollInfo   *coll;
! 
! 				coll = findCollationByOid(attcollation);
! 				if (coll)
! 				{
! 					/* always schema-qualify, don't try to be smart */
! 					appendPQExpBuffer(q, " COLLATE %s.",
! 									  fmtId(coll->dobj.namespace->dobj.name));
! 					appendPQExpBuffer(q, "%s",
! 									  fmtId(coll->dobj.name));
! 				}
  			}
  		}
! 		else	/* binary_upgrade - see under dumpTableSchema() */
! 		{
! 			appendPQExpBuffer(q, "%s INTEGER /* dummy */", fmtId(attname));
! 
! 			/* stash separately for insertion after the CREATE TYPE */
! 			appendPQExpBuffer(dropped,
! 							  "\n-- For binary upgrade, recreate dropped column.\n");
! 			appendPQExpBuffer(dropped, "UPDATE pg_catalog.pg_attribute\n"
! 							  "SET attlen = %s, "
! 							  "attalign = '%s', attbyval = false\n"
! 							  "WHERE attname = ", attlen, attalign);
! 			appendStringLiteralAH(dropped, attname, fout);
! 			appendPQExpBuffer(dropped, "\n  AND attrelid = ");
! 			appendStringLiteralAH(dropped, fmtId(tyinfo->dobj.name), fout);
! 			appendPQExpBuffer(dropped, "::pg_catalog.regclass;\n");
! 
! 			appendPQExpBuffer(dropped, "ALTER TYPE %s ",
! 							  fmtId(tyinfo->dobj.name));
! 			appendPQExpBuffer(dropped, "DROP ATTRIBUTE %s;\n",
! 							  fmtId(attname));
! 		}
  	}
  	appendPQExpBuffer(q, "\n);\n");
+ 	appendPQExpBufferStr(q, dropped->data);
  
  	/*
  	 * DROP must be fully qualified in case same name appears in pg_catalog
***************
*** 8077,8082 **** dumpCompositeType(Archive *fout, TypeInfo *tyinfo)
--- 8126,8132 ----
  
  	PQclear(res);
  	destroyPQExpBuffer(q);
+ 	destroyPQExpBuffer(dropped);
  	destroyPQExpBuffer(delq);
  	destroyPQExpBuffer(labelq);
  	destroyPQExpBuffer(query);
diff --git a/src/test/regress/exindex a6fb36e..dd814af 100644
*** a/src/test/regress/expected/alter_table.out
--- b/src/test/regress/expected/alter_table.out
***************
*** 1955,1960 **** Table "public.test_tbl2_subclass"
--- 1955,1963 ----
  Inherits: test_tbl2
  
  DROP TABLE test_tbl2_subclass;
+ CREATE TYPE test_type3 AS (a int);
+ CREATE TABLE test_tbl3 (c) AS SELECT '(1)'::test_type3;
+ ALTER TYPE test_type3 DROP ATTRIBUTE a, ADD ATTRIBUTE b int;
  CREATE TYPE test_type_empty AS ();
  DROP TYPE test_type_empty;
  --
diff --git a/src/test/regress/sql/alter_table.sqindex 4b2afe8..6cede13 100644
*** a/src/test/regress/sql/alter_table.sql
--- b/src/test/regress/sql/alter_table.sql
***************
*** 1371,1376 **** ALTER TYPE test_type2 RENAME ATTRIBUTE a TO aa CASCADE;
--- 1371,1380 ----
  
  DROP TABLE test_tbl2_subclass;
  
+ CREATE TYPE test_type3 AS (a int);
+ CREATE TABLE test_tbl3 (c) AS SELECT '(1)'::test_type3;
+ ALTER TYPE test_type3 DROP ATTRIBUTE a, ADD ATTRIBUTE b int;
+ 
  CREATE TYPE test_type_empty AS ();
  DROP TYPE test_type_empty;
  
#2Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Noah Misch (#1)
Re: ALTER TYPE DROP + composite-typed col vs. pg_upgrade

On 28.04.2011 15:41, Noah Misch wrote:

Now that we have ALTER TYPE DROP ATTRIBUTE, pg_dump --binary-upgrade must, for
the sake of composite-typed columns, preserve the dropped-column configuration
of stand-alone composite types. Here's a test case:

create type t as (x int, y int);
create table has_a (tcol t);
insert into has_a values ('(1,2)');
table has_a; -- (1,2)
alter type t drop attribute y cascade, add attribute z int cascade;
table has_a; -- (1,)
table has_a; -- after pg_upgrade: (1,2)

Apparently I did not fully test the last version after merging it with upstream
changes, because it did not work. Sorry for that. This version updates the
queries correctly and adds a test case. A regular "make check" passes the new
test case with or without the rest of this patch. However, a comparison of
regression database dumps before and after a pg_upgrade will reveal the problem
given this new test case. See, for example, Peter's recent patch to have the
contrib/pg_upgrade "make check" do this.

Ok, committed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

#3Noah Misch
noah@leadboat.com
In reply to: Heikki Linnakangas (#2)
Re: ALTER TYPE DROP + composite-typed col vs. pg_upgrade

On Sat, May 21, 2011 at 08:25:30AM -0400, Heikki Linnakangas wrote:

On 28.04.2011 15:41, Noah Misch wrote:

Now that we have ALTER TYPE DROP ATTRIBUTE, pg_dump --binary-upgrade must, for
the sake of composite-typed columns, preserve the dropped-column configuration
of stand-alone composite types. Here's a test case:

create type t as (x int, y int);
create table has_a (tcol t);
insert into has_a values ('(1,2)');
table has_a; -- (1,2)
alter type t drop attribute y cascade, add attribute z int cascade;
table has_a; -- (1,)
table has_a; -- after pg_upgrade: (1,2)

Apparently I did not fully test the last version after merging it with upstream
changes, because it did not work. Sorry for that. This version updates the
queries correctly and adds a test case. A regular "make check" passes the new
test case with or without the rest of this patch. However, a comparison of
regression database dumps before and after a pg_upgrade will reveal the problem
given this new test case. See, for example, Peter's recent patch to have the
contrib/pg_upgrade "make check" do this.

Ok, committed.

Thank you.