Bug in ALTER COLUMN SET DATA TYPE ?
Hi,
I noticed this behavior on master and it seems like a bug to me:
postgres=# CREATE TABLE test (a float check (a > 10.2));
CREATE TABLE
postgres=# CREATE TABLE test_child() INHERITS(test);
CREATE TABLE
postgres=# ALTER TABLE test ALTER COLUMN a SET DATA TYPE numeric;
ERROR: constraint must be added to child
tables too
The error message seems unintended. Sure, we are changing the data type of
a column which has a constraint on it. The ALTER TABLE mechanism would drop
and recreate that constraint after changing the data type of the column.
The ATAddCheckConstraint() function checks if the "recurse" flag is passed
(basically check against ONLY clause). If the flag is not passed and the
table has children, it raises the above mentioned exception. This is right
for a normal ADD CONSTRAINT operation because we don't want to allow
constraint addition ONLY on the parent table unless there are no child
tables currently on the parent. But when constraint is being re-added as a
side-effect of another ALTER TABLE command, we shouldn't really be raising
an exception because ATPrepCmd() would have expanded to child tables and
there would appropriate commands in the work queue to recreate constraints
on all the child tables as well.
So I think we need to teach ATAddCheckConstraint() to not do this check if
its being called from AT_PASS_OLD_INDEX of ALTER TABLE.
I can work up a patch if we are in agreement that this indeed is a bug and
the approach that I mentioned for fixing this is also right. Comments ?
Thanks,
Pavan
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
I noticed this behavior on master and it seems like a bug to me:
postgres=# CREATE TABLE test (a float check (a > 10.2));
CREATE TABLE
postgres=# CREATE TABLE test_child() INHERITS(test);
CREATE TABLE
postgres=# ALTER TABLE test ALTER COLUMN a SET DATA TYPE numeric;
ERROR: constraint must be added to child tables too
Interestingly, this works in 8.3 and earlier ... but it fails as
described in all later versions. So we broke it quite some time back.
It would be good to identify exactly what change broke it.
regards, tom lane
On Fri, Nov 2, 2012 at 8:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
I noticed this behavior on master and it seems like a bug to me:
postgres=# CREATE TABLE test (a float check (a > 10.2));
CREATE TABLE
postgres=# CREATE TABLE test_child() INHERITS(test);
CREATE TABLE
postgres=# ALTER TABLE test ALTER COLUMN a SET DATA TYPE numeric;
ERROR: constraint must be added to child tables tooInterestingly, this works in 8.3 and earlier ... but it fails as
described in all later versions. So we broke it quite some time back.
It would be good to identify exactly what change broke it.
Hmm.. I haven't yet tested on other branches. But I'm surprised that you
find it broken on so much old branches. "git annotate" suggests that the
offending error message was added by commit
09ff76fcdb275769ac4d1a45a67416735613d04b and that commit is dated Apr 20,
2012
Thanks,
Pavan
postgres=# CREATE TABLE test (a float check (a > 10.2));
CREATE TABLE
postgres=# CREATE TABLE test_child() INHERITS(test);
CREATE TABLE
postgres=# ALTER TABLE test ALTER COLUMN a SET DATA TYPE numeric;
ERROR: constraint must be added to child tables tooInterestingly, this works in 8.3 and earlier ... but it fails as
described in all later versions. So we broke it quite some time back.
It would be good to identify exactly what change broke it.Hmm.. I haven't yet tested on other branches. But I'm surprised that you
find it broken on so much old branches. "git annotate" suggests that the
offending error message was added by commit
09ff76fcdb275769ac4d1a45a67416735613d04b and that commit is dated Apr 20,
2012
Mea Culpa. I did indeed add that error message. But it's strange when Tom
says that this is broken since 8.3!
Regards,
Nikhils
--
StormDB - http://www.stormdb.com
The Database Cloud
Postgres-XC Support and Service
Nikhil Sontakke <nikkhils@gmail.com> writes:
Hmm.. I haven't yet tested on other branches. But I'm surprised that you
find it broken on so much old branches. "git annotate" suggests that the
offending error message was added by commit
09ff76fcdb275769ac4d1a45a67416735613d04b and that commit is dated Apr 20,
2012
Mea Culpa. I did indeed add that error message. But it's strange when Tom
says that this is broken since 8.3!
In the 8.4 branch, the error is coming from here:
regression=# \set VERBOSITY verbose
regression=# ALTER TABLE test ALTER COLUMN a TYPE numeric;
ERROR: 42P16: constraint must be added to child tables too
LOCATION: ATAddCheckConstraint, tablecmds.c:4467
which appears to have been added in commit cd902b331. I think the
commit Pavan is looking at just moved the logic around.
regards, tom lane
On Fri, Nov 2, 2012 at 9:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
In the 8.4 branch, the error is coming from here:
regression=# \set VERBOSITY verbose
regression=# ALTER TABLE test ALTER COLUMN a TYPE numeric;
ERROR: 42P16: constraint must be added to child tables too
LOCATION: ATAddCheckConstraint, tablecmds.c:4467which appears to have been added in commit cd902b331. I think the
commit Pavan is looking at just moved the logic around.
I also though that for a moment, but the commit that I mentioned did not
move that error message from somewhere else. I also tested by resetting
to commit 1f0363001166ef6a43619846e44cfb9dbe7335ed (previous to the
offending commit) and don't see any error.
But since you're seeing it, may it was taken out by some other older commit
and then added back again. Will look more into it
Thanks,
Pavan
On Fri, Nov 2, 2012 at 9:09 PM, Pavan Deolasee <pavan.deolasee@gmail.com>wrote:
I also though that for a moment, but the commit that I mentioned did not
move that error message from somewhere else. I also tested by resetting
to commit 1f0363001166ef6a43619846e44cfb9dbe7335ed (previous to the
offending commit) and don't see any error.But since you're seeing it, may it was taken out by some other older
commit and then added back again. Will look more into it
Here is more trace:
The error message was first added by:
commit cd902b331dc4b0c170e800441a98f9213d98b46b
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Fri May 9 23:32:05 2008 +0000
It was then removed by:
commit 61d81bd28dbec65a6b144e0cd3d0bfe25913c3ac
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Mon Dec 5 15:10:18 2011 -0300
And then again added back by:
commit 09ff76fcdb275769ac4d1a45a67416735613d04b
Author: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri Apr 20 23:46:20 2012 -0300
So coming back to the issue, do you think it's a good idea to teach
ATAddCheckConstraint() that the call is coming from a late phase of ALTER
TABLE ?
Thanks,
Pavan
So coming back to the issue, do you think it's a good idea to teach
ATAddCheckConstraint() that the call is coming from a late phase of ALTER
TABLE ?+1
You mentioned AT_PASS_OLD_INDEX in your original mail, but I guess that you
meant that we should check for AT_PASS_OLD_CONSTR and then not raise an
error?
Regards,
Nikhils
--
StormDB - http://www.stormdb.com
The Database Cloud
Postgres-XC Support and Service
On Sat, Nov 3, 2012 at 10:21 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
So coming back to the issue, do you think it's a good idea to teach
ATAddCheckConstraint() that the call is coming from a late phase of ALTER
TABLE ?+1
You mentioned AT_PASS_OLD_INDEX in your original mail, but I guess that
you meant that we should check for AT_PASS_OLD_CONSTR and then not raise
an error?
Yeah, I meant AT_PASS_OLD_CONSTR.
Thanks,
Pavan
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
On Sat, Nov 3, 2012 at 10:21 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
So coming back to the issue, do you think it's a good idea to teach
ATAddCheckConstraint() that the call is coming from a late phase of ALTER
TABLE ?
+1
You mentioned AT_PASS_OLD_INDEX in your original mail, but I guess that
you meant that we should check for AT_PASS_OLD_CONSTR and then not raise
an error?
Yeah, I meant AT_PASS_OLD_CONSTR.
It's clear that we need to pass down the information that this action is
coming from re-creation of a check constraint, but I think the above
proposal for how to do it is pretty wrong-headed. The pass structure
has never meant anything for semantics of individual AlterTable actions;
it's only used to make sure those actions are done in an appropriate
order. Furthermore, the pass number isn't available where it would be
needed --- you'd have to pass it down through several levels of
subroutine that don't have another reason to care about it.
I'm inclined to think the cleanest solution is to add another value of
enum AlterTableType, perhaps "AT_ReAddConstraint", to signal that we're
executing a re-add; and then add another bool parameter to
ATExecAddConstraint to tell the latter not to complain if child tables
exist. This is more in line with pre-existing coding choices such as
the use of AT_AddConstraintRecurse.
regards, tom lane
On Mon, Nov 5, 2012 at 4:41 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
It's clear that we need to pass down the information that this action is
coming from re-creation of a check constraint, but I think the above
proposal for how to do it is pretty wrong-headed.
Yeah, I only meant that we need to teach ATExecAddConstraint that its being
called from the specific pass of ALTER TABLE and wanted to get agreement on
that. I hadn't thought about any particular implementation. So your
proposal below looks absolutely fine and clean.
I'm inclined to think the cleanest solution is to add another value of
enum AlterTableType, perhaps "AT_ReAddConstraint", to signal that we're
executing a re-add; and then add another bool parameter to
ATExecAddConstraint to tell the latter not to complain if child tables
exist. This is more in line with pre-existing coding choices such as
the use of AT_AddConstraintRecurse.
Please see attached patch which does what you suggested above. May be it
needs a little more commentary to record why we made this specific change.
Please let me know if you think so and want me to do that.
Thanks,
Pavan
Attachments:
alter-type-readd-constraint.patchapplication/octet-stream; name=alter-type-readd-constraint.patchDownload
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 359d478..50c93fe 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -335,13 +335,15 @@ static void ATExecAddIndex(AlteredTableInfo *tab, Relation rel,
IndexStmt *stmt, bool is_rebuild, LOCKMODE lockmode);
static void ATExecAddConstraint(List **wqueue,
AlteredTableInfo *tab, Relation rel,
- Constraint *newConstraint, bool recurse, LOCKMODE lockmode);
+ Constraint *newConstraint, bool recurse, bool check_no_children,
+ LOCKMODE lockmode);
static void ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
IndexStmt *stmt, LOCKMODE lockmode);
static void ATAddCheckConstraint(List **wqueue,
AlteredTableInfo *tab, Relation rel,
Constraint *constr,
- bool recurse, bool recursing, LOCKMODE lockmode);
+ bool recurse, bool recursing, bool check_no_children,
+ LOCKMODE lockmode);
static void ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
Constraint *fkconstraint, LOCKMODE lockmode);
static void ATExecDropConstraint(Relation rel, const char *constrName,
@@ -3248,11 +3250,15 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
break;
case AT_AddConstraint: /* ADD CONSTRAINT */
ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
- false, lockmode);
+ false, true, lockmode);
+ break;
+ case AT_ReAddConstraint: /* Re-add previously dropped CONSTRAINT */
+ ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
+ false, false, lockmode);
break;
case AT_AddConstraintRecurse: /* ADD CONSTRAINT with recursion */
ATExecAddConstraint(wqueue, tab, rel, (Constraint *) cmd->def,
- true, lockmode);
+ true, true, lockmode);
break;
case AT_AddIndexConstraint: /* ADD CONSTRAINT USING INDEX */
ATExecAddIndexConstraint(tab, rel, (IndexStmt *) cmd->def, lockmode);
@@ -5499,7 +5505,8 @@ ATExecAddIndexConstraint(AlteredTableInfo *tab, Relation rel,
*/
static void
ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
- Constraint *newConstraint, bool recurse, LOCKMODE lockmode)
+ Constraint *newConstraint, bool recurse, bool check_no_children,
+ LOCKMODE lockmode)
{
Assert(IsA(newConstraint, Constraint));
@@ -5512,7 +5519,8 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
{
case CONSTR_CHECK:
ATAddCheckConstraint(wqueue, tab, rel,
- newConstraint, recurse, false, lockmode);
+ newConstraint, recurse, false, check_no_children,
+ lockmode);
break;
case CONSTR_FOREIGN:
@@ -5568,7 +5576,7 @@ ATExecAddConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
static void
ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
Constraint *constr, bool recurse, bool recursing,
- LOCKMODE lockmode)
+ bool check_no_children, LOCKMODE lockmode)
{
List *newcons;
ListCell *lcon;
@@ -5646,14 +5654,24 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
children = find_inheritance_children(RelationGetRelid(rel), lockmode);
/*
- * Check if ONLY was specified with ALTER TABLE. If so, allow the
- * contraint creation only if there are no children currently. Error out
- * otherwise.
+ * Check if either ONLY was specified with ALTER TABLE or if we are adding
+ * back a constraint in AT_PASS_OLD_CONSTR pass, that was previously
+ * dropped constraint. We don't recurse in both the cases.
*/
- if (!recurse && children != NIL)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be added to child tables too")));
+ if (!recurse)
+ {
+ /*
+ * We have been told not to recurse. Unless we have been explicitly
+ * told not to check for exitsing children, error out if there are
+ * existing children
+ */
+ if (check_no_children && children != NIL)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("constraint must be added to child tables too")));
+ else
+ return;
+ }
foreach(child, children)
{
@@ -5670,7 +5688,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* Recurse to child */
ATAddCheckConstraint(wqueue, childtab, childrel,
- constr, recurse, true, lockmode);
+ constr, recurse, true, true, lockmode);
heap_close(childrel, NoLock);
}
@@ -7914,6 +7932,7 @@ ATPostAlterTypeParse(Oid oldId, char *cmd,
case AT_AddConstraint:
Assert(IsA(cmd->def, Constraint));
con = (Constraint *) cmd->def;
+ cmd->subtype = AT_ReAddConstraint;
/* rewriting neither side of a FK */
if (con->contype == CONSTR_FOREIGN &&
!rewrite && !tab->rewrite)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 09b15e7..b58fa82 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1196,6 +1196,7 @@ typedef enum AlterTableType
AT_AddIndex, /* add index */
AT_ReAddIndex, /* internal to commands/tablecmds.c */
AT_AddConstraint, /* add constraint */
+ AT_ReAddConstraint, /* re-add previously dropped constraint */
AT_AddConstraintRecurse, /* internal to commands/tablecmds.c */
AT_ValidateConstraint, /* validate constraint */
AT_ValidateConstraintRecurse, /* internal to commands/tablecmds.c */
Pavan Deolasee <pavan.deolasee@gmail.com> writes:
Please see attached patch which does what you suggested above. May be it
needs a little more commentary to record why we made this specific change.
Please let me know if you think so and want me to do that.
Applied with some cosmetic adjustments and addition of a regression
test.
regards, tom lane