Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two
From: Robert Haas on 30 Jul 2010 09:45
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.
>
> A test script is provided below and a proposed patch is attached to this
> email.

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.

--
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: Tom Lane on 30 Jul 2010 10:46
Robert Haas <robertmhaas(a)gmail.com> writes:
> On Fri, Jul 30, 2010 at 10:23 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
>> 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.

> Since the output in the previous email apparently wasn't sufficient
> for you to understand what the problem is, here it is in more detail.
> ...
> Adding a column to the toplevel parent of the inheritance hierarchy
> and then dropping it again shouldn't leave a leftover copy of the
> column in the grandchild.

Actually, it probably should. The inheritance rules were intentionally
designed to avoid dropping inherited columns that could conceivably
still contain valuable data. There isn't enough information in the
inhcount/islocal data structure to recognize that a multiply-inherited
column is ultimately derived from only one distant ancestor, but it was
agreed that an exact tracking scheme would be more complication than it
was worth. Instead, the mechanism is designed to ensure that no column
will be dropped if it conceivably is still wanted --- not that columns
might not be left behind and require another drop step.

*Please* go re-read the old discussions before you propose tampering
with this behavior. In particular I really really do not believe that
any one-line fix is going to make things better --- almost certainly
it will break other cases. Being materially more intelligent would
require a more complex data structure.

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: Yeb Havinga on 30 Jul 2010 11:30
Tom Lane wrote:
> Robert Haas <robertmhaas(a)gmail.com> writes:
>> Since the output in the previous email apparently wasn't sufficient
>> for you to understand what the problem is, here it is in more detail.
>> ...
>> Adding a column to the toplevel parent of the inheritance hierarchy
>> and then dropping it again shouldn't leave a leftover copy of the
>> column in the grandchild.
>
> Actually, it probably should. The inheritance rules were intentionally
> designed to avoid dropping inherited columns that could conceivably
> still contain valuable data. There isn't enough information in the
> inhcount/islocal data structure to recognize that a multiply-inherited
> column is ultimately derived from only one distant ancestor, but it was
> agreed that an exact tracking scheme would be more complication than it
> was worth. Instead, the mechanism is designed to ensure that no column
> will be dropped if it conceivably is still wanted --- not that columns
> might not be left behind and require another drop step.
This is not about dropping columns or not, but about proper maintenance
of the housekeeping of coninhcount, defined as "The number of direct
inheritance ancestors this constraint has. A constraint with a nonzero
number of ancestors cannot be dropped nor renamed".

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.

This wrong number is a bug.
> *Please* go re-read the old discussions before you propose tampering
> with this behavior. In particular I really really do not believe that
> any one-line fix is going to make things better --- almost certainly
> it will break other cases.
Our (more than one line) patch was explicitly designed to walk from a
specific parent to a child exactly once. It passes regression tests.

regards,
Yeb Havinga


--
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 10:32
On Fri, Jul 30, 2010 at 10:23 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> 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.

Well, I'm glad you spent a lot of time hashing it out, but you've got
a bug. :-)

Since the output in the previous email apparently wasn't sufficient
for you to understand what the problem is, here it is in more detail.
Initial setup:

DROP SCHEMA IF EXISTS test_inheritance CASCADE;
CREATE SCHEMA test_inheritance;
SET search_path TO test_inheritance;
CREATE TABLE level_0_parent (i int);
CREATE TABLE level_0_child (a text) INHERITS (level_0_parent);
CREATE TABLE level_1_parent() INHERITS (level_0_parent);
CREATE TABLE level_1_child() INHERITS (level_0_child, level_1_parent);
CREATE TABLE level_2_parent() INHERITS (level_1_parent);
CREATE TABLE level_2_child() INHERITS (level_1_child, level_2_parent);

Then:

rhaas=# \d level_2_child
Table "test_inheritance.level_2_child"
Column | Type | Modifiers
--------+---------+-----------
i | integer |
a | text |
Inherits: level_1_child,
level_2_parent

rhaas=# ALTER TABLE level_0_parent ADD COLUMN a_new_column integer;
NOTICE: merging definition of column "a_new_column" for child "level_1_child"
NOTICE: merging definition of column "a_new_column" for child "level_2_child"
NOTICE: merging definition of column "a_new_column" for child "level_2_child"
ALTER TABLE
rhaas=# ALTER TABLE level_0_parent DROP COLUMN a_new_column;
ALTER TABLE
rhaas=# \d level_2_child
Table "test_inheritance.level_2_child"
Column | Type | Modifiers
--------------+---------+-----------
i | integer |
a | text |
a_new_column | integer |
Inherits: level_1_child,
level_2_parent

Adding a column to the toplevel parent of the inheritance hierarchy
and then dropping it again shouldn't leave a leftover copy of the
column in the grandchild.

rhaas=# ALTER TABLE level_2_child DROP COLUMN a_new_column;
ERROR: cannot drop inherited column "a_new_column"

--
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: Robert Haas on 30 Jul 2010 10:19
On Fri, Jul 30, 2010 at 10:11 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> 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?

We didn't. That logic is broken, too. Using the OP's test setup:

rhaas=# alter table level_0_parent add column a_new_column integer;
NOTICE: merging definition of column "a_new_column" for child "level_1_child"
NOTICE: merging definition of column "a_new_column" for child "level_2_child"
NOTICE: merging definition of column "a_new_column" for child "level_2_child"
ALTER TABLE
rhaas=# SELECT t.oid, t.relname, a.attinhcount
FROM pg_class t
JOIN pg_attribute a ON (a.attrelid = t.oid)
JOIN pg_namespace n ON (t.relnamespace = n.oid)
WHERE n.nspname = 'test_inheritance' AND a.attname = 'a_new_column'
ORDER BY t.oid;
oid | relname | attinhcount
-------+----------------+-------------
16420 | level_0_parent | 0
16423 | level_0_child | 1
16429 | level_1_parent | 1
16432 | level_1_child | 2
16438 | level_2_parent | 1
16441 | level_2_child | 3
(6 rows)

The attached patch (please review) appears to fix the coninhcount
case. I haven't tracked down the attinhcount case yet.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4 5
Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two