Problem identifying constraints which should not be inherited

There's no way in the pg_ tables to identify when a check constraint on
an inherited table should not be inherited. Example:

create table t1 (col1 number(10));

create table t2 () inherits t2;

alter table only t1 add constraint ck_col1 check (number <> 0);

Now, run pg_dump and notice that the check constraint is not created
with ONLY in the dump and, therefore, ends up on t2 when executed.

Chris Fischer
Database Engineer

<http://www.channeladvisor.com/>

ChannelAdvisor Corporation
2701 Aerial Center Parkway
Morrisville | North Carolina | 27560
919.228.2011

Many Channels. One Advisor.
www.channeladvisor.com <http://www.channeladvisor.com/>

<http://feeds.feedburner.com/ChanneladvisorBlog>

"Chris Fischer" <Chris(dot)Fischer(at)channeladvisor(dot)com> writes:
> alter table only t1 add constraint ck_col1 check (number <> 0);

The bug here is that we allow that. Continuing your example,

regression=# insert into t2 values(0);
INSERT 0 1
regression=# select * from t1;
col1
------
0
(1 row)

which sure looks to me like a violation of the principle of least
surprise.

This has come up before and I think the consensus was to disallow
non-inherited check constraints; not sure why it hasn't been done yet.

regards, tom lane

Added to TODO:

> o Require all CHECK constraints to be inherited
>
> http://archives.postgresql.org/pgsql-bugs/2007-04/msg00026.php

---------------------------------------------------------------------------

Tom Lane wrote:
> "Chris Fischer" <Chris(dot)Fischer(at)channeladvisor(dot)com> writes:
> > alter table only t1 add constraint ck_col1 check (number <> 0);
>
> The bug here is that we allow that. Continuing your example,
>
> regression=# insert into t2 values(0);
> INSERT 0 1
> regression=# select * from t1;
> col1
> ------
> 0
> (1 row)
>
> which sure looks to me like a violation of the principle of least
> surprise.
>
> This has come up before and I think the consensus was to disallow
> non-inherited check constraints; not sure why it hasn't been done yet.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Hi,

On Fri, Mar 7, 2008 at 6:37 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:

>
> Added to TODO:
>
> > o Require all CHECK constraints to be inherited
> >
> > http://archives.postgresql.org/pgsql-bugs/2007-04/msg00026.php
>
>
PFA, a small patch attached which should fix this. I have made relevant
changes in the relevant regression files too.

I was wondering though if there are other locations where we might need to
add checks to ensure that ALTER TABLE ONLY parentrel operations are ok? I
did see checks for this in some other operations like ADD COLUMN already in
place too.

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS <nikkhils(at)gmail(dot)com> writes:
> On Fri, Mar 7, 2008 at 6:37 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>> Added to TODO:
>> o Require all CHECK constraints to be inherited

> PFA, a small patch attached which should fix this.

If it's a small patch, it's wrong by definition. AFAICS there is no way
to fix this correctly that doesn't involve catalog changes. The point
of the TODO is that you have to enforce that the inherited constraint
sticks around, eg can't be dropped on a child table while it's still
present on the parent. There are implications for pg_dump too.

regards, tom lane

Hi,

On Wed, Mar 19, 2008 at 8:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> NikhilS <nikkhils(at)gmail(dot)com> writes:
> > On Fri, Mar 7, 2008 at 6:37 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> >> Added to TODO:
> >> o Require all CHECK constraints to be inherited
>
> > PFA, a small patch attached which should fix this.
>
> If it's a small patch, it's wrong by definition. AFAICS there is no way
> to fix this correctly that doesn't involve catalog changes. The point
> of the TODO is that you have to enforce that the inherited constraint
> sticks around, eg can't be dropped on a child table while it's still
> present on the parent. There are implications for pg_dump too.
>

Ok, I understand. But even then this could patch could be considered even if
it does not solve the TODO completely, no? It atleast disallows ONLY ADD
CONSTRAINT on the parent.

Regards,
Nikhils

--
EnterpriseDB http://www.enterprisedb.com

NikhilS escribió:

> On Wed, Mar 19, 2008 at 8:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> > If it's a small patch, it's wrong by definition. AFAICS there is no way
> > to fix this correctly that doesn't involve catalog changes. The point
> > of the TODO is that you have to enforce that the inherited constraint
> > sticks around, eg can't be dropped on a child table while it's still
> > present on the parent. There are implications for pg_dump too.
>
> Ok, I understand. But even then this could patch could be considered even if
> it does not solve the TODO completely, no? It atleast disallows ONLY ADD
> CONSTRAINT on the parent.

