Re: Review of patch renaming constraints

Compiling on Ubuntu 10.04 LTS AMD64 on a GoGrid virtual machine from 2012-01-12 git checkout.

Patch applied fine.

Docs are present, build, look good and are clear.

Changes to gram.y required Bison 2.5 to compile. Are we requiring Bison 2.5 now? There's no configure check for it, so it took me quite a while to figure out what was wrong.

Make check passed. Patch has tests for rename constraint.

Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints:

create table master ( category text not null, status int not null, value text );

alter table master add constraint master_key primary key ( category, status );

alter table master rename constraint master_key to master_primary_key;

create table partition_1 () inherits ( master );

create table partition_2 () inherits ( master );

alter table master rename constraint master_primary_key to master_key;

postgres=# alter table master rename constraint master_primary_key to master_key;
ERROR: constraint "master_primary_key" for table "partition_1" does not exist
STATEMENT: alter table master rename constraint master_primary_key to master_key;
ERROR: constraint "master_primary_key" for table "partition_1" does not exist

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
San Francisco

On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote:
> Compiling on Ubuntu 10.04 LTS AMD64 on a GoGrid virtual machine from 2012-01-12 git checkout.
>
> Patch applied fine.
>
> Docs are present, build, look good and are clear.
>
> Changes to gram.y required Bison 2.5 to compile. Are we requiring Bison 2.5 now? There's no configure check for it, so it took me quite a while to figure out what was wrong.

I can't reproduce that. I think there might be something wrong with
your installation. The same issue was reported for my COLLATION FOR
patch from the same environment.

> Make check passed. Patch has tests for rename constraint.
>
> Most normal uses of alter table ... rename constraint ... worked normally. However, the patch does not deal correctly with constraints which are not inherited, such as primary key constraints:

That appears to be because creating a primary key constraint does not
set pg_constraint.conisonly correctly. This was introduced recently
with noninherited check constraints.

> > Make check passed. Patch has tests for rename constraint.
> >
> > Most normal uses of alter table ... rename constraint ... worked
> normally. However, the patch does not deal correctly with constraints
> which are not inherited, such as primary key constraints:
>
> That appears to be because creating a primary key constraint does not
> set pg_constraint.conisonly correctly. This was introduced recently
> with noninherited check constraints.
>
>
> Umm, conisonly is set as false from primary key entries in pg_constraint.
And primary keys are anyways not inherited. So why is the conisonly field
interfering in rename? Seems quite orthogonal to me.

Regards,
Nikhils

On fre, 2012-01-20 at 09:08 +0530, Nikhil Sontakke wrote:
> > Umm, conisonly is set as false from primary key entries in
> pg_constraint.
> And primary keys are anyways not inherited. So why is the conisonly
> field interfering in rename? Seems quite orthogonal to me.

In the past, each kind of constraint was either always inherited or
always not, implicitly. Now, for check constraints we can choose what
we want, and in the future, perhaps we will want to choose for primary
keys as well. So having conisonly is really a good step into that
future, and we should use it uniformly.

> > And primary keys are anyways not inherited. So why is the conisonly
> > field interfering in rename? Seems quite orthogonal to me.
>
> In the past, each kind of constraint was either always inherited or
> always not, implicitly. Now, for check constraints we can choose what
> we want, and in the future, perhaps we will want to choose for primary
> keys as well. So having conisonly is really a good step into that
> future, and we should use it uniformly.
>
>
Agreed. And right now primary key constraints are not marked as only making
them available for inheritance in the future. Or you prefer it otherwise?

Anyways, fail to see the direct connection between this and renaming. Might
have to look at this patch for that.

Regards,
Nikhils

On fre, 2012-01-20 at 11:32 +0530, Nikhil Sontakke wrote:
> Agreed. And right now primary key constraints are not marked as only
> making them available for inheritance in the future. Or you prefer it
> otherwise?
>
> Anyways, fail to see the direct connection between this and renaming.
> Might have to look at this patch for that.

It checks conisonly to determine whether it needs to rename the
constraint in child tables as well. Since a primary has conisonly =
false, it goes to the child tables, but the constraint it not there.

In the past, we have treated this merely as an implementation artifact:
check constraints are inherited, primary key constraints are not. Now
we can choose for check constraints, with inherited being the default.
Having inheritable primary key constraints is a possible future feature.
So we need to think a littler harder now how to work that into the
existing logic. This also ties in with the other thread about having
this in CREATE TABLE syntax.

On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote:
> Most normal uses of alter table ... rename constraint ... worked
> normally. However, the patch does not deal correctly with constraints
> which are not inherited, such as primary key constraints:

New patch which works around that issue.

Hi,

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On tor, 2012-01-12 at 22:43 -0600, Joshua Berkus wrote:
>> Most normal uses of alter table ... rename constraint ... worked
>> normally. However, the patch does not deal correctly with constraints
>> which are not inherited, such as primary key constraints:
>
> New patch which works around that issue.

I've been reviewing this new patch and it seems ready for commiter for
me: the code indeed looks like it's always been there, the corner cases
are covered in the added regression tests, including the one Josh ran
into problem with in the previous round of testing.

The regression test covering made me lazy enough not to retry the patch
here, I trust Peter on testing his own work here :)

I'll update my command trigger patch as soon as this one makes it in.

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support