On Fri, Jul 3, 2015 at 11:59 PM, Petr Jelinek wrote:
> Well for indexes you don't really need to add the new AT command, as
> IndexStmt has char *idxcomment which it will automatically uses as comment
> if not NULL. While I am not huge fan of the idxcomment it doesn't seem to
> be easy to remove it in the future and it's what transformTableLikeClause
> uses so it might be good to be consistent with that.
Oh, right, I completely missed your point and this field in IndexStmt.
Yes it is better to be consistent in this case and to use it. It makes
as well the code easier to follow.
Btw, regarding the new AT routines, I am getting find of them, it
makes easier to follow which command is added where in the command
queues.
Updated patch attached.
--
Michael
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
