Re: [HACKERS] Check constraints on partition parents only?
>> Comments and further feedback, if any, appreciated. > > Did you look at how this conflicts with my patch to add not null > rows to pg_constraint? > > https://commitfest.postgresql.org/action/patch_view?id=601 > I was certainly not aware of this patch in the commitfest. Your patch has a larger footprint with more functional changes in it. IMHO, it will be easiest to queue this non-inheritable constraints patch behind your patch in the commitfest. There will be certain bitrot, which I can fix once your patch gets committed. Regards, Nikhils -- 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] Check constraints on partition parents only?
Excerpts from Nikhil Sontakke's message of vie jul 29 14:12:37 -0400 2011: > Hi all, > > PFA, patch which implements non-inheritable "ONLY" constraints. This > has been achieved by introducing a new column "conisonly" in > pg_constraint catalog. Specification of 'ONLY' in the ALTER TABLE ADD > CONSTRAINT CHECK command is used to set this new column to true. > Constraints which have this column set to true cannot be inherited by > present and future children ever. > > The psql and pg_dump binaries have been modified to account for such > persistent non-inheritable check constraints. This patch also has > documentation changes along with relevant changes to the test cases. > The regression runs pass fine with this patch applied. > > Comments and further feedback, if any, appreciated. Did you look at how this conflicts with my patch to add not null rows to pg_constraint? https://commitfest.postgresql.org/action/patch_view?id=601 -- Álvaro Herrera 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] Check constraints on partition parents only?
Hi all, PFA, patch which implements non-inheritable "ONLY" constraints. This has been achieved by introducing a new column "conisonly" in pg_constraint catalog. Specification of 'ONLY' in the ALTER TABLE ADD CONSTRAINT CHECK command is used to set this new column to true. Constraints which have this column set to true cannot be inherited by present and future children ever. The psql and pg_dump binaries have been modified to account for such persistent non-inheritable check constraints. This patch also has documentation changes along with relevant changes to the test cases. The regression runs pass fine with this patch applied. Comments and further feedback, if any, appreciated. Regards, Nikhils diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 5e5f8a7..683ad67 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -1995,6 +1995,16 @@ + conisonly + bool + + + This constraint is defined locally for the relation. It is a + non-inheritable constraint. + + + + conkey int2[] pg_attribute.attnum diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 4c2a4cd..3ee3ec0 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -984,6 +984,14 @@ ALTER TABLE distributors ADD CONSTRAINT zipchk CHECK (char_length(zipcode) = 5); + To add a check constraint only to a table and not to its children: + +ALTER TABLE ONLY distributors ADD CONSTRAINT zipchk CHECK (char_length(zipcode) = 5); + + (The check constraint will not be inherited by future children too.) + + + To remove a check constraint from a table and all its children: ALTER TABLE distributors DROP CONSTRAINT zipchk; diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 4399493..1b382b8 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -98,10 +98,10 @@ static Oid AddNewRelationType(const char *typeName, Oid new_array_type); static void RelationRemoveInheritance(Oid relid); static void StoreRelCheck(Relation rel, char *ccname, Node *expr, - bool is_validated, bool is_local, int inhcount); + bool is_validated, bool is_local, int inhcount, bool is_only); static void StoreConstraints(Relation rel, List *cooked_constraints); static bool MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr, - bool allow_merge, bool is_local); + bool allow_merge, bool is_local, bool is_only); static void SetRelationNumChecks(Relation rel, int numchecks); static Node *cookConstraint(ParseState *pstate, Node *raw_constraint, @@ -1860,7 +1860,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr) */ static void StoreRelCheck(Relation rel, char *ccname, Node *expr, - bool is_validated, bool is_local, int inhcount) + bool is_validated, bool is_local, int inhcount, bool is_only) { char *ccbin; char *ccsrc; @@ -1943,7 +1943,8 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr, ccbin,/* Binary form of check constraint */ ccsrc,/* Source form of check constraint */ is_local, /* conislocal */ - inhcount);/* coninhcount */ + inhcount, /* coninhcount */ + is_only); /* conisonly */ pfree(ccbin); pfree(ccsrc); @@ -1984,7 +1985,7 @@ StoreConstraints(Relation rel, List *cooked_constraints) break; case CONSTR_CHECK: StoreRelCheck(rel, con->name, con->expr, !con->skip_validation, - con->is_local, con->inhcount); + con->is_local, con->inhcount, con->is_only); numchecks++; break; default: @@ -2100,6 +2101,7 @@ AddRelationNewConstraints(Relation rel, cooked->skip_validation = false; cooked->is_local = is_local; cooked->inhcount = is_local ? 0 : 1; + cooked->is_only = false; cookedConstraints = lappend(cookedConstraints, cooked); } @@ -2167,7 +2169,7 @@ AddRelationNewConstraints(Relation rel, * what ATAddCheckConstraint wants.)
Re: [HACKERS] Check constraints on partition parents only?
> We could imagine doing something like CHECK ONLY (foo), but that seems > quite non-orthogonal with (a) everything else in CREATE TABLE, and > (b) ALTER TABLE ONLY. > > Yeah, I thought about CHECK ONLY support as part of table definition, but as you say - it appears to be too non-standard right now and we can always go back to this later if the need be felt. Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
> > Yeah, I have already hacked it a bit. This constraint now needs to be > > spit out later as an ALTER command with ONLY attached to it > > appropriately. Earlier all CHECK constraints were generally emitted as > > part of the table definition itself. > > IIRC, there's already support for splitting out a constraint that way, > in order to deal with circular dependencies. You just need to treat > this as an additional reason for splitting. > > Yeah, I have indeed followed the existing separate printing logic for "ONLY" constraints. Had to make the table dependent on this constraint to print the constraint *after* the table definition. Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
> > Yeah, I have already hacked it a bit. This constraint now needs to be > > spit out later as an ALTER command with ONLY attached to it > > appropriately. Earlier all CHECK constraints were generally emitted as > > part of the table definition itself. > > Hrm. That doesn't seem so good. Maybe we've got the design wrong > here. It doesn't seem like we want to lose the ability to define > arbitrary constraints at table-creation time. > > Well the handling is different now for "ONLY" constraints only. The normal constraints can still be attached at table-creation time. Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
Robert Haas writes: > On Fri, Jul 29, 2011 at 8:30 AM, Nikhil Sontakke wrote: >> Yeah, I have already hacked it a bit. This constraint now needs to be >> spit out later as an ALTER command with ONLY attached to it >> appropriately. Earlier all CHECK constraints were generally emitted as >> part of the table definition itself. > Hrm. That doesn't seem so good. Maybe we've got the design wrong > here. It doesn't seem like we want to lose the ability to define > arbitrary constraints at table-creation time. Well, you can't define arbitrary indexes within the CREATE TABLE syntax, either. This does not bother me a lot. We could imagine doing something like CHECK ONLY (foo), but that seems quite non-orthogonal with (a) everything else in CREATE TABLE, and (b) ALTER TABLE ONLY. regards, tom lane -- 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] Check constraints on partition parents only?
Nikhil Sontakke writes: >> (Also, don't forget you need to hack pg_dump, too.) > Yeah, I have already hacked it a bit. This constraint now needs to be > spit out later as an ALTER command with ONLY attached to it > appropriately. Earlier all CHECK constraints were generally emitted as > part of the table definition itself. IIRC, there's already support for splitting out a constraint that way, in order to deal with circular dependencies. You just need to treat this as an additional reason for splitting. regards, tom lane -- 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] Check constraints on partition parents only?
On Fri, Jul 29, 2011 at 8:30 AM, Nikhil Sontakke wrote: > Yeah, I have already hacked it a bit. This constraint now needs to be > spit out later as an ALTER command with ONLY attached to it > appropriately. Earlier all CHECK constraints were generally emitted as > part of the table definition itself. Hrm. That doesn't seem so good. Maybe we've got the design wrong here. It doesn't seem like we want to lose the ability to define arbitrary constraints at table-creation time. -- 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] Check constraints on partition parents only?
> psql=# \d a > Table "public.a" > Column | Type | Modifiers > +-+--- > b | integer | > Check constraints: >"achk" CHECK (false) >"bchk" CHECK (b > 0) > > Is this acceptable? Or we need to put in work into psql to show ONLY > somewhere in the description? If yes, ONLY CHECK sounds weird, maybe > we should use LOCAL CHECK or some such mention: > > Check constraints: >"achk" LOCAL CHECK (false) >"bchk" CHECK (b > 0) I think you need to stick with "ONLY". Using two different words is just going to create confusion. You could fool around with where exactly you put it on the line, but switching to a different word seems like not a good idea. Ok, maybe something like: "achk" (ONLY) CHECK (false) >>(Also, don't forget you need to hack pg_dump, too.) Yeah, I have already hacked it a bit. This constraint now needs to be spit out later as an ALTER command with ONLY attached to it appropriately. Earlier all CHECK constraints were generally emitted as part of the table definition itself. Regards, Nikhils -- 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] Check constraints on partition parents only?
On Fri, Jul 29, 2011 at 7:41 AM, Nikhil Sontakke wrote: > Hi, > >>>Any preferences for the name? >>> connoinh >>> conisonly >>> constatic or confixed >> >> I'd probably pick conisonly from those choices. >> > > The use of "\d" inside psql will show ONLY constraints without any > embellishments similar to normal constraints. E.g. > > > ALTER TABLE ONLY a ADD CONSTRAINT achk CHECK (FALSE); > > ALTER TABLE a ADD CONSTRAINT bchk CHECK (b > 0); > > psql=# \d a > Table "public.a" > Column | Type | Modifiers > +-+--- > b | integer | > Check constraints: > "achk" CHECK (false) > "bchk" CHECK (b > 0) > > Is this acceptable? Or we need to put in work into psql to show ONLY > somewhere in the description? If yes, ONLY CHECK sounds weird, maybe > we should use LOCAL CHECK or some such mention: > > Check constraints: > "achk" LOCAL CHECK (false) > "bchk" CHECK (b > 0) I think you need to stick with "ONLY". Using two different words is just going to create confusion. You could fool around with where exactly you put it on the line, but switching to a different word seems like not a good idea. (Also, don't forget you need to hack pg_dump, too.) -- 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] Check constraints on partition parents only?
Hi, >>Any preferences for the name? >> connoinh >> conisonly >> constatic or confixed > > I'd probably pick conisonly from those choices. > The use of "\d" inside psql will show ONLY constraints without any embellishments similar to normal constraints. E.g. ALTER TABLE ONLY a ADD CONSTRAINT achk CHECK (FALSE); ALTER TABLE a ADD CONSTRAINT bchk CHECK (b > 0); psql=# \d a Table "public.a" Column | Type | Modifiers +-+--- b | integer | Check constraints: "achk" CHECK (false) "bchk" CHECK (b > 0) Is this acceptable? Or we need to put in work into psql to show ONLY somewhere in the description? If yes, ONLY CHECK sounds weird, maybe we should use LOCAL CHECK or some such mention: Check constraints: "achk" LOCAL CHECK (false) "bchk" CHECK (b > 0) Regards, Nikhils -- 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] Check constraints on partition parents only?
On Thu, Jul 28, 2011 at 10:01 AM, Nikhil Sontakke wrote: > Yeah, in your case too an initdb would be required, so might as well go down > the route of a new column. Any preferences for the name? > connoinh > conisonly > constatic or confixed I'd probably pick conisonly from those choices. -- 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] Check constraints on partition parents only?
> This approach certainly can't work, because a table can be both an > inheritance parent and an inheritance child. It could have an ONLY > constraint, and also inherit a copy of the same constraint for one or > more parents. IOW, the fact that conislocal = true does not mean that > coninhcount is irrelevant. Oh I see. > I think what you probably want to do is > either (a) add a new column or (b) change conislocal to a char value > and make it three-valued: > > n = inherited constraint, no local definition > o = defined locally as an "ONLY" constraint > i = defined locally as a non-ONLY constraint > > I think I favor the latter approach as more space-efficient, but I > hear Tom muttering about backward-compatibility... > > Yeah, in your case too an initdb would be required, so might as well go down the route of a new column. Any preferences for the name? connoinh conisonly constatic or confixed Others? Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
On Thu, Jul 28, 2011 at 9:43 AM, Nikhil Sontakke wrote: > Alternatively we could bring about the same > by using a combination of conislocal and coninhcnt. For ONLY constraints, we > will need to percolate this information down to the point where we define it > in the code. We can then mark ONLY constraints to have conislocal set to > TRUE and coninhcnt set to a special value (-1) This approach certainly can't work, because a table can be both an inheritance parent and an inheritance child. It could have an ONLY constraint, and also inherit a copy of the same constraint for one or more parents. IOW, the fact that conislocal = true does not mean that coninhcount is irrelevant. I think what you probably want to do is either (a) add a new column or (b) change conislocal to a char value and make it three-valued: n = inherited constraint, no local definition o = defined locally as an "ONLY" constraint i = defined locally as a non-ONLY constraint I think I favor the latter approach as more space-efficient, but I hear Tom muttering about backward-compatibility... -- 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] Check constraints on partition parents only?
Nikhil Sontakke writes: > What we need is to persist information of a particular constraint to be as > specified - ONLY for this table. We could do that by adding a new column in > pg_constraint like 'connoinh' or something, but I guess we would prefer not > to get into the initdb business. Uh, why not? I trust you're not imagining this would get back-patched. > Alternatively we could bring about the same > by using a combination of conislocal and coninhcnt. Ugh. New column, please. If you're wondering why, see the flak Robert has been taking lately for replacing pg_class.relistemp. Random changes in the semantics of existing columns are trouble. regards, tom lane -- 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] Check constraints on partition parents only?
> Now that we have coninhcnt, conislocal etc... we can probably support > ONLY. But I agree with Robert it's probably a bit more than an > afternoon to crank out :-) > Heh, agreed :), I was just looking for some quick and early feedback. So what we need is basically a way to indicate that a particular constraint is non-inheritable forever (meaning - even for future children) and that should work? Right now, it seems that the "ONLY" usage in the SQL only translates to a recurse or no-recurse operation. For the parent, a constraint is marked with conislocal set to true (coninhcount is 0) and for children, coninhcount is used to indicate inheritance of that constraint with conislocal being set to false. What we need is to persist information of a particular constraint to be as specified - ONLY for this table. We could do that by adding a new column in pg_constraint like 'connoinh' or something, but I guess we would prefer not to get into the initdb business. Alternatively we could bring about the same by using a combination of conislocal and coninhcnt. For ONLY constraints, we will need to percolate this information down to the point where we define it in the code. We can then mark ONLY constraints to have conislocal set to TRUE and coninhcnt set to a special value (-1). So to summarize, what I am proposing is: Introduce new column connoinh (boolean) in pg_constraint OR in existing infrastructure: Normal constraints: conislocal (true) coninhcnt (0) (inheritable like today) Inherited constraints: conislocal (false) coninhcnt (n > 0) ONLY constraints:conislocal (true) coninhcnt (-1) (not inheritable) With this arrangment, pg_dump will be able to easily identify and spit out ONLY specifications for specific constraints and then they won't be blindly passed on to children table under these new semantics. Thoughts? Anything missing? Please let me know. Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
On Wed, Jul 27, 2011 at 14:08, Tom Lane wrote: > Yeah. If we're going to allow this then we should just have a concept > of a non-inherited constraint, full stop. This might just be a matter > of removing the error thrown in ATAddCheckConstraint, but I'd be worried > about whether pg_dump will handle the case correctly, what happens when > a new child is added later, etc etc. [ For those who missed it ] pg_dump getting things wrong was a big reason to disallow ONLYconstraints. That is pg_dump did not treat ONLY constraints correctly, it always tried to stick them on the parent table: http://archives.postgresql.org/pgsql-bugs/2007-04/msg00026.php I for example had some backups that had to be manually fixed (by removing constraints) to get them to import. I would wager the mentioned clients that have been doing this have broken backups as well :-( Now that we have coninhcnt, conislocal etc... we can probably support ONLY. But I agree with Robert it's probably a bit more than an afternoon to crank out :-) -- 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] Check constraints on partition parents only?
On Wed, Jul 27, 2011 at 4:08 PM, Tom Lane wrote: > Robert Haas writes: >> Well, I don't have anything strongly against the idea of an >> uninherited constraint, though it sounds like Tom does. But I think >> allowing it just in the case of CHECK (false) would be pretty silly. >> And, I'm fairly certain that this isn't going to play nice with >> coninhcount... local constraints would have to be marked as local, >> else the wrong things will happen later on when you drop them. > > Yeah. If we're going to allow this then we should just have a concept > of a non-inherited constraint, full stop. This might just be a matter > of removing the error thrown in ATAddCheckConstraint, but I'd be worried > about whether pg_dump will handle the case correctly, what happens when > a new child is added later, etc etc. Right. I'm fairly sure all that stuff is gonna break with the proposed implementation. It's a solvable problem, but it's going to take more than an afternoon to crank it out. -- 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] Check constraints on partition parents only?
On 07/27/2011 04:14 PM, David E. Wheeler wrote: On Jul 27, 2011, at 1:08 PM, Tom Lane wrote: Yeah. If we're going to allow this then we should just have a concept of a non-inherited constraint, full stop. This might just be a matter of removing the error thrown in ATAddCheckConstraint, but I'd be worried about whether pg_dump will handle the case correctly, what happens when a new child is added later, etc etc. Is this looking at the wrong problem? The reason I've wanted to get a parent check constraint not to fire in a child is because I'm using the parent/child relationship for partioning. Will this be relevant if/when an independent partitioning feature is added that does not rely on table inheritance? Yes, I have clients using inheritance for non-partitioning purposes, and they would love to have this. cheers andrew -- 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] Check constraints on partition parents only?
On Jul 27, 2011, at 1:08 PM, Tom Lane wrote: > Yeah. If we're going to allow this then we should just have a concept > of a non-inherited constraint, full stop. This might just be a matter > of removing the error thrown in ATAddCheckConstraint, but I'd be worried > about whether pg_dump will handle the case correctly, what happens when > a new child is added later, etc etc. Is this looking at the wrong problem? The reason I've wanted to get a parent check constraint not to fire in a child is because I'm using the parent/child relationship for partioning. Will this be relevant if/when an independent partitioning feature is added that does not rely on table inheritance? Best, David -- 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] Check constraints on partition parents only?
Robert Haas writes: > Well, I don't have anything strongly against the idea of an > uninherited constraint, though it sounds like Tom does. But I think > allowing it just in the case of CHECK (false) would be pretty silly. > And, I'm fairly certain that this isn't going to play nice with > coninhcount... local constraints would have to be marked as local, > else the wrong things will happen later on when you drop them. Yeah. If we're going to allow this then we should just have a concept of a non-inherited constraint, full stop. This might just be a matter of removing the error thrown in ATAddCheckConstraint, but I'd be worried about whether pg_dump will handle the case correctly, what happens when a new child is added later, etc etc. regards, tom lane -- 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] Check constraints on partition parents only?
On Wed, Jul 27, 2011 at 6:39 AM, Nikhil Sontakke wrote: >>> >>> Yeah, but I think we need to take that chance. At the very least, we >>> need to support the equivalent of a non-inherited CHECK (false) on >>> parent tables. >> >> Indeed. I usually enforce that with a trigger that raises an exception, >> but of course that doesn't help at all with constraint exclusion, and I saw >> a result just a few weeks ago (I forget the exact details) where it appeared >> that the plan chosen was significantly worse because the parent table wasn't >> excluded, so there's a non-trivial downside from having this restriction. >> > > The downside appears to be non-trivial indeed! I cooked up the attached > patch to try to allow ALTER...ONLY...CHECK(false) on parent tables. > > If this approach looks acceptable, I can provide a complete patch later with > some documentation changes (I think we ought to tell about this special case > in the documentation) and a minor test case along with it (if the need be > felt for the test case). > Although partitioning ought to be looked at from a different angle > completely, maybe this small patch can help out a bit in the current scheme > of things, although this is indeed a unusual special casing... Thoughts? Well, I don't have anything strongly against the idea of an uninherited constraint, though it sounds like Tom does. But I think allowing it just in the case of CHECK (false) would be pretty silly. And, I'm fairly certain that this isn't going to play nice with coninhcount... local constraints would have to be marked as local, else the wrong things will happen later on when you drop them. -- 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] Check constraints on partition parents only?
Hi, > Yeah, but I think we need to take that chance. At the very least, we >> need to support the equivalent of a non-inherited CHECK (false) on >> parent tables. >> > > Indeed. I usually enforce that with a trigger that raises an exception, but > of course that doesn't help at all with constraint exclusion, and I saw a > result just a few weeks ago (I forget the exact details) where it appeared > that the plan chosen was significantly worse because the parent table wasn't > excluded, so there's a non-trivial downside from having this restriction. > > The downside appears to be non-trivial indeed! I cooked up the attached patch to try to allow ALTER...ONLY...CHECK(false) on parent tables. If this approach looks acceptable, I can provide a complete patch later with some documentation changes (I think we ought to tell about this special case in the documentation) and a minor test case along with it (if the need be felt for the test case). Although partitioning ought to be looked at from a different angle completely, maybe this small patch can help out a bit in the current scheme of things, although this is indeed a unusual special casing... Thoughts? Regards, Nikhils diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 82bb756..5340402 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -5433,6 +5433,7 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, ListCell *lcon; List *children; ListCell *child; + bool skip_children = false; /* At top level, permission check was done in ATPrepCmd, else do it */ if (recursing) @@ -5502,9 +5503,31 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, * tables; else the addition would put them out of step. */ if (children && !recurse) - ereport(ERROR, + { + /* + * Try a bit harder and check if this is a CHECK(FALSE) kinda + * constraint. Allow if so, otherwise error out + */ + if (list_length(newcons) == 1) + { + CookedConstraint *cooked = linitial(newcons); + + if (cooked->contype == CONSTR_CHECK && cooked->expr) + { +Node *expr = cooked->expr; +if (IsA(expr, Const) && ((Const *)expr)->consttype == BOOLOID && + ((Const *)expr)->constvalue == 0) +{ + skip_children = true; +} + } + } + + if (!skip_children) + ereport(ERROR, (errcode(ERRCODE_INVALID_TABLE_DEFINITION), errmsg("constraint must be added to child tables too"))); + } foreach(child, children) { @@ -5512,6 +5535,13 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel, Relation childrel; AlteredTableInfo *childtab; + /* + * Skipping the constraint should be good enough for the special case. + * No need to even release the locks on the children immediately.. + */ + if (skip_children) + break; + /* find_inheritance_children already got lock */ childrel = heap_open(childrelid, NoLock); CheckTableNotInUse(childrel, "ALTER TABLE"); -- 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] Check constraints on partition parents only?
On Jul 25, 2011, at 9:59 PM, Jerry Sievers wrote: > That our version of partitioning can be overloaded like this though I > think adds power. A bit of which we lost adding the restrictgion. That's why I'd be opposed to any partitioning scheme that removed the ability to have different fields in different children. We've found that ability to be very useful. Likewise, I think we need to have intelligent plans involving a parent table that's either completely empty or mostly empty. As for dealing with inheritance and putting stuff on some children but not others, take a look at http://pgfoundry.org/projects/enova-tools/. There's a presentation there that discusses how we solved these issues and it includes the tools we created to do it. Note that we're close to releasing a cleaner version of that stuff, so if you decide to use it please ping me off-list if we haven't gotten the new stuff posted. -- Jim C. Nasby, Database Architect j...@nasby.net 512.569.9461 (cell) http://jim.nasby.net -- 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] Check constraints on partition parents only?
Andrew Dunstan writes: > On 07/25/2011 10:31 PM, Jerry Sievers wrote: >> Hackers; >> >> I just noticed that somewhere between 8.2 and 8.4, an exception is >> raised trying to alter table ONLY some_partition_parent ADD CHECK >> (foo). >> > > > 8.4 had this change: > >* > > Force child tables to inherit CHECK constraints from parents > (Alex Hunsaker, Nikhil Sontakke, Tom) > > Formerly it was possible to drop such a constraint from a > child table, allowing rows that violate the constraint to be > visible when scanning the parent table. This was deemed > inconsistent, as well as contrary to SQL standard. > > > You're not the only one who occasionally bangs his head against it. > > cheers > > andrew Thanks Andrew!... Yeah, I figured it was a documented change but too lazy tonight to browse release notes :-) The previous behavior was to me convenient, but I agree, probably lead to some confusion too. That our version of partitioning can be overloaded like this though I think adds power. A bit of which we lost adding the restrictgion. > > > > -- Jerry Sievers e: jerry.siev...@comcast.net p: 305.321.1144 -- 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] Check constraints on partition parents only?
On Tue, Jul 26, 2011 at 10:51:58AM -0400, Tom Lane wrote: > Robert Haas writes: > > On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke > > wrote: > >> Hmmm, but then it does open up the possibility of naive users shooting > >> themselves in the foot. It can be easy to conjure up a > >> parent-only-constraint that does not gel too well with its children. And > >> that's precisely why this feature was added in the first place.. > > > Yeah, but I think we need to take that chance. At the very least, we > > need to support the equivalent of a non-inherited CHECK (false) on > > parent tables. > > No, the right solution is to invent an actual concept of partitioned > tables, not to keep adding ever-weirder frammishes to inheritance so > that it can continue to provide an awkward, poorly-performing emulation > of them. Other SQL engines have partitions of types list, range and hash, and some can sub-partition. I'm thinking it might be easiest to do the first before adding layers of partition structure, although we should probably bear in mind that such layers will eventually exist. Does the wiki on this need updating? http://wiki.postgresql.org/wiki/Table_partitioning Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- 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] Check constraints on partition parents only?
Robert Haas writes: > On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke > wrote: >> Hmmm, but then it does open up the possibility of naive users shooting >> themselves in the foot. It can be easy to conjure up a >> parent-only-constraint that does not gel too well with its children. And >> that's precisely why this feature was added in the first place.. > Yeah, but I think we need to take that chance. At the very least, we > need to support the equivalent of a non-inherited CHECK (false) on > parent tables. No, the right solution is to invent an actual concept of partitioned tables, not to keep adding ever-weirder frammishes to inheritance so that it can continue to provide an awkward, poorly-performing emulation of them. regards, tom lane -- 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] Check constraints on partition parents only?
On 07/26/2011 09:08 AM, Robert Haas wrote: On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke wrote: Yeah. I think it's good that there's a barrier to blindly dropping a constraint that may be important to have on children, but there should be a way to override that. Hmmm, but then it does open up the possibility of naive users shooting themselves in the foot. It can be easy to conjure up a parent-only-constraint that does not gel too well with its children. And that's precisely why this feature was added in the first place.. Yeah, but I think we need to take that chance. At the very least, we need to support the equivalent of a non-inherited CHECK (false) on parent tables. Indeed. I usually enforce that with a trigger that raises an exception, but of course that doesn't help at all with constraint exclusion, and I saw a result just a few weeks ago (I forget the exact details) where it appeared that the plan chosen was significantly worse because the parent table wasn't excluded, so there's a non-trivial downside from having this restriction. cheers andrew -- 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] Check constraints on partition parents only?
On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke wrote: >> Yeah. I think it's good that there's a barrier to blindly dropping a >> constraint that may be important to have on children, but there should >> be a way to override that. > > Hmmm, but then it does open up the possibility of naive users shooting > themselves in the foot. It can be easy to conjure up a > parent-only-constraint that does not gel too well with its children. And > that's precisely why this feature was added in the first place.. Yeah, but I think we need to take that chance. At the very least, we need to support the equivalent of a non-inherited CHECK (false) on parent tables. -- 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] Check constraints on partition parents only?
> > 8.4 had this change: > > > > * > > Force child tables to inherit CHECK constraints from parents > > (Alex Hunsaker, Nikhil Sontakke, Tom) > > > You're not the only one who occasionally bangs his head against it. > > Sorry for the occasional head bumps :) > Yeah. I think it's good that there's a barrier to blindly dropping a > constraint that may be important to have on children, but there should > be a way to override that. > > Hmmm, but then it does open up the possibility of naive users shooting themselves in the foot. It can be easy to conjure up a parent-only-constraint that does not gel too well with its children. And that's precisely why this feature was added in the first place.. Regards, Nikhils
Re: [HACKERS] Check constraints on partition parents only?
Excerpts from Andrew Dunstan's message of lun jul 25 22:44:32 -0400 2011: > > On 07/25/2011 10:31 PM, Jerry Sievers wrote: > > Hackers; > > > > I just noticed that somewhere between 8.2 and 8.4, an exception is > > raised trying to alter table ONLY some_partition_parent ADD CHECK > > (foo). > 8.4 had this change: > > * > Force child tables to inherit CHECK constraints from parents > (Alex Hunsaker, Nikhil Sontakke, Tom) > You're not the only one who occasionally bangs his head against it. Yeah. I think it's good that there's a barrier to blindly dropping a constraint that may be important to have on children, but there should be a way to override that. -- Álvaro Herrera 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] Check constraints on partition parents only?
On 07/25/2011 10:31 PM, Jerry Sievers wrote: Hackers; I just noticed that somewhere between 8.2 and 8.4, an exception is raised trying to alter table ONLY some_partition_parent ADD CHECK (foo). I can understand why it makes sense to handle this as an error. Howeverin practice on a few systems that I used to manage this would be a problem. 1. I got into the habit of putting CHECK (false) on the parent table if it was an always empty base table, This is just really documentation indicating that this table can't hold rows and of course, having the partition selector trigger raise exception if falling through the if/else logic on a new row insertion enforces the constraint but is less obvious. Ok, so no real problem here. Just one example. 2. Atypical partitioning implementation where the parent table was for initial insert/update of "live" records in an OLTP system with high update/insert ratio. This table was partitioned retroactively in such a way transparent to the application. The app would eventually update a row one final time and set a status field to some terminal status, at which time we'd fire a trigger to move the row down into a partition. Record expiry took place periodically by dropping a partition and creating a new one. In that case, imagine the application user runs with sql_inheritance to off and so, sees only the live data which resulted in a huge performance boost. Reporting apps and in fact all other users ran with sql_inheritance to on as usual and so, see all the data. Suppose the status field had several non-terminal values and one or a few terminal values. The differing check constraints on parent and child tables made it easy to see the intent and I presume with constraint_exclusion set to on, let queries on behalf of regular users that had specified a non-terminal state visit only the tiny parent table. Parent might have CHECK (status in (1,2,3)) and children CHECK (status = 4). I'll assume not many sites are architected this way but #2 here shows a more compelling example of why it might be useful to allow check constraints added to only a partition parent. 8.4 had this change: * Force child tables to inherit CHECK constraints from parents (Alex Hunsaker, Nikhil Sontakke, Tom) Formerly it was possible to drop such a constraint from a child table, allowing rows that violate the constraint to be visible when scanning the parent table. This was deemed inconsistent, as well as contrary to SQL standard. You're not the only one who occasionally bangs his head against it. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Check constraints on partition parents only?
Hackers; I just noticed that somewhere between 8.2 and 8.4, an exception is raised trying to alter table ONLY some_partition_parent ADD CHECK (foo). I can understand why it makes sense to handle this as an error. Howeverin practice on a few systems that I used to manage this would be a problem. 1. I got into the habit of putting CHECK (false) on the parent table if it was an always empty base table, This is just really documentation indicating that this table can't hold rows and of course, having the partition selector trigger raise exception if falling through the if/else logic on a new row insertion enforces the constraint but is less obvious. Ok, so no real problem here. Just one example. 2. Atypical partitioning implementation where the parent table was for initial insert/update of "live" records in an OLTP system with high update/insert ratio. This table was partitioned retroactively in such a way transparent to the application. The app would eventually update a row one final time and set a status field to some terminal status, at which time we'd fire a trigger to move the row down into a partition. Record expiry took place periodically by dropping a partition and creating a new one. In that case, imagine the application user runs with sql_inheritance to off and so, sees only the live data which resulted in a huge performance boost. Reporting apps and in fact all other users ran with sql_inheritance to on as usual and so, see all the data. Suppose the status field had several non-terminal values and one or a few terminal values. The differing check constraints on parent and child tables made it easy to see the intent and I presume with constraint_exclusion set to on, let queries on behalf of regular users that had specified a non-terminal state visit only the tiny parent table. Parent might have CHECK (status in (1,2,3)) and children CHECK (status = 4). I'll assume not many sites are architected this way but #2 here shows a more compelling example of why it might be useful to allow check constraints added to only a partition parent. Comments? -- Jerry Sievers Postgres DBA/Development Consulting e: postgres.consult...@comcast.net p: 305.321.1144 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers