On 2012-11-27 12:02:09 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2012-11-26 16:39:39 -0500, Tom Lane wrote:
> >> I looked at this a bit. I am very unhappy with the proposed kluge to
> >> open indexes NoLock in some places. Even if that's safe today, which
> >> I don't believe in the least, any future change in this area could break
> >> it.
>
> > I am not happy with it either. But I failed to see a better way. Note
> > that in most of the cases a lock actually is acquired shortly
> > afterwards, just a ->indisvalid or an ->indisready check away.
>
> I think you're missing the point.
True.
> The proposed patch will result in
> RelationGetIndexAttrBitmap trying to do index_open() on an index that
> might be getting dropped *right now* by a concurrent DROP INDEX
> CONCURRENTLY. If the DROP commits while index_open is running, its
> SnapshotNow catalog searches will stop finding relevant rows. From
> the point of view of index_open, the relation data structure is then
> corrupt (for example, it might not find pg_attribute entries for all
> the columns) and so it will throw an error.
> We need to fix things so that the final pre-deletion state of the
> pg_index row tells observer backends not to even try to open the index.
> (Thus, it will not matter whether they come across the pg_index row just
> before or just after the deleting transaction commits --- either way,
> they don't try to touch the index.) So that has to be a different state
> from the initial state used during CREATE INDEX CONCURRENTLY.
>
> The point of not wanting to open the index NoLock (and for that matter
> of having DROP INDEX CONCURRENTLY take AccessExclusiveLock once it
> thinks nobody is touching the index) is to make sure that if somehow
> somebody is touching the index anyway, they don't see the index's
> catalog entries as corrupt. They'll either all be there or all not.
> It's only a belt-and-suspenders safety measure, but I don't want to
> give it up.
So the idea would be to skip about-to-be-dropped indexes in
RelationGetIndexList directly - we don't ever need to watch those, not
even for HOT - because we only have the necessary knowledge there. The
normal valid/ready checks will be done at the callsites of
RelationGetIndexList(). But those checks can be done in a locked manner
now.
Are you already working on a fix? Or shall I work on an updated patch?
I vote for introducing wrapper functions/macro to do the
about-to-be-dropped check, its hard enough to understand as-is.
Greetings,
Andres Freund
--Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services
