Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
On Tue, Jul 14, 2015 at 1:42 AM, Heikki Linnakangas wrote: 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. Ouch. I had a look at the patches and they look neater than what I drafted. Thanks! 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. Well, while it's clearly a bug I don't think that it is worth the risk to destabilize back branches older than 9.5 in this code path. So +1 for doing it the way you are suggesting. We could still revisit that later on if there are more complaints, but I doubt there will be much. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
On 07/14/2015 10:29 AM, Michael Paquier wrote: On Tue, Jul 14, 2015 at 1:42 AM, Heikki Linnakangas wrote: 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. Well, while it's clearly a bug I don't think that it is worth the risk to destabilize back branches older than 9.5 in this code path. So +1 for doing it the way you are suggesting. We could still revisit that later on if there are more complaints, but I doubt there will be much. Ok, committed to master and 9.5. Thanks! - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
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
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
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. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
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,
+
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
On Thu, Jul 2, 2015 at 11:16 PM, Petr Jelinek p...@2ndquadrant.com wrote:
I was going through the code and have few comments:
- Why do you change the return value of TryReuseIndex? Can't we use reuse
the same OidIsValid(stmt-oldNode) check that ATExecAddIndex is doing
instead?
As pointed out by Heikki previously, that is actually unnecessary,
comments are still lost even if the index is reused for constraints.
So perhaps for simplicity we could just unconditionally recreate the
comments all the time if they are available.
- I think the changes to ATPostAlterTypeParse should follow more closely the
coding of transformTableLikeClause - namely use the idxcomment
I am not sure I follow here. Could you elaborate?
- Also the changes to ATPostAlterTypeParse move the code somewhat
uncomfortably over the 80 char width, I don't really see a way to fix that
except for moving some of the nesting out to another function.
Yes, I did some refactoring here by adding a set of new routines
dedicated to attach generated commands to the correct queue. This way
the code is empty of large switch/case blocks.
Update patch is attached, with the issue reported by Heikki upthread
fixed as well.
Regards,
--
Michael
From 2244608d3d06b89202b506f31b00a7d58a57f9c5 Mon Sep 17 00:00:00 2001
From: Michael Paquier michael@otacoo.com
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,
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
On 2015-07-03 15:50, Michael Paquier wrote: On Thu, Jul 2, 2015 at 11:16 PM, Petr Jelinek p...@2ndquadrant.com wrote: I was going through the code and have few comments: - Why do you change the return value of TryReuseIndex? Can't we use reuse the same OidIsValid(stmt-oldNode) check that ATExecAddIndex is doing instead? As pointed out by Heikki previously, that is actually unnecessary, comments are still lost even if the index is reused for constraints. So perhaps for simplicity we could just unconditionally recreate the comments all the time if they are available. Ah ok, I missed Heikki's email. - I think the changes to ATPostAlterTypeParse should follow more closely the coding of transformTableLikeClause - namely use the idxcomment I am not sure I follow here. Could you elaborate? 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. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [BUGS] BUG #13126: table constraint loses its comment
On 2015-05-27 15:10, Michael Paquier wrote: On Wed, Apr 29, 2015 at 1:30 PM, Michael Paquier michael.paqu...@gmail.com wrote: On Sun, Apr 26, 2015 at 6:05 AM, Tom Lane t...@sss.pgh.pa.us wrote: x...@resolvent.net writes: In some circumstances, the comment on a table constraint disappears. Here is an example: Hm, yeah. The problem is that ATExecAlterColumnType() rebuilds all the affected indexes from scratch, and it isn't doing anything about copying their comments to the new objects (either comments on the constraints, or comments directly on the indexes). The least painful way to fix it might be to charter ATPostAlterTypeCleanup to create COMMENT commands and add those to the appropriate work queue, rather than complicating the data structure initially emitted by ATExecAlterColumnType. But it'd still be a fair amount of new code I'm afraid. Not planning to fix this personally, but maybe someone else would like to take it up. In order to fix this, an idea would be to add a new routine in ruleutils.c that generates the COMMENT query string, and then call it directly from tablecmds.c. On master, I imagine that we could even add some SQL interface if there is some need. Thoughts? After looking at this problem, I noticed that the test case given above does not cover everything: primary key indexes, constraints (CHECK for example) and indexes alone have also their comments removed after ALTER TABLE when those objects are re-created. I finished with the patch attached to fix everything, patch that includes a set of regression tests covering all the code paths added. I was going through the code and have few comments: - Why do you change the return value of TryReuseIndex? Can't we use reuse the same OidIsValid(stmt-oldNode) check that ATExecAddIndex is doing instead? - I think the changes to ATPostAlterTypeParse should follow more closely the coding of transformTableLikeClause - namely use the idxcomment - Also the changes to ATPostAlterTypeParse move the code somewhat uncomfortably over the 80 char width, I don't really see a way to fix that except for moving some of the nesting out to another function. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
