Re: Bugs in CREATE/DROP INDEX CONCURRENTLY

On 2012-11-27 23:46:58 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2012-11-27 16:31:15 -0500, Tom Lane wrote:
> >> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> >>> Isn't inisprimary updated when an ALTER TABLE ... ADD PRIMARY KEY
> >>> ... USING someindex ; is done? Also I think indoption might be written
> >>> to as well.
>
> >> Ugh, you're right. Somebody wasn't paying attention when those ALTER
> >> commands were added.
>
> On closer look, indoption is never updated --- perhaps you were thinking
> about pg_class.reloptions. indisprimary, indimmediate, and
> indisclustered are all subject to post-creation updates though.

Yea, I haven't really checked what inoption actually does.

> >> We could probably alleviate the consequences of this by having those
> >> operations reset indcheckxmin if the tuple's old xmin is below the
> >> GlobalXmin horizon. That's something for later though --- it's not
> >> a data corruption issue, it just means that the index might unexpectedly
> >> not be used for queries for a little bit after an ALTER.
>
> > mark_index_clustered() does the same btw, its not a problem in the
> > CLUSTER ... USING ...; case because that creates a new pg_index entry
> > anyway but in the ALTER TABLE one thats not the case.
>
> After further study I think the situation is that
>
> (1) The indisvalid/indisready/indislive updates in CREATE/DROP INDEX
> CONCURRENTLY can, and must, be done in-place since we don't have
> exclusive lock on the parent table.
>
> (2) All the other updates can be transactional because we hold
> sufficient locks to ensure that nothing bad will happen. The proposed
> reductions in ALTER TABLE lock strength would break this in several
> cases, but that's a problem for another day.

> Attached is a very preliminary draft patch for this. I've not addressed
> the question of whether we can clear indcheckxmin during transactional
> updates of pg_index rows, but I think it covers everything else talked
> about in this thread.

Looks good on a quick lookthrough. Will play a bit more once the
indexcheckxmin stuff is sorted out.

Some comments:
- INDEX_DROP_CLEAR_READY clears indislive, perhasp INDEX_DROP_SET_DEAD
or NOT_ALIVE is more appropriate?
- I noticed while trying my old isolationtester test that
heap_update_inplace disregards any locks on the tuple. I don't really
see a scenario where this is problematic right now, seems a bit
dangerous for the future though.

Greetings,

Andres Freund

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