Re: Bugs in CREATE/DROP INDEX CONCURRENTLY - Mailing list pgsql-hackers

On 2012-11-27 13:45:08 -0500, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2012-11-26 22:44:49 -0500, Tom Lane wrote:
> >> I looked through the code a bit, and I think we should be able to make
> >> this work, but note there are also places that update indcheckxmin using
> >> heap_update, and that's just as dangerous as changing the other two
> >> flags via heap_update, if you don't have exclusive lock on the table.
>
> > Isn't setting indexcheckxmin already problematic in the case of CREATE
> > INDEX CONCURRENTLY? index_build already runs in a separate transaction
> > there.
>
> Yeah, you are right, except that AFAICS indcheckxmin is really only
> needed for regular non-concurrent CREATE INDEX, which needs it because
> it commits without waiting for readers that might be bothered by broken
> HOT chains.  In a concurrent CREATE INDEX, we handle that problem by
> waiting out all such readers before setting indisvalid.  So the
> concurrent code path should not be bothering to set indcheckxmin at all,
> I think.  (This is underdocumented.)

Seems to be correct to me.

> Looking closer at the comment in reindex_index, what it's really full of
> angst about is that simple_heap_update will update the tuple's xmin
> *when we would rather that it didn't*.  So switching to update-in-place
> there will solve a problem, not create one.

It strikes me that the whole idea of reusing xmin when indexcheckxmin is
set is overly clever and storing the xid itself would have be been
better... Too late though.

> In short, all flag changes in pg_index should be done by
> update-in-place, and the tuple's xmin will never change from the
> original creating transaction (until frozen).

Hm. That doesn't sound that easy to guarantee. Several ALTER TABLE and
ALTER INDEX operations are expected to work transactionally right
now. As far as I see there is nothing that prohibits a indexcheckxmin
requiring index to be altered while indexcheckxmin is still required?

> What we want the xmin to do, for indcheckxmin purposes, is reflect the
> time at which the index started to be included in HOT-safety decisions.
> Since we're never going to move the xmin, that means that *we cannot
> allow REINDEX to mark a dead index live again*.  Once DROP INDEX
> CONCURRENTLY has reached the final state, you can't revalidate the index
> by reindexing it, you'll have to drop it and then make a brand new one.
> That seems like a pretty minor compromise.

That would be a regression compared with the current state though. We
have officially documented REINDEX as a way out of INVALID indexes...

If we store the xid of the reindexing transaction there that might be
pessimal (because there should be not HOT safety problems) but should
always be correct, or am I missing something?

> What I propose to do next is create a patch for HEAD that includes a
> new pg_index flag column, since I think the logic will be clearer
> with that.  Once we're happy with that, we can back-port the patch
> into a form that uses the existing flag columns.
>
> Anybody feel like bikeshedding the flag column name?  I'm thinking
> "indislive" but maybe there's a better name.

I personally would slightly favor indisdead instead...

> > Btw, even if we manage to find a sensible fix for this I would suggest
> > postponing it after the next back branch release.
>
> AFAICS this is a data loss/corruption problem, and as such a "must fix".
> If we can't have it done by next week, I'd rather postpone the releases
> until it is done.

Ok, just seemed like a rather complex fix in a short time for something
that seemingly hasn't been noticed since 8.3. I am a bit worried about
introducing something worse while fixing this.

Greetings,

Andres Freund

--Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services