Re: how to create a non-inherited CHECK constraint in CREATE TABLE

It appears that the only way to create a non-inherited CHECK constraint
is using ALTER TABLE. Is there no support in CREATE TABLE planned?
That looks a bit odd.

Excerpts from Peter Eisentraut's message of mar ene 17 13:59:57 -0300 2012:
> It appears that the only way to create a non-inherited CHECK constraint
> is using ALTER TABLE. Is there no support in CREATE TABLE planned?
> That looks a bit odd.

There are no plans to do that AFAIR, though maybe you could convince
Nikhil to write the patch to do so.

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

On Jan 17, 2012, at 11:07 AM, Alvaro Herrera wrote:
> Excerpts from Peter Eisentraut's message of mar ene 17 13:59:57 -0300 2012:
>> It appears that the only way to create a non-inherited CHECK constraint
>> is using ALTER TABLE. Is there no support in CREATE TABLE planned?
>> That looks a bit odd.
>
> There are no plans to do that AFAIR, though maybe you could convince
> Nikhil to write the patch to do so.

That certainly doesn't meet the principle of least surprise... CREATE TABLE should support this.
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

> >> It appears that the only way to create a non-inherited CHECK constraint
> >> is using ALTER TABLE. Is there no support in CREATE TABLE planned?
> >> That looks a bit odd.
> >
> > There are no plans to do that AFAIR, though maybe you could convince
> > Nikhil to write the patch to do so.
>
> That certainly doesn't meet the principle of least surprise... CREATE
> TABLE should support this.
>

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

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

But, let me have a stab at it when I get some free cycles.

Regards,
Nikhils

On Wed, Jan 18, 2012 at 12:10 AM, Nikhil Sontakke <nikkhils(at)gmail(dot)com> wrote:
>> >> It appears that the only way to create a non-inherited CHECK constraint
>> >> is using ALTER TABLE.  Is there no support in CREATE TABLE planned?
>> >> That looks a bit odd.
>> >
>> > There are no plans to do that AFAIR, though maybe you could convince
>> > Nikhil to write the patch to do so.
>>
>> That certainly doesn't meet the principle of least surprise... CREATE
>> TABLE should support this.
>
> 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
> then:
>
> http://postgresql.1045698.n5.nabble.com/Check-constraints-on-partition-parents-only-tt4633334.html#a4647144
>
> But, let me have a stab at it when I get some free cycles.

I agree with Peter that we should have we should have CHECK ONLY.
ONLY is really a property of the constraint, not the ALTER TABLE
command -- if it were otherwise, we wouldn't need to store it the
system catalogs, but of course we do. The fact that it's not a
standard property isn't a reason not to have proper syntax for it.

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

On ons, 2012-01-18 at 18:17 -0500, Robert Haas wrote:
> I agree with Peter that we should have we should have CHECK ONLY.
> ONLY is really a property of the constraint, not the ALTER TABLE
> command -- if it were otherwise, we wouldn't need to store it the
> system catalogs, but of course we do. The fact that it's not a
> standard property isn't a reason not to have proper syntax for it.

Clearly, we will eventually want to support inherited and non-inherited
constraints of all types. Currently, each type of constraint has an
implicit default regarding this property:

check - inherited
not null - inherited
foreign key - not inherited
primary key - not inherited
unique - not inherited
exclusion - not inherited

As discussed above, we need to have a syntax that is attached to the
constraint, not the table operation that creates the constraint, so that
we can also create these in CREATE TABLE.

How should we resolve these different defaults?

Also, in ALTER TABLE, if you want to add either an inherited or not
inherited constraint to a parent table, you should really say ALTER
TABLE ONLY in either case. Because it's conceivably valid that ALTER
TABLE foo ADD CHECK () NOINHERIT would add an independent, not inherited
check constraint to each child table.

