From 6d98dfd7f191dfe99f24c3022f30af8fc6a624dd Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Sat, 4 Jul 2015 20:40:42 +0900
Subject: [PATCH] Ensure COMMENT persistency of indexes and constraint with
 ALTER TABLE

When rewriting a table, in some cases indexes and constraints present
on it need to be recreated from scratch, making any existing comment
entry, as known as a description in pg_description, disappear after
ALTER TABLE.

This commit fixes this issue by tracking the existing constraint,
indexes, and combinations of both when running ALTER TABLE and recreate
COMMENT entries when appropriate. A set of regression tests is added
to test all the new code paths added.
---
 src/backend/commands/tablecmds.c          | 268 +++++++++++++++++++++---------
 src/include/nodes/parsenodes.h            |   1 +
 src/test/regress/expected/alter_table.out |  95 +++++++++++
 src/test/regress/sql/alter_table.sql      |  37 +++++
 4 files changed, 327 insertions(+), 74 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d394713..79de187 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -316,6 +316,13 @@ static void ATRewriteTables(AlterTableStmt *parsetree,
 				List **wqueue, LOCKMODE lockmode);
 static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode);
 static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
+static void ATAttachQueueCommand(Oid oldId, Oid refRelId, List **wqueue,
+				Node *stm, Relation rel, bool rewrite);
+static void ATAttachQueueIndexStmt(Oid oldId, List **wqueue,
+				IndexStmt *stmt, Relation rel, bool rewrite);
+static void ATAttachQueueAlterTableStmt(Oid oldId, Oid refRelId,
+							List **wqueue, AlterTableStmt *stmt,
+							Relation rel, bool rewrite);
 static void ATSimplePermissions(Relation rel, int allowed_targets);
 static void ATWrongRelkindError(Relation rel, int allowed_targets);
 static void ATSimpleRecursion(List **wqueue, Relation rel,
@@ -386,6 +393,10 @@ static void ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab,
 static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
 					 char *cmd, List **wqueue, LOCKMODE lockmode,
 					 bool rewrite);
+static void RebuildConstraintComment(AlteredTableInfo *tab,
+					 int cmdidx,
+					 Oid objid,
+					 List *objname);
 static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
 static void TryReuseForeignKey(Oid oldId, Constraint *con);
 static void change_owner_fix_column_acls(Oid relationOid,
@@ -3498,6 +3509,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			address = ATExecAddIndex(tab, rel, (IndexStmt *) cmd->def, true,
 									 lockmode);
 			break;
+		case AT_ReAddComment:	/* Re-add existing comment */
+			CommentObject((CommentStmt *) cmd->def);
+			break;
 		case AT_AddConstraint:	/* ADD CONSTRAINT */
 			address =
 				ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
@@ -4251,6 +4265,150 @@ ATGetQueueEntry(List **wqueue, Relation rel)
 	return tab;
 }
 
