Horton

Thread

  1. Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-10-05T19:45:31Z

    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.

  2. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-10-06T08:42:38Z

    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

  3. Re: Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-10-06T17:18:44Z

    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?

  4. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-10-07T06:28:00Z

    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

  5. Re: Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-10-07T06:48:40Z

    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?

  6. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-10-08T03:30:51Z

    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 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?
>

  7. Re: Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-10-09T06:40:36Z

    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.

  8. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-04T02:14:20Z

    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

  9. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-04T07:22:56Z

    > 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

  10. Re: Review: Non-inheritable check constraints

    Greg Smith <greg@2ndquadrant.com> — 2011-12-16T18:02:20Z

    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

  11. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-16T19:06:51Z

    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

  12. Re: Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-12-16T20:50:12Z

    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";

  13. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-16T21:01:56Z

    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

  14. Re: Review: Non-inheritable check constraints

    Alex Hunsaker <badalex@gmail.com> — 2011-12-16T21:07:05Z

    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

  15. Re: Review: Non-inheritable check constraints

    Nikhil Sontakke <nikhil.sontakke@enterprisedb.com> — 2011-12-17T09:03:42Z

    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
> 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
>
>

  16. Re: Review: Non-inheritable check constraints

    Robert Haas <robertmhaas@gmail.com> — 2011-12-19T13:13:04Z

    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

  17. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-19T17:07:02Z

    > 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

  18. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-19T18:05:29Z

    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

  19. Re: Review: Non-inheritable check constraints

    Robert Haas <robertmhaas@gmail.com> — 2011-12-19T21:11:49Z

    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

  20. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-19T21:17:04Z

    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

  21. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-20T01:32:57Z

    > > PFA, revised version containing the above changes based on Alvaro's v4
> > patch.
>
> Committed with these fixes, and with my fix for the pg_dump issue noted
> by Alex, too.  Thanks.
>
>
Thanks a lot Alvaro!

Regards,
Nikhils


> --
> Álvaro Herrera <alvherre@commandprompt.com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

  22. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-20T02:21:19Z

    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

  23. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-20T04:38:58Z

    > 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

  24. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-20T06:14:29Z

    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

  25. Re: Review: Non-inheritable check constraints

    Robert Haas <robertmhaas@gmail.com> — 2011-12-20T13:32:49Z

    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

  26. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-20T14:39:38Z

    > > Agreed. I just tried out the scenarios laid out by you both with and
> without
> > the committed patch and AFAICS, normal inheritance semantics have been
> > preserved properly even after the commit.
>
> No, they haven't.  I didn't expect this to break anything when you
> have two constraints with different names.  The problem is when you
> have two constraints with the same name.
>
> Testing reveals that this is, in fact, broken:
>
> rhaas=# create table A(ff1 int);
> CREATE TABLE
> rhaas=# create table B () inherits (A);
> CREATE TABLE
> rhaas=# create table C () inherits (B);
> CREATE TABLE
> rhaas=# alter table only b add constraint chk check (ff1 > 0);
> ALTER TABLE
> rhaas=# alter table a add constraint chk check (ff1 > 0);
> NOTICE:  merging constraint "chk" with inherited definition
> ALTER TABLE
>
> At this point, you'll find that a has a constraint, and b has a
> constraint, but *c does not have a constraint*.  That's bad, because
> a's constraint wasn't "only" and should therefore have propagated all
> the way down the tree.
>
>
Apologies, I did not check this particular scenario.

I guess, here, we should not allow merging of the inherited constraint into
an "only" constraint. Because that breaks the semantics for "only"
constraints. If this sounds ok, I can whip up a patch for the same.

Regards,
Nikhils

  27. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-20T15:03:33Z

    > rhaas=# create table A(ff1 int);
>> CREATE TABLE
>> rhaas=# create table B () inherits (A);
>> CREATE TABLE
>> rhaas=# create table C () inherits (B);
>> CREATE TABLE
>> rhaas=# alter table only b add constraint chk check (ff1 > 0);
>> ALTER TABLE
>> rhaas=# alter table a add constraint chk check (ff1 > 0);
>> NOTICE:  merging constraint "chk" with inherited definition
>> ALTER TABLE
>>
>> At this point, you'll find that a has a constraint, and b has a
>> constraint, but *c does not have a constraint*.  That's bad, because
>> a's constraint wasn't "only" and should therefore have propagated all
>> the way down the tree.
>>
>>
> Apologies, I did not check this particular scenario.
>
> I guess, here, we should not allow merging of the inherited constraint
> into an "only" constraint. Because that breaks the semantics for "only"
> constraints. If this sounds ok, I can whip up a patch for the same.
>
>
PFA, patch which does just this.

