Re: [HACKERS] Review: Non-inheritable check constraints
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: MergeWithExistingConstraint() in heap.c MergeConstraintsIntoExisting() in tablecmds.c Nice names indeed :) Yeah, I added a comment about the other one on each of them. 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? -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 variations possible with inheritance.. BTW thank you for doing the work on this. Probably the reason no one bothers too much with inheritance is that even something that's seemingly simple such as what you tried to do here has all these little details that's hard to get right. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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? Nothing much as yet. I think we are mostly there with the code but will keep on trying some more variations along the lines of what Robert tried out whenever I get the time next. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 that even something that's seemingly simple such as what you tried to do here has all these little details that's hard to get right. Thanks Alvaro. Yeah, inheritance is semantics quicksand :) Appreciate all the help from you, Robert and Alex Hunsaker on this. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 become fragile (apart from the non-support for CREATE TABLE based ONLY constraints)? The second bug is a variant of the first. And I have provided a patch for it. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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. IOW, I prefer to keep the syntax we have. Another disadvantage of the current syntax becomes evident when you look at the pg_dump output. If you pg_dump a regular constraint, the constraint gets added as part of the table definition, and the rows are all checked as they are inserted. If you pg_dump an ONLY constraint, the constraint gets added after loading the data, requiring an additional full-table scan to validate it. I am tempted to say we should revert this and rethink. I don't believe we are only a small patch away from finding all the bugs here. Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type of syntax, then +1 for reverting this and a subsequent revised submission. 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 this sounds ok, I can whip up a patch for the same. PFA, patch which does just this. postgres=# alter table a add constraint chk check (ff1 0); ERROR: constraint chk for relation b is an ONLY constraint. Cannot merge I think the basic idea is fine -- the constraint certainly cannot be merged, and we can't continue without merging it because of the inconsistency it would create. The error message is wrong though. I suggest ERROR: constraint name %s on relation %s conflicts with non-inherited constraint on relation %s HINT: Specify a different constraint name. The errmsg seems a bit long though -- anybody has a better suggestion? -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 only constraint. Because that breaks the semantics for only constraints. If this sounds ok, I can whip up a patch for the same. PFA, patch which does just this. postgres=# alter table a add constraint chk check (ff1 0); ERROR: constraint chk for relation b is an ONLY constraint. Cannot merge I think the basic idea is fine -- the constraint certainly cannot be merged, and we can't continue without merging it because of the inconsistency it would create. I was just looking at this as well. 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 only constraint, but it doesn't, even with this patch. I think that part of the problem here is fuzzy thinking about the meaning of the word ONLY in ALTER TABLE ONLY b. The word ONLY there is really supposed to mean that the operation is performed on b but not on its descendents; but that's not what you're doing: you're really performing a different operation. In theory, for a table with no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to be identical, but here they are not. I think that's probably bad. Another manifestation of this problem is that there's no way to add an ONLY constraint in a CREATE TABLE command. I think that's bad, too: it should be possible to declare any constraint at table creation time, and if the ONLY were part of the constraint definition rather than part of the target-table specification, that would work fine. As it is, it doesn't. I am tempted to say we should revert this and rethink. I don't believe we are only a small patch away from finding all the bugs here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 only constraint, but it doesn't, even with this patch. As you rightly point out, this is because we cannot specify ONLY constraints inside a CREATE TABLE as of now. I think that part of the problem here is fuzzy thinking about the meaning of the word ONLY in ALTER TABLE ONLY b. The word ONLY there is really supposed to mean that the operation is performed on b but not on its descendents; but that's not what you're doing: you're really performing a different operation. In theory, for a table with no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to be identical, but here they are not. I think that's probably bad. ONLY has inheritance based connotations for present/future children. ALTER ONLY combined with ADD is honored better now with this patch IMO (atleast for constraints I think). Another manifestation of this problem is that there's no way to add an ONLY constraint in a CREATE TABLE command. I think that's bad, too: it should be possible to declare any constraint at table creation time, and if the ONLY were part of the constraint definition rather than part of the target-table specification, that would work fine. As it is, it doesn't. Well, the above was thought about during the original discussion and eventually we felt that CREATE TABLE already has other issues as well, so not having this done as part of creating a table was considered acceptable: http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt464.html#a4647144 I am tempted to say we should revert this and rethink. I don't believe we are only a small patch away from finding all the bugs here. Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type of syntax, then +1 for reverting this and a subsequent revised submission. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 refused to inherit. Regards, Nikhils On Fri, Dec 23, 2011 at 8:55 AM, Nikhil Sontakke nikkh...@gmail.com wrote: 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 only constraint, but it doesn't, even with this patch. As you rightly point out, this is because we cannot specify ONLY constraints inside a CREATE TABLE as of now. I think that part of the problem here is fuzzy thinking about the meaning of the word ONLY in ALTER TABLE ONLY b. The word ONLY there is really supposed to mean that the operation is performed on b but not on its descendents; but that's not what you're doing: you're really performing a different operation. In theory, for a table with no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to be identical, but here they are not. I think that's probably bad. ONLY has inheritance based connotations for present/future children. ALTER ONLY combined with ADD is honored better now with this patch IMO (atleast for constraints I think). Another manifestation of this problem is that there's no way to add an ONLY constraint in a CREATE TABLE command. I think that's bad, too: it should be possible to declare any constraint at table creation time, and if the ONLY were part of the constraint definition rather than part of the target-table specification, that would work fine. As it is, it doesn't. Well, the above was thought about during the original discussion and eventually we felt that CREATE TABLE already has other issues as well, so not having this done as part of creating a table was considered acceptable: http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt464.html#a4647144 I am tempted to say we should revert this and rethink. I don't believe we are only a small patch away from finding all the bugs here. Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type of syntax, then +1 for reverting this and a subsequent revised submission. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 TABLE rhaas=# alter table b inherit a; ALTER TABLE This needs to fail if chk is an only constraint, but it doesn't, even with this patch. As you rightly point out, this is because we cannot specify ONLY constraints inside a CREATE TABLE as of now. No, it's not related to that. You could do it even without that, by creating a table then adding an ONLY constraint and only later doing the ALTER TABLE / INHERIT. The problem we need to fix here is that this command needs to check that there are no unmergeable constraints; and if there are any, error out. I think that part of the problem here is fuzzy thinking about the meaning of the word ONLY in ALTER TABLE ONLY b. The word ONLY there is really supposed to mean that the operation is performed on b but not on its descendents; but that's not what you're doing: you're really performing a different operation. In theory, for a table with no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to be identical, but here they are not. I think that's probably bad. ONLY has inheritance based connotations for present/future children. ALTER ONLY combined with ADD is honored better now with this patch IMO (atleast for constraints I think). 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. IOW, I prefer to keep the syntax we have. I am tempted to say we should revert this and rethink. I don't believe we are only a small patch away from finding all the bugs here. Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type of syntax, then +1 for reverting this and a subsequent revised submission. 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. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 two functions which try to merge existing constraints: MergeWithExistingConstraint() in heap.c MergeConstraintsIntoExisting() in tablecmds.c Nice names indeed :) 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.. Regards, Nikhils only_constraint_no_merge_v2.0.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 expect this to break anything when you have two constraints with different names. The problem is when you have two constraints with the same name. Testing reveals that this is, in fact, broken: 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); NOTICE: merging constraint chk with inherited definition ALTER TABLE At this point, you'll find that a has a constraint, and b has a constraint, but *c does not have a constraint*. That's bad, because a's constraint wasn't only and should therefore have propagated all the way down the tree. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 different names. The problem is when you have two constraints with the same name. Testing reveals that this is, in fact, broken: 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); NOTICE: merging constraint chk with inherited definition ALTER TABLE At this point, you'll find that a has a constraint, and b has a constraint, but *c does not have a constraint*. That's bad, because a's constraint wasn't only and should therefore have propagated all the way down the tree. 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 this sounds ok, I can whip up a patch for the same. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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); NOTICE: merging constraint chk with inherited definition ALTER TABLE At this point, you'll find that a has a constraint, and b has a constraint, but *c does not have a constraint*. That's bad, because a's constraint wasn't only and should therefore have propagated all the way down the tree. 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 this sounds ok, I can whip up a patch for the same. PFA, patch which does just this. postgres=# alter table a add constraint chk check (ff1 0); ERROR: constraint chk for relation b is an ONLY constraint. Cannot merge Regards, Nikhils only_constraint_no_merge.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 ATExecDropConstraint() doesn't need any adjustment. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 scan
of all the constraints for the given relation.
@@ -6755,6 +6765,7 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
HeapTupletuple;
boolfound = false;
boolis_check_constraint = false;
+boolis_only_constraint = false;
/* At top level, permission check was done in ATPrepCmd, else do it
*/
if (recursing)
@@ -6791,6 +6802,12 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
/* Right now only CHECK constraints can be inherited */
if (con-contype == CONSTRAINT_CHECK)
is_check_constraint = true;
+
+if (con-conisonly)
+{
+Assert(is_check_constraint);
+is_only_constraint = true;
+}
/*
* Perform the actual constraint deletion
@@ -6802,6 +6819,9 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
performDeletion(conobj, behavior);
found = true;
+
+/* constraint found - break from the while loop now */
+break;
}
systable_endscan(scan);
@@ -6830,7 +6850,7 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
* routines, we have to do this one level of recursion at a time; we
can't
* use find_all_inheritors to do it in one pass.
*/
-if (is_check_constraint)
+if (is_check_constraint !is_only_constraint)
children = find_inheritance_children(RelationGetRelid(rel),
lockmode);
else
children = NIL;
PFA, revised version containing the above changes based on Alvaro's v4
patch.
Regards,
Nikhils
non_inh_check_v5.0.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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
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 seem to be off by one looks fine.
Uhm ... you're right that convalidated is present in 9.1 [...] So I
don't think we really need to add a separate branch for 9.1 here, but it
certainly needs a comment improvement.
Hrm... What am I missing?
I was saying that it should all be = 9.2. There are no
convalidated=false check constraints in 9.1, so the extra branch is
useless. This is sufficient:
@@ -6019,8 +6019,13 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
tbinfo-dobj.name);
resetPQExpBuffer(q);
- if (g_fout-remoteVersion = 90100)
+ if (g_fout-remoteVersion = 90200)
{
+ /*
+* conisonly and convalidated are new in 9.2 (actually, the
latter
+* is there in 9.1, but it wasn't ever false for check
constraints
+* until 9.2).
+*/
appendPQExpBuffer(q, SELECT tableoid, oid, conname,
pg_catalog.pg_get_constraintdef(oid) AS consrc,
conislocal, convalidated, conisonly
--
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 from A. We add an only constraint to B and a non-only constraint to A. Now, what happens in each of the following scenarios? 1. We drop the constraint from B without specifying ONLY. 2. We drop the constraint from B *with* ONLY. 3. We drop the constraint from A without specifying ONLY. 4. We drop the constraint from A *with* ONLY. Off the top of my head, I suspect that #1 should be an error; #2 should succeed, leaving only the inherited version of the constraint on B; #3 should remove the constraint from A and leave it on B but I'm not sure what should happen to C, and I have no clear vision of what #4 should do. As a followup question, if we do #2 followed by #4, or #4 followed by #2, do we end up with the same final state in both cases? Here's another scenario. B inherits from A. We a constraint to A using ONLY, and then drop it without ONLY. Does that work or fail? Also, what happens we add matching constraints to B and A, in each case using ONLY, and then remove the constraint from A without using ONLY? Does anything happen to B's constraint? Why or why not? Just to be clear, I like the feature. But I've done some work on this code before, and it is amazingly easy for to screw it up and end up with bugs... so I think lots of careful thought is in order. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Re: [HACKERS] Review: Non-inheritable check constraints
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 welcome. 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. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 definitely planning to test out those scenarios and will respond some time late today. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 hierarchies. It is specific to that table, period. It's not just that. Suppose that C inherits from B which inherits from A. We add an only constraint to B and a non-only constraint to A. Now, what happens in each of the following scenarios? An example against latest HEAD should help here: create table A(ff1 int); create table B () inherits (A); create table C () inherits (B); alter table A add constraint Achk check (ff1 10); The above will attach Achk to A, B and C alter table only B add constraint Bchk check (ff1 0); The above will attach Bchk ONLY to table B 1. We drop the constraint from B without specifying ONLY. postgres=# alter table B drop constraint Achk; ERROR: cannot drop inherited constraint achk of relation b The above is existing inheritance based behaviour. Now let's look at the ONLY constraint: postgres=# alter table B drop constraint Bchk; ALTER TABLE Since this constraint is not part of any hierarchy, it can be removed. postgres=# alter table only B add constraint bchk check (ff1 0); ALTER TABLE postgres=# alter table only B drop constraint Bchk; ALTER TABLE So only constraints do not need the only B qualification to be deleted. They work both ways and can always be deleted without any issues. 2. We drop the constraint from B *with* ONLY. postgres=# alter table only B drop constraint Achk; ERROR: cannot drop inherited constraint achk of relation b The above is existing inheritance based behavior. So regardless of ONLY an inherited constraint cannot be removed from the middle of the hierarchy. 3. We drop the constraint from A without specifying ONLY. postgres=# alter table A drop constraint Achk; ALTER TABLE This removes the constraint from the entire hierarchy across A, B and C. Again existing inheritance behavior. 4. We drop the constraint from A *with* ONLY. postgres=# alter table only A drop constraint Achk; ALTER TABLE This converts the Achk constraints belonging to B into a local one. C still has it as an inherited constraint from B. We can now delete those constraints as per existing inheritance semantics. However I hope the difference between these and ONLY constraints are clear. The Achk constraint associated with B can get inherited in the future whereas only constraints will not be. Off the top of my head, I suspect that #1 should be an error; It's an error for inherited constraints, but not for only constraints. #2 should succeed, leaving only the inherited version of the constraint on B; Yeah, only constraints removal succeeds, whereas inherited constraints cannot be removed. #3 should remove the constraint from A and leave it on B but I'm not sure what should happen to C, This removes the entire hierarchy. and I have no clear vision of what #4 should do. This removes the constraint from A, but maintains the inheritance relationship between B and C. Again standard existing inheritance semantics. As a followup question, if we do #2 followed by #4, or #4 followed by #2, do we end up with the same final state in both cases? Yeah. #2 is not able to do much really because we do not allow inherited constraints to be removed from the mid of the hierarchy. Here's another scenario. B inherits from A. We a constraint to A using ONLY, and then drop it without ONLY. Does that work or fail? The constraint gets added to A and since it is an only constraint, its removal both with and without only A works just fine. Also, what happens we add matching constraints to B and A, in each case using ONLY, and then remove the constraint from A without using ONLY? Does anything happen to B's constraint? Why or why not? Again the key differentiation here is that only constraints are bound to that table and wont be inherited ever. So this works just fine. postgres=# alter table only A add constraint A2chk check (ff1 10); ALTER TABLE postgres=# alter table only B add constraint A2chk check (ff1 10); ALTER TABLE Just to be clear, I like the feature. But I've done some work on this code before, and it is amazingly easy for to screw it up and end up with bugs... so I think lots of careful thought is in order. 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. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 *is_only;
+
+if (pset.sversion 90100)
+is_only = r.conisonly;
+else
+is_only = false AS conisonly;
+
printfPQExpBuffer(buf,
- SELECT r.conname,
+ SELECT r.conname, %s,
pg_catalog.pg_get_constraintdef(r.oid,
true)\n
FROM pg_catalog.pg_constraint r\n
- WHERE r.conrelid = '%s' AND r.contype = 'c'\nORDER BY
1;,
- oid);
+ WHERE r.conrelid = '%s' AND r.contype = 'c'\n
+ ORDER BY 2, 1;,
+ is_only, oid);
result = PSQLexec(buf.data, false);
if (!result)
goto error_return;
My preference would be to see true values first, so might want to modify it
to
ORDER BY 2 desc, 1
to have that effect. Your call finally.
Regards,
Nikhils
On Sat, Dec 17, 2011 at 12:36 AM, Alvaro Herrera alvhe...@commandprompt.com
wrote:
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 action is to raise an error. Or maybe I'm
misreading
what it does (because I haven't compiled it yet).
Hmmm, good question. IIRC, the patch will pass is_only as true only if
it going to be a locally defined, non-inheritable constraint. So I
went by the logic that since it was ok to merge and mark a constraint
as locally defined, it should be ok to mark it non-inheritable from
this moment on with that new local definition?
I think I misread what that was trying to do. I thought it would turn
on the is only bit on a constraint that a child had inherited from a
previous parent, but that was obviously wrong now that I think about it
again.
With this open question, this looks like it's back in Alvaro's hands
again to me. This one started the CF as Ready for Committer and seems
stalled there for now. I'm not going to touch its status, just pointing
this fact out.
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.
Thanks.
--
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 I haven't compiled it yet). Hmmm, good question. IIRC, the patch will pass is_only as true only if it going to be a locally defined, non-inheritable constraint. So I went by the logic that since it was ok to merge and mark a constraint as locally defined, it should be ok to mark it non-inheritable from this moment on with that new local definition? With this open question, this looks like it's back in Alvaro's hands again to me. This one started the CF as Ready for Committer and seems stalled there for now. I'm not going to touch its status, just pointing this fact out. -- Greg Smith 2ndQuadrant USg...@2ndquadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Re: [HACKERS] Review: Non-inheritable check constraints
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 action is to raise an error. Or maybe I'm misreading what it does (because I haven't compiled it yet). Hmmm, good question. IIRC, the patch will pass is_only as true only if it going to be a locally defined, non-inheritable constraint. So I went by the logic that since it was ok to merge and mark a constraint as locally defined, it should be ok to mark it non-inheritable from this moment on with that new local definition? I think I misread what that was trying to do. I thought it would turn on the is only bit on a constraint that a child had inherited from a previous parent, but that was obviously wrong now that I think about it again. With this open question, this looks like it's back in Alvaro's hands again to me. This one started the CF as Ready for Committer and seems stalled there for now. I'm not going to touch its status, just pointing this fact out. 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. Thanks. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support non_inh_check_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 seem to be off by one looks fine. I
assume I/we missed that in the original patch. I also adjusted the
version check in describe.c to be consistent with the other version
checks in that file (= 90200 instead of 90100).
(Also, nice catch on false AS as r.conisonly in describe.c)
--
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***
*** 5996,6003 getTableAttrs(TableInfo *tblinfo, int numTables)
tbinfo-dobj.name);
resetPQExpBuffer(q);
! if (g_fout-remoteVersion = 90100)
{
appendPQExpBuffer(q, SELECT tableoid, oid,
conname,
pg_catalog.pg_get_constraintdef(oid) AS consrc,
conislocal,
convalidated, conisonly
--- 5996,6004
tbinfo-dobj.name);
resetPQExpBuffer(q);
! if (g_fout-remoteVersion = 90200)
{
+ /* conisonly is new in 9.2 */
appendPQExpBuffer(q, SELECT tableoid, oid,
conname,
pg_catalog.pg_get_constraintdef(oid) AS consrc,
conislocal,
convalidated, conisonly
***
*** 6007,6012 getTableAttrs(TableInfo *tblinfo, int numTables)
--- 6008,6026
ORDER BY
conname,
tbinfo-dobj.catId.oid);
}
+ else if (g_fout-remoteVersion = 90100)
+ {
+ /* conisvalidated is new in 9.1 */
+ appendPQExpBuffer(q, SELECT tableoid, oid,
conname,
+
pg_catalog.pg_get_constraintdef(oid) AS consrc,
+ conislocal,
convalidated,
+ false as
conisonly
+ FROM
pg_catalog.pg_constraint
+ WHERE
conrelid = '%u'::pg_catalog.oid
+AND
contype = 'c'
+ ORDER BY
conname,
+
tbinfo-dobj.catId.oid);
+ }
else if (g_fout-remoteVersion = 80400)
{
appendPQExpBuffer(q, SELECT tableoid, oid,
conname,
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
***
*** 1783,1789 describeOneTableDetails(const char *schemaname,
{
char *is_only;
! if (pset.sversion 90100)
is_only = r.conisonly;
else
is_only = false AS conisonly;
--- 1783,1789
{
char *is_only;
! if (pset.sversion = 90200)
is_only = r.conisonly;
else
is_only = false AS conisonly;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 commit it. I adjusted the regression test a bit too. Other than the version checks seem to be off by one looks fine. I assume I/we missed that in the original patch. I also adjusted the version check in describe.c to be consistent with the other version checks in that file (= 90200 instead of 90100). Uhm ... you're right that convalidated is present in 9.1 but AFAIR it's only used for FKs, not CHECKs which is what this code path is about (for CHECKs I only introduced it in 9.2, which is the patch that caused the merge conflict in the first place). FKs use a completely separate path in pg_dump which doesn't need the separate convalidated check. So I don't think we really need to add a separate branch for 9.1 here, but it certainly needs a comment improvement. -- Álvaro Herrera alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 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 seem to be off by one looks fine. Uhm ... you're right that convalidated is present in 9.1 [...] So I don't think we really need to add a separate branch for 9.1 here, but it certainly needs a comment improvement. Hrm... What am I missing? $ inh_v4/bin/psql -c 'select version();' -d test version - PostgreSQL 9.1.0 on x86_64-unknown-linux-gnu, compiled by gcc (GCC) 4.6.1 20110819 (prerelease), 64-bit (1 row) $ inh_v4/bin/pg_dump test pg_dump: SQL command failed pg_dump: Error message from server: ERROR: column conisonly does not exist LINE 1: ...aintdef(oid) AS consrc, conislocal, convalidated, conisonly ... ^ pg_dump: The command was: SELECT tableoid, oid, conname, pg_catalog.pg_get_constraintdef(oid) AS consrc, conislocal, convalidated, conisonly FROM pg_catalog.pg_constraint WHERE conrelid = '237964'::pg_catalog.oidAND contype = 'c' ORDER BY conname -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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
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 the thorough review and subsequent changes!
PFA an updated patch with the check removed and a comment or two added.
I've also marked it ready.
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:
@@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char *ccname,
Node *expr,
con-conislocal = true;
else
con-coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con-conisonly = true;
+ }
simple_heap_update(conDesc, tup-t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
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 I haven't compiled it yet).
--
Álvaro Herrera alvhe...@commandprompt.com
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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
*ccname, Node *expr,
con-conislocal = true;
else
con-coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con-conisonly = true;
+ }
simple_heap_update(conDesc, tup-t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
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 I haven't compiled it yet).
Hmmm, good question. IIRC, the patch will pass is_only as true only if it
going to be a locally defined, non-inheritable constraint. So I went by the
logic that since it was ok to merge and mark a constraint as locally
defined, it should be ok to mark it non-inheritable from this moment on
with that new local definition?
Regards,
Nikhils
Regards,
Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 other comments, we can mark this as 'Ready for Commiter'. Again, thanks for the thorough review and subsequent changes! PFA an updated patch with the check removed and a comment or two added. I've also marked it ready. non_inh_check_v3.patch.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 level, we can be sure. But I remember not being too comfortable about the right position to ascertain this characteristic. Well I traced through it here was my thinking (maybe should be a comment?): 1: AlterTable() calls ATController() with recurse = interpretInhOption(stmt-relation-inhOpt 2: ATController() calls ATPrepCmd() with recurse and recursing = false 3: ATPrepCmd() saves the recurse flag with the subtup AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint 4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when subtype == AT_AddConstraintRecurse, recurse = false otherwise 5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and recursing = false 6: now we are in ATAddCheckConstraint() where recurse == interpretInhOption(rv-inhOpt) and recursing == false. Recursing is only true when ATAddCheckConstaint() loops through children and recursively calls ATAddCheckConstraint() 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 the SQL_inheritance guc too. So in that case, it's possible that recurse is false and child tables are present, no? Infact as I now remember, the reason my patch was looping through was to handle this very case. It was based on the assumptions that some constraints might be ONLY type and some can be inheritable. Although admittedly the current ALTER TABLE functionality does not allow this. So maybe we can still keep this check around IMO. Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 the SQL_inheritance guc too. So in that case, it's possible that recurse is false and child tables are present, no? Well... Do we really want to differentiate between those two case? I would argue no. Given that: set sql_inhertance to off; alter table xxx alter column; behaves the same as set sql_inhertance to on; alter table only xxx alter column; Why should we treat constraints differently? Or put another way if set sql_inhertance off makes alter table behave with an implicit only, shouldn't add/drop constraint respect that? Infact as I now remember, the reason my patch was looping through was to handle this very case. It was based on the assumptions that some constraints might be ONLY type and some can be inheritable. Although admittedly the current ALTER TABLE functionality does not allow this. Hrm... Ill I see is a user who turned off sql_inhertance wondering why they have to specify ONLY on some alter table commands and not others. I think if we want to support ONLY constraint types in the way you are thinking about them, we need to put ONLY some place else (alter table xxx add only constraint ?). Personally I don't see a reason to have that kind of constraint. Mostly because I don't see how its functionally different. Is it somehow? Anyone else have any thoughts on this? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Review: Non-inheritable check constraints
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 the thorough review and subsequent changes! Regards, Nikhils On Fri, Oct 7, 2011 at 12:18 PM, Alex Hunsaker bada...@gmail.com wrote: 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 the SQL_inheritance guc too. So in that case, it's possible that recurse is false and child tables are present, no? Well... Do we really want to differentiate between those two case? I would argue no. Given that: set sql_inhertance to off; alter table xxx alter column; behaves the same as set sql_inhertance to on; alter table only xxx alter column; Why should we treat constraints differently? Or put another way if set sql_inhertance off makes alter table behave with an implicit only, shouldn't add/drop constraint respect that? Infact as I now remember, the reason my patch was looping through was to handle this very case. It was based on the assumptions that some constraints might be ONLY type and some can be inheritable. Although admittedly the current ALTER TABLE functionality does not allow this. Hrm... Ill I see is a user who turned off sql_inhertance wondering why they have to specify ONLY on some alter table commands and not others. I think if we want to support ONLY constraint types in the way you are thinking about them, we need to put ONLY some place else (alter table xxx add only constraint ?). Personally I don't see a reason to have that kind of constraint. Mostly because I don't see how its functionally different. Is it somehow? Anyone else have any thoughts on this?
Re: [HACKERS] Review: Non-inheritable check constraints
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 !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 level, we can be sure. But I remember not being too comfortable about the right position to ascertain this characteristic. I also moved the is_only check in AtAddCheckConstraint() to before we grab and loop through any children. Seemed a bit wasteful to loop through the new constraints just to set a flag so that we could bail out while looping through the children. Ditto comment for this function. I thought this function went to great lengths to spit out a proper error in case of inconsistencies between parent and child. But if your check makes it simpler, that's good! You also forgot to bump Natts_pg_constraint. Ouch. Thanks for the catch. PFA the above changes as well as being rebased against master. Thanks Alex, appreciate the review! Regards, Nikhils
Re: [HACKERS] Review: Non-inheritable check constraints
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 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 level, we can be sure. But I remember not being too comfortable about the right position to ascertain this characteristic. Well I traced through it here was my thinking (maybe should be a comment?): 1: AlterTable() calls ATController() with recurse = interpretInhOption(stmt-relation-inhOpt 2: ATController() calls ATPrepCmd() with recurse and recursing = false 3: ATPrepCmd() saves the recurse flag with the subtup AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint 4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when subtype == AT_AddConstraintRecurse, recurse = false otherwise 5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and recursing = false 6: now we are in ATAddCheckConstraint() where recurse == interpretInhOption(rv-inhOpt) and recursing == false. Recursing is only true when ATAddCheckConstaint() loops through children and recursively calls ATAddCheckConstraint() So with it all spelled out now I see the constraint must be added to child tables too check is dead code. Ill take out that check and then mark it as ready for commiter (and Ill add any comments about the logic of the recurse flag above if I can think of a concise way to put it). Sound good? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