+
+/*
+ * ATAttachQueueCommand
+ *
+ * Attach each generated command to the proper place in the work queue.
+ * Note this could result in creation of entirely new work-queue entries.
+ *
+ * Also note that the command subtypes have to be tweaked, because it
+ * turns out that re-creation of indexes and constraints has to act a bit
+ * differently from initial creation.
+ */
+static void
+ATAttachQueueCommand(Oid oldId, Oid refRelId, List **wqueue,
+					 Node *stm, Relation rel, bool rewrite)
+{
+	switch (nodeTag(stm))
+	{
+		case T_IndexStmt:
+			ATAttachQueueIndexStmt(oldId, wqueue,
+								   (IndexStmt *) stm, rel, rewrite);
+			break;
+		case T_AlterTableStmt:
+			ATAttachQueueAlterTableStmt(oldId, refRelId, wqueue,
+										(AlterTableStmt *) stm,
+										rel, rewrite);
+			break;
+		default:
+			elog(ERROR, "unexpected statement type: %d",
+				 (int) nodeTag(stm));
+	}
+}
+
+
+/*
+ * ATAttachQueueIndexStmt
+ *
+ * Attach to the correct queue the given IndexStmt, re-creating at the same
+ * time a comment for it if necessary.
+ */
+static void
+ATAttachQueueIndexStmt(Oid oldId, List **wqueue,
+					   IndexStmt *stmt, Relation rel, bool rewrite)
+{
+	AlterTableCmd	   *newcmd;
+	AlteredTableInfo   *tab;
+
+	if (!rewrite)
+		TryReuseIndex(oldId, stmt);
+
+	tab = ATGetQueueEntry(wqueue, rel);
+	newcmd = makeNode(AlterTableCmd);
+	newcmd->subtype = AT_ReAddIndex;
+	newcmd->def = (Node *) stmt;
+	tab->subcmds[AT_PASS_OLD_INDEX] =
+		lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+	/* Add COMMENT entry for new index */
+	stmt->idxcomment = GetComment(oldId, RelationRelationId, 0);
+}
+
+/*
+ * ATAttachQueueAlterTableStmt
+ *
+ * Attach each generated command to the correct queue for the given
+ * AlterTableStmt, re-creating any existing COMMENT object for the
+ * commands generating new objects.
+ */
+static void
+ATAttachQueueAlterTableStmt(Oid oldId, Oid refRelId, List **wqueue,
+							AlterTableStmt *stmt, Relation rel,
+							bool rewrite)
+{
+	ListCell		 *lcmd;
+	AlteredTableInfo *tab;
+
+	tab = ATGetQueueEntry(wqueue, rel);
+	foreach(lcmd, stmt->cmds)
+	{
+		AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+		Constraint *con;
+
+		switch (cmd->subtype)
+		{
+			case AT_AddIndex:
+				{
+					IndexStmt  *stmt = (IndexStmt *) cmd->def;
+					Oid			indoid = get_constraint_index(oldId);
+					List	   *objnames;
+
+					Assert(IsA(cmd->def, IndexStmt));
+					if (!rewrite)
+						TryReuseIndex(indoid, stmt);
+					cmd->subtype = AT_ReAddIndex;
+					tab->subcmds[AT_PASS_OLD_INDEX] =
+						lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
+
+					/* Create identical COMMENT entry for new index */
+					objnames = list_make2(
+								makeString(get_rel_name(tab->relid)),
+								makeString(stmt->idxname));
+
+					/* Check the COMMENT of this constraint ... */
+					RebuildConstraintComment(tab,
+											 AT_PASS_OLD_INDEX,
+											 oldId,
+											 objnames);
+
+					/* ... And the comment on the index itself */
+					stmt->idxcomment = GetComment(indoid,
+												  RelationRelationId, 0);
+				}
+				break;
+			case AT_AddConstraint:
+				{
+					List *objnames;
+
+					Assert(IsA(cmd->def, Constraint));
+					con = (Constraint *) cmd->def;
+					con->old_pktable_oid = refRelId;
+					/* rewriting neither side of a FK */
+					if (con->contype == CONSTR_FOREIGN &&
+						!rewrite && tab->rewrite == 0)
+						TryReuseForeignKey(oldId, con);
+					cmd->subtype = AT_ReAddConstraint;
+					tab->subcmds[AT_PASS_OLD_CONSTR] =
+						lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+
+					/* create COMMENT entry for the new constraint */
+					objnames = list_make2(
+						makeString(get_rel_name(tab->relid)),
+						makeString(con->conname));
+					RebuildConstraintComment(tab,
+											 AT_PASS_OLD_CONSTR,
+											 oldId,
+											 objnames);
+				}
+				break;
+			default:
+				elog(ERROR, "unexpected statement type: %d",
+					 (int) cmd->subtype);
+		}
+	}
+}
+
 /*
  * ATSimplePermissions
  *
@@ -8633,84 +8791,46 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 	rel = relation_open(oldRelId, NoLock);
 
 	/*
-	 * Attach each generated command to the proper place in the work queue.
-	 * Note this could result in creation of entirely new work-queue entries.
-	 *
-	 * Also note that we have to tweak the command subtypes, because it turns
-	 * out that re-creation of indexes and constraints has to act a bit
-	 * differently from initial creation.
+	 * Attach each generated command to the correct queue.
 	 */
 	foreach(list_item, querytree_list)
