Re: [HACKERS] Review: Non-inheritable check constraints

2012-01-16 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of vie dic 23 01:02:10 -0300 2011: FWIW, here's a quick fix for the issue that Robert pointed out. Thanks, applied. Again it's a variation of the first issue that he reported. We have two functions which try to merge existing constraints:

Re: [HACKERS] Review: Non-inheritable check constraints

2012-01-16 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of vie dic 23 01:02:10 -0300 2011: I have also tried to change the error message as per Alvaro's suggestions. I will really try to see if we have other issues. Really cannot have Robert reporting all the bugs and getting annoyed, but there are lotsa

Re: [HACKERS] Review: Non-inheritable check constraints

2012-01-16 Thread Nikhil Sontakke
I have also tried to change the error message as per Alvaro's suggestions. I will really try to see if we have other issues. Really cannot have Robert reporting all the bugs and getting annoyed, but there are lotsa variations possible with inheritance.. So did you find anything?

Re: [HACKERS] Review: Non-inheritable check constraints

2012-01-16 Thread Nikhil Sontakke
I will really try to see if we have other issues. Really cannot have Robert reporting all the bugs and getting annoyed, but there are lotsa variations possible with inheritance.. BTW thank you for doing the work on this. Probably the reason no one bothers too much with inheritance is

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-26 Thread Nikhil Sontakke
I don't think this is a given ... In fact, IMO if we're only two or three fixes away from having it all nice and consistent, I think reverting is not necessary. Sure. It's the if part of that sentence that I'm not too sure about. Any specific area of the code that you think is/has

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-23 Thread Robert Haas
On Thu, Dec 22, 2011 at 10:54 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: I agree with Robert that this usage of ALTER TABLE ONLY is slightly different from other usages of the same command, but I disagree that this means that we need another command to do what we want to do here.

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of mar dic 20 12:03:33 -0300 2011: Apologies, I did not check this particular scenario. I guess, here, we should not allow merging of the inherited constraint into an only constraint. Because that breaks the semantics for only constraints. If

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Robert Haas
On Thu, Dec 22, 2011 at 2:43 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Nikhil Sontakke's message of mar dic 20 12:03:33 -0300 2011: Apologies, I did not check this particular scenario. I guess, here, we should not allow merging of the inherited constraint into an

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Nikhil Sontakke
Hi, There is at least one other problem. Consider: rhaas=# create table a (ff1 int, constraint chk check (ff1 0)); CREATE TABLE rhaas=# create table b (ff1 int, constraint chk check (ff1 0)); CREATE TABLE rhaas=# alter table b inherit a; ALTER TABLE This needs to fail if chk is an

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Nikhil Sontakke
And yeah, certainly there's a bug as you point out. postgres=# create table a1 (ff1 int, constraint chk check (ff1 0)); postgres=# create table b1 (ff1 int); postgres=# alter table only b1 add constraint chk check (ff1 0); postgres=# alter table b1 inherit a1; The last command should have

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of vie dic 23 00:25:26 -0300 2011: Hi, There is at least one other problem. Consider: rhaas=# create table a (ff1 int, constraint chk check (ff1 0)); CREATE TABLE rhaas=# create table b (ff1 int, constraint chk check (ff1 0)); CREATE

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-22 Thread Nikhil Sontakke
I don't think this is a given ... In fact, IMO if we're only two or three fixes away from having it all nice and consistent, I think reverting is not necessary. FWIW, here's a quick fix for the issue that Robert pointed out. Again it's a variation of the first issue that he reported. We have

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-20 Thread Robert Haas
On Tue, Dec 20, 2011 at 1:14 AM, Nikhil Sontakke nikkh...@gmail.com wrote: Agreed. I just tried out the scenarios laid out by you both with and without the committed patch and AFAICS, normal inheritance semantics have been preserved properly even after the commit. No, they haven't. I didn't

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-20 Thread Nikhil Sontakke
Agreed. I just tried out the scenarios laid out by you both with and without the committed patch and AFAICS, normal inheritance semantics have been preserved properly even after the commit. No, they haven't. I didn't expect this to break anything when you have two constraints with

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-20 Thread Nikhil Sontakke
rhaas=# create table A(ff1 int); CREATE TABLE rhaas=# create table B () inherits (A); CREATE TABLE rhaas=# create table C () inherits (B); CREATE TABLE rhaas=# alter table only b add constraint chk check (ff1 0); ALTER TABLE rhaas=# alter table a add constraint chk check (ff1 0);

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Robert Haas
On Fri, Dec 16, 2011 at 2:06 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Yeah.  Nikhil, Alex, this is the merged patch.  Have a look that it still works for you (particularly the pg_dump bits) and I'll commit it. I adjusted the regression test a bit too. It seems hard to believe that

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Nikhil Sontakke
It seems hard to believe that ATExecDropConstraint() doesn't need any adjustment. Yeah, I think we could return early on for only type of constraints. Also, shouldn't the systable scan break out of the while loop after a matching constraint name has been found? As of now, it's doing a full

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Alvaro Herrera
Excerpts from Alex Hunsaker's message of vie dic 16 18:07:05 -0300 2011: On Fri, Dec 16, 2011 at 14:01, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011: On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Robert Haas
On Mon, Dec 19, 2011 at 12:07 PM, Nikhil Sontakke nikkh...@gmail.com wrote: It seems hard to believe that ATExecDropConstraint() doesn't need any adjustment. Yeah, I think we could return early on for only type of constraints. It's not just that. Suppose that C inherits from B which inherits

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of lun dic 19 14:07:02 -0300 2011: PFA, revised version containing the above changes based on Alvaro's v4 patch. Committed with these fixes, and with my fix for the pg_dump issue noted by Alex, too. Thanks. -- Álvaro Herrera

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Nikhil Sontakke
PFA, revised version containing the above changes based on Alvaro's v4 patch. Committed with these fixes, and with my fix for the pg_dump issue noted by Alex, too. Thanks. Thanks a lot Alvaro! Regards, Nikhils -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company -

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Alvaro Herrera
Excerpts from Nikhil Sontakke's message of lun dic 19 22:32:57 -0300 2011: PFA, revised version containing the above changes based on Alvaro's v4 patch. Committed with these fixes, and with my fix for the pg_dump issue noted by Alex, too. Thanks. Thanks a lot Alvaro! You're

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Nikhil Sontakke
But did you see Robert Haas' response upthread? It looks like there's still some work to do -- at least some research to determine that we're correctly handling all those cases. As the committer I'm responsible for it, but I don't have resources to get into it any time soon. Yeah, am

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-19 Thread Nikhil Sontakke
Hi Robert, First of all, let me state that this ONLY feature has not messed around with existing inheritance semantics. It allows attaching a constraint to any table (which can be part of any hierarchy) without the possibility of it ever playing any part in future or existing inheritance

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-17 Thread Nikhil Sontakke
Hi Alvaro, The patch looks ok to me. I see that we now sort the constraints by conisonly value too: @@ -1781,12 +1781,20 @@ describeOneTableDetails(const char *schemaname, /* print table (and column) check constraints */ if (tableinfo.checks) { +char

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-16 Thread Greg Smith
On 12/04/2011 02:22 AM, Nikhil Sontakke wrote: Is it okay to modify an existing constraint to mark it as only, even if it was originally inheritable? This is not clear to me. Maybe the safest course of action is to raise an error. Or maybe I'm misreading what it does (because

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-16 Thread Alvaro Herrera
Excerpts from Greg Smith's message of vie dic 16 15:02:20 -0300 2011: On 12/04/2011 02:22 AM, Nikhil Sontakke wrote: Is it okay to modify an existing constraint to mark it as only, even if it was originally inheritable? This is not clear to me. Maybe the safest course of

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-16 Thread Alex Hunsaker
On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera alvhe...@commandprompt.com wrote: Yeah.  Nikhil, Alex, this is the merged patch.  Have a look that it still works for you (particularly the pg_dump bits) and I'll commit it. I adjusted the regression test a bit too. Other than the version checks

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-16 Thread Alvaro Herrera
Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011: On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera alvhe...@commandprompt.com wrote: Yeah.  Nikhil, Alex, this is the merged patch.  Have a look that it still works for you (particularly the pg_dump bits) and I'll

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-16 Thread Alex Hunsaker
On Fri, Dec 16, 2011 at 14:01, Alvaro Herrera alvhe...@commandprompt.com wrote: Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011: On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera alvhe...@commandprompt.com wrote: Yeah.  Nikhil, Alex, this is the merged patch.  Have a

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-03 Thread Alvaro Herrera
Excerpts from Alex Hunsaker's message of dom oct 09 03:40:36 -0300 2011: On Fri, Oct 7, 2011 at 21:30, Nikhil Sontakke nikkh...@gmail.com wrote: Hi Alex, I guess we both are in agreement with each other :) After sleeping over it, I think that check is indeed dead code with this new

Re: [HACKERS] Review: Non-inheritable check constraints

2011-12-03 Thread Nikhil Sontakke
I had a look at this patch today. The pg_dump bits conflict with another patch I committed a few days ago, so I'm about to merge them. I have one question which is about this hunk: Thanks for taking a look Alvaro. @@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-09 Thread Alex Hunsaker
On Fri, Oct 7, 2011 at 21:30, Nikhil Sontakke nikkh...@gmail.com wrote: Hi Alex, I guess we both are in agreement with each other :) After sleeping over it, I think that check is indeed dead code with this new non-inheritable check constraints functionality in place. So unless you have some

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-07 Thread Nikhil Sontakke
Hi Alex, Hmmm, your patch checks for a constraint being only via: !recurse !recursing I hope that is good enough to conclusively conclude that the constraint is 'only'. This check was not too readable in the existing code for me anyways ;). If we check at the grammar

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-07 Thread Alex Hunsaker
On Fri, Oct 7, 2011 at 00:28, Nikhil Sontakke nikkh...@gmail.com wrote: Hi Alex, So with it all spelled out now I see the constraint must be added to child tables too check is dead code. Thanks the above step-wise explanation helps. But AFAICS, the default inhOpt value can be governed by

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-07 Thread Nikhil Sontakke
Hi Alex, I guess we both are in agreement with each other :) After sleeping over it, I think that check is indeed dead code with this new non-inheritable check constraints functionality in place. So unless you have some other comments, we can mark this as 'Ready for Commiter'. Again, thanks for

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-06 Thread Nikhil Sontakke
Hi Alex, I didn't care for the changes to gram.y so I reworked it a bit so we now pass is_only to AddRelationNewConstraint() (like we do with is_local). Seemed simpler but maybe I missed something. Comments? Hmmm, your patch checks for a constraint being only via: !recurse

Re: [HACKERS] Review: Non-inheritable check constraints

2011-10-06 Thread Alex Hunsaker
On Thu, Oct 6, 2011 at 02:42, Nikhil Sontakke nikkh...@gmail.com wrote: Hi Alex, I didn't care for the changes to gram.y so I reworked it a bit so we now pass is_only to AddRelationNewConstraint() (like we do with is_local). Seemed simpler but maybe I missed something. Comments? Hmmm, your

[HACKERS] Review: Non-inheritable check constraints

2011-10-05 Thread Alex Hunsaker
Hi! *Waves* First off, it all seems to work as described: - regressions pass - domains work - tried various inherit options (merging constraints, alter table no inherit etc) - pg_dump/restore I didn't care for the changes to gram.y so I reworked it a bit so we now pass is_only to