Re: patch for check constraints using multiple inheritance

On Fri, Jul 30, 2010 at 11:35 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> In the case of coninhcount, I believe that the fix actually is quite
> simple, although of course it's possible that I'm missing something.
> Currently, ATAddConstraint() first calls AddRelationNewConstraints()
> to add the constraint, or possibly merge it into an existing,
> inherited constraint; it then adds the constraint to the phase-3 work
> queue so that it will be checked; and finally it recurses to its own
> children.  It seems to me that it should only recurse to its children
> if the constraint was really added, rather than merged into an
> existing constraint, because if it was merged into an existing
> constraint, then the children already have it.  I'm still trying to
> figure out why this bug doesn't show up in simpler cases, but I think
> that's the root of the issue.

OK, it looks like level_2_parent is actually irrelevant to this issue.So here's a slightly simplified test case:

DROP SCHEMA IF EXISTS test_inheritance CASCADE;
CREATE SCHEMA test_inheritance;
SET search_path TO test_inheritance;

CREATE TABLE top (i int);
CREATE TABLE mid1 () INHERITS (top);
CREATE TABLE mid2 () INHERITS (top);
CREATE TABLE bottom () INHERITS (mid1, mid2);
CREATE TABLE basement () INHERITS (bottom);

ALTER TABLE top ADD CONSTRAINT a_check_constraint CHECK (i = 0);

You can also trigger it this way:

DROP SCHEMA IF EXISTS test_inheritance CASCADE;
CREATE SCHEMA test_inheritance;
SET search_path TO test_inheritance;

CREATE TABLE top1 (i int);
CREATE TABLE top2 (i int);
CREATE TABLE bottom () INHERITS (top1, top2);
CREATE TABLE basement () INHERITS (bottom);

ALTER TABLE top1 ADD CONSTRAINT a_check_constraint CHECK (i = 0);
ALTER TABLE top2 ADD CONSTRAINT a_check_constraint CHECK (i = 0);

In other words, the problem occurs when table A inherits a constraint
from multiple parents, and table B inherits from table A.  Most of the
code (including ALTER TABLE .. DROP CONSTRAINT and ALTER TABLE .. [NO]
INHERIT) maintains the invariant that coninhcount is the number of
direct parents from which the constraint is inherited, but the ADD
CONSTRAINT fails to do so in this particular case.  So at this point,
I'm fairly confident that this is the correct fix.  I think the reason
we haven't noticed this before is that it's most likely to occur when
you have a diamond inheritance pattern with another child hanging off
the bottom, which is not something most people probably do.

It appears that the coninhcount mechanism was introduced in 8.4; prior
to that, we didn't really make an effort to get this right.
Therefore, I believe the patch I posted upthread should be applied to
HEAD, REL9_0_STABLE, and REL8_4_STABLE.

This still leaves open the question of what to do about the similar
case involving attributes:

DROP SCHEMA IF EXISTS test_inheritance CASCADE;
CREATE SCHEMA test_inheritance;
SET search_path TO test_inheritance;

CREATE TABLE top (i int);
CREATE TABLE mid1 () INHERITS (top);
CREATE TABLE mid2 () INHERITS (top);
CREATE TABLE bottom () INHERITS (mid1, mid2);
CREATE TABLE basement () INHERITS (bottom);

ALTER TABLE top ADD COLUMN a_table_column integer;

That problem looks significantly more difficult to solve, though.

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