From 2244608d3d06b89202b506f31b00a7d58a57f9c5 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Fri, 3 Jul 2015 22:47:22 +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 | 287 ++++++++++++++++++++++-------- 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, 346 insertions(+), 74 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d394713..78e6b5c 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,12 @@ 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 RebuildObjectComment(AlteredTableInfo *tab, + int cmdidx, + ObjectType objtype, + Oid objid, + Oid classoid, + 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 +3511,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 +4267,162 @@ 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); + + /* create COMMENT entry for new index */ + RebuildObjectComment(tab, + AT_PASS_OLD_INDEX, + OBJECT_INDEX, + oldId, RelationRelationId, + list_make1(makeString(stmt->idxname))); +} + +/* + * 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 constraint comment... */ + RebuildObjectComment(tab, + AT_PASS_OLD_INDEX, + OBJECT_TABCONSTRAINT, + oldId, + ConstraintRelationId, + objnames); + + /* ... And the comment on the index itself */ + RebuildObjectComment(tab, + AT_PASS_OLD_INDEX, + OBJECT_INDEX, + indoid, + RelationRelationId, + list_make1(makeString(get_rel_name(indoid)))); + } + 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 new constraint */ + objnames = list_make2( + makeString(get_rel_name(tab->relid)), + makeString(con->conname)); + RebuildObjectComment(tab, + AT_PASS_OLD_CONSTR, + OBJECT_TABCONSTRAINT, + oldId, + ConstraintRelationId, + objnames); + } + break; + default: + elog(ERROR, "unexpected statement type: %d", + (int) cmd->subtype); + } + } +} + /* * ATSimplePermissions * @@ -8633,84 +8805,51 @@ 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 + * an index or a constraint that is being re-added. + */ +static void +RebuildObjectComment(AlteredTableInfo *tab, + int cmdidx, + ObjectType objtype, + Oid objid, + Oid classoid, + List *objname) +{ + CommentStmt *cmd; + char *comment_str; + AlterTableCmd *newcmd; + + Assert(objtype == OBJECT_INDEX || + objtype == OBJECT_TABCONSTRAINT); + + /* Look for comment for object wanted, and leave if none */ + comment_str = GetComment(objid, classoid, 0); + if (comment_str == NULL) + return; - relation_close(rel, NoLock); + /* Build node CommentStmt */ + cmd = makeNode(CommentStmt); + cmd->objtype = objtype; + 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