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.consulting@comcast.net
p: 305.321.1144
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
Andrew Dunstan <andrew@dunslane.net> 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.sievers@comcast.net
p: 305.321.1144
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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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
On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
On 07/26/2011 09:08 AM, Robert Haas wrote:
On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
On Tue, Jul 26, 2011 at 10:51:58AM -0400, Tom Lane wrote:
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, Jul 26, 2011 at 4:12 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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 <david@fetter.org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david.fetter@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
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 jim@nasby.net
512.569.9461 (cell) http://jim.nasby.net
Jim,
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.
Well, I don't think that anyone is proposing making constraint exclusion
go away. However, we also need a new version of partitioning which
happens "below" the table level. I don't agree that the new
partitioning needs -- at least at the start -- the level of flexibility
which CE gives the user. In order to get simplicity, we have to
sacrifice flexibility.
In fact, I'd suggest extreme simplicity for the first version of this,
with just key partitioning. That is:
CREATE TABLE <table_name> (
... cols ... )
PARTITION ON <key_expression>
[ AUTOMATIC CREATE ];
... where <key_expression> can be any immutable expression on one or
more columns of some_table. This actually covers range partitioning as
well, provided that the ranges can be expressed as the results of an
expression (e.g. EXTRACT ('month' FROM date_processed ) ).
For the optional AUTOMATIC CREATE phrase, new values for key_expression
would result in the automatic creation of new partitions when they
appear (this has some potential deadlocking issues, so it's not ideal
for a lot of applications). Otherwise, you'd create partitions manually:
CREATE PARTITION ON <table_name> KEY <key_value>;
DROP PARTITION ON <table_name> KEY <key_value>;
... where <key_value> is some valid value which could result from
<key_expression>.
Yes, this is a very narrow and simplistic partitioning spec. However,
it would cover 80% of the use cases I see in the field or on IRC, while
being 80% simpler than CE. And CE would still be there for those who
need it.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
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
Attachments:
special_case_CHECK_parent.patchtext/x-patch; charset=US-ASCII; name=special_case_CHECK_parent.patchDownload
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");
On Wed, Jul 27, 2011 at 6:39 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
Robert Haas <robertmhaas@gmail.com> 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
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
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
On Wed, Jul 27, 2011 at 4:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> 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
On Wed, Jul 27, 2011 at 14:08, Tom Lane <tgl@sss.pgh.pa.us> 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 :-)
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
Nikhil Sontakke <nikhil.sontakke@enterprisedb.com> 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
On Thu, Jul 28, 2011 at 9:43 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
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 constraintI 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
On Thu, Jul 28, 2011 at 10:01 AM, Nikhil Sontakke
<nikhil.sontakke@enterprisedb.com> 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
On Tue, Jul 26, 2011 at 7:58 PM, Josh Berkus <josh@agliodbs.com> wrote:
Jim,
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.
Well, I don't think that anyone is proposing making constraint exclusion
go away. However, we also need a new version of partitioning which
happens "below" the table level. I don't agree that the new
partitioning needs -- at least at the start -- the level of flexibility
which CE gives the user. In order to get simplicity, we have to
sacrifice flexibility.
Agreed.
In fact, I'd suggest extreme simplicity for the first version of this,
with just key partitioning. That is:CREATE TABLE <table_name> (
... cols ... )
PARTITION ON <key_expression>
[ AUTOMATIC CREATE ];
I think that the automatic create feature is just about impossible to
implement reliably, at least not without autonomous transactions.
There are big problems here in the case of concurrent activity.
What Itagaki Takahiro proposed a year ago was basically something
where you would say, OK, I want to partition on this column (or maybe
expression). And then you say:
If the value is less than v1, put it in a partition called p1.
If the value is less than v2, put it in a position called p2.
<repeat ad nauseum, and then, optionally:>
If the value is not less than any of the above, put it in a partition
called poverflow.
I like that design, not least but also not only because it's similar
to what one of our competitors does.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Robert,
If the value is less than v1, put it in a partition called p1.
If the value is less than v2, put it in a position called p2.
<repeat ad nauseum, and then, optionally:>
If the value is not less than any of the above, put it in a partition
called poverflow.I like that design, not least but also not only because it's similar
to what one of our competitors does.
Sure. I'm just restarting the discussion from the point of "what's the
very simplest implementation of partitioning we could create and still
be useful?"
There's value in similicity. First, by having a very simple
implementation it's more likely someone will code it. If we let
-hackers pile on the "must have X feature" to a new partitioning
implementation, it'll never get built.
Second, the key-based partitioning I described would actually be
preferred to what you describe by a lot of users I know, because it's
even simpler than what you propose, which means less contract DBA work
they have to pay for to set it up.
I'm sure what we eventually implement will be a compromise. I just want
to push the discussion away from the "must have every feature under the
sun" direction and towards something that might actually work.
Oh, and no question that automatic partitioning will be a PITA and might
not be implemented for years. But it's a serious user desire.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
On Thu, Jul 28, 2011 at 12:53 PM, Josh Berkus <josh@agliodbs.com> wrote:
Robert,
If the value is less than v1, put it in a partition called p1.
If the value is less than v2, put it in a position called p2.
<repeat ad nauseum, and then, optionally:>
If the value is not less than any of the above, put it in a partition
called poverflow.
Sure. I'm just restarting the discussion from the point of "what's the
very simplest implementation of partitioning we could create and still
be useful?"
Second, the key-based partitioning I described would actually be
preferred to what you describe by a lot of users I know, because it's
even simpler than what you propose, which means less contract DBA work
they have to pay for to set it up.
But part of the desire for "simple partitioning" is to make sure the
query planner and execution knows about partitions, can do exclude
unnecessary partitions from queries. If partion knowledge doesn't
help the query plans, its not much use excpt to reduce table size,
which isn't a hard task with the current inheritance options.
But if the "partition" selection is an opaque "simple key" type
function, you haven't given the planner/executor anything better to be
able to pick partitions for queries, unless the query is an exact "key
=" type of operation.
So I'm failing to see the benefit of that "key based" partitioning,
even if that key-based function was something like date_trunc on a
timestamp...
a.
--
Aidan Van Dyk Create like a god,
aidan@highrise.ca command like a king,
http://www.highrise.ca/ work like a slave.
On Thu, Jul 28, 2011 at 10:20:57AM -0400, Robert Haas wrote:
What Itagaki Takahiro proposed a year ago was basically something
where you would say, OK, I want to partition on this column (or maybe
expression). And then you say:If the value is less than v1, put it in a partition called p1.
If the value is less than v2, put it in a position called p2.
<repeat ad nauseum, and then, optionally:>
If the value is not less than any of the above, put it in a partition
called poverflow.I like that design, not least but also not only because it's similar
to what one of our competitors does.
FWIW, this seems to me to be the most useful design, because the other
nice use for partitioning is being able to throw away old data without
leaving huge chunks of deleted row. If the column you partition on
is a timestamp, then the above scheme makes it easy to just drop the
oldest partition when the disk is nearly full.
Have a nice day,
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
He who writes carelessly confesses thereby at the very outset that he does
not attach much importance to his own thoughts.
-- Arthur Schopenhauer
Aidan Van Dyk <aidan@highrise.ca> writes:
On Thu, Jul 28, 2011 at 12:53 PM, Josh Berkus <josh@agliodbs.com> wrote:
Second, the key-based partitioning I described would actually be
preferred to what you describe by a lot of users I know, because it's
even simpler than what you propose, which means less contract DBA work
they have to pay for to set it up.
But part of the desire for "simple partitioning" is to make sure the
query planner and execution knows about partitions, can do exclude
unnecessary partitions from queries. If partion knowledge doesn't
help the query plans, its not much use excpt to reduce table size,
which isn't a hard task with the current inheritance options.
But if the "partition" selection is an opaque "simple key" type
function, you haven't given the planner/executor anything better to be
able to pick partitions for queries, unless the query is an exact "key
=" type of operation.
Right. I think the *minimum* requirement for intelligent planning is
that the partitioning be based on ranges of a btree-sortable type.
Single values is a special case of that (for discrete types anyway),
but it doesn't cover enough cases to be the primary definition.
regards, tom lane
Hi,
Any preferences for the name?
connoinh
conisonly
constatic or confixedI'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
On Fri, Jul 29, 2011 at 7:41 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi,
Any preferences for the name?
connoinh
conisonly
constatic or confixedI'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
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
On Fri, Jul 29, 2011 at 8:30 AM, Nikhil Sontakke <nikkhils@gmail.com> 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
Nikhil Sontakke <nikkhils@gmail.com> 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
Robert Haas <robertmhaas@gmail.com> writes:
On Fri, Jul 29, 2011 at 8:30 AM, Nikhil Sontakke <nikkhils@gmail.com> 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
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
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
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
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
Attachments:
non_inheritable_check_constraints.patchtext/plain; charset=UTF-8; name=non_inheritable_check_constraints.patchDownload
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 @@
</row>
<row>
+ <entry><structfield>conisonly</structfield></entry>
+ <entry><type>bool</type></entry>
+ <entry></entry>
+ <entry>
+ This constraint is defined locally for the relation. It is a
+ non-inheritable constraint.
+ </entry>
+ </row>
+
+ <row>
<entry><structfield>conkey</structfield></entry>
<entry><type>int2[]</type></entry>
<entry><literal><link linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.attnum</></entry>
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);
</para>
<para>
+ To add a check constraint only to a table and not to its children:
+<programlisting>
+ALTER TABLE ONLY distributors ADD CONSTRAINT zipchk CHECK (char_length(zipcode) = 5);
+</programlisting>
+ (The check constraint will not be inherited by future children too.)
+ </para>
+
+ <para>
To remove a check constraint from a table and all its children:
<programlisting>
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.)
*/
if (MergeWithExistingConstraint(rel, ccname, expr,
- allow_merge, is_local))
+ allow_merge, is_local, cdef->is_only))
continue;
}
else
@@ -2214,7 +2216,7 @@ AddRelationNewConstraints(Relation rel,
* OK, store it.
*/
StoreRelCheck(rel, ccname, expr, !cdef->skip_validation, is_local,
- is_local ? 0 : 1);
+ is_local ? 0 : 1, cdef->is_only);
numchecks++;
@@ -2226,6 +2228,7 @@ AddRelationNewConstraints(Relation rel,
cooked->skip_validation = cdef->skip_validation;
cooked->is_local = is_local;
cooked->inhcount = is_local ? 0 : 1;
+ cooked->is_only = cdef->is_only;
cookedConstraints = lappend(cookedConstraints, cooked);
}
@@ -2251,7 +2254,7 @@ AddRelationNewConstraints(Relation rel,
*/
static bool
MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
- bool allow_merge, bool is_local)
+ bool allow_merge, bool is_local, bool is_only)
{
bool found;
Relation conDesc;
@@ -2313,6 +2316,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;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 75b4c14..477cad3 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1160,7 +1160,8 @@ index_constraint_create(Relation heapRelation,
NULL,
NULL,
true, /* islocal */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* isonly */
/*
* Register the index as internally dependent on the constraint.
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 6997994..cfe82ea 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -66,7 +66,8 @@ CreateConstraintEntry(const char *constraintName,
const char *conBin,
const char *conSrc,
bool conIsLocal,
- int conInhCount)
+ int conInhCount,
+ bool conIsOnly)
{
Relation conDesc;
Oid conOid;
@@ -169,6 +170,7 @@ CreateConstraintEntry(const char *constraintName,
values[Anum_pg_constraint_confmatchtype - 1] = CharGetDatum(foreignMatchType);
values[Anum_pg_constraint_conislocal - 1] = BoolGetDatum(conIsLocal);
values[Anum_pg_constraint_coninhcount - 1] = Int32GetDatum(conInhCount);
+ values[Anum_pg_constraint_conisonly - 1] = BoolGetDatum(conIsOnly);
if (conkeyArray)
values[Anum_pg_constraint_conkey - 1] = PointerGetDatum(conkeyArray);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 82bb756..ac2368f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -569,6 +569,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
cooked->skip_validation = false;
cooked->is_local = true; /* not used for defaults */
cooked->inhcount = 0; /* ditto */
+ cooked->is_only = false;
cookedDefaults = lappend(cookedDefaults, cooked);
descriptor->attrs[attnum - 1]->atthasdef = true;
}
@@ -1566,6 +1567,10 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
char *name = check[i].ccname;
Node *expr;
+ /* ignore if the constraint is non-inheritable */
+ if (check[i].cconly)
+ continue;
+
/* adjust varattnos of ccbin here */
expr = stringToNode(check[i].ccbin);
change_varattnos_of_a_node(expr, newattno);
@@ -1584,6 +1589,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
cooked->skip_validation = false;
cooked->is_local = false;
cooked->inhcount = 1;
+ cooked->is_only = false;
constraints = lappend(constraints, cooked);
}
}
@@ -5433,6 +5439,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)
@@ -5499,12 +5506,26 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
/*
* If we are told not to recurse, there had better not be any child
- * tables; else the addition would put them out of step.
+ * tables; else the addition would put them out of step. Unless these are
+ * ONLY type of constraints of course.
*/
if (children && !recurse)
- ereport(ERROR,
+ {
+ foreach(lcon, newcons)
+ {
+ CookedConstraint *ccon = (CookedConstraint *) lfirst(lcon);
+
+ if (ccon->is_only)
+ skip_children = true;
+ else
+ skip_children = false;
+ }
+
+ if (!skip_children)
+ ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("constraint must be added to child tables too")));
+ }
foreach(child, children)
{
@@ -5512,6 +5533,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");
@@ -5799,7 +5827,8 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
NULL,
NULL,
true, /* islocal */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* isonly */
/*
* Create the triggers that will enforce the constraint.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 4c31f19..d24fb6b 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -451,7 +451,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
NULL,
NULL,
true, /* islocal */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* isonly */
}
/*
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 7c27f85..a6c1ab3 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2542,8 +2542,9 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
expr, /* Tree form of check constraint */
ccbin, /* Binary form of check constraint */
ccsrc, /* Source form of check constraint */
- true, /* is local */
- 0); /* inhcount */
+ true, /* is local */
+ 0, /* inhcount */
+ false); /* is only */
/*
* Return the compiled constraint expression so the calling routine can
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7a51456..e04d082 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2328,6 +2328,7 @@ _copyConstraint(Constraint *from)
COPY_LOCATION_FIELD(location);
COPY_NODE_FIELD(raw_expr);
COPY_STRING_FIELD(cooked_expr);
+ COPY_SCALAR_FIELD(is_only);
COPY_NODE_FIELD(keys);
COPY_NODE_FIELD(exclusions);
COPY_NODE_FIELD(options);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 4052a9a..4e13451 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2257,6 +2257,7 @@ _equalConstraint(Constraint *a, Constraint *b)
COMPARE_LOCATION_FIELD(location);
COMPARE_NODE_FIELD(raw_expr);
COMPARE_STRING_FIELD(cooked_expr);
+ COMPARE_SCALAR_FIELD(is_only);
COMPARE_NODE_FIELD(keys);
COMPARE_NODE_FIELD(exclusions);
COMPARE_NODE_FIELD(options);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index b5be09a..f0d65b5 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2585,6 +2585,8 @@ _outConstraint(StringInfo str, Constraint *node)
break;
case CONSTR_CHECK:
+ if (node->is_only)
+ appendStringInfo(str, " (ONLY) ");
appendStringInfo(str, "CHECK");
WRITE_NODE_FIELD(raw_expr);
WRITE_STRING_FIELD(cooked_expr);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index ac094aa..df0af9d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -1621,6 +1621,24 @@ AlterTableStmt:
n->relation = $3;
n->cmds = $4;
n->relkind = OBJECT_TABLE;
+ /* Check if ONLY was used in the relation expr */
+ if (n->relation->inhOpt == INH_NO)
+ {
+ ListCell *lcmd;
+ foreach(lcmd, n->cmds)
+ {
+ AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
+
+ /* mark check constraints as non-inheritable */
+ if (cmd->subtype == AT_AddConstraint)
+ {
+ Constraint *n = (Constraint *)cmd->def;
+
+ if (n->contype == CONSTR_CHECK)
+ n->is_only = true;
+ }
+ }
+ }
$$ = (Node *)n;
}
| ALTER INDEX qualified_name alter_table_cmds
@@ -2625,6 +2643,7 @@ ColConstraintElem:
n->location = @1;
n->raw_expr = $3;
n->cooked_expr = NULL;
+ n->is_only = false;
$$ = (Node *)n;
}
| DEFAULT b_expr
@@ -2758,6 +2777,7 @@ ConstraintElem:
n->location = @1;
n->raw_expr = $3;
n->cooked_expr = NULL;
+ n->is_only = false;
processCASbits($5, @5, "CHECK",
NULL, NULL, &n->skip_validation,
yyscanner);
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 809222b..940f122 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3251,6 +3251,7 @@ CheckConstraintFetch(Relation relation)
RelationGetRelationName(relation));
check[found].ccvalid = conform->convalidated;
+ check[found].cconly = conform->conisonly;
check[found].ccname = MemoryContextStrdup(CacheMemoryContext,
NameStr(conform->conname));
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index f2ee57c..7f40948 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -5895,11 +5895,22 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
tbinfo->dobj.name);
resetPQExpBuffer(q);
- if (g_fout->remoteVersion >= 80400)
+ if (g_fout->remoteVersion > 90100)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "conislocal "
+ "conislocal, 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, "
+ "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
+ "conislocal, false AS conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -5910,7 +5921,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "true AS conislocal "
+ "true AS conislocal, false AS conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -5922,7 +5933,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
/* no pg_get_constraintdef, must use consrc */
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"'CHECK (' || consrc || ')' AS consrc, "
- "true AS conislocal "
+ "true AS conislocal, false AS conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -5935,7 +5946,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, 0 AS oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal "
+ "true AS conislocal, false AS conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -5946,7 +5957,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal "
+ "true AS conislocal, false AS conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -5959,7 +5970,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
"(SELECT oid FROM pg_class WHERE relname = 'pg_relcheck') AS tableoid, "
"oid, rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal "
+ "true AS conislocal, false AS conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -5999,7 +6010,11 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
constrs[j].condeferrable = false;
constrs[j].condeferred = false;
constrs[j].conislocal = (PQgetvalue(res, j, 4)[0] == 't');
- constrs[j].separate = false;
+ constrs[j].conisonly = (PQgetvalue(res, j, 5)[0] == 't');
+ if (constrs[j].conisonly)
+ constrs[j].separate = true;
+ else
+ constrs[j].separate = false;
constrs[j].dobj.dump = tbinfo->dobj.dump;
@@ -6008,8 +6023,15 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
* --- this is so that any other dependencies of the
* constraint will be emitted before we try to create the
* table.
+ *
+ * But if it is an ONLY object, the constraint has to appear
+ * after the create table.
*/
- addObjectDependency(&tbinfo->dobj,
+ if (constrs[j].conisonly)
+ addObjectDependency(&constrs[j].dobj,
+ tbinfo->dobj.dumpId);
+ else
+ addObjectDependency(&tbinfo->dobj,
constrs[j].dobj.dumpId);
/*
@@ -12823,9 +12845,9 @@ dumpConstraint(Archive *fout, ConstraintInfo *coninfo)
/* Ignore if not to be dumped separately */
if (coninfo->separate)
{
- /* not ONLY since we want it to propagate to children */
- appendPQExpBuffer(q, "ALTER TABLE %s\n",
- fmtId(tbinfo->dobj.name));
+ /* add ONLY if we do not want it to propagate to children */
+ appendPQExpBuffer(q, "ALTER TABLE %s %s\n",
+ coninfo->conisonly? "ONLY":"", fmtId(tbinfo->dobj.name));
appendPQExpBuffer(q, " ADD CONSTRAINT %s %s;\n",
fmtId(coninfo->dobj.name),
coninfo->condef);
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index c95614b..ee25311 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -376,6 +376,7 @@ typedef struct _constraintInfo
bool condeferrable; /* TRUE if constraint is DEFERRABLE */
bool condeferred; /* TRUE if constraint is INITIALLY DEFERRED */
bool conislocal; /* TRUE if constraint has local definition */
+ bool conisonly; /* TRUE if constraint is non-inheritable */
bool separate; /* TRUE if must dump as separate item */
} ConstraintInfo;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index b50c5d6..00c2051 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1729,12 +1729,18 @@ 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 r.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);
+ is_only, oid);
result = PSQLexec(buf.data, false);
if (!result)
goto error_return;
@@ -1747,9 +1753,10 @@ describeOneTableDetails(const char *schemaname,
for (i = 0; i < tuples; i++)
{
/* untranslated contraint name and def */
- printfPQExpBuffer(&buf, " \"%s\" %s",
+ printfPQExpBuffer(&buf, " \"%s\"%s%s",
PQgetvalue(result, i, 0),
- PQgetvalue(result, i, 1));
+ (strcmp(PQgetvalue(result, i, 1), "t") == 0) ? " (ONLY) ":" ",
+ PQgetvalue(result, i, 2));
printTableAddFooter(&cont, buf.data);
}
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index 8b99cb8..d5e1333 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -30,6 +30,7 @@ typedef struct constrCheck
char *ccname;
char *ccbin; /* nodeToString representation of expr */
bool ccvalid;
+ bool cconly; /* this is a non-inheritable constraint */
} ConstrCheck;
/* This structure contains constraints of a tuple */
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index aee2d88..d3a588f 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -33,6 +33,7 @@ typedef struct CookedConstraint
bool skip_validation; /* skip validation? (only for CHECK) */
bool is_local; /* constraint has local (non-inherited) def */
int inhcount; /* number of times constraint is inherited */
+ bool is_only; /* constraint has local def and cannot be inherited */
} CookedConstraint;
extern Relation heap_create(const char *relname,
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 1566af2..b8fb01d 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -88,6 +88,9 @@ CATALOG(pg_constraint,2606)
/* Number of times inherited from direct parent relation(s) */
int4 coninhcount;
+ /* Has a local definition and cannot be inherited */
+ bool conisonly;
+
/*
* VARIABLE LENGTH FIELDS start here. These fields may be NULL, too.
*/
@@ -165,14 +168,15 @@ typedef FormData_pg_constraint *Form_pg_constraint;
#define Anum_pg_constraint_confmatchtype 13
#define Anum_pg_constraint_conislocal 14
#define Anum_pg_constraint_coninhcount 15
-#define Anum_pg_constraint_conkey 16
-#define Anum_pg_constraint_confkey 17
-#define Anum_pg_constraint_conpfeqop 18
-#define Anum_pg_constraint_conppeqop 19
-#define Anum_pg_constraint_conffeqop 20
-#define Anum_pg_constraint_conexclop 21
-#define Anum_pg_constraint_conbin 22
-#define Anum_pg_constraint_consrc 23
+#define Anum_pg_constraint_conisonly 16
+#define Anum_pg_constraint_conkey 17
+#define Anum_pg_constraint_confkey 18
+#define Anum_pg_constraint_conpfeqop 19
+#define Anum_pg_constraint_conppeqop 20
+#define Anum_pg_constraint_conffeqop 21
+#define Anum_pg_constraint_conexclop 22
+#define Anum_pg_constraint_conbin 23
+#define Anum_pg_constraint_consrc 24
/* Valid values for contype */
@@ -227,7 +231,8 @@ extern Oid CreateConstraintEntry(const char *constraintName,
const char *conBin,
const char *conSrc,
bool conIsLocal,
- int conInhCount);
+ int conInhCount,
+ bool conIsOnly);
extern void RemoveConstraintById(Oid conId);
extern void RenameConstraintById(Oid conId, const char *newname);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 92e40d3..d3dc246 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1522,6 +1522,7 @@ typedef struct Constraint
/* Fields used for constraints with expressions (CHECK and DEFAULT): */
Node *raw_expr; /* expr, as untransformed parse tree */
char *cooked_expr; /* expr, as nodeToString representation */
+ bool is_only; /* has local definition, cannot be inherited */
/* Fields used for unique constraints (UNIQUE and PRIMARY KEY): */
List *keys; /* String nodes naming referenced column(s) */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 005a88b..d6c1bc1 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -493,16 +493,16 @@ select test2 from atacc2;
drop table atacc2 cascade;
NOTICE: drop cascades to table atacc3
drop table atacc1;
--- adding only to a parent is disallowed as of 8.4
+-- adding only to a parent is allowed as of 9.2
create table atacc1 (test int);
create table atacc2 (test2 int) inherits (atacc1);
--- fail:
+-- ok:
alter table only atacc1 add constraint foo check (test>0);
-ERROR: constraint must be added to child tables too
-- ok:
alter table only atacc2 add constraint foo check (test>0);
--- check constraint not there on parent
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
+ERROR: new row for relation "atacc1" violates check constraint "foo"
insert into atacc1 (test) values (3);
-- check constraint is there on child
insert into atacc2 (test) values (-3);
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index d59ca44..16abada 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -681,6 +681,41 @@ select * from d;
32 | one | two | three
(1 row)
+-- Test non-inheritable parent constraints
+create table p1(ff1 int);
+alter table only p1 add constraint p1chk check (ff1 > 0);
+alter table p1 add constraint p2chk check (ff1 > 10);
+-- conisonly should be true for ONLY constraint
+select pc.relname, pgc.conname, pgc.contype, pgc.conislocal, pgc.coninhcount, pgc.conisonly from pg_class as pc inner join pg_constraint as pgc on (pgc.conrelid = pc.oid) where pc.relname = 'p1';
+ relname | conname | contype | conislocal | coninhcount | conisonly
+---------+---------+---------+------------+-------------+-----------
+ p1 | p1chk | c | t | 0 | t
+ p1 | p2chk | c | t | 0 | f
+(2 rows)
+
+-- Test that child does not inherit ONLY constraints
+create table c1 () inherits (p1);
+\d p1
+ Table "public.p1"
+ Column | Type | Modifiers
+--------+---------+-----------
+ ff1 | integer |
+Check constraints:
+ "p1chk" (ONLY) CHECK (ff1 > 0)
+ "p2chk" CHECK (ff1 > 10)
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d c1
+ Table "public.c1"
+ Column | Type | Modifiers
+--------+---------+-----------
+ ff1 | integer |
+Check constraints:
+ "p2chk" CHECK (ff1 > 10)
+Inherits: p1
+
+drop table p1 cascade;
+NOTICE: drop cascades to table c1
-- Tests for casting between the rowtypes of parent and child
-- tables. See the pgsql-hackers thread beginning Dec. 4/04
create table base (i integer);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 95e898c..61d63d5 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -450,15 +450,15 @@ select test2 from atacc2;
drop table atacc2 cascade;
drop table atacc1;
--- adding only to a parent is disallowed as of 8.4
+-- adding only to a parent is allowed as of 9.2
create table atacc1 (test int);
create table atacc2 (test2 int) inherits (atacc1);
--- fail:
+-- ok:
alter table only atacc1 add constraint foo check (test>0);
-- ok:
alter table only atacc2 add constraint foo check (test>0);
--- check constraint not there on parent
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
insert into atacc1 (test) values (3);
-- check constraint is there on child
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 3087a14..9e992ab 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -188,6 +188,20 @@ insert into d values('test','one','two','three');
alter table a alter column aa type integer using bit_length(aa);
select * from d;
+-- Test non-inheritable parent constraints
+create table p1(ff1 int);
+alter table only p1 add constraint p1chk check (ff1 > 0);
+alter table p1 add constraint p2chk check (ff1 > 10);
+-- conisonly should be true for ONLY constraint
+select pc.relname, pgc.conname, pgc.contype, pgc.conislocal, pgc.coninhcount, pgc.conisonly from pg_class as pc inner join pg_constraint as pgc on (pgc.conrelid = pc.oid) where pc.relname = 'p1';
+
+-- Test that child does not inherit ONLY constraints
+create table c1 () inherits (p1);
+\d p1
+\d c1
+
+drop table p1 cascade;
+
-- Tests for casting between the rowtypes of parent and child
-- tables. See the pgsql-hackers thread beginning Dec. 4/04
create table base (i integer);
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 <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
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?
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