Re: Support for REINDEX CONCURRENTLY

On Mon, Jan 28, 2013 at 8:59 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-01-28 20:50:21 +0900, Michael Paquier wrote:
> > On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund <andres(at)anarazel(dot)de>
> wrote:
> >
> > > > Another argument that would be enough for a rejection of this patch
> by a
> > > > committer is the problem of invalid toast indexes that cannot be
> removed
> > > up
> > > > cleanly by an operator. As long as there is not a clean solution for
> > > > that...
> > >
> > > I think that part is relatively easy to fix, I wouldn't worry too
> > > much.
> > > The more complex part is how to get tuptoaster.c to update the
> > > concurrently created index. That's what I worry about. Its not going
> > > through the normal executor paths but manually updates the toast
> > > index - which means it won't update the indisready && !indisvalid
> > > index...
> > >
> > I included in the patch some stuff to update the reltoastidxid of the
> > parent relation of the toast index. Have a look at
> > index.c:index_concurrent_swap. The particular case I had in mind was if
> > there is a failure of the server during the concurrent reindex of a toast
> > index.
>
> Thats not enough unfortunately. The problem scenario is the following:
>
> toast table: pg_toast.pg_toast_16384
> toast index (via reltoastidxid): pg_toast.pg_toast_16384_index
> REINDEX CONCURRENTLY PHASE #1
> REINDEX CONCURRENTLY PHASE #2
> toast table: pg_toast.pg_toast_16384
> toast index (via reltoastidxid): pg_toast.pg_toast_16384_index, ready &
> valid
> toast index (via pg_index): pg_toast.pg_toast_16384_index_tmp, ready &
> !valid
>
> If a tuple gets toasted in this state tuptoaster.c will update
> 16384_index but not 16384_index_tmp. In normal tables this works because
> nodeModifyTable uses ExecInsertIndexTuples which updates all ready
> indexes. tuptoaster.c does something different though, it calls
> index_insert exactly on the one expected index, not on the other ones.
>
> Makes sense?
>
I didn't know toast indexes followed this code path. Thanks for the
details.

>
> > When server restarts, the toast relation will have an invalid index
> > and this cannot be dropped by an operator via SQL.
>
> That requires about two lines of special case code in
> RangeVarCallbackForDropRelation, that doesn't seem to be too bad to me.
>
> I.e. allow the case where its IsSystemClass(classform) && relkind ==
> RELKIND_INDEX && !indisvalid.
>
OK, I thought it was more complicated.
--
Michael Paquier
http://michael.otacoo.com