So, there are all kinds of inconsistencies and backward compatibility
problems lurking here. We might need either a grand transition plan or
document the heck out of these inconsistencies.

Hi,

So, I have a patch for this. This patch introduces support for

CHECK ONLY syntax while doing a CREATE TABLE as well as during the usual
ALTER TABLE command.

Example:

create table atacc7 (test int, test2 int CHECK ONLY (test>0), CHECK
(test2>10));
create table atacc8 () inherits (atacc7);

postgres=# \d+ atacc7
Table "public.atacc7"
Column | Type | Modifiers | Storage | Description
--------+---------+-----------+---------+-------------
test | integer | | plain |
test2 | integer | | plain |
Check constraints:
"atacc7_test2_check" CHECK (test2 > 10)
"atacc7_test_check" CHECK ONLY (test > 0)
Child tables: atacc8
Has OIDs: no

postgres=# \d+ atacc8
Table "public.atacc8"
Column | Type | Modifiers | Storage | Description
--------+---------+-----------+---------+-------------
test | integer | | plain |
test2 | integer | | plain |
Check constraints:
"atacc7_test2_check" CHECK (test2 > 10)
Inherits: atacc7
Has OIDs: no

This patch removes the support for :

ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b > 0);

and uses

ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b > 0);

Is this what we want? Or we would want the earlier support in place for
backward compatibility as well? We are actually introducing this in 9.2 so
I guess we can remove this.

This is a much cleaner implementation and we might not even need the
changes in pg_dump now because the pg_get_constraintdef can provide the
info about the ONLY part too. So some cleanup can be done if needed.

I know it's a bit late in the commitfest, but if this patch makes this
feature more "complete", maybe we should consider...

Thoughts?

P.S Here's the discussion thread in its entirety for reference:
http://postgresql.1045698.n5.nabble.com/how-to-create-a-non-inherited-CHECK-constraint-in-CREATE-TABLE-td5152184.html

Regards,
Nikhils

On Thu, Feb 2, 2012 at 1:32 AM, Peter Eisentraut
<peter_e(at)gmx(dot)net> wrote:

> On ons, 2012-01-18 at 18:17 -0500, Robert Haas wrote:
> > I agree with Peter that we should have we should have CHECK ONLY.
> > ONLY is really a property of the constraint, not the ALTER TABLE
> > command -- if it were otherwise, we wouldn't need to store it the
> > system catalogs, but of course we do. The fact that it's not a
> > standard property isn't a reason not to have proper syntax for it.
>
> Clearly, we will eventually want to support inherited and non-inherited
> constraints of all types. Currently, each type of constraint has an
> implicit default regarding this property:
>
> check - inherited
> not null - inherited
> foreign key - not inherited
> primary key - not inherited
> unique - not inherited
> exclusion - not inherited
>
> As discussed above, we need to have a syntax that is attached to the
> constraint, not the table operation that creates the constraint, so that
> we can also create these in CREATE TABLE.
>
> How should we resolve these different defaults?
>
> Also, in ALTER TABLE, if you want to add either an inherited or not
> inherited constraint to a parent table, you should really say ALTER
> TABLE ONLY in either case. Because it's conceivably valid that ALTER
> TABLE foo ADD CHECK () NOINHERIT would add an independent, not inherited
> check constraint to each child table.
>
> So, there are all kinds of inconsistencies and backward compatibility
> problems lurking here. We might need either a grand transition plan or
> document the heck out of these inconsistencies.
>
>
>

Excerpts from Nikhil Sontakke's message of mié abr 11 15:07:45 -0300 2012:

> This patch removes the support for :
>
> ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b > 0);
>
> and uses
>
> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b > 0);
>
> Is this what we want? Or we would want the earlier support in place for
> backward compatibility as well? We are actually introducing this in 9.2 so
> I guess we can remove this.

I'm not quite following that logic. I don't think support for the
previous syntax should be removed -- does it cause some serious problem?

