Re: Support for REINDEX CONCURRENTLY

On Thu, Mar 7, 2013 at 7:19 AM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2013-03-07 05:26:31 +0900, Michael Paquier wrote:
> > On Thu, Mar 7, 2013 at 2:34 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
> wrote:
> >
> > > On Thu, Mar 7, 2013 at 2:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
> > > wrote:
> > > >> Indexes:
> > > >> "hoge_pkey" PRIMARY KEY, btree (i)
> > > >> "hoge_pkey_cct" PRIMARY KEY, btree (i) INVALID
> > > >> "hoge_pkey_cct1" PRIMARY KEY, btree (i) INVALID
> > > >> "hoge_pkey_cct_cct" PRIMARY KEY, btree (i)
> > > >
> > > > Huh, why did that go through? It should have errored out?
> > >
> > > I'm not sure why. Anyway hoge_pkey_cct_cct should not appear or should
> > > be marked as invalid, I think.
> > >
> > CHECK_FOR_INTERRUPTS were not added at each phase and they are needed in
> > case process is interrupted by user. This has been mentioned in a pas
> > review but it was missing, so it might have slipped out during a
> > refactoring or smth. Btw, I am surprised to see that this *_cct_cct index
> > has been created knowing that hoge_pkey_cct is invalid. I tried with the
> > latest version of the patch and even the patch attached but couldn't
> > reproduce it.
>
> The strange think about "hoge_pkey_cct_cct" is that it seems to imply
> that an invalid index was reindexed concurrently?
>
> But I don't see how it could happen either. Fujii, can you reproduce it?
>
Curious about that also.

> > >> + The recommended recovery method in such cases is to drop the
> > > concurrent
> > > >> + index and try again to perform <command>REINDEX
> CONCURRENTLY</>.
> > > >>
> > > >> If an invalid index depends on the constraint like primary key,
> "drop
> > > >> the concurrent
> > > >> index" cannot actually drop the index. In this case, you need to
> issue
> > > >> "alter table
> > > >> ... drop constraint ..." to recover the situation. I think this
> > > >> informataion should be
> > > >> documented.
> > > >
> > > > I think we just shouldn't set ->isprimary on the temporary indexes.
> Now
> > > > we switch only the relfilenodes and not the whole index, that should
> be
> > > > perfectly fine.
> > >
> > > Sounds good. But, what about other constraint case like unique
> constraint?
> > > Those other cases also can be resolved by not setting ->isprimary?
> > >
> > We should stick with the concurrent index being a twin of the index it
> > rebuilds for consistency.
>
> I don't think its legal. We cannot simply have two indexes with
> 'indisprimary'. Especially not if bot are valid.
> Also, there will be no pg_constraint row that refers to it which
> violates very valid expectations that both users and pg may have.
>
So what to do with that?
Mark the concurrent index as valid, then validate it and finally mark it as
invalid inside the same transaction at phase 4?
That's moving 2 lines of code...
--
Michael