No, because you would then feel that the TODO item is completed and not
provide a patch for the whole problem :-)

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Hi,

On Thu, Mar 20, 2008 at 6:11 PM, Alvaro Herrera <alvherre(at)commandprompt(dot)com>
wrote:

> NikhilS escribió:
>
> > On Wed, Mar 19, 2008 at 8:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> > > If it's a small patch, it's wrong by definition. AFAICS there is no
> way
> > > to fix this correctly that doesn't involve catalog changes. The point
> > > of the TODO is that you have to enforce that the inherited constraint
> > > sticks around, eg can't be dropped on a child table while it's still
> > > present on the parent. There are implications for pg_dump too.
> >
> > Ok, I understand. But even then this could patch could be considered
> even if
> > it does not solve the TODO completely, no? It atleast disallows ONLY ADD
> > CONSTRAINT on the parent.
>
> No, because you would then feel that the TODO item is completed and not
> provide a patch for the whole problem :-)
>

:)
Guess, I should have been wordier in my earlier response and should have
mentioned:
"This patch, if acceptable can be considered as a small step towards the
TODO"
too.

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> NikhilS escribi:
>> Ok, I understand. But even then this could patch could be considered even if
>> it does not solve the TODO completely, no? It atleast disallows ONLY ADD
>> CONSTRAINT on the parent.

> No, because you would then feel that the TODO item is completed and not
> provide a patch for the whole problem :-)

More to the point, it takes a capability away from the user without
actually solving the problem we need to solve, namely to guarantee
consistency between parent and child constraints. You can be sure
that there is someone out there who will complain that we've broken
his application when we disallow this, and we need to be able to
point to some positive benefit we got from it.

regards, tom lane

Hi,

On Thu, Mar 20, 2008 at 7:36 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

>
> More to the point, it takes a capability away from the user without
> actually solving the problem we need to solve, namely to guarantee
> consistency between parent and child constraints. You can be sure
> that there is someone out there who will complain that we've broken
> his application when we disallow this, and we need to be able to
> point to some positive benefit we got from it.
>

Agreed. So I think we need to implement the whole enchilada or nothing at
all. We need to do the following for this:

* Add logic to disallow ADD CONSTRAINT ONLY to parent of an inheritance
hierarchy

* Add logic to mark inherited constraints in the children:
This can be achieved by introducing a new bool "coninherited" attribute in
pg_constraint. This will be set to true on only those check constraints that
are added to children via the inheritance mechanism

* Add logic to disallow dropping inherited constraints directly on children
Obviously they will get dropped if a DROP CONSTRAINT is fired on the parent.
with recurse set to true (this is the default behaviour)

* Modify the pg_dump logic to use the new pg_constraint based attribute
logic for version 80300 (or should it be PG_VERSION_NUM 80400?) onwards.
Infact the current logic to determine if a check constraint is inherited is
to compare its name with the parent constraint name and the comment already
mentions that we should make changes in pg_constraint to avoid this
rudimentary way of determing the inheritance :).

Am important decision here is about adding a new attribute to pg_constraint
as it is the only sane way of determining inherited constraints, but that
will require an initdb. Comments?

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS wrote:
> Am important decision here is about adding a new attribute to pg_constraint
> as it is the only sane way of determining inherited constraints, but that
> will require an initdb. Comments?

There's no problem forcing an initdb at this point in the release cycle.
We will do that for sure many times before we reach beta stage.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

NikhilS <nikkhils(at)gmail(dot)com> writes:
> ...
> * Add logic to mark inherited constraints in the children:
> This can be achieved by introducing a new bool "coninherited" attribute in
> pg_constraint. This will be set to true on only those check constraints that
> are added to children via the inheritance mechanism

It probably needs to be a count not a bool. You can/should use the
handling of inherited attributes as a pattern to follow --- look at
attislocal and attinhcount.

regards, tom lane

Hi Alex,

On Fri, Mar 28, 2008 at 4:58 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:

