Обсуждение: Problem identifying constraints which should not be inherited

"Chris Fischer" <Chris.Fischer@channeladvisor.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.Fischer@channeladvisor.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@momjian.us>        http://momjian.us
  EnterpriseDB                             http://postgres.enterprisedb.com

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

Hi,

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

NikhilS <nikkhils@gmail.com> writes:
> On Fri, Mar 7, 2008 at 6:37 AM, Bruce Momjian <bruce@momjian.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@sss.pgh.pa.us> wrote:

> NikhilS <nikkhils@gmail.com> writes:
> > On Fri, Mar 7, 2008 at 6:37 AM, Bruce Momjian <bruce@momjian.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@sss.pgh.pa.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@commandprompt.com>
wrote:

> NikhilS escribi=F3:
>
> > On Wed, Mar 19, 2008 at 8:23 PM, Tom Lane <tgl@sss.pgh.pa.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
--=20
EnterpriseDB http://www.enterprisedb.com

Alvaro Herrera <alvherre@commandprompt.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,


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@gmail.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,


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@gmail.com> wrote:
> Hi Alex,
>
>
> On Fri, Mar 28, 2008 at 4:58 AM, Alex Hunsaker <badalex@gmail.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

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