-	{
-		Node	   *stm = (Node *) lfirst(list_item);
-		AlteredTableInfo *tab;
-
-		switch (nodeTag(stm))
-		{
-			case T_IndexStmt:
-				{
-					IndexStmt  *stmt = (IndexStmt *) stm;
-					AlterTableCmd *newcmd;
-
-					if (!rewrite)
-						TryReuseIndex(oldId, stmt);
-
-					tab = ATGetQueueEntry(wqueue, rel);
-					newcmd = makeNode(AlterTableCmd);
-					newcmd->subtype = AT_ReAddIndex;
-					newcmd->def = (Node *) stmt;
-					tab->subcmds[AT_PASS_OLD_INDEX] =
-						lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
-					break;
-				}
-			case T_AlterTableStmt:
-				{
-					AlterTableStmt *stmt = (AlterTableStmt *) stm;
-					ListCell   *lcmd;
-
-					tab = ATGetQueueEntry(wqueue, rel);
-					foreach(lcmd, stmt->cmds)
-					{
-						AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
-						Constraint *con;
+		ATAttachQueueCommand(oldId, refRelId, wqueue,
+							 (Node *) lfirst(list_item),
+							 rel, rewrite);
+	relation_close(rel, NoLock);
+}
 
-						switch (cmd->subtype)
-						{
-							case AT_AddIndex:
-								Assert(IsA(cmd->def, IndexStmt));
-								if (!rewrite)
-									TryReuseIndex(get_constraint_index(oldId),
-												  (IndexStmt *) cmd->def);
-								cmd->subtype = AT_ReAddIndex;
-								tab->subcmds[AT_PASS_OLD_INDEX] =
-									lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
-								break;
-							case AT_AddConstraint:
-								Assert(IsA(cmd->def, Constraint));
-								con = (Constraint *) cmd->def;
-								con->old_pktable_oid = refRelId;
-								/* rewriting neither side of a FK */
-								if (con->contype == CONSTR_FOREIGN &&
-									!rewrite && tab->rewrite == 0)
-									TryReuseForeignKey(oldId, con);
-								cmd->subtype = AT_ReAddConstraint;
-								tab->subcmds[AT_PASS_OLD_CONSTR] =
-									lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
-								break;
-							default:
-								elog(ERROR, "unexpected statement type: %d",
-									 (int) cmd->subtype);
-						}
-					}
-					break;
-				}
-			default:
-				elog(ERROR, "unexpected statement type: %d",
-					 (int) nodeTag(stm));
-		}
-	}
+/*
+ * Subroutine for ATPostAlterTypeParse() to recreate a comment entry for
+ * a constraint that is being re-added.
+ */
+static void
+RebuildConstraintComment(AlteredTableInfo *tab,
+						 int cmdidx,
+						 Oid objid,
+						 List *objname)
+{
+	CommentStmt *cmd;
+	char *comment_str;
+	AlterTableCmd *newcmd;
+
+	/* Look for comment for object wanted, and leave if none */
+	comment_str = GetComment(objid, ConstraintRelationId, 0);
+	if (comment_str == NULL)
+		return;
 
-	relation_close(rel, NoLock);
+	/* Build node CommentStmt */
+	cmd = makeNode(CommentStmt);
+	cmd->objtype = OBJECT_TABCONSTRAINT;
+	cmd->objname = objname;
+	cmd->objargs = NIL;
+	cmd->comment = comment_str;
+
+	/* Append it to list of commands */
+	newcmd = makeNode(AlterTableCmd);
+	newcmd->subtype = AT_ReAddComment;
+	newcmd->def = (Node *) cmd;
+	tab->subcmds[cmdidx] = lappend(tab->subcmds[cmdidx], newcmd);
 }
 
 /*
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 868905b..0ec4c96 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1471,6 +1471,7 @@ typedef enum AlterTableType
 	AT_DropColumnRecurse,		/* internal to commands/tablecmds.c */
 	AT_AddIndex,				/* add index */
 	AT_ReAddIndex,				/* internal to commands/tablecmds.c */
+	AT_ReAddComment,			/* internal to commands/tablecmds.c */
 	AT_AddConstraint,			/* add constraint */
 	AT_AddConstraintRecurse,	/* internal to commands/tablecmds.c */
 	AT_ReAddConstraint,			/* internal to commands/tablecmds.c */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3ad2c55..f4d7207 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2400,6 +2400,101 @@ Check constraints:
 
 DROP TABLE alter2.tt8;
 DROP SCHEMA alter2;
