From 5953d4008d23d6e75ec8b55545b0a707e06b349e Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Wed, 27 May 2015 22:01:24 +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 | 149 ++++++++++++++++++++++++++---- src/include/nodes/parsenodes.h | 1 + src/test/regress/expected/alter_table.out | 66 +++++++++++++ src/test/regress/sql/alter_table.sql | 27 ++++++ 4 files changed, 223 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 84dbee0..0dd909d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -386,7 +386,13 @@ 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 TryReuseIndex(Oid oldId, IndexStmt *stmt); +static void RebuildObjectComment(AlteredTableInfo *tab, + int cmdidx, + ObjectType objtype, + Oid objid, + Oid classoid, + List *objname); +static bool TryReuseIndex(Oid oldId, IndexStmt *stmt); static void TryReuseForeignKey(Oid oldId, Constraint *con); static void change_owner_fix_column_acls(Oid relationOid, Oid oldOwnerId, Oid newOwnerId); @@ -3498,6 +3504,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, @@ -8636,9 +8645,10 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, { IndexStmt *stmt = (IndexStmt *) stm; AlterTableCmd *newcmd; + bool reused = false; if (!rewrite) - TryReuseIndex(oldId, stmt); + reused = TryReuseIndex(oldId, stmt); tab = ATGetQueueEntry(wqueue, rel); newcmd = makeNode(AlterTableCmd); @@ -8646,6 +8656,14 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, newcmd->def = (Node *) stmt; tab->subcmds[AT_PASS_OLD_INDEX] = lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd); + + /* create COMMENT entry for new index */ + if (!reused) + RebuildObjectComment(tab, + AT_PASS_OLD_INDEX, + OBJECT_INDEX, + oldId, RelationRelationId, + list_make1(makeString(stmt->idxname))); break; } case T_AlterTableStmt: @@ -8662,25 +8680,72 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, 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); + { + bool reused = false; + IndexStmt *stmt = (IndexStmt *) cmd->def; + Oid indoid = get_constraint_index(oldId); + + Assert(IsA(cmd->def, IndexStmt)); + if (!rewrite) + reused = 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 */ + if (!reused) + { + List *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: - 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); + { + bool reuse = false; + 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); + reuse = true; + } + cmd->subtype = AT_ReAddConstraint; + tab->subcmds[AT_PASS_OLD_CONSTR] = + lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd); + + /* create COMMENT entry for new constraint */ + if (!reuse) + { + List *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", @@ -8699,12 +8764,54 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd, } /* + * 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; + + /* 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); +} + +/* * Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible() * for the real analysis, then mutates the IndexStmt based on that verdict. + * + * Returns true if the index is reused, and false otherwise. */ -static void +static bool TryReuseIndex(Oid oldId, IndexStmt *stmt) { + bool res = false; + if (CheckIndexCompatible(oldId, stmt->accessMethod, stmt->indexParams, @@ -8714,7 +8821,9 @@ TryReuseIndex(Oid oldId, IndexStmt *stmt) stmt->oldNode = irel->rd_node.relNode; index_close(irel, NoLock); + res = true; } + return res; } /* 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 65274bc..8fb3fe5 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2397,6 +2397,72 @@ Check constraints: DROP TABLE alter2.tt8; DROP SCHEMA alter2; +-- Check comment persistency across ALTER TABLE commands for constraints +-- and indexes +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 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 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 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 b5ee7b0..448eebe 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -1593,6 +1593,33 @@ 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 +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 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 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 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.1