Prev: knngist - 0.8
Next: [HACKERS] reducing NUMERIC size for 9.1, take two
From: Yeb Havinga on 30 Jul 2010 15:22
Robert Haas wrote:
>> 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.
Yes. (then the children already have it -> already have it from the
current parent) - now I understand how AddRelationNewConstraints is
used, this effectively blocks > 1 times propagation to childs from the
same parent, and that is what our patch was written todo too.
> OK, it looks like level_2_parent is actually irrelevant to this issue.
> So here's a slightly simplified test case:
>
> 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);
>
This is, probably provable, the smallest case where this problem can
occur. Removing any table would mean that this bug is not hit anymore.
> This still leaves open the question of what to do about the similar
> case involving attributes:
>
> That problem looks significantly more difficult to solve, though.
>
I'm looking at ATPrepAddColumn right now, where there is actually some
comments about getting the right attinhcount in the case of multiple
inherited children, but it's the first time I'm looking at this part of
PostgreSQL and it needs some time to sink in. It looks a bit like at the
place of the comment /* child should see column as singly inherited */,
maybe the recursion could be stopped there as well, i.e. not only
setting inhcount to 1, but actually adding the child relation one time
to the wqueue.

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

While I certainly agree that accidentally dropping a column that
should be retained is a lot worse than accidentally retaining a column
that should be dropped, that doesn't make the current behavior
correct. I don't think that's really an arguable point. Another drop
step doesn't help: the leftover column is undroppable short of nuking
the whole table.

So let's focus on what the actual problem is here, what is required to
fix it, and whether or not and how far the fix can be back-patched.
There are two separate bugs here, so we should consider them
individually. In each case, the problem is that with a certain
(admittedly rather strange) inheritance hierarchy, adding a column to
the top-level parent results in the inheritance count in the
bottom-most child ending up as 3 rather than 2. 2 is the correct
value because the bottom-most child has 2 immediate parents. The
consequence of ending up with a count of 3 rather than 2 is that if
the constraint/column added at the top-level is subsequent dropped,
necessarily also from the top-level, the bottom-level child ends up
with the constraint/column still in existence and with an inheritance
count of 1. This isn't the correct behavior, and furthermore the
child object can't be dropped, because it still believes that it's
inherited (even though its parents no longer have a copy to inherit).

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.

attinhcount exhibits analogous misbehavior, but if you're saying the
fix for that case is going to be a lot more complicated, I think I
agree with you. In that case, it seems that we traverse the
inheritance hierarchy during the prep phase, at which point we don't
yet know whether we're going to end up merging anything. It might be
that a fix for this part of the problem ends up being too complex to
back-patch.

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

The behavior of DROP CONSTRAINT matches the fine documentation. The
behavior of ADD CONSTRAINT does not. Perhaps there's a definition of
coninhcount that would make 3 the right answer, but (a) I'm not sure
what that definition would be and (b) it's certainly not the one in
the manual.

--
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: Yeb Havinga on 30 Jul 2010 16:38
Yeb Havinga wrote:
> Robert Haas wrote:
>> This still leaves open the question of what to do about the similar
>> case involving attributes:
>>
>> That problem looks significantly more difficult to solve, though.
>>
> I'm looking at ATPrepAddColumn right now, where there is actually some
> comments about getting the right attinhcount in the case of multiple
> inherited children, but it's the first time I'm looking at this part
> of PostgreSQL and it needs some time to sink in. It looks a bit like
> at the place of the comment /* child should see column as singly
> inherited */, maybe the recursion could be stopped there as well, i.e.
> not only setting inhcount to 1, but actually adding the child relation
> one time to the wqueue.
I believe the crux is to stop double recursion from a parent in
ATOneLevelRecursion. I did a quick test by adding a globally defined

static List *visitedrels = NIL;

together by adding in front of ATOneLevelRecursion this:

if (list_member_oid(visitedrels, relid))
return;
visitedrels = lappend_oid(visitedrels, relid);

Running Roberts example gives the correct attinhcount for the basement.

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_table_column'
ORDER BY oid;

oid | relname | attinhcount
-------+----------+-------------
16566 | top | 0
16569 | mid1 | 1
16572 | mid2 | 1
16575 | bottom | 2
16578 | basement | 1

regards,
Yeb Havinga

PS: forgot to say thanks for looking into this problem earlier in the
thread. Thanks!


--
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 31 Jul 2010 12:02
On Fri, Jul 30, 2010 at 4:38 PM, Yeb Havinga <yebhavinga(a)gmail.com> wrote:
>> I'm looking at ATPrepAddColumn right now, where there is actually some
>> comments about getting the right attinhcount in the case of multiple
>> inherited children, but it's the first time I'm looking at this part of
>> PostgreSQL and it needs some time to sink in. It looks a bit like at the
>> place of the comment /* child should see column as singly inherited */,
>> maybe the recursion could be stopped there as well, i.e. not only setting
>> inhcount to 1, but actually adding the child relation one time to the
>> wqueue.
>
> I believe the crux is to stop double recursion from a parent in
> ATOneLevelRecursion. I did a quick test by adding a globally defined
>
> static List *visitedrels = NIL;

I agree that's the crux of the problem, but I can't see solving it
with a global variable. I realize you were just testing...

> PS: forgot to say thanks for looking into this problem earlier in the
> thread. Thanks!

yw

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

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