Thread: 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:Interestingly, this works in 8.3 and earlier ... but it fails as
> 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
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));Interestingly, this works in 8.3 and earlier ... but it fails as
> 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
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
--
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:4467
which 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
--
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 ?+1You 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
Attachment
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