On 07/08/2015 08:12 AM, Michael Paquier wrote:
On Tue, Jul 7, 2015 at 6:24 PM, Petr Jelinek <p...@2ndquadrant.com> wrote:
On 2015-07-04 13:45, Michael Paquier wrote:
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.
Cool, I am happy with the patch now. Marking as ready for committer.
Thanks for the review.
I don't much like splitting the code across multiple helper functions,
it makes the overall logic more difficult to follow, although I agree
that the indentation has made the pretty hard to read as it is. I'm
planning to commit the attached two patches. The first one is just
reformatting changes to ATExecAlterColumnType(), turning the switch-case
statements into if-else blocks. If-else doesn't cause so much
indentation, and allows defining variables local to the "cases" more
naturally, so it alleviates the indentation problem somewhat. The second
patch is the actual bug fix.
There was one bug in this patch: the COMMENT statement that was
constructed didn't schema-qualify the relation, so if the ALTERed table
was not in search_path, the operation would fail with a "relation not
found" error (or add the comment to wrong object). Fixed that.
I plan to commit the attached patches later today or tomorrow. But how
do you feel about back-patching this? It should be possible to
backpatch, although at a quick test it seems that there have been small
changes to the affected code in many versions, so it would require some
work. Also, in back-branches we'd need to put the new AT_ReAddComment
type to the end of the list, like we've done when we added
AT_ReAddConstraint in 9.3. I'm inclined to backpatch this to 9.5 only,
even though this is clearly a bug fix, on the grounds that it's not a
very serious bug and there's always some risk in backpatching.
- Heikki
>From c4865eb873a9cafb7e247cc69b7030243b74f3be Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 13 Jul 2015 19:22:31 +0300
Subject: [PATCH 1/2] Reformat code in ATPostAlterTypeParse.
The code in ATPostAlterTypeParse was very deeply indented, mostly because
there were two nested switch-case statements, which add a lot of
indentation. Use if-else blocks instead, to make the code less indented
and more readable.
This is in preparation for next patch that makes some actualy changes to
the function. These cosmetic parts have been separated to make it easier
to see the real changes in the other patch.
---
src/backend/commands/tablecmds.c | 104 +++++++++++++++++++--------------------
1 file changed, 51 insertions(+), 53 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index d394713..e7b23f1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8645,69 +8645,67 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
Node *stm = (Node *) lfirst(list_item);
AlteredTableInfo *tab;
- switch (nodeTag(stm))
+ tab = ATGetQueueEntry(wqueue, rel);
+
+ if (IsA(stm, IndexStmt))
+ {
+ IndexStmt *stmt = (IndexStmt *) stm;
+ AlterTableCmd *newcmd;
+
+ if (!rewrite)
+ TryReuseIndex(oldId, stmt);
+
+ 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);
+ }
+ else if (IsA(stm, AlterTableStmt))
{
- case T_IndexStmt:
+ AlterTableStmt *stmt = (AlterTableStmt *) stm;
+ ListCell *lcmd;
+
+ foreach(lcmd, stmt->cmds)
+ {
+ AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+
+ if (cmd->subtype == AT_AddIndex)
{
- IndexStmt *stmt = (IndexStmt *) stm;
- AlterTableCmd *newcmd;
+ Assert(IsA(cmd->def, IndexStmt));
if (!rewrite)
- TryReuseIndex(oldId, stmt);
+ TryReuseIndex(get_constraint_index(oldId),
+ (IndexStmt *) cmd->def);
- tab = ATGetQueueEntry(wqueue, rel);
- newcmd = makeNode(AlterTableCmd);
- newcmd->subtype = AT_ReAddIndex;
- newcmd->def = (Node *) stmt;
+ cmd->subtype = AT_ReAddIndex;
tab->subcmds[AT_PASS_OLD_INDEX] =
- lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
- break;
+ lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
}
- case T_AlterTableStmt:
+ else if (cmd->subtype == AT_AddConstraint)
{
- AlterTableStmt *stmt = (AlterTableStmt *) stm;
- ListCell *lcmd;
-
- tab = ATGetQueueEntry(wqueue, rel);
- foreach(lcmd, stmt->cmds)
- {
- AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
- Constraint *con;
-
- 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;
+ Constraint *con;
+
+ 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);
}
- default:
- elog(ERROR, "unexpected statement type: %d",
- (int) nodeTag(stm));
+ else
+ elog(ERROR, "unexpected statement type: %d",
+ (int) cmd->subtype);
+ }
}
+ else
+ elog(ERROR, "unexpected statement type: %d",
+ (int) nodeTag(stm));
}
relation_close(rel, NoLock);
--
2.1.4
>From 9a50ee7f08107c7e8499c0a07034dc8104de55d6 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakan...@iki.fi>
Date: Mon, 13 Jul 2015 19:22:35 +0300
Subject: [PATCH 2/2] 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.
Original patch by Michael Paquier, reviewed by Petr Jelinek and me. This
fixes bug #13126, reported by Kirill Simonov.
---
src/backend/commands/tablecmds.c | 65 ++++++++++++++++++++++++++++++-
src/include/nodes/parsenodes.h | 1 +
src/test/regress/expected/alter_table.out | 63 ++++++++++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 36 +++++++++++++++++
4 files changed, 163 insertions(+), 2 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index e7b23f1..1c7eded 100644
--- 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",
@@ -8712,6 +8739,40 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
}
/*
+ * 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 868905b..a567c50 100644
--- 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 3ad2c55..6b9291b 100644
--- 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 29c1875..9f755fa 100644
--- 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
--
2.1.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