> This is a much cleaner implementation and we might not even need the
> changes in pg_dump now because the pg_get_constraintdef can provide the
> info about the ONLY part too. So some cleanup can be done if needed.
>
> I know it's a bit late in the commitfest, but if this patch makes this
> feature more "complete", maybe we should consider...
>
> Thoughts?

Personally I don't think we should consider this for 9.2.

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Nikhil Sontakke's message of mi abr 11 15:07:45 -0300 2012:
>> This patch removes the support for :
>>
>> ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b > 0);
>>
>> and uses
>>
>> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b > 0);
>>
>> I know it's a bit late in the commitfest, but if this patch makes this
>> feature more "complete", maybe we should consider...

> Personally I don't think we should consider this for 9.2.

Well, if we're going to regret having offered the other syntax, now
would be the time to figure that out, before we ship it not after.
I would go so far as to say that if we don't accept this for 9.2
we probably shouldn't accept it at all, because two different ways
to spell the same thing isn't nice.

I don't really care for the idea that the ONLY goes in a different place
for this operation than for every other kind of ALTER TABLE, but it does
make sense if you subscribe to the quoted theory that ONLY is a property
of the constraint and not the ALTER command as such.

regards, tom lane

On 04/11/2012 02:45 PM, Tom Lane wrote:
> Alvaro Herrera<alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Nikhil Sontakke's message of mié abr 11 15:07:45 -0300 2012:
>>> This patch removes the support for :
>>>
>>> ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b> 0);
>>>
>>> and uses
>>>
>>> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b> 0);
>>>
>>> I know it's a bit late in the commitfest, but if this patch makes this
>>> feature more "complete", maybe we should consider...
>> Personally I don't think we should consider this for 9.2.
> Well, if we're going to regret having offered the other syntax, now
> would be the time to figure that out, before we ship it not after.
> I would go so far as to say that if we don't accept this for 9.2
> we probably shouldn't accept it at all, because two different ways
> to spell the same thing isn't nice.
>
> I don't really care for the idea that the ONLY goes in a different place
> for this operation than for every other kind of ALTER TABLE, but it does
> make sense if you subscribe to the quoted theory that ONLY is a property
> of the constraint and not the ALTER command as such.
>
>

I think I rather dislike it. ONLY should be followed by the name of the
parent table whose children it causes us to exclude, IMNSHO. Moving it
elsewhere doesn't seem to me to be a blow for clarity at all.

cheers

andrew

Excerpts from Andrew Dunstan's message of mié abr 11 15:51:51 -0300 2012:
>
> On 04/11/2012 02:45 PM, Tom Lane wrote:
> > Alvaro Herrera<alvherre(at)commandprompt(dot)com> writes:
> >> Excerpts from Nikhil Sontakke's message of mié abr 11 15:07:45 -0300 2012:
> >>> This patch removes the support for :
> >>>
> >>> ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b> 0);
> >>>
> >>> and uses
> >>>
> >>> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b> 0);
> >>>
> >>> I know it's a bit late in the commitfest, but if this patch makes this
> >>> feature more "complete", maybe we should consider...
> >> Personally I don't think we should consider this for 9.2.
> > Well, if we're going to regret having offered the other syntax, now
> > would be the time to figure that out, before we ship it not after.
> > I would go so far as to say that if we don't accept this for 9.2
> > we probably shouldn't accept it at all, because two different ways
> > to spell the same thing isn't nice.
> >
> > I don't really care for the idea that the ONLY goes in a different place
> > for this operation than for every other kind of ALTER TABLE, but it does
> > make sense if you subscribe to the quoted theory that ONLY is a property
> > of the constraint and not the ALTER command as such.
>
> I think I rather dislike it. ONLY should be followed by the name of the
> parent table whose children it causes us to exclude, IMNSHO. Moving it
> elsewhere doesn't seem to me to be a blow for clarity at all.

