Review: Non-inheritable check constraints

Hi Alex,

I didn't care for the changes to gram.y so I reworked it a bit so we

now pass is_only to AddRelationNewConstraint() (like we do with
is_local). Seemed simpler but maybe I missed something. Comments?

Hmmm, your patch checks for a constraint being "only" via:

!recurse && !recursing

I hope that is good enough to conclusively conclude that the constraint is
'only'. This check was not too readable in the existing code for me anyways
;). If we check at the grammar level, we can be sure. But I remember not
being too comfortable about the right position to ascertain this
characteristic.

I also moved the is_only check in AtAddCheckConstraint() to before we
grab and loop through any children. Seemed a bit wasteful to loop
through the new constraints just to set a flag so that we could bail
out while looping through the children.

Ditto comment for this function. I thought this function went to great
lengths to spit out a proper error in case of inconsistencies between parent
and child. But if your check makes it simpler, that's good!

You also forgot to bump Natts_pg_constraint.

Ouch. Thanks for the catch.

PFA the above changes as well as being "rebased" against master.

Thanks Alex, appreciate the review!

Regards,
Nikhils

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:

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.

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

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

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

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