Retain comments on indexes and constraints at ALTER TABLE ... TYPE ...

When a column's datatype is changed, ATExecAlterColumnType() rebuilds all
the affected indexes and constraints, and the comments from the old
indexes/constraints were not carried over.

To fix, create a synthetic COMMENT ON command in the work queue, to re-add
any comments on constraints. For indexes, there's a comment field in
IndexStmt that is used.

This fixes bug #13126, reported by Kirill Simonov. Original patch by
Michael Paquier, reviewed by Petr Jelinek and me. This bug is present in
all versions, but only backpatch to 9.5. Given how minor the issue is, it
doesn't seem worth the work and risk to backpatch further than that.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e7b23f1621cd3a901b3b8209ec1ea10ac0a00485..1c7eded9a79cddb7e24bd98f0deddee5a9089fcc 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -386,6 +386,8 @@ 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 pass,
+                                                Oid objid, Relation rel, char *conname);
 static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
 static void TryReuseForeignKey(Oid oldId, Constraint *con);
 static void change_owner_fix_column_acls(Oid relationOid,
@@ -3514,6 +3516,9 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
                                ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
                                                                        false, true, lockmode);
                        break;
+               case AT_ReAddComment:   /* Re-add existing comment */
+                       address = CommentObject((CommentStmt *) cmd->def);
+                       break;
                case AT_AddIndexConstraint:             /* ADD CONSTRAINT USING INDEX */
                        address = ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def,
                                                                                           lockmode);
@@ -8654,6 +8659,8 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 
                        if (!rewrite)
                                TryReuseIndex(oldId, stmt);
+                       /* keep the index's comment */
+                       stmt->idxcomment = GetComment(oldId, RelationRelationId, 0);
 
                        newcmd = makeNode(AlterTableCmd);
                        newcmd->subtype = AT_ReAddIndex;
@@ -8672,15 +8679,29 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
 
                                if (cmd->subtype == AT_AddIndex)
                                {
+                                       IndexStmt  *indstmt;
+                                       Oid                     indoid;
+
                                        Assert(IsA(cmd->def, IndexStmt));
 
+                                       indstmt = (IndexStmt *) cmd->def;
+                                       indoid = get_constraint_index(oldId);
+
                                        if (!rewrite)
-                                               TryReuseIndex(get_constraint_index(oldId),
-                                                                         (IndexStmt *) cmd->def);
+                                               TryReuseIndex(indoid, indstmt);
+                                       /* keep any comment on the index */
+                                       indstmt->idxcomment = GetComment(indoid,
+                                                                                                        RelationRelationId, 0);
 
                                        cmd->subtype = AT_ReAddIndex;
                                        tab->subcmds[AT_PASS_OLD_INDEX] =
                                                lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
+
+                                       /* recreate any comment on the constraint */
+                                       RebuildConstraintComment(tab,
+                                                                                        AT_PASS_OLD_INDEX,
+                                                                                        oldId,
+                                                                                        rel, indstmt->idxname);
                                }
                                else if (cmd->subtype == AT_AddConstraint)
                                {
@@ -8697,6 +8718,12 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
                                        cmd->subtype = AT_ReAddConstraint;
                                        tab->subcmds[AT_PASS_OLD_CONSTR] =
                                                lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+
+                                       /* recreate any comment on the constraint */
+                                       RebuildConstraintComment(tab,
+                                                                                        AT_PASS_OLD_CONSTR,
+                                                                                        oldId,
+                                                                                        rel, con->conname);
                                }
                                else
                                        elog(ERROR, "unexpected statement type: %d",
@@ -8711,6 +8738,40 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
        relation_close(rel, NoLock);
 }
 
+/*
+ * Subroutine for ATPostAlterTypeParse() to recreate a comment entry for
+ * a constraint that is being re-added.
+ */
+static void
+RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
+                                                Relation rel, char *conname)
+{
+       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;
+
+       /* Build node CommentStmt */
+       cmd = makeNode(CommentStmt);
+       cmd->objtype = OBJECT_TABCONSTRAINT;
+       cmd->objname = list_make3(
+                                  makeString(get_namespace_name(RelationGetNamespace(rel))),
+                                                         makeString(RelationGetRelationName(rel)),
+                                                         makeString(conname));
+       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[pass] = lappend(tab->subcmds[pass], newcmd);
+}
+
 /*
  * Subroutine for ATPostAlterTypeParse().  Calls out to CheckIndexCompatible()
  * for the real analysis, then mutates the IndexStmt based on that verdict.

diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 868905b0c16f6240ca38f8f20d90fdd14c192460..a567c50da7279c960b4c4ca503b1b81854427cc8 100644 (file)
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1474,6 +1474,7 @@ typedef enum AlterTableType
        AT_AddConstraint,                       /* add constraint */
        AT_AddConstraintRecurse,        /* internal to commands/tablecmds.c */
        AT_ReAddConstraint,                     /* internal to commands/tablecmds.c */
