Review: Non-inheritable check constraints
Hi! *Waves*
First off, it all seems to work as described:
- regressions pass
- domains work
- tried various inherit options (merging constraints, alter table no
inherit etc)
- pg_dump/restore
I didn't care for the changes to gram.y so I reworked it a bit so we
now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?
I also moved the is_only check in AtAddCheckConstraint() to before we
grab and loop through any children. Seemed a bit wasteful to loop
through the new constraints just to set a flag so that we could bail
out while looping through the children.
You also forgot to bump Natts_pg_constraint.
PFA the above changes as well as being "rebased" against master.
Attachments:
Hi Alex,
I didn't care for the changes to gram.y so I reworked it a bit so we
now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?
Hmmm, your patch checks for a constraint being "only" via:
!recurse && !recursing
I hope that is good enough to conclusively conclude that the constraint is
'only'. This check was not too readable in the existing code for me anyways
;). If we check at the grammar level, we can be sure. But I remember not
being too comfortable about the right position to ascertain this
characteristic.
I also moved the is_only check in AtAddCheckConstraint() to before we
grab and loop through any children. Seemed a bit wasteful to loop
through the new constraints just to set a flag so that we could bail
out while looping through the children.
Ditto comment for this function. I thought this function went to great
lengths to spit out a proper error in case of inconsistencies between parent
and child. But if your check makes it simpler, that's good!
You also forgot to bump Natts_pg_constraint.
Ouch. Thanks for the catch.
PFA the above changes as well as being "rebased" against master.
Thanks Alex, appreciate the review!
Regards,
Nikhils
On Thu, Oct 6, 2011 at 02:42, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi Alex,
I didn't care for the changes to gram.y so I reworked it a bit so we
now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?Hmmm, your patch checks for a constraint being "only" via:
!recurse && !recursing
I hope that is good enough to conclusively conclude that the constraint is
'only'. This check was not too readable in the existing code for me anyways
;). If we check at the grammar level, we can be sure. But I remember not
being too comfortable about the right position to ascertain this
characteristic.
Well I traced through it here was my thinking (maybe should be a comment?):
1: AlterTable() calls ATController() with recurse =
interpretInhOption(stmt->relation->inhOpt
2: ATController() calls ATPrepCmd() with recurse and recursing = false
3: ATPrepCmd() saves the recurse flag with the subtup
"AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint
4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when
subtype == AT_AddConstraintRecurse, recurse = false otherwise
5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and
recursing = false
6: now we are in ATAddCheckConstraint() where recurse ==
interpretInhOption(rv->inhOpt) and recursing == false. Recursing is
only true when ATAddCheckConstaint() loops through children and
recursively calls ATAddCheckConstraint()
So with it all spelled out now I see the "constraint must be added to
child tables too" check is dead code.
Ill take out that check and then mark it as ready for commiter (and
Ill add any comments about the logic of the recurse flag above if I
can think of a concise way to put it). Sound good?
Hi Alex,
Hmmm, your patch checks for a constraint being "only" via:
!recurse && !recursing
I hope that is good enough to conclusively conclude that the constraint
is
'only'. This check was not too readable in the existing code for me
anyways
;). If we check at the grammar level, we can be sure. But I remember not
being too comfortable about the right position to ascertain this
characteristic.Well I traced through it here was my thinking (maybe should be a comment?):
1: AlterTable() calls ATController() with recurse =
interpretInhOption(stmt->relation->inhOpt
2: ATController() calls ATPrepCmd() with recurse and recursing = false
3: ATPrepCmd() saves the recurse flag with the subtup
"AT_AddConstraintRecurse, otherwise the subtype is AT_AddConstraint
4: ATExecCmd() calls ATExecAddConstraint() with recurse == true when
subtype == AT_AddConstraintRecurse, recurse = false otherwise
5: ATExecAddConstraint() calls ATAddCheckConstraint() with recuse and
recursing = false
6: now we are in ATAddCheckConstraint() where recurse ==
interpretInhOption(rv->inhOpt) and recursing == false. Recursing is
only true when ATAddCheckConstaint() loops through children and
recursively calls ATAddCheckConstraint()So with it all spelled out now I see the "constraint must be added to
child tables too" check is dead code.
Thanks the above step-wise explanation helps.
But AFAICS, the default inhOpt value can be governed by the SQL_inheritance
guc too. So in that case, it's possible that recurse is false and child
tables are present, no?
Infact as I now remember, the reason my patch was looping through was to
handle this very case. It was based on the assumptions that some constraints
might be ONLY type and some can be inheritable. Although admittedly the
current ALTER TABLE functionality does not allow this.
So maybe we can still keep this check around IMO.
Regards,
Nikhils
On Fri, Oct 7, 2011 at 00:28, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi Alex,
So with it all spelled out now I see the "constraint must be added to
child tables too" check is dead code.Thanks the above step-wise explanation helps.
But AFAICS, the default inhOpt value can be governed by the SQL_inheritance
guc too. So in that case, it's possible that recurse is false and child
tables are present, no?
Well... Do we really want to differentiate between those two case? I
would argue no.
Given that:
set sql_inhertance to off;
alter table xxx alter column;
behaves the same as
set sql_inhertance to on;
alter table only xxx alter column;
Why should we treat constraints differently? Or put another way if set
sql_inhertance off makes alter table behave with an implicit only,
shouldn't add/drop constraint respect that?
Infact as I now remember, the reason my patch was looping through was to
handle this very case. It was based on the assumptions that some constraints
might be ONLY type and some can be inheritable.
Although admittedly the current ALTER TABLE functionality does not allow this.
Hrm... Ill I see is a user who turned off sql_inhertance wondering why
they have to specify ONLY on some alter table commands and not others.
I think if we want to support "ONLY" constraint types in the way you
are thinking about them, we need to put ONLY some place else (alter
table xxx add only constraint ?). Personally I don't see a reason to
have that kind of constraint. Mostly because I don't see how its
functionally different. Is it somehow?
Anyone else have any thoughts on this?
Hi Alex,
I guess we both are in agreement with each other :)
After sleeping over it, I think that check is indeed dead code with this new
non-inheritable check constraints functionality in place. So unless you have
some other comments, we can mark this as 'Ready for Commiter'.
Again, thanks for the thorough review and subsequent changes!
Regards,
Nikhils
On Fri, Oct 7, 2011 at 12:18 PM, Alex Hunsaker <badalex@gmail.com> wrote:
Show quoted text
On Fri, Oct 7, 2011 at 00:28, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi Alex,
So with it all spelled out now I see the "constraint must be added to
child tables too" check is dead code.Thanks the above step-wise explanation helps.
But AFAICS, the default inhOpt value can be governed by the
SQL_inheritance
guc too. So in that case, it's possible that recurse is false and child
tables are present, no?Well... Do we really want to differentiate between those two case? I
would argue no.Given that:
set sql_inhertance to off;
alter table xxx alter column;
behaves the same as
set sql_inhertance to on;
alter table only xxx alter column;Why should we treat constraints differently? Or put another way if set
sql_inhertance off makes alter table behave with an implicit only,
shouldn't add/drop constraint respect that?Infact as I now remember, the reason my patch was looping through was to
handle this very case. It was based on the assumptions that someconstraints
might be ONLY type and some can be inheritable.
Although admittedly the current ALTER TABLE functionality does not allowthis.
Hrm... Ill I see is a user who turned off sql_inhertance wondering why
they have to specify ONLY on some alter table commands and not others.
I think if we want to support "ONLY" constraint types in the way you
are thinking about them, we need to put ONLY some place else (alter
table xxx add only constraint ?). Personally I don't see a reason to
have that kind of constraint. Mostly because I don't see how its
functionally different. Is it somehow?Anyone else have any thoughts on this?
On Fri, Oct 7, 2011 at 21:30, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi Alex,
I guess we both are in agreement with each other :)
After sleeping over it, I think that check is indeed dead code with this new
non-inheritable check constraints functionality in place. So unless you have
some other comments, we can mark this as 'Ready for Commiter'.Again, thanks for the thorough review and subsequent changes!
PFA an updated patch with the check removed and a comment or two added.
I've also marked it ready.
Attachments:
Excerpts from Alex Hunsaker's message of dom oct 09 03:40:36 -0300 2011:
On Fri, Oct 7, 2011 at 21:30, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Hi Alex,
I guess we both are in agreement with each other :)
After sleeping over it, I think that check is indeed dead code with this new
non-inheritable check constraints functionality in place. So unless you have
some other comments, we can mark this as 'Ready for Commiter'.Again, thanks for the thorough review and subsequent changes!
PFA an updated patch with the check removed and a comment or two added.
I've also marked it ready.
I had a look at this patch today. The pg_dump bits conflict with
another patch I committed a few days ago, so I'm about to merge them.
I have one question which is about this hunk:
@@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
con->conislocal = true;
else
con->coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con->conisonly = true;
+ }
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;Is it okay to modify an existing constraint to mark it as "only", even
if it was originally inheritable? This is not clear to me. Maybe the
safest course of action is to raise an error. Or maybe I'm misreading
what it does (because I haven't compiled it yet).
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I had a look at this patch today. The pg_dump bits conflict with
another patch I committed a few days ago, so I'm about to merge them.
I have one question which is about this hunk:
Thanks for taking a look Alvaro.
@@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char
*ccname, Node *expr,
con->conislocal = true;
else
con->coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con->conisonly = true;
+ }
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;Is it okay to modify an existing constraint to mark it as "only", even
if it was originally inheritable? This is not clear to me. Maybe the
safest course of action is to raise an error. Or maybe I'm misreading
what it does (because I haven't compiled it yet).
Hmmm, good question. IIRC, the patch will pass is_only as true only if it
going to be a locally defined, non-inheritable constraint. So I went by the
logic that since it was ok to merge and mark a constraint as locally
defined, it should be ok to mark it non-inheritable from this moment on
with that new local definition?
Regards,
Nikhils
Regards,
Nikhils
On 12/04/2011 02:22 AM, Nikhil Sontakke wrote:
Is it okay to modify an existing constraint to mark it as "only", even
if it was originally inheritable? This is not clear to me. Maybe the
safest course of action is to raise an error. Or maybe I'm misreading
what it does (because I haven't compiled it yet).Hmmm, good question. IIRC, the patch will pass is_only as true only if
it going to be a locally defined, non-inheritable constraint. So I
went by the logic that since it was ok to merge and mark a constraint
as locally defined, it should be ok to mark it non-inheritable from
this moment on with that new local definition?
With this open question, this looks like it's back in Alvaro's hands
again to me. This one started the CF as "Ready for Committer" and seems
stalled there for now. I'm not going to touch its status, just pointing
this fact out.
--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
Excerpts from Greg Smith's message of vie dic 16 15:02:20 -0300 2011:
On 12/04/2011 02:22 AM, Nikhil Sontakke wrote:
Is it okay to modify an existing constraint to mark it as "only", even
if it was originally inheritable? This is not clear to me. Maybe the
safest course of action is to raise an error. Or maybe I'm misreading
what it does (because I haven't compiled it yet).Hmmm, good question. IIRC, the patch will pass is_only as true only if
it going to be a locally defined, non-inheritable constraint. So I
went by the logic that since it was ok to merge and mark a constraint
as locally defined, it should be ok to mark it non-inheritable from
this moment on with that new local definition?
I think I misread what that was trying to do. I thought it would turn
on the "is only" bit on a constraint that a child had inherited from a
previous parent, but that was obviously wrong now that I think about it
again.
With this open question, this looks like it's back in Alvaro's hands
again to me. This one started the CF as "Ready for Committer" and seems
stalled there for now. I'm not going to touch its status, just pointing
this fact out.
Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.
Thanks.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Attachments:
non_inh_check_v4.patchapplication/octet-stream; name=non_inh_check_v4.patchDownload
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index b8cc16f..c23f187 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2036,6 +2036,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 00a477e..3b111a4 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, either.)
+ </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 e11d896..2f6a6ff 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -92,10 +92,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,
@@ -1859,7 +1859,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;
@@ -1942,7 +1942,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);
@@ -1983,7 +1984,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:
@@ -2026,7 +2027,8 @@ AddRelationNewConstraints(Relation rel,
List *newColDefaults,
List *newConstraints,
bool allow_merge,
- bool is_local)
+ bool is_local,
+ bool is_only)
{
List *cookedConstraints = NIL;
TupleDesc tupleDesc;
@@ -2099,6 +2101,7 @@ AddRelationNewConstraints(Relation rel,
cooked->skip_validation = false;
cooked->is_local = is_local;
cooked->inhcount = is_local ? 0 : 1;
+ cooked->is_only = is_only;
cookedConstraints = lappend(cookedConstraints, cooked);
}
@@ -2166,7 +2169,7 @@ AddRelationNewConstraints(Relation rel,
* what ATAddCheckConstraint wants.)
*/
if (MergeWithExistingConstraint(rel, ccname, expr,
- allow_merge, is_local))
+ allow_merge, is_local, is_only))
continue;
}
else
@@ -2213,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, is_only);
numchecks++;
@@ -2225,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 = is_only;
cookedConstraints = lappend(cookedConstraints, cooked);
}
@@ -2250,7 +2254,8 @@ 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;
@@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
con->conislocal = true;
else
con->coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con->conisonly = true;
+ }
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 758872f..f9075c4 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1155,7 +1155,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 135736a..c764ff6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -579,6 +579,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;
}
@@ -638,7 +639,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
*/
if (rawDefaults || stmt->constraints)
AddRelationNewConstraints(rel, rawDefaults, stmt->constraints,
- true, true);
+ true, true, false);
/*
* Clean up. We keep lock on new relation (although it shouldn't be
@@ -1599,6 +1600,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);
@@ -1617,6 +1622,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);
}
}
@@ -4501,7 +4507,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* This function is intended for CREATE TABLE, so it processes a
* _list_ of defaults, but we just do one.
*/
- AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true);
+ AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true, false);
/* Make the additional catalog changes visible */
CommandCounterIncrement();
@@ -4898,7 +4904,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
* This function is intended for CREATE TABLE, so it processes a
* _list_ of defaults, but we just do one.
*/
- AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true);
+ AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true, false);
}
}
@@ -5562,10 +5568,16 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
* omitted from the returned list, which is what we want: we do not need
* to do any validation work. That can only happen at child tables,
* though, since we disallow merging at the top level.
+ *
+ * Note: we set is_only based on the recurse flag which is false when
+ * interpretInhOption() of our statement returns false all the way up
+ * in AlterTable and gets passed all the way down to here.
*/
newcons = AddRelationNewConstraints(rel, NIL,
list_make1(copyObject(constr)),
- recursing, !recursing);
+ recursing, /* allow_merge */
+ !recursing, /* is_local */
+ !recurse && !recursing); /* is_only */
/* Add each to-be-validated constraint to Phase 3's queue */
foreach(lcon, newcons)
@@ -5606,21 +5618,18 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
return;
/*
+ * Adding an ONLY constraint? No need to find our children
+ */
+ if (!recurse && !recursing)
+ return;
+
+ /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
*/
children = find_inheritance_children(RelationGetRelid(rel), lockmode);
- /*
- * If we are told not to recurse, there had better not be any child
- * tables; else the addition would put them out of step.
- */
- if (children && !recurse)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be added to child tables too")));
-
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);
@@ -5914,7 +5923,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 f4c93e5..eb51140 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -449,7 +449,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 2b8f9ae..eda43d8 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2934,7 +2934,8 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
ccbin, /* Binary form of check constraint */
ccsrc, /* Source form of check constraint */
true, /* is local */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* is only */
/*
* Return the compiled constraint expression so the calling routine can
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 603e4c1..f9ad75e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3261,6 +3261,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 2fa99dd..da765f4 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6000,7 +6000,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "conislocal, convalidated "
+ "conislocal, convalidated, conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6011,7 +6011,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "conislocal, true AS convalidated "
+ "conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6022,7 +6023,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6034,7 +6036,8 @@ 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 convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6047,7 +6050,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, 0 AS oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6058,7 +6062,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6071,7 +6076,8 @@ 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 convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6097,6 +6103,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
for (j = 0; j < numConstrs; j++)
{
bool validated = PQgetvalue(res, j, 5)[0] == 't';
+ bool isonly = PQgetvalue(res, j, 6)[0] == 't';
constrs[j].dobj.objType = DO_CONSTRAINT;
constrs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0));
@@ -6113,12 +6120,14 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
constrs[j].condeferrable = false;
constrs[j].condeferred = false;
constrs[j].conislocal = (PQgetvalue(res, j, 4)[0] == 't');
+ constrs[j].conisonly = isonly;
/*
* An unvalidated constraint needs to be dumped separately, so
* that potentially-violating existing data is loaded before
- * the constraint.
+ * the constraint. An ONLY constraint needs to be dumped
+ * separately too.
*/
- constrs[j].separate = !validated;
+ constrs[j].separate = !validated || isonly;
constrs[j].dobj.dump = tbinfo->dobj.dump;
@@ -6126,12 +6135,12 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
* Mark the constraint as needing to appear before the table
* --- this is so that any other dependencies of the
* constraint will be emitted before we try to create the
- * table. If the constraint is not validated, it will be
+ * table. If the constraint is to be dumped separately, it will be
* dumped after data is loaded anyway, so don't do it. (There's
* an automatic dependency in the opposite direction anyway, so
* don't need to add one manually here.)
*/
- if (validated)
+ if (!constrs[j].separate)
addObjectDependency(&tbinfo->dobj,
constrs[j].dobj.dumpId);
@@ -13148,9 +13157,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 31442f1..3bfeb31 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -378,6 +378,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 dcafdd2..9d5d6dd 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1781,12 +1781,20 @@ describeOneTableDetails(const char *schemaname,
/* print table (and column) check constraints */
if (tableinfo.checks)
{
+ char *is_only;
+
+ if (pset.sversion > 90100)
+ is_only = "r.conisonly";
+ else
+ is_only = "false AS conisonly";
+
printfPQExpBuffer(&buf,
- "SELECT r.conname, "
+ "SELECT r.conname, %s, "
"pg_catalog.pg_get_constraintdef(r.oid, true)\n"
"FROM pg_catalog.pg_constraint r\n"
- "WHERE r.conrelid = '%s' AND r.contype = 'c'\nORDER BY 1;",
- oid);
+ "WHERE r.conrelid = '%s' AND r.contype = 'c'\n"
+ "ORDER BY 2, 1;",
+ is_only, oid);
result = PSQLexec(buf.data, false);
if (!result)
goto error_return;
@@ -1799,9 +1807,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..0793813 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,
@@ -91,7 +92,8 @@ extern List *AddRelationNewConstraints(Relation rel,
List *newColDefaults,
List *newConstraints,
bool allow_merge,
- bool is_local);
+ bool is_local,
+ bool is_only);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 1566af2..dae42e8 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.
*/
@@ -149,7 +152,7 @@ typedef FormData_pg_constraint *Form_pg_constraint;
* compiler constants for pg_constraint
* ----------------
*/
-#define Natts_pg_constraint 23
+#define Natts_pg_constraint 24
#define Anum_pg_constraint_conname 1
#define Anum_pg_constraint_connamespace 2
#define Anum_pg_constraint_contype 3
@@ -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/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 87432a8..57096f2 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -498,22 +498,21 @@ 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:
-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
+alter table only atacc1 add constraint foo check (test>0);
+-- check constraint is not there on child
+insert into atacc2 (test) values (-3);
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
+ERROR: new row for relation "atacc1" violates check constraint "foo"
+DETAIL: Failing row contains (-3).
insert into atacc1 (test) values (3);
--- check constraint is there on child
-insert into atacc2 (test) values (-3);
-ERROR: new row for relation "atacc2" violates check constraint "foo"
-DETAIL: Failing row contains (-3, null).
-insert into atacc2 (test) values (3);
+-- fail, violating row:
+alter table only atacc2 add constraint foo check (test>0);
+ERROR: check constraint "foo" is violated by some row
drop table atacc2;
drop table atacc1;
-- test unique constraint adding
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index d958da2..c840356 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -683,6 +683,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:
+ "p2chk" CHECK (ff1 > 10)
+ "p1chk" (ONLY) CHECK (ff1 > 0)
+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 a477f04..faafb22 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -450,20 +450,19 @@ 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:
-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
+alter table only atacc1 add constraint foo check (test>0);
+-- check constraint is not there on child
+insert into atacc2 (test) values (-3);
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
insert into atacc1 (test) values (3);
--- check constraint is there on child
-insert into atacc2 (test) values (-3);
-insert into atacc2 (test) values (3);
+-- fail, violating row:
+alter table only atacc2 add constraint foo check (test>0);
drop table atacc2;
drop table atacc1;
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index deda5a5..6914404 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);
On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.
Other than the version checks seem to be off by one looks fine. I
assume I/we missed that in the original patch. I also adjusted the
version check in describe.c to be consistent with the other version
checks in that file (>= 90200 instead of > 90100).
(Also, nice catch on "false AS as r.conisonly" in describe.c)
--
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***************
*** 5996,6003 **** getTableAttrs(TableInfo *tblinfo, int numTables)
tbinfo->dobj.name); resetPQExpBuffer(q);
! if (g_fout->remoteVersion >= 90100)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
"conislocal, convalidated, conisonly "
--- 5996,6004 ----
tbinfo->dobj.name); resetPQExpBuffer(q);
! if (g_fout->remoteVersion >= 90200)
{
+ /* conisonly is new in 9.2 */
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
"conislocal, convalidated, conisonly "
***************
*** 6007,6012 **** getTableAttrs(TableInfo *tblinfo, int numTables)
--- 6008,6026 ----
"ORDER BY conname",
tbinfo->dobj.catId.oid);
}
+ else if (g_fout->remoteVersion >= 90100)
+ {
+ /* conisvalidated is new in 9.1 */
+ appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
+ "pg_catalog.pg_get_constraintdef(oid) AS consrc, "
+ "conislocal, convalidated, "
+ "false as conisonly "
+ "FROM pg_catalog.pg_constraint "
+ "WHERE conrelid = '%u'::pg_catalog.oid "
+ " AND contype = 'c' "
+ "ORDER BY conname",
+ tbinfo->dobj.catId.oid);
+ }
else if (g_fout->remoteVersion >= 80400)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
*** a/src/bin/psql/describe.c
--- b/src/bin/psql/describe.c
***************
*** 1783,1789 **** describeOneTableDetails(const char *schemaname,
{
char *is_only;! if (pset.sversion > 90100)
is_only = "r.conisonly";
else
is_only = "false AS conisonly";
--- 1783,1789 ----
{
char *is_only;! if (pset.sversion >= 90200)
is_only = "r.conisonly";
else
is_only = "false AS conisonly";
Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011:
On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.Other than the version checks seem to be off by one looks fine. I
assume I/we missed that in the original patch. I also adjusted the
version check in describe.c to be consistent with the other version
checks in that file (>= 90200 instead of > 90100).
Uhm ... you're right that convalidated is present in 9.1 but AFAIR it's
only used for FKs, not CHECKs which is what this code path is about (for
CHECKs I only introduced it in 9.2, which is the patch that caused the
merge conflict in the first place). FKs use a completely separate path
in pg_dump which doesn't need the separate convalidated check. So I
don't think we really need to add a separate branch for 9.1 here, but it
certainly needs a comment improvement.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Fri, Dec 16, 2011 at 14:01, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011:
On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.Other than the version checks seem to be off by one looks fine.
Uhm ... you're right that convalidated is present in 9.1 [...] So I
don't think we really need to add a separate branch for 9.1 here, but it
certainly needs a comment improvement.
Hrm... What am I missing?
$ inh_v4/bin/psql -c 'select version();' -d test
version
---------------------------------------------------------------------------------------------------------
PostgreSQL 9.1.0 on x86_64-unknown-linux-gnu, compiled by gcc (GCC)
4.6.1 20110819 (prerelease), 64-bit
(1 row)
$ inh_v4/bin/pg_dump test
pg_dump: SQL command failed
pg_dump: Error message from server: ERROR: column "conisonly" does not exist
LINE 1: ...aintdef(oid) AS consrc, conislocal, convalidated, conisonly ...
^
pg_dump: The command was: SELECT tableoid, oid, conname,
pg_catalog.pg_get_constraintdef(oid) AS consrc, conislocal,
convalidated, conisonly FROM pg_catalog.pg_constraint WHERE conrelid =
'237964'::pg_catalog.oid AND contype = 'c' ORDER BY conname
Hi Alvaro,
The patch looks ok to me. I see that we now sort the constraints by
conisonly value too:
@@ -1781,12 +1781,20 @@ describeOneTableDetails(const char *schemaname,
/* print table (and column) check constraints */
if (tableinfo.checks)
{
+ char *is_only;
+
+ if (pset.sversion > 90100)
+ is_only = "r.conisonly";
+ else
+ is_only = "false AS conisonly";
+
printfPQExpBuffer(&buf,
- "SELECT r.conname, "
+ "SELECT r.conname, %s, "
"pg_catalog.pg_get_constraintdef(r.oid,
true)\n"
"FROM pg_catalog.pg_constraint r\n"
- "WHERE r.conrelid = '%s' AND r.contype = 'c'\nORDER BY
1;",
- oid);
+ "WHERE r.conrelid = '%s' AND r.contype = 'c'\n"
+ "ORDER BY 2, 1;",
+ is_only, oid);
result = PSQLexec(buf.data, false);
if (!result)
goto error_return;My preference would be to see true values first, so might want to modify it
to
"ORDER BY 2 desc, 1"
to have that effect. Your call finally.
Regards,
Nikhils
On Sat, Dec 17, 2011 at 12:36 AM, Alvaro Herrera <alvherre@commandprompt.com
Show quoted text
wrote:
Excerpts from Greg Smith's message of vie dic 16 15:02:20 -0300 2011:
On 12/04/2011 02:22 AM, Nikhil Sontakke wrote:
Is it okay to modify an existing constraint to mark it as "only",
even
if it was originally inheritable? This is not clear to me. Maybe
the
safest course of action is to raise an error. Or maybe I'm
misreading
what it does (because I haven't compiled it yet).
Hmmm, good question. IIRC, the patch will pass is_only as true only if
it going to be a locally defined, non-inheritable constraint. So I
went by the logic that since it was ok to merge and mark a constraint
as locally defined, it should be ok to mark it non-inheritable from
this moment on with that new local definition?I think I misread what that was trying to do. I thought it would turn
on the "is only" bit on a constraint that a child had inherited from a
previous parent, but that was obviously wrong now that I think about it
again.With this open question, this looks like it's back in Alvaro's hands
again to me. This one started the CF as "Ready for Committer" and seems
stalled there for now. I'm not going to touch its status, just pointing
this fact out.Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.Thanks.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 16, 2011 at 2:06 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.
It seems hard to believe that ATExecDropConstraint() doesn't need any
adjustment.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
It seems hard to believe that ATExecDropConstraint() doesn't need any
adjustment.
Yeah, I think we could return early on for "only" type of constraints.
Also, shouldn't the systable scan break out of the while loop after a
matching constraint name has been found? As of now, it's doing a full scan
of all the constraints for the given relation.
@@ -6755,6 +6765,7 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
HeapTuple tuple;
bool found = false;
bool is_check_constraint = false;
+ bool is_only_constraint = false;/* At top level, permission check was done in ATPrepCmd, else do it
*/
if (recursing)
@@ -6791,6 +6802,12 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
/* Right now only CHECK constraints can be inherited */ if (con->contype == CONSTRAINT_CHECK) is_check_constraint = true; + + if (con->conisonly) + { + Assert(is_check_constraint); + is_only_constraint = true; + }/*
* Perform the actual constraint deletion
@@ -6802,6 +6819,9 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
performDeletion(&conobj, behavior);
found = true; + + /* constraint found - break from the while loop now */ + break; }systable_endscan(scan);
@@ -6830,7 +6850,7 @@ ATExecDropConstraint(Relation rel, const char
*constrName,
* routines, we have to do this one level of recursion at a time; we
can't
* use find_all_inheritors to do it in one pass. */ - if (is_check_constraint) + if (is_check_constraint && !is_only_constraint) children = find_inheritance_children(RelationGetRelid(rel),
lockmode);
else
children = NIL;
PFA, revised version containing the above changes based on Alvaro's v4
patch.
Regards,
Nikhils
Attachments:
non_inh_check_v5.0.patchapplication/octet-stream; name=non_inh_check_v5.0.patchDownload
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index d948ed4..be4bbc7 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2037,6 +2037,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 00a477e..3b111a4 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, either.)
+ </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 e11d896..2f6a6ff 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -92,10 +92,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,
@@ -1859,7 +1859,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;
@@ -1942,7 +1942,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);
@@ -1983,7 +1984,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:
@@ -2026,7 +2027,8 @@ AddRelationNewConstraints(Relation rel,
List *newColDefaults,
List *newConstraints,
bool allow_merge,
- bool is_local)
+ bool is_local,
+ bool is_only)
{
List *cookedConstraints = NIL;
TupleDesc tupleDesc;
@@ -2099,6 +2101,7 @@ AddRelationNewConstraints(Relation rel,
cooked->skip_validation = false;
cooked->is_local = is_local;
cooked->inhcount = is_local ? 0 : 1;
+ cooked->is_only = is_only;
cookedConstraints = lappend(cookedConstraints, cooked);
}
@@ -2166,7 +2169,7 @@ AddRelationNewConstraints(Relation rel,
* what ATAddCheckConstraint wants.)
*/
if (MergeWithExistingConstraint(rel, ccname, expr,
- allow_merge, is_local))
+ allow_merge, is_local, is_only))
continue;
}
else
@@ -2213,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, is_only);
numchecks++;
@@ -2225,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 = is_only;
cookedConstraints = lappend(cookedConstraints, cooked);
}
@@ -2250,7 +2254,8 @@ 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;
@@ -2312,6 +2317,11 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
con->conislocal = true;
else
con->coninhcount++;
+ if (is_only)
+ {
+ Assert(is_local);
+ con->conisonly = true;
+ }
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 758872f..f9075c4 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1155,7 +1155,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 135736a..543998e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -579,6 +579,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;
}
@@ -638,7 +639,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId)
*/
if (rawDefaults || stmt->constraints)
AddRelationNewConstraints(rel, rawDefaults, stmt->constraints,
- true, true);
+ true, true, false);
/*
* Clean up. We keep lock on new relation (although it shouldn't be
@@ -1599,6 +1600,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);
@@ -1617,6 +1622,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);
}
}
@@ -4501,7 +4507,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* This function is intended for CREATE TABLE, so it processes a
* _list_ of defaults, but we just do one.
*/
- AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true);
+ AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true, false);
/* Make the additional catalog changes visible */
CommandCounterIncrement();
@@ -4898,7 +4904,7 @@ ATExecColumnDefault(Relation rel, const char *colName,
* This function is intended for CREATE TABLE, so it processes a
* _list_ of defaults, but we just do one.
*/
- AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true);
+ AddRelationNewConstraints(rel, list_make1(rawEnt), NIL, false, true, false);
}
}
@@ -5562,10 +5568,16 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
* omitted from the returned list, which is what we want: we do not need
* to do any validation work. That can only happen at child tables,
* though, since we disallow merging at the top level.
+ *
+ * Note: we set is_only based on the recurse flag which is false when
+ * interpretInhOption() of our statement returns false all the way up
+ * in AlterTable and gets passed all the way down to here.
*/
newcons = AddRelationNewConstraints(rel, NIL,
list_make1(copyObject(constr)),
- recursing, !recursing);
+ recursing, /* allow_merge */
+ !recursing, /* is_local */
+ !recurse && !recursing); /* is_only */
/* Add each to-be-validated constraint to Phase 3's queue */
foreach(lcon, newcons)
@@ -5606,21 +5618,18 @@ ATAddCheckConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
return;
/*
+ * Adding an ONLY constraint? No need to find our children
+ */
+ if (!recurse && !recursing)
+ return;
+
+ /*
* Propagate to children as appropriate. Unlike most other ALTER
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
*/
children = find_inheritance_children(RelationGetRelid(rel), lockmode);
- /*
- * If we are told not to recurse, there had better not be any child
- * tables; else the addition would put them out of step.
- */
- if (children && !recurse)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("constraint must be added to child tables too")));
-
foreach(child, children)
{
Oid childrelid = lfirst_oid(child);
@@ -5914,7 +5923,8 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel,
NULL,
NULL,
true, /* islocal */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* isonly */
/*
* Create the triggers that will enforce the constraint.
@@ -6755,6 +6765,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
HeapTuple tuple;
bool found = false;
bool is_check_constraint = false;
+ bool is_only_constraint = false;
/* At top level, permission check was done in ATPrepCmd, else do it */
if (recursing)
@@ -6791,6 +6802,12 @@ ATExecDropConstraint(Relation rel, const char *constrName,
/* Right now only CHECK constraints can be inherited */
if (con->contype == CONSTRAINT_CHECK)
is_check_constraint = true;
+
+ if (con->conisonly)
+ {
+ Assert(is_check_constraint);
+ is_only_constraint = true;
+ }
/*
* Perform the actual constraint deletion
@@ -6802,6 +6819,9 @@ ATExecDropConstraint(Relation rel, const char *constrName,
performDeletion(&conobj, behavior);
found = true;
+
+ /* constraint found - break from the while loop now */
+ break;
}
systable_endscan(scan);
@@ -6830,7 +6850,7 @@ ATExecDropConstraint(Relation rel, const char *constrName,
* routines, we have to do this one level of recursion at a time; we can't
* use find_all_inheritors to do it in one pass.
*/
- if (is_check_constraint)
+ if (is_check_constraint && !is_only_constraint)
children = find_inheritance_children(RelationGetRelid(rel), lockmode);
else
children = NIL;
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index f4c93e5..eb51140 100644
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -449,7 +449,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 2b8f9ae..eda43d8 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2934,7 +2934,8 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
ccbin, /* Binary form of check constraint */
ccsrc, /* Source form of check constraint */
true, /* is local */
- 0); /* inhcount */
+ 0, /* inhcount */
+ false); /* is only */
/*
* Return the compiled constraint expression so the calling routine can
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index 603e4c1..f9ad75e 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -3261,6 +3261,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 7dfa1dd..dfa4e82 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6023,7 +6023,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "conislocal, convalidated "
+ "conislocal, convalidated, conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6034,7 +6034,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "conislocal, true AS convalidated "
+ "conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6045,7 +6046,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
{
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6057,7 +6059,8 @@ 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 convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_catalog.pg_constraint "
"WHERE conrelid = '%u'::pg_catalog.oid "
" AND contype = 'c' "
@@ -6070,7 +6073,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, 0 AS oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6081,7 +6085,8 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
appendPQExpBuffer(q, "SELECT tableoid, oid, "
"rcname AS conname, "
"'CHECK (' || rcsrc || ')' AS consrc, "
- "true AS conislocal, true AS convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6094,7 +6099,8 @@ 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 convalidated "
+ "true AS conislocal, true AS convalidated, "
+ "false as conisonly "
"FROM pg_relcheck "
"WHERE rcrelid = '%u'::oid "
"ORDER BY rcname",
@@ -6120,6 +6126,7 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
for (j = 0; j < numConstrs; j++)
{
bool validated = PQgetvalue(res, j, 5)[0] == 't';
+ bool isonly = PQgetvalue(res, j, 6)[0] == 't';
constrs[j].dobj.objType = DO_CONSTRAINT;
constrs[j].dobj.catId.tableoid = atooid(PQgetvalue(res, j, 0));
@@ -6136,12 +6143,14 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
constrs[j].condeferrable = false;
constrs[j].condeferred = false;
constrs[j].conislocal = (PQgetvalue(res, j, 4)[0] == 't');
+ constrs[j].conisonly = isonly;
/*
* An unvalidated constraint needs to be dumped separately, so
* that potentially-violating existing data is loaded before
- * the constraint.
+ * the constraint. An ONLY constraint needs to be dumped
+ * separately too.
*/
- constrs[j].separate = !validated;
+ constrs[j].separate = !validated || isonly;
constrs[j].dobj.dump = tbinfo->dobj.dump;
@@ -6149,12 +6158,12 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
* Mark the constraint as needing to appear before the table
* --- this is so that any other dependencies of the
* constraint will be emitted before we try to create the
- * table. If the constraint is not validated, it will be
+ * table. If the constraint is to be dumped separately, it will be
* dumped after data is loaded anyway, so don't do it. (There's
* an automatic dependency in the opposite direction anyway, so
* don't need to add one manually here.)
*/
- if (validated)
+ if (!constrs[j].separate)
addObjectDependency(&tbinfo->dobj,
constrs[j].dobj.dumpId);
@@ -13193,9 +13202,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 31442f1..3bfeb31 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -378,6 +378,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 dcafdd2..9d5d6dd 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1781,12 +1781,20 @@ describeOneTableDetails(const char *schemaname,
/* print table (and column) check constraints */
if (tableinfo.checks)
{
+ char *is_only;
+
+ if (pset.sversion > 90100)
+ is_only = "r.conisonly";
+ else
+ is_only = "false AS conisonly";
+
printfPQExpBuffer(&buf,
- "SELECT r.conname, "
+ "SELECT r.conname, %s, "
"pg_catalog.pg_get_constraintdef(r.oid, true)\n"
"FROM pg_catalog.pg_constraint r\n"
- "WHERE r.conrelid = '%s' AND r.contype = 'c'\nORDER BY 1;",
- oid);
+ "WHERE r.conrelid = '%s' AND r.contype = 'c'\n"
+ "ORDER BY 2, 1;",
+ is_only, oid);
result = PSQLexec(buf.data, false);
if (!result)
goto error_return;
@@ -1799,9 +1807,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..0793813 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,
@@ -91,7 +92,8 @@ extern List *AddRelationNewConstraints(Relation rel,
List *newColDefaults,
List *newConstraints,
bool allow_merge,
- bool is_local);
+ bool is_local,
+ bool is_only);
extern void StoreAttrDefault(Relation rel, AttrNumber attnum, Node *expr);
diff --git a/src/include/catalog/pg_constraint.h b/src/include/catalog/pg_constraint.h
index 1566af2..dae42e8 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.
*/
@@ -149,7 +152,7 @@ typedef FormData_pg_constraint *Form_pg_constraint;
* compiler constants for pg_constraint
* ----------------
*/
-#define Natts_pg_constraint 23
+#define Natts_pg_constraint 24
#define Anum_pg_constraint_conname 1
#define Anum_pg_constraint_connamespace 2
#define Anum_pg_constraint_contype 3
@@ -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/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 87432a8..57096f2 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -498,22 +498,21 @@ 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:
-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
+alter table only atacc1 add constraint foo check (test>0);
+-- check constraint is not there on child
+insert into atacc2 (test) values (-3);
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
+ERROR: new row for relation "atacc1" violates check constraint "foo"
+DETAIL: Failing row contains (-3).
insert into atacc1 (test) values (3);
--- check constraint is there on child
-insert into atacc2 (test) values (-3);
-ERROR: new row for relation "atacc2" violates check constraint "foo"
-DETAIL: Failing row contains (-3, null).
-insert into atacc2 (test) values (3);
+-- fail, violating row:
+alter table only atacc2 add constraint foo check (test>0);
+ERROR: check constraint "foo" is violated by some row
drop table atacc2;
drop table atacc1;
-- test unique constraint adding
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index d958da2..c840356 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -683,6 +683,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:
+ "p2chk" CHECK (ff1 > 10)
+ "p1chk" (ONLY) CHECK (ff1 > 0)
+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 a477f04..faafb22 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -450,20 +450,19 @@ 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:
-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
+alter table only atacc1 add constraint foo check (test>0);
+-- check constraint is not there on child
+insert into atacc2 (test) values (-3);
+-- check constraint is there on parent
insert into atacc1 (test) values (-3);
insert into atacc1 (test) values (3);
--- check constraint is there on child
-insert into atacc2 (test) values (-3);
-insert into atacc2 (test) values (3);
+-- fail, violating row:
+alter table only atacc2 add constraint foo check (test>0);
drop table atacc2;
drop table atacc1;
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index deda5a5..6914404 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 Alex Hunsaker's message of vie dic 16 18:07:05 -0300 2011:
On Fri, Dec 16, 2011 at 14:01, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Alex Hunsaker's message of vie dic 16 17:50:12 -0300 2011:
On Fri, Dec 16, 2011 at 12:06, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Yeah. Nikhil, Alex, this is the merged patch. Have a look that it
still works for you (particularly the pg_dump bits) and I'll commit it.
I adjusted the regression test a bit too.Other than the version checks seem to be off by one looks fine.
Uhm ... you're right that convalidated is present in 9.1 [...] So I
don't think we really need to add a separate branch for 9.1 here, but it
certainly needs a comment improvement.Hrm... What am I missing?
I was saying that it should all be >= 9.2. There are no
convalidated=false check constraints in 9.1, so the extra branch is
useless. This is sufficient:
@@ -6019,8 +6019,13 @@ getTableAttrs(TableInfo *tblinfo, int numTables)
tbinfo->dobj.name);
resetPQExpBuffer(q);
- if (g_fout->remoteVersion >= 90100)
+ if (g_fout->remoteVersion >= 90200)
{
+ /*
+ * conisonly and convalidated are new in 9.2 (actually, the latter
+ * is there in 9.1, but it wasn't ever false for check constraints
+ * until 9.2).
+ */
appendPQExpBuffer(q, "SELECT tableoid, oid, conname, "
"pg_catalog.pg_get_constraintdef(oid) AS consrc, "
"conislocal, convalidated, conisonly "--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Mon, Dec 19, 2011 at 12:07 PM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
It seems hard to believe that ATExecDropConstraint() doesn't need any
adjustment.Yeah, I think we could return early on for "only" type of constraints.
It's not just that. Suppose that C inherits from B which inherits
from A. We add an "only" constraint to B and a non-"only" constraint
to "A". Now, what happens in each of the following scenarios?
1. We drop the constraint from "B" without specifying ONLY.
2. We drop the constraint from "B" *with* ONLY.
3. We drop the constraint from "A" without specifying ONLY.
4. We drop the constraint from "A" *with* ONLY.
Off the top of my head, I suspect that #1 should be an error; #2
should succeed, leaving only the inherited version of the constraint
on B; #3 should remove the constraint from A and leave it on B but I'm
not sure what should happen to C, and I have no clear vision of what
#4 should do.
As a followup question, if we do #2 followed by #4, or #4 followed by
#2, do we end up with the same final state in both cases?
Here's another scenario. B inherits from A. We a constraint to A
using ONLY, and then drop it without ONLY. Does that work or fail?
Also, what happens we add matching constraints to B and A, in each
case using ONLY, and then remove the constraint from A without using
ONLY? Does anything happen to B's constraint? Why or why not?
Just to be clear, I like the feature. But I've done some work on this
code before, and it is amazingly easy for to screw it up and end up
with bugs... so I think lots of careful thought is in order.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Nikhil Sontakke's message of lun dic 19 14:07:02 -0300 2011:
PFA, revised version containing the above changes based on Alvaro's v4
patch.
Committed with these fixes, and with my fix for the pg_dump issue noted
by Alex, too. Thanks.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
PFA, revised version containing the above changes based on Alvaro's v4
patch.Committed with these fixes, and with my fix for the pg_dump issue noted
by Alex, too. Thanks.
Thanks a lot Alvaro!
Regards,
Nikhils
Show quoted text
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Nikhil Sontakke's message of lun dic 19 22:32:57 -0300 2011:
PFA, revised version containing the above changes based on Alvaro's v4
patch.Committed with these fixes, and with my fix for the pg_dump issue noted
by Alex, too. Thanks.Thanks a lot Alvaro!
You're welcome.
But did you see Robert Haas' response upthread? It looks like there's
still some work to do -- at least some research to determine that we're
correctly handling all those cases. As the committer I'm responsible
for it, but I don't have resources to get into it any time soon.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
But did you see Robert Haas' response upthread? It looks like there's
still some work to do -- at least some research to determine that we're
correctly handling all those cases. As the committer I'm responsible
for it, but I don't have resources to get into it any time soon.
Yeah, am definitely planning to test out those scenarios and will respond
some time late today.
Regards,
Nikhils
Hi Robert,
First of all, let me state that this "ONLY" feature has not messed around
with existing inheritance semantics. It allows attaching a constraint to
any table (which can be part of any hierarchy) without the possibility of
it ever playing any part in future or existing inheritance hierarchies. It
is specific to that table, period.
It's not just that. Suppose that C inherits from B which inherits
from A. We add an "only" constraint to B and a non-"only" constraint
to "A". Now, what happens in each of the following scenarios?
An example against latest HEAD should help here:
create table A(ff1 int);
create table B () inherits (A);
create table C () inherits (B);
alter table A add constraint Achk check (ff1 > 10);
The above will attach Achk to A, B and C
alter table only B add constraint Bchk check (ff1 > 0);
The above will attach Bchk ONLY to table B
1. We drop the constraint from "B" without specifying ONLY.
postgres=# alter table B drop constraint Achk;
ERROR: cannot drop inherited constraint "achk" of relation "b"
The above is existing inheritance based behaviour.
Now let's look at the ONLY constraint:
postgres=# alter table B drop constraint Bchk;
ALTER TABLE
Since this constraint is not part of any hierarchy, it can be removed.
postgres=# alter table only B add constraint bchk check (ff1 > 0);
ALTER TABLE
postgres=# alter table only B drop constraint Bchk;
ALTER TABLE
So "only" constraints do not need the "only B" qualification to be
deleted. They work both ways and can always be deleted without any issues.
2. We drop the constraint from "B" *with* ONLY.
postgres=# alter table only B drop constraint Achk;
ERROR: cannot drop inherited constraint "achk" of relation "b"
The above is existing inheritance based behavior. So regardless of
ONLY an inherited constraint cannot be removed from the middle of the
hierarchy.
3. We drop the constraint from "A" without specifying ONLY.
postgres=# alter table A drop constraint Achk;
ALTER TABLE
This removes the constraint from the entire hierarchy across A, B and
C. Again existing inheritance behavior.
4. We drop the constraint from "A" *with* ONLY.
postgres=# alter table only A drop constraint Achk;
ALTER TABLE
This converts the Achk constraints belonging to B into a local one. C
still has it as an inherited constraint from B. We can now delete those
constraints as per existing inheritance semantics. However I hope the
difference between these and ONLY constraints are clear. The Achk
constraint associated with B can get inherited in the future whereas "only"
constraints will not be.
Off the top of my head, I suspect that #1 should be an error;
It's an error for inherited constraints, but not for "only" constraints.
#2
should succeed, leaving only the inherited version of the constraint
on B;
Yeah, only constraints removal succeeds, whereas inherited constraints
cannot be removed.
#3 should remove the constraint from A and leave it on B but I'm
not sure what should happen to C,
This removes the entire hierarchy.
and I have no clear vision of what
#4 should do.
This removes the constraint from A, but maintains the inheritance
relationship between B and C. Again standard existing inheritance semantics.
As a followup question, if we do #2 followed by #4, or #4 followed by
#2, do we end up with the same final state in both cases?
Yeah. #2 is not able to do much really because we do not allow inherited
constraints to be removed from the mid of the hierarchy.
Here's another scenario. B inherits from A. We a constraint to A
using ONLY, and then drop it without ONLY. Does that work or fail?
The constraint gets added to A and since it is an "only" constraint, its
removal both with and without "only A" works just fine.
Also, what happens we add matching constraints to B and A, in each
case using ONLY, and then remove the constraint from A without using
ONLY? Does anything happen to B's constraint? Why or why not?
Again the key differentiation here is that "only" constraints are bound to
that table and wont be inherited ever. So this works just fine.
postgres=# alter table only A add constraint A2chk check (ff1 > 10);
ALTER TABLE
postgres=# alter table only B add constraint A2chk check (ff1 > 10);
ALTER TABLE
Just to be clear, I like the feature. But I've done some work on this
code before, and it is amazingly easy for to screw it up and end up
with bugs... so I think lots of careful thought is in order.
Agreed. I just tried out the scenarios laid out by you both with and
without the committed patch and AFAICS, normal inheritance semantics have
been preserved properly even after the commit.
Regards,
Nikhils
On Tue, Dec 20, 2011 at 1:14 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Agreed. I just tried out the scenarios laid out by you both with and without
the committed patch and AFAICS, normal inheritance semantics have been
preserved properly even after the commit.
No, they haven't. I didn't expect this to break anything when you
have two constraints with different names. The problem is when you
have two constraints with the same name.
Testing reveals that this is, in fact, broken:
rhaas=# create table A(ff1 int);
CREATE TABLE
rhaas=# create table B () inherits (A);
CREATE TABLE
rhaas=# create table C () inherits (B);
CREATE TABLE
rhaas=# alter table only b add constraint chk check (ff1 > 0);
ALTER TABLE
rhaas=# alter table a add constraint chk check (ff1 > 0);
NOTICE: merging constraint "chk" with inherited definition
ALTER TABLE
At this point, you'll find that a has a constraint, and b has a
constraint, but *c does not have a constraint*. That's bad, because
a's constraint wasn't "only" and should therefore have propagated all
the way down the tree.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Agreed. I just tried out the scenarios laid out by you both with and
without
the committed patch and AFAICS, normal inheritance semantics have been
preserved properly even after the commit.No, they haven't. I didn't expect this to break anything when you
have two constraints with different names. The problem is when you
have two constraints with the same name.Testing reveals that this is, in fact, broken:
rhaas=# create table A(ff1 int);
CREATE TABLE
rhaas=# create table B () inherits (A);
CREATE TABLE
rhaas=# create table C () inherits (B);
CREATE TABLE
rhaas=# alter table only b add constraint chk check (ff1 > 0);
ALTER TABLE
rhaas=# alter table a add constraint chk check (ff1 > 0);
NOTICE: merging constraint "chk" with inherited definition
ALTER TABLEAt this point, you'll find that a has a constraint, and b has a
constraint, but *c does not have a constraint*. That's bad, because
a's constraint wasn't "only" and should therefore have propagated all
the way down the tree.
Apologies, I did not check this particular scenario.
I guess, here, we should not allow merging of the inherited constraint into
an "only" constraint. Because that breaks the semantics for "only"
constraints. If this sounds ok, I can whip up a patch for the same.
Regards,
Nikhils
rhaas=# create table A(ff1 int);
CREATE TABLE
rhaas=# create table B () inherits (A);
CREATE TABLE
rhaas=# create table C () inherits (B);
CREATE TABLE
rhaas=# alter table only b add constraint chk check (ff1 > 0);
ALTER TABLE
rhaas=# alter table a add constraint chk check (ff1 > 0);
NOTICE: merging constraint "chk" with inherited definition
ALTER TABLEAt this point, you'll find that a has a constraint, and b has a
constraint, but *c does not have a constraint*. That's bad, because
a's constraint wasn't "only" and should therefore have propagated all
the way down the tree.Apologies, I did not check this particular scenario.
I guess, here, we should not allow merging of the inherited constraint
into an "only" constraint. Because that breaks the semantics for "only"
constraints. If this sounds ok, I can whip up a patch for the same.
PFA, patch which does just this.
postgres=# alter table a add constraint chk check (ff1 > 0);
ERROR: constraint "chk" for relation "b" is an ONLY constraint. Cannot
merge
Regards,
Nikhils
Attachments:
only_constraint_no_merge.patchapplication/octet-stream; name=only_constraint_no_merge.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 2f6a6ff..e38cb86 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2307,12 +2307,17 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
(errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("constraint \"%s\" for relation \"%s\" already exists",
ccname, RelationGetRelationName(rel))));
- /* OK to update the tuple */
- ereport(NOTICE,
- (errmsg("merging constraint \"%s\" with inherited definition",
- ccname)));
+
tup = heap_copytuple(tup);
con = (Form_pg_constraint) GETSTRUCT(tup);
+
+ /* If the constraint is "only" then cannot merge */
+ if (con->conisonly)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("constraint \"%s\" for relation \"%s\" is an ONLY constraint. Cannot merge",
+ ccname, RelationGetRelationName(rel))));
+
if (is_local)
con->conislocal = true;
else
@@ -2322,6 +2327,10 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
Assert(is_local);
con->conisonly = true;
}
+ /* OK to update the tuple */
+ ereport(NOTICE,
+ (errmsg("merging constraint \"%s\" with inherited definition",
+ ccname)));
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
Excerpts from Nikhil Sontakke's message of mar dic 20 12:03:33 -0300 2011:
Apologies, I did not check this particular scenario.
I guess, here, we should not allow merging of the inherited constraint
into an "only" constraint. Because that breaks the semantics for "only"
constraints. If this sounds ok, I can whip up a patch for the same.PFA, patch which does just this.
postgres=# alter table a add constraint chk check (ff1 > 0);
ERROR: constraint "chk" for relation "b" is an ONLY constraint. Cannot
merge
I think the basic idea is fine -- the constraint certainly cannot be
merged, and we can't continue without merging it because of the
inconsistency it would create.
The error message is wrong though. I suggest
ERROR: constraint name "%s" on relation "%s" conflicts with non-inherited constraint on relation "%s"
HINT: Specify a different constraint name.
The errmsg seems a bit long though -- anybody has a better suggestion?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, Dec 22, 2011 at 2:43 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Nikhil Sontakke's message of mar dic 20 12:03:33 -0300 2011:
Apologies, I did not check this particular scenario.
I guess, here, we should not allow merging of the inherited constraint
into an "only" constraint. Because that breaks the semantics for "only"
constraints. If this sounds ok, I can whip up a patch for the same.PFA, patch which does just this.
postgres=# alter table a add constraint chk check (ff1 > 0);
ERROR: constraint "chk" for relation "b" is an ONLY constraint. Cannot
mergeI think the basic idea is fine -- the constraint certainly cannot be
merged, and we can't continue without merging it because of the
inconsistency it would create.
I was just looking at this as well. There is at least one other
problem. Consider:
rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# alter table b inherit a;
ALTER TABLE
This needs to fail if chk is an "only" constraint, but it doesn't,
even with this patch.
I think that part of the problem here is fuzzy thinking about the
meaning of the word "ONLY" in "ALTER TABLE ONLY b". The word "ONLY"
there is really supposed to mean that the operation is performed on b
but not on its descendents; but that's not what you're doing: you're
really performing a different operation. In theory, for a table with
no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
be identical, but here they are not. I think that's probably bad.
Another manifestation of this problem is that there's no way to add an
ONLY constraint in a CREATE TABLE command. I think that's bad, too:
it should be possible to declare any constraint at table creation
time, and if the ONLY were part of the constraint definition rather
than part of the target-table specification, that would work fine. As
it is, it doesn't.
I am tempted to say we should revert this and rethink. I don't
believe we are only a small patch away from finding all the bugs here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hi,
There is at least one other
problem. Consider:rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# alter table b inherit a;
ALTER TABLEThis needs to fail if chk is an "only" constraint, but it doesn't,
even with this patch.
As you rightly point out, this is because we cannot specify ONLY
constraints inside a CREATE TABLE as of now.
I think that part of the problem here is fuzzy thinking about the
meaning of the word "ONLY" in "ALTER TABLE ONLY b". The word "ONLY"
there is really supposed to mean that the operation is performed on b
but not on its descendents; but that's not what you're doing: you're
really performing a different operation. In theory, for a table with
no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
be identical, but here they are not. I think that's probably bad.
ONLY has inheritance based connotations for present/future children. ALTER
ONLY combined with ADD is honored better now with this patch IMO (atleast
for constraints I think).
Another manifestation of this problem is that there's no way to add an
ONLY constraint in a CREATE TABLE command. I think that's bad, too:
it should be possible to declare any constraint at table creation
time, and if the ONLY were part of the constraint definition rather
than part of the target-table specification, that would work fine. As
it is, it doesn't.
Well, the above was thought about during the original discussion and
eventually we felt that CREATE TABLE already has other issues as well, so
not having this done as part of creating a table was considered acceptable:
I am tempted to say we should revert this and rethink. I don't
believe we are only a small patch away from finding all the bugs here.
Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type
of syntax, then +1 for reverting this and a subsequent revised submission.
Regards,
Nikhils
And yeah, certainly there's a bug as you point out.
postgres=# create table a1 (ff1 int, constraint chk check (ff1 > 0));
postgres=# create table b1 (ff1 int);
postgres=# alter table only b1 add constraint chk check (ff1 > 0);
postgres=# alter table b1 inherit a1;
The last command should have refused to inherit.
Regards,
Nikhils
On Fri, Dec 23, 2011 at 8:55 AM, Nikhil Sontakke <nikkhils@gmail.com> wrote:
Show quoted text
Hi,
There is at least one other
problem. Consider:rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# alter table b inherit a;
ALTER TABLEThis needs to fail if chk is an "only" constraint, but it doesn't,
even with this patch.As you rightly point out, this is because we cannot specify ONLY
constraints inside a CREATE TABLE as of now.I think that part of the problem here is fuzzy thinking about the
meaning of the word "ONLY" in "ALTER TABLE ONLY b". The word "ONLY"
there is really supposed to mean that the operation is performed on b
but not on its descendents; but that's not what you're doing: you're
really performing a different operation. In theory, for a table with
no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
be identical, but here they are not. I think that's probably bad.ONLY has inheritance based connotations for present/future children. ALTER
ONLY combined with ADD is honored better now with this patch IMO (atleast
for constraints I think).Another manifestation of this problem is that there's no way to add an
ONLY constraint in a CREATE TABLE command. I think that's bad, too:
it should be possible to declare any constraint at table creation
time, and if the ONLY were part of the constraint definition rather
than part of the target-table specification, that would work fine. As
it is, it doesn't.Well, the above was thought about during the original discussion and
eventually we felt that CREATE TABLE already has other issues as well, so
not having this done as part of creating a table was considered acceptable:I am tempted to say we should revert this and rethink. I don't
believe we are only a small patch away from finding all the bugs here.Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT
type of syntax, then +1 for reverting this and a subsequent revised
submission.Regards,
Nikhils
Excerpts from Nikhil Sontakke's message of vie dic 23 00:25:26 -0300 2011:
Hi,
There is at least one other
problem. Consider:rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# alter table b inherit a;
ALTER TABLEThis needs to fail if chk is an "only" constraint, but it doesn't,
even with this patch.
As you rightly point out, this is because we cannot specify ONLY
constraints inside a CREATE TABLE as of now.
No, it's not related to that. You could do it even without that, by
creating a table then adding an ONLY constraint and only later doing the
ALTER TABLE / INHERIT. The problem we need to fix here is that this
command needs to check that there are no unmergeable constraints; and if
there are any, error out.
I think that part of the problem here is fuzzy thinking about the
meaning of the word "ONLY" in "ALTER TABLE ONLY b". The word "ONLY"
there is really supposed to mean that the operation is performed on b
but not on its descendents; but that's not what you're doing: you're
really performing a different operation. In theory, for a table with
no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
be identical, but here they are not. I think that's probably bad.ONLY has inheritance based connotations for present/future children. ALTER
ONLY combined with ADD is honored better now with this patch IMO (atleast
for constraints I think).
I agree with Robert that this usage of ALTER TABLE ONLY is slightly
different from other usages of the same command, but I disagree that
this means that we need another command to do what we want to do here.
IOW, I prefer to keep the syntax we have.
I am tempted to say we should revert this and rethink. I don't
believe we are only a small patch away from finding all the bugs here.Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type
of syntax, then +1 for reverting this and a subsequent revised submission.
I don't think this is a given ... In fact, IMO if we're only two or
three fixes away from having it all nice and consistent, I think
reverting is not necessary.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I don't think this is a given ... In fact, IMO if we're only two or
three fixes away from having it all nice and consistent, I think
reverting is not necessary.
FWIW, here's a quick fix for the issue that Robert pointed out. Again it's
a variation of the first issue that he reported. We have two functions
which try to merge existing constraints:
MergeWithExistingConstraint() in heap.c
MergeConstraintsIntoExisting() in tablecmds.c
Nice names indeed :)
I have also tried to change the error message as per Alvaro's suggestions.
I will really try to see if we have other issues. Really cannot have Robert
reporting all the bugs and getting annoyed, but there are lotsa variations
possible with inheritance..
Regards,
Nikhils
Attachments:
only_constraint_no_merge_v2.0.patchapplication/octet-stream; name=only_constraint_no_merge_v2.0.patchDownload
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 2f6a6ff..ec777fd 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2307,12 +2307,17 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
(errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("constraint \"%s\" for relation \"%s\" already exists",
ccname, RelationGetRelationName(rel))));
- /* OK to update the tuple */
- ereport(NOTICE,
- (errmsg("merging constraint \"%s\" with inherited definition",
- ccname)));
+
tup = heap_copytuple(tup);
con = (Form_pg_constraint) GETSTRUCT(tup);
+
+ /* If the constraint is "only" then cannot merge */
+ if (con->conisonly)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("constraint \"%s\" conflicts with non-inherited constraint on relation \"%s\"",
+ ccname, RelationGetRelationName(rel))));
+
if (is_local)
con->conislocal = true;
else
@@ -2322,6 +2327,10 @@ MergeWithExistingConstraint(Relation rel, char *ccname, Node *expr,
Assert(is_local);
con->conisonly = true;
}
+ /* OK to update the tuple */
+ ereport(NOTICE,
+ (errmsg("merging constraint \"%s\" with inherited definition",
+ ccname)));
simple_heap_update(conDesc, &tup->t_self, tup);
CatalogUpdateIndexes(conDesc, tup);
break;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8473c9e..c615ac0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8950,6 +8950,10 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
if (parent_con->contype != CONSTRAINT_CHECK)
continue;
+ /* if ONLY constraint, cannot inherit */
+ if (parent_con->conisonly)
+ continue;
+
/* Search for a child constraint matching this one */
ScanKeyInit(&child_key,
Anum_pg_constraint_conrelid,
@@ -8977,6 +8981,13 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
RelationGetRelationName(child_rel),
NameStr(parent_con->conname))));
+ /* If the constraint is "only" then cannot merge */
+ if (child_con->conisonly)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("constraint \"%s\" conflicts with non-inherited constraint on child table \"%s\"",
+ NameStr(child_con->conname), RelationGetRelationName(child_rel))));
+
/*
* OK, bump the child constraint's inheritance count. (If we fail
* later on, this change will just roll back.)
On Thu, Dec 22, 2011 at 10:54 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
I agree with Robert that this usage of ALTER TABLE ONLY is slightly
different from other usages of the same command, but I disagree that
this means that we need another command to do what we want to do here.
IOW, I prefer to keep the syntax we have.
Another disadvantage of the current syntax becomes evident when you
look at the pg_dump output. If you pg_dump a regular constraint, the
constraint gets added as part of the table definition, and the rows
are all checked as they are inserted. If you pg_dump an ONLY
constraint, the constraint gets added after loading the data,
requiring an additional full-table scan to validate it.
I am tempted to say we should revert this and rethink. I don't
believe we are only a small patch away from finding all the bugs here.Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type
of syntax, then +1 for reverting this and a subsequent revised submission.I don't think this is a given ... In fact, IMO if we're only two or
three fixes away from having it all nice and consistent, I think
reverting is not necessary.
Sure. It's the "if" part of that sentence that I'm not too sure about.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
I don't think this is a given ... In fact, IMO if we're only two or
three fixes away from having it all nice and consistent, I think
reverting is not necessary.Sure. It's the "if" part of that sentence that I'm not too sure about.
Any specific area of the code that you think is/has become fragile (apart
from the non-support for CREATE TABLE based ONLY constraints)? The second
bug is a variant of the first. And I have provided a patch for it.
Regards,
Nikhils
Excerpts from Nikhil Sontakke's message of vie dic 23 01:02:10 -0300 2011:
FWIW, here's a quick fix for the issue that Robert pointed out.
Thanks, applied.
Again it's
a variation of the first issue that he reported. We have two functions
which try to merge existing constraints:MergeWithExistingConstraint() in heap.c
MergeConstraintsIntoExisting() in tablecmds.cNice names indeed :)
Yeah, I added a comment about the other one on each of them.
I have also tried to change the error message as per Alvaro's suggestions.
I will really try to see if we have other issues. Really cannot have Robert
reporting all the bugs and getting annoyed, but there are lotsa variations
possible with inheritance..
So did you find anything?
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Excerpts from Nikhil Sontakke's message of vie dic 23 01:02:10 -0300 2011:
I have also tried to change the error message as per Alvaro's suggestions.
I will really try to see if we have other issues. Really cannot have Robert
reporting all the bugs and getting annoyed, but there are lotsa variations
possible with inheritance..
BTW thank you for doing the work on this. Probably the reason no one
bothers too much with inheritance is that even something that's
seemingly simple such as what you tried to do here has all these little
details that's hard to get right.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
I have also tried to change the error message as per Alvaro's
suggestions.
I will really try to see if we have other issues. Really cannot have
Robert
reporting all the bugs and getting annoyed, but there are lotsa
variations
possible with inheritance..
So did you find anything?
Nothing much as yet. I think we are mostly there with the code but will
keep on trying some more variations along the lines of what Robert tried
out whenever I get the time next.
Regards,
Nikhils
I will really try to see if we have other issues. Really cannot have
Robert
reporting all the bugs and getting annoyed, but there are lotsa
variations
possible with inheritance..
BTW thank you for doing the work on this. Probably the reason no one
bothers too much with inheritance is that even something that's
seemingly simple such as what you tried to do here has all these little
details that's hard to get right.
Thanks Alvaro. Yeah, inheritance is "semantics" quicksand :)
Appreciate all the help from you, Robert and Alex Hunsaker on this.
Regards,
Nikhils