postgres=# alter table a add constraint chk check (ff1 > 0);
ERROR:  constraint "chk" for relation "b" is an ONLY constraint. Cannot
merge

Regards,
Nikhils

  28. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-22T19:43:35Z

    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

  29. Re: Review: Non-inheritable check constraints

    Robert Haas <robertmhaas@gmail.com> — 2011-12-22T19:54:00Z

    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
>> merge
>
> I think the basic idea is fine -- the constraint certainly cannot be
> merged, and we can't continue without merging it because of the
> inconsistency it would create.

I was just looking at this as well.  There is at least one other
problem.  Consider:

rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
CREATE TABLE
rhaas=# alter table b inherit a;
ALTER TABLE

This needs to fail if chk is an "only" constraint, but it doesn't,
even with this patch.

I think that part of the problem here is fuzzy thinking about the
meaning of the word "ONLY" in "ALTER TABLE ONLY b".  The word "ONLY"
there is really supposed to mean that the operation is performed on b
but not on its descendents; but that's not what you're doing: you're
really performing a different operation.  In theory, for a table with
no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
be identical, but here they are not.  I think that's probably bad.

Another manifestation of this problem is that there's no way to add an
ONLY constraint in a CREATE TABLE command.  I think that's bad, too:
it should be possible to declare any constraint at table creation
time, and if the ONLY were part of the constraint definition rather
than part of the target-table specification, that would work fine.  As
it is, it doesn't.

I am tempted to say we should revert this and rethink.  I don't
believe we are only a small patch away from finding all the bugs here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  30. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-23T03:25:26Z

    Hi,


> There is at least one other
> problem.  Consider:
>
> rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
> CREATE TABLE
> rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
> CREATE TABLE
> rhaas=# alter table b inherit a;
> ALTER TABLE
>
> This needs to fail if chk is an "only" constraint, but it doesn't,
> even with this patch.
>
>
As you rightly point out, this is because we cannot specify ONLY
constraints inside a CREATE TABLE as of now.


> I think that part of the problem here is fuzzy thinking about the
> meaning of the word "ONLY" in "ALTER TABLE ONLY b".  The word "ONLY"
> there is really supposed to mean that the operation is performed on b
> but not on its descendents; but that's not what you're doing: you're
> really performing a different operation.  In theory, for a table with
> no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
> be identical, but here they are not.  I think that's probably bad.
>
>
ONLY has inheritance based connotations for present/future children. ALTER
ONLY combined with ADD is honored better now with this patch IMO (atleast
for constraints I think).


> Another manifestation of this problem is that there's no way to add an
> ONLY constraint in a CREATE TABLE command.  I think that's bad, too:
> it should be possible to declare any constraint at table creation
> time, and if the ONLY were part of the constraint definition rather
> than part of the target-table specification, that would work fine.  As
> it is, it doesn't.
>
>
Well, the above was thought about during the original discussion and
eventually we felt that CREATE TABLE already has other issues as well, so
not having this done as part of creating a table was considered acceptable:

http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt4633334.html#a4647144


> I am tempted to say we should revert this and rethink.  I don't
> believe we are only a small patch away from finding all the bugs here.
>
>
Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type
of syntax, then +1 for reverting this and a subsequent revised submission.

Regards,
Nikhils

  31. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-23T03:30:31Z

    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:

> Hi,
>
>
>> There is at least one other
>> problem.  Consider:
>>
>> rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
>> CREATE TABLE
>> rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
>> CREATE TABLE
>> rhaas=# alter table b inherit a;
>> ALTER TABLE
>>
>> This needs to fail if chk is an "only" constraint, but it doesn't,
>> even with this patch.
>>
>>
> As you rightly point out, this is because we cannot specify ONLY
> constraints inside a CREATE TABLE as of now.
>
>
>> I think that part of the problem here is fuzzy thinking about the
>> meaning of the word "ONLY" in "ALTER TABLE ONLY b".  The word "ONLY"
>> there is really supposed to mean that the operation is performed on b
>> but not on its descendents; but that's not what you're doing: you're
>> really performing a different operation.  In theory, for a table with
>> no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
>> be identical, but here they are not.  I think that's probably bad.
>>
>>
> ONLY has inheritance based connotations for present/future children. ALTER
> ONLY combined with ADD is honored better now with this patch IMO (atleast
> for constraints I think).
>
>
>> Another manifestation of this problem is that there's no way to add an
>> ONLY constraint in a CREATE TABLE command.  I think that's bad, too:
>> it should be possible to declare any constraint at table creation
>> time, and if the ONLY were part of the constraint definition rather
>> than part of the target-table specification, that would work fine.  As
>> it is, it doesn't.
>>
>>
> Well, the above was thought about during the original discussion and
> eventually we felt that CREATE TABLE already has other issues as well, so
> not having this done as part of creating a table was considered acceptable:
>
>
> http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt4633334.html#a4647144
>
>
>> I am tempted to say we should revert this and rethink.  I don't
>> believe we are only a small patch away from finding all the bugs here.
>>
>>
> Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT
> type of syntax, then +1 for reverting this and a subsequent revised
> submission.
>
> Regards,
> Nikhils
>

  32. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2011-12-23T03:54:22Z

    Excerpts from Nikhil Sontakke's message of vie dic 23 00:25:26 -0300 2011:
