Re: 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(dot)deolasee(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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.
>
>
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 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.
>>
>>
> 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(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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(dot)deolasee(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)us>
Date: Fri May 9 23:32:05 2008 +0000

It was then removed by:
commit 61d81bd28dbec65a6b144e0cd3d0bfe25913c3ac
Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
Date: Mon Dec 5 15:10:18 2011 -0300

And then again added back by:
commit 09ff76fcdb275769ac4d1a45a67416735613d04b
Author: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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(at)gmail(dot)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(dot)deolasee(at)gmail(dot)com> writes:
> On Sat, Nov 3, 2012 at 10:21 AM, Nikhil Sontakke <nikkhils(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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

Pavan Deolasee <pavan(dot)deolasee(at)gmail(dot)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