On Thu, Oct 6, 2011 at 11:38 PM, Alex Hunsaker <badalex@gmail.com> wrote:
>> Oh, I see the problem, and I now agree that it's the DROP CONSTRAINT
>> code that is buggy.
>
> Want me to roll this fix in as part of the alter table only constraint
> patch? Or keep it split out? We might want to backpatch to at least
> 8.3 where HOT was introduced (yes looks like the bug existed back
> then). I suppose its a fairly narrow chance to hit this bug so I could
> see the argument for not back patching...
Yeah, I'm not inclined to back-patch it. The chance of hitting this
in older versions must be very small, or somebody would have noticed
by now. If we get a report from the field, we can always back-patch
it then, but right now it doesn't seem worth taking any risks for.
On a related note, your fix seems slightly fragile to me ... because
we're pulling a CCI out of the innermost loop, but a CCI can still
happen inside that same loop if we recurse, because the recursive call
will do one before returning. Now, maybe that's OK, because the
recursive call in that case will just be deleting the tuple, so there
won't be a new version for us to stumble over. The only way we could
trip up in that case is if there were two identically named
constraints. We'd have to visit the first tuple, update it, then
visit the second tuple, recurse (thus incrementing the command
counter), and then visit the updated version of the first tuple. And
that should be impossible, because we've got code to disallow multiple
constraints on the same relation with the same name (though no unique
index, for some reason). Still, that's a long chain of reasoning, so
I'm wondering if we can't come up with something that is more
obviously correct.
If we're confident that the inner loop here should never iterate more
than once (i.e. the lack of a unique index is not an ominous sign)
then maybe we should just rewrite this so that the inner loop scans
until it finds a match and then terminates. Then, outside the loop,
we check whether a tuple was found and if so process it - but without
ever going back to look for another one. See attached.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company