> Hi,
> 
> > There is at least one other
> > problem.  Consider:
> >
> > rhaas=# create table a (ff1 int, constraint chk check (ff1 > 0));
> > CREATE TABLE
> > rhaas=# create table b (ff1 int, constraint chk check (ff1 > 0));
> > CREATE TABLE
> > rhaas=# alter table b inherit a;
> > ALTER TABLE
> >
> > This needs to fail if chk is an "only" constraint, but it doesn't,
> > even with this patch.

> As you rightly point out, this is because we cannot specify ONLY
> constraints inside a CREATE TABLE as of now.

No, it's not related to that.  You could do it even without that, by
creating a table then adding an ONLY constraint and only later doing the
ALTER TABLE / INHERIT.  The problem we need to fix here is that this
command needs to check that there are no unmergeable constraints; and if
there are any, error out.

> > I think that part of the problem here is fuzzy thinking about the
> > meaning of the word "ONLY" in "ALTER TABLE ONLY b".  The word "ONLY"
> > there is really supposed to mean that the operation is performed on b
> > but not on its descendents; but that's not what you're doing: you're
> > really performing a different operation.  In theory, for a table with
> > no current descendents, ALTER TABLE ONLY b and ALTER TABLE b ought to
> > be identical, but here they are not.  I think that's probably bad.
> >
>
> ONLY has inheritance based connotations for present/future children. ALTER
> ONLY combined with ADD is honored better now with this patch IMO (atleast
> for constraints I think).

I agree with Robert that this usage of ALTER TABLE ONLY is slightly
different from other usages of the same command, but I disagree that
this means that we need another command to do what we want to do here.
IOW, I prefer to keep the syntax we have.

> > I am tempted to say we should revert this and rethink.  I don't
> > believe we are only a small patch away from finding all the bugs here.
>
> Sure, if we all think that CREATE TABLE should support ONLY CONSTRAINT type
> of syntax, then +1 for reverting this and a subsequent revised submission.

I don't think this is a given ...  In fact, IMO if we're only two or
three fixes away from having it all nice and consistent, I think
reverting is not necessary.

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  33. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-23T04:02:10Z

    > 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

  34. Re: Review: Non-inheritable check constraints

    Robert Haas <robertmhaas@gmail.com> — 2011-12-23T15:27:32Z

    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

  35. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2011-12-26T14:49:28Z

    > > 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

  36. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-01-16T22:28:56Z

    Excerpts from Nikhil Sontakke's message of vie dic 23 01:02:10 -0300 2011:

> FWIW, here's a quick fix for the issue that Robert pointed out.

Thanks, applied.

> Again it's
> a variation of the first issue that he reported. We have two functions
> which try to merge existing constraints:
> 
> MergeWithExistingConstraint() in heap.c
> MergeConstraintsIntoExisting() in tablecmds.c
> 
> Nice names indeed :)

Yeah, I added a comment about the other one on each of them.

> I have also tried to change the error message as per Alvaro's suggestions.
> I will really try to see if we have other issues. Really cannot have Robert
> reporting all the bugs and getting annoyed, but there are lotsa variations
> possible with inheritance..

So did you find anything?

-- 
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

  37. Re: Review: Non-inheritable check constraints

    Alvaro Herrera <alvherre@commandprompt.com> — 2012-01-16T22:30:51Z

    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

  38. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2012-01-17T06:36:28Z

    > > 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

  39. Re: Review: Non-inheritable check constraints

    Nikhils <nikkhils@gmail.com> — 2012-01-17T06:38:54Z

    > > 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