Re: Support for REINDEX CONCURRENTLY

On 2013-01-28 20:50:21 +0900, Michael Paquier wrote:
> On Mon, Jan 28, 2013 at 8:44 PM, Andres Freund <andres@anarazel.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?

> 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.