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,

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

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@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ó:

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

On Thu, Mar 20, 2008 at 7:36 PM, Tom Lane <tgl@sss.pgh.pa.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@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,

On Fri, Mar 28, 2008 at 4:58 AM, Alex Hunsaker <badalex@gmail.com> wrote:

On Thu, Mar 27, 2008 at 5:14 AM, NikhilS <nikkhils@gmail.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@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!

Hi Alvaro

On Fri, Mar 28, 2008 at 6:05 PM, Alvaro Herrera <alvherre@commandprompt.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