> On Thu, Mar 27, 2008 at 5:14 AM, NikhilS <nikkhils(at)gmail(dot)com> wrote:
> > * Add logic to disallow ADD CONSTRAINT ONLY to parent of an inheritance
> > hierarchy
> >
> > * Add logic to mark inherited constraints in the children:
> > This can be achieved by introducing a new bool "coninherited" attribute
> in
> > pg_constraint. This will be set to true on only those check constraints
> that
> > are added to children via the inheritance mechanism
> >
> > * Add logic to disallow dropping inherited constraints directly on
> children
> > Obviously they will get dropped if a DROP CONSTRAINT is fired on the
> parent.
> > with recurse set to true (this is the default behaviour)
> >
> > * Modify the pg_dump logic to use the new pg_constraint based attribute
> > logic for version 80300 (or should it be PG_VERSION_NUM 80400?) onwards.
> > Infact the current logic to determine if a check constraint is inherited
> is
> > to compare its name with the parent constraint name and the comment
> already
> > mentions that we should make changes in pg_constraint to avoid this
> > rudimentary way of determing the inheritance :).
> >
> >
> Attached is a WIP patch I have been playing with in my spare time. It
> should take care of the first 2. It does nothing for pg_dump or set
> (not) null/set default.
>
> Note it has some gross points (see comment in
> src/backend/catalog/heap.c AddRelationRawConstraints) and the
> regression tests I added are not quite up to par (and probably a bit
> redundant). But in the interest of saving work I thought i would post
> it.
>
>
I took a quick look and it seems to be on the lines of attislocal and
attinhcount which is a good thing. I am not sure about your syscache related
changes though and also about using enums for pg_constraint attributes which
deviates from other catalog specifications. Its good that you posted your
WIP here immediately or it would have caused duplication of efforts from my
side :)

I will take a look at the pg_dump related changes if you want. We will need
changes in flagInhAttrs() and in getTableAttrs() to query the backend for
these 2 attributes for post 80300 versions.

P.S Alvaro, I think this patch did not reach the mailing list and was
stalled due to size restrictions or something.

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com

NikhilS escribió:

> P.S Alvaro, I think this patch did not reach the mailing list and was
> stalled due to size restrictions or something.

Argh, you are right. We don't have this on the archives anywhere :-(
Probably it got stuck on Maia ...

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

NikhilS escribió:

> P.S Alvaro, I think this patch did not reach the mailing list and was
> stalled due to size restrictions or something.

OK, it's archived now:
http://archives.postgresql.org/pgsql-patches/2008-03/msg00392.php
Thanks.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Mar 28, 2008 at 4:07 AM, NikhilS <nikkhils(at)gmail(dot)com> wrote:
> Hi Alex,
>
>
> On Fri, Mar 28, 2008 at 4:58 AM, Alex Hunsaker <badalex(at)gmail(dot)com> wrote:
> > Attached is a WIP patch I have been playing with in my spare time. It
> > should take care of the first 2. It does nothing for pg_dump or set
> > (not) null/set default.
> >

Note that should say first 3 items...

>
> I took a quick look and it seems to be on the lines of attislocal and
> attinhcount which is a good thing. I am not sure about your syscache related
> changes though and also about using enums for pg_constraint attributes which
> deviates from other catalog specifications. Its good that you posted your
> WIP here immediately or it would have caused duplication of efforts from my
> side :)
>

Yeah I was planning on taking the enums out. Or at least the one i
did in pg_constraint.h.... Like I said it was a WIP :) The syscache
stuff mainly for ease of use but can be taken out easily enough if
anyone objects. (I added unique index on conrelid, constrname and
SearchSysCache(CONSTRNAME...) for the curious who did not look at the
patch).

Ill take out the enums and see about getting a real fix for
create table (x int check constraint ...) inherits ((some table that
has an x int check constraint)

expect v2 sometime later today....

> I will take a look at the pg_dump related changes if you want. We will need
> changes in flagInhAttrs() and in getTableAttrs() to query the backend for
> these 2 attributes for post 80300 versions.
>

That would be great!

> P.S Alvaro, I think this patch did not reach the mailing list and was
> stalled due to size restrictions or something.
>
>
> Regards,
> Nikhils
> --
> EnterpriseDB http://www.enterprisedb.com

Hi Alvaro

On Fri, Mar 28, 2008 at 6:05 PM, Alvaro Herrera <alvherre(at)commandprompt(dot)com>
wrote:

> NikhilS escribió:
>
> > I will take a look at the pg_dump related changes if you want. We will
> need
> > changes in flagInhAttrs() and in getTableAttrs() to query the backend
> for
> > these 2 attributes for post 80300 versions.
>
> Oh, BTW, I have not added this patch to any Commitfest page on the wiki,
> since it has obvious things that need more work. If you do work on
> them, please post the improved patch later and add it to the
> corresponding Commitfest, as appropriate.
>

I submitted the combined latest patch to the patches list yesterday. How can
I add it to the commitfest (presumably to the May one?)? Please let me know.

Regards,
Nikhils
--
EnterpriseDB http://www.enterprisedb.com