+       AT_ReAddComment,                        /* internal to commands/tablecmds.c */
        AT_AlterConstraint,                     /* alter constraint */
        AT_ValidateConstraint,          /* validate constraint */
        AT_ValidateConstraintRecurse,           /* internal to commands/tablecmds.c */

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3ad2c55775c64e61e1f01f8929a04dfb95f5f1c3..6b9291b7242bcf9661739c4cb7fa506c14bbe307 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2400,6 +2400,69 @@ Check constraints:
 
 DROP TABLE alter2.tt8;
 DROP SCHEMA alter2;
+-- Check that comments on constraints and indexes are not lost at ALTER TABLE.
+CREATE TABLE comment_test (
+  id int,
+  positive_col int CHECK (positive_col > 0),
+  indexed_col int,
+  CONSTRAINT comment_test_pk PRIMARY KEY (id));
+CREATE INDEX comment_test_index ON comment_test(indexed_col);
+COMMENT ON COLUMN comment_test.id IS 'Column ''id'' on comment_test';
+COMMENT ON INDEX comment_test_index IS 'Simple index on comment_test';
+COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test IS 'CHECK constraint on comment_test.positive_col';
+COMMENT ON CONSTRAINT comment_test_pk ON comment_test IS 'PRIMARY KEY constraint of comment_test';
+COMMENT ON INDEX comment_test_pk IS 'Index backing the PRIMARY KEY of comment_test';
+SELECT col_description('comment_test'::regclass, 1) as comment;
+           comment           
+-----------------------------
+ Column 'id' on comment_test
+(1 row)
+
+SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+       index        |                    comment                    
+--------------------+-----------------------------------------------
+ comment_test_index | Simple index on comment_test
+ comment_test_pk    | Index backing the PRIMARY KEY of comment_test
+(2 rows)
+
+SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+           constraint            |                    comment                    
+---------------------------------+-----------------------------------------------
+ comment_test_pk                 | PRIMARY KEY constraint of comment_test
+ comment_test_positive_col_check | CHECK constraint on comment_test.positive_col
+(2 rows)
+
+-- Change the datatype of all the columns. ALTER TABLE is optimized to not
+-- rebuild an index if the new data type is binary compatible with the old
+-- one. Check do a dummy ALTER TABLE that doesn't change the datatype
+-- first, to test that no-op codepath, and another one that does.
+ALTER TABLE comment_test ALTER COLUMN indexed_col SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN indexed_col SET DATA TYPE text;
+ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE text;
+ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
+-- Check that the comments are intact.
+SELECT col_description('comment_test'::regclass, 1) as comment;
+           comment           
+-----------------------------
+ Column 'id' on comment_test
+(1 row)
+
+SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+       index        |                    comment                    
+--------------------+-----------------------------------------------
+ comment_test_pk    | Index backing the PRIMARY KEY of comment_test
+ comment_test_index | Simple index on comment_test
+(2 rows)
+
+SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+           constraint            |                    comment                    
+---------------------------------+-----------------------------------------------
+ comment_test_positive_col_check | CHECK constraint on comment_test.positive_col
+ comment_test_pk                 | PRIMARY KEY constraint of comment_test
+(2 rows)
+
 -- 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 29c1875d2eaec34c58be594aa18fc65330e25828..9f755fae33f73d891496e3a9fac4ccb7bf860124 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -1594,6 +1594,42 @@ ALTER TABLE IF EXISTS tt8 SET SCHEMA alter2;
 DROP TABLE alter2.tt8;
 DROP SCHEMA alter2;
 
+
+-- Check that comments on constraints and indexes are not lost at ALTER TABLE.
+CREATE TABLE comment_test (
+  id int,
+  positive_col int CHECK (positive_col > 0),
+  indexed_col int,
+  CONSTRAINT comment_test_pk PRIMARY KEY (id));
+CREATE INDEX comment_test_index ON comment_test(indexed_col);
+
+COMMENT ON COLUMN comment_test.id IS 'Column ''id'' on comment_test';
+COMMENT ON INDEX comment_test_index IS 'Simple index on comment_test';
+COMMENT ON CONSTRAINT comment_test_positive_col_check ON comment_test IS 'CHECK constraint on comment_test.positive_col';
+COMMENT ON CONSTRAINT comment_test_pk ON comment_test IS 'PRIMARY KEY constraint of comment_test';
+COMMENT ON INDEX comment_test_pk IS 'Index backing the PRIMARY KEY of comment_test';
+
+SELECT col_description('comment_test'::regclass, 1) as comment;
+SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+
+-- Change the datatype of all the columns. ALTER TABLE is optimized to not
+-- rebuild an index if the new data type is binary compatible with the old
+-- one. Check do a dummy ALTER TABLE that doesn't change the datatype
+-- first, to test that no-op codepath, and another one that does.
+ALTER TABLE comment_test ALTER COLUMN indexed_col SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN indexed_col SET DATA TYPE text;
+ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN id SET DATA TYPE text;
+ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE int;
+ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE bigint;
+
+-- Check that the comments are intact.
+SELECT col_description('comment_test'::regclass, 1) as comment;
+SELECT indexrelid::regclass as index, obj_description(indexrelid, 'pg_class') as comment FROM pg_index where indrelid = 'comment_test'::regclass;
+SELECT conname as constraint, obj_description(oid, 'pg_constraint') as comment FROM pg_constraint where conrelid = 'comment_test'::regclass;
+
+
 -- 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