If that's the only objection, maybe we could use a different keyword
then, perhaps NOINHERIT:

ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK NOINHERIT (b> 0);

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

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Andrew Dunstan's message of mi abr 11 15:51:51 -0300 2012:
>> On 04/11/2012 02:45 PM, Tom Lane wrote:
>>> I don't really care for the idea that the ONLY goes in a different place
>>> for this operation than for every other kind of ALTER TABLE, but it does
>>> make sense if you subscribe to the quoted theory that ONLY is a property
>>> of the constraint and not the ALTER command as such.

>> I think I rather dislike it. ONLY should be followed by the name of the
>> parent table whose children it causes us to exclude, IMNSHO. Moving it
>> elsewhere doesn't seem to me to be a blow for clarity at all.

> If that's the only objection, maybe we could use a different keyword
> then, perhaps NOINHERIT:

> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK NOINHERIT (b> 0);

I could live with that. "CHECK ONLY" isn't particularly transparent as
to what it means, anyway. "CHECK NOINHERIT" seems a lot clearer.

I'd propose "CHECK NO INHERIT", though, as (a) it seems better English
and (b) it avoids creating any new keyword.

regards, tom lane

On 04/11/2012 03:06 PM, Tom Lane wrote:
> I'd propose "CHECK NO INHERIT", though, as (a) it seems better English
> and (b) it avoids creating any new keyword.

I could live with that too.

cheers

andrew

On Wed, Apr 11, 2012 at 2:45 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Nikhil Sontakke's message of mié abr 11 15:07:45 -0300 2012:
>>> This patch removes the support for :
>>>
>>> ALTER TABLE ONLY constraint_rename_test ADD CONSTRAINT con2 CHECK (b > 0);
>>>
>>> and uses
>>>
>>> ALTER TABLE constraint_rename_test ADD CONSTRAINT con2 CHECK ONLY (b > 0);
>>>
>>> I know it's a bit late in the commitfest, but if this patch makes this
>>> feature more "complete", maybe we should consider...
>
>> Personally I don't think we should consider this for 9.2.
>
> Well, if we're going to regret having offered the other syntax, now
> would be the time to figure that out, before we ship it not after.
> I would go so far as to say that if we don't accept this for 9.2
> we probably shouldn't accept it at all, because two different ways
> to spell the same thing isn't nice.

+1 for fixing up the syntax before 9.2 goes out the door. I think the
original syntax was misguided to begin with.

CHECK NO INHERIT sounds fine to me; will that display ALTER TABLE ONLY
x as the one true way of doing this?

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> +1 for fixing up the syntax before 9.2 goes out the door. I think the
> original syntax was misguided to begin with.

Well, it was fine in isolation, but once you consider how to make CREATE
TABLE do this too, it's hard to avoid the conclusion that you need to
attach the modifier to the CHECK constraint not the ALTER TABLE command.

> CHECK NO INHERIT sounds fine to me; will that display ALTER TABLE ONLY
> x as the one true way of doing this?

s/display/displace/, I think you meant? Yeah, that's what I understand
the proposal to be.

regards, tom lane

Hi,

Cumulative reaction to all the responses first:

Whoa! :)

I was under the impression that a majority of us felt that the current
mechanism was inadequate. Also if you go through the nabble thread, the
fact that CREATE TABLE did not support such constraints was considered to
be an annoyance. And I was enquired if/when I can provide this
functionality. Apologies though with the timing.

> > +1 for fixing up the syntax before 9.2 goes out the door. I think the
> > original syntax was misguided to begin with.
>
> Well, it was fine in isolation, but once you consider how to make CREATE
> TABLE do this too, it's hard to avoid the conclusion that you need to
> attach the modifier to the CHECK constraint not the ALTER TABLE command.
>
>
Yeah, exactly.

