Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two
From: Tom Lane on 30 Jul 2010 10:11
Robert Haas <robertmhaas(a)gmail.com> writes:
> On Thu, Jul 29, 2010 at 6:57 AM, Henk Enting <h.d.enting(a)mgrid.net> wrote:
>> We ran into a problem on 9.0beta3 with check constraints using table
>> inheritance in a multi-level hierarchy with multiple inheritance.

> Thanks for the report. This bug also appears to exist in 8.4; I'm not
> sure yet how far back it goes. I'm not so sure your proposed patch is
> the right fix, though; it seems like it ought to be the job of
> AddRelationNewConstraints() and MergeWithExistingConstraint() to make
> sure that the right thing happens, here.

The original design idea was that coninhcount/conislocal would act
exactly like attinhcount/attislocal do for multiply-inherited columns.
Where did we fail to copy that logic?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 30 Jul 2010 11:39
Yeb Havinga <yebhavinga(a)gmail.com> writes:
> Regard the following lattice (direction from top to bottom):

> 1
> |\
> 2 3
> \|\
> 4 5
> \|
> 6

> When adding a constraint to 1, the proper coninhcount for that
> constraint on relation 6 is 2. But the code currently counts to 3, since
> 6 is reached by paths 1-2-4-5, 1-3-4-6, 1-3-5-6.

Mph. I'm not sure that 3 is wrong. You have to consider what happens
during a DROP CONSTRAINT, which as far as I saw this patch didn't
address.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 30 Jul 2010 10:23
Robert Haas <robertmhaas(a)gmail.com> writes:
> On Fri, Jul 30, 2010 at 10:11 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>> The original design idea was that coninhcount/conislocal would act
>> exactly like attinhcount/attislocal do for multiply-inherited columns.
>> Where did we fail to copy that logic?

> We didn't. That logic is broken, too.

Uh, full stop there. If you think that the multiply-inherited column
logic is wrong, it's you that is mistaken --- or at least you're going
to have to do a lot more than just assert that you don't like it.
We spent a *lot* of time hashing that behavior out, back around 7.3.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 30 Jul 2010 12:22
On Fri, Jul 30, 2010 at 11:35 AM, Robert Haas <robertmhaas(a)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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Alex Hunsaker on 30 Jul 2010 13:34
On Fri, Jul 30, 2010 at 10:22, Robert Haas <robertmhaas(a)gmail.com> wrote:

> 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);

The other problem with the current code is it creates a dump that is
not consistent with \d. The dump gets it "right" in the sense that
basement does not have the constraint. We could debate what
coninhcount should be, but clearly there is a bug here. [ FYI with
your patch the dump is, of course, consistent again (no
a_check_constraint) ]

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3 4 5
Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two