Re: patch for check constraints using multiple inheritance

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