> > CHECK NO INHERIT sounds fine to me; will that display ALTER TABLE ONLY
> > x as the one true way of doing this?
>
> s/display/displace/, I think you meant? Yeah, that's what I understand
> the proposal to be.
>
>
Displace yes. It would error out if someone says

ALTER TABLE ONLY... CHECK ();

suggesting to use the ONLY with the CHECK.

This patch does this and also makes both CREATE TABLE and ALTER TABLE use
it in a uniform manner.

Regarding "NO INHERIT" versus "ONLY", we again have had discussions on the
longish original thread quite a while back:

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

But now if we prefer NO INHERIT, I can live with that.

Regards,
Nikhils

On Thu, Apr 12, 2012 at 10:50:31AM +0530, Nikhil Sontakke wrote:
> > > CHECK NO INHERIT sounds fine to me; will that display ALTER TABLE ONLY
> > > x as the one true way of doing this?
> >
> > s/display/displace/, I think you meant? Yeah, that's what I understand
> > the proposal to be.

+1 for that proposal.

> Displace yes. It would error out if someone says
>
> ALTER TABLE ONLY... CHECK ();
>
> suggesting to use the ONLY with the CHECK.

I'd say the behavior for that case can revert to the PostgreSQL 9.1 behavior.
If the table has children, raise an error. Otherwise, add an inheritable
CHECK constraint, albeit one lacking inheritors at that moment.

Thanks,
nm

> > Displace yes. It would error out if someone says
> >
> > ALTER TABLE ONLY... CHECK ();
> >
> > suggesting to use the ONLY with the CHECK.
>
> I'd say the behavior for that case can revert to the PostgreSQL 9.1
> behavior.
> If the table has children, raise an error. Otherwise, add an inheritable
> CHECK constraint, albeit one lacking inheritors at that moment.
>
>
Ok, that sounds reasonable.

Another thing that we should consider is that if we are replacing ONLY with
NO INHERIT, then instead of just making a cosmetic syntactic change, we
should also replace all the is*only type of field names with noinherit for
the sake of completeness and uniformity.

Regards,
Nikhils

Excerpts from Nikhil Sontakke's message of lun abr 16 03:56:06 -0300 2012:
> > > Displace yes. It would error out if someone says
> > >
> > > ALTER TABLE ONLY... CHECK ();
> > >
> > > suggesting to use the ONLY with the CHECK.
> >
> > I'd say the behavior for that case can revert to the PostgreSQL 9.1
> > behavior.
> > If the table has children, raise an error. Otherwise, add an inheritable
> > CHECK constraint, albeit one lacking inheritors at that moment.
> >
> Ok, that sounds reasonable.

Good, I agree with that too.

Are you going to submit an updated patch? I started working on your
original a couple of days ago but got distracted by some family news
here. I'll send it to you so that you can start from there, to avoid
duplicate work.

> Another thing that we should consider is that if we are replacing ONLY with
> NO INHERIT, then instead of just making a cosmetic syntactic change, we
> should also replace all the is*only type of field names with noinherit for
> the sake of completeness and uniformity.

Yeah, I was considering the same thing. "conisonly" isn't a very good
name on its own (it only made sense because the ONLY came from "ALTER
TABLE ONLY").

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

Hi,

Are you going to submit an updated patch? I started working on your
> original a couple of days ago but got distracted by some family news
> here. I'll send it to you so that you can start from there, to avoid
> duplicate work.
>
>
Thanks Alvaro. PFA, a revised patch with the "NO INHERIT" support in the
complete sense. I also fixed up the ALTER TABLE ONLY ... CHECK semantics as
per Noah's suggestion upthread. We now allow ONLY if the table has no
children currently. The pg_dump sources have also been cleaned up now that
these constraints need not be dumped separately. So this is a good change
overall.

Have used connoinherit as the name in the catalog and Alvaro replaced
is_only references with is_no_inherit so that stays. I leave it to the
eventual committer now to pick up any more variations if desired :)

Regards,
Nikhils