+-- Check comment persistency across ALTER TABLE commands for constraints
+-- and indexes. ALTER TABLE is optimized to check when an index can be
+-- reused or not depending on the data type changed, hence check that as
+-- well with some dummy commands.
+CREATE TABLE comment_table_1 (id1 int, id2 int);
+-- Simple index
+CREATE INDEX comment_table_1_index ON comment_table_1(id2);
+COMMENT ON INDEX comment_table_1_index IS 'Simple index on comment_table_1';
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+           description           
+---------------------------------
+ Simple index on comment_table_1
+(1 row)
+
+ALTER TABLE comment_table_1 ALTER COLUMN id2 SET DATA TYPE text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+           description           
+---------------------------------
+ Simple index on comment_table_1
+(1 row)
+
+ALTER TABLE comment_table_1 ALTER COLUMN id2 SET DATA TYPE text USING id2::int::text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+           description           
+---------------------------------
+ Simple index on comment_table_1
+(1 row)
+
+-- Index with constraint
+ALTER TABLE comment_table_1 ADD CONSTRAINT comment_table_1_pk PRIMARY KEY (id1);
+COMMENT ON INDEX comment_table_1_pk IS 'Index of PRIMARY KEY of comment_table_1';
+COMMENT ON CONSTRAINT comment_table_1_pk ON comment_table_1 IS 'PRIMARY KEY constraint of comment_table_1';
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+               description               
+-----------------------------------------
+ Index of PRIMARY KEY of comment_table_1
+(1 row)
+
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+                description                
+-------------------------------------------
+ PRIMARY KEY constraint of comment_table_1
+(1 row)
+
+ALTER TABLE comment_table_1 ALTER COLUMN id1 SET DATA TYPE int;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+               description               
+-----------------------------------------
+ Index of PRIMARY KEY of comment_table_1
+(1 row)
+
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+                description                
+-------------------------------------------
+ PRIMARY KEY constraint of comment_table_1
+(1 row)
+
+ALTER TABLE comment_table_1 ALTER COLUMN id1 SET DATA TYPE text USING id1::int::text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+               description               
+-----------------------------------------
+ Index of PRIMARY KEY of comment_table_1
+(1 row)
+
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+                description                
+-------------------------------------------
+ PRIMARY KEY constraint of comment_table_1
+(1 row)
+
+DROP TABLE comment_table_1;
+-- Independent constraint
+CREATE TABLE comment_table_2 (id1 int CONSTRAINT positive_id1 CHECK (id1 > 0));
+COMMENT ON CONSTRAINT positive_id1 ON comment_table_2 IS 'CHECK constraint of comment_table_2';
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+             description             
+-------------------------------------
+ CHECK constraint of comment_table_2
+(1 row)
+
+ALTER TABLE comment_table_2 ALTER COLUMN id1 SET DATA TYPE int;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+             description             
+-------------------------------------
+ CHECK constraint of comment_table_2
+(1 row)
+
+ALTER TABLE comment_table_2 ALTER COLUMN id1 SET DATA TYPE bigint USING id1::int::bigint;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+             description             
+-------------------------------------
+ CHECK constraint of comment_table_2
+(1 row)
+
+DROP TABLE comment_table_2;
 -- Check that we map relation oids to filenodes and back correctly.  Only
 -- display bad mappings so the test output doesn't change all the time.  A
 -- filenode function call can return NULL for a relation dropped concurrently
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 29c1875..0f5347f 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1594,6 +1594,43 @@ ALTER TABLE IF EXISTS tt8 SET SCHEMA alter2;
 DROP TABLE alter2.tt8;
 DROP SCHEMA alter2;
 
+-- Check comment persistency across ALTER TABLE commands for constraints
+-- and indexes. ALTER TABLE is optimized to check when an index can be
+-- reused or not depending on the data type changed, hence check that as
+-- well with some dummy commands.
+CREATE TABLE comment_table_1 (id1 int, id2 int);
+-- Simple index
+CREATE INDEX comment_table_1_index ON comment_table_1(id2);
+COMMENT ON INDEX comment_table_1_index IS 'Simple index on comment_table_1';
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+ALTER TABLE comment_table_1 ALTER COLUMN id2 SET DATA TYPE text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+ALTER TABLE comment_table_1 ALTER COLUMN id2 SET DATA TYPE text USING id2::int::text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_index'::regclass;
+-- Index with constraint
+ALTER TABLE comment_table_1 ADD CONSTRAINT comment_table_1_pk PRIMARY KEY (id1);
+COMMENT ON INDEX comment_table_1_pk IS 'Index of PRIMARY KEY of comment_table_1';
+COMMENT ON CONSTRAINT comment_table_1_pk ON comment_table_1 IS 'PRIMARY KEY constraint of comment_table_1';
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+ALTER TABLE comment_table_1 ALTER COLUMN id1 SET DATA TYPE int;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+ALTER TABLE comment_table_1 ALTER COLUMN id1 SET DATA TYPE text USING id1::int::text;
+SELECT description FROM pg_description WHERE objoid = 'comment_table_1_pk'::regclass;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'comment_table_1_pk';
+DROP TABLE comment_table_1;
+-- Independent constraint
+CREATE TABLE comment_table_2 (id1 int CONSTRAINT positive_id1 CHECK (id1 > 0));
+COMMENT ON CONSTRAINT positive_id1 ON comment_table_2 IS 'CHECK constraint of comment_table_2';
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+ALTER TABLE comment_table_2 ALTER COLUMN id1 SET DATA TYPE int;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+ALTER TABLE comment_table_2 ALTER COLUMN id1 SET DATA TYPE bigint USING id1::int::bigint;
+SELECT description FROM pg_description, pg_constraint WHERE objoid = oid AND conname = 'positive_id1';
+DROP TABLE comment_table_2;
+
+
 -- Check that we map relation oids to filenodes and back correctly.  Only
 -- display bad mappings so the test output doesn't change all the time.  A
 -- filenode function call can return NULL for a relation dropped concurrently
-- 
2.4.5

