Thread

1. Review of patch renaming constraints

   Josh Berkus <josh@agliodbs.com> — 2012-01-13T04:43:38Z

   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
   
   

2. Re: Review of patch renaming constraints

   Peter Eisentraut <peter_e@gmx.net> — 2012-01-19T19:04:46Z

   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.
   
   
   
   

3. Re: Review of patch renaming constraints

   Nikhils <nikkhils@gmail.com> — 2012-01-20T03:38:33Z

   > > 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
   

4. Re: Review of patch renaming constraints

   Peter Eisentraut <peter_e@gmx.net> — 2012-01-20T05:09:24Z

   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.
   
   
   

5. Re: Review of patch renaming constraints

   Nikhils <nikkhils@gmail.com> — 2012-01-20T06:02:25Z

   > > 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
   

6. Re: Review of patch renaming constraints

   Peter Eisentraut <peter_e@gmx.net> — 2012-01-21T22:13:55Z

   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.
   
   
   

7. Re: Review of patch renaming constraints

   Peter Eisentraut <peter_e@gmx.net> — 2012-02-01T20:32:29Z

   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.
   
   

8. Re: Review of patch renaming constraints

   Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2012-03-09T10:43:59Z

   Hi,
   
   Peter Eisentraut <peter_e@gmx.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