Re: Support for REINDEX CONCURRENTLY

Hi,

On 2013-01-28 20:31:48 +0900, Michael Paquier wrote:
> On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2013-01-27 07:54:43 +0900, Michael Paquier wrote:
> > I think you're misunderstanding how this part works a bit. We don't
> > acquire locks on the table itself, but we get a list of all transactions
> > we would conflict with if we were to acquire a lock of a certain
> > strength on the table (GetLockConflicts(locktag, mode)). We then wait
> > for each transaction in the resulting list via the VirtualXact mechanism
> > (VirtualXactLock(*lockholder)).
> > It doesn't matter all that waiting happens in the same transaction the
> > initial index build is done in as long as we keep the session locks
> > preventing other schema modifications. Nobody can go back and see an
> > older index list after we've done the above wait once.
> >
> Don't worry I got it. I just thought that it was necessary to wait for the
> locks taken on the parent relation by other backends just *before* building
> the index. It seemed more stable.

I don't see any need for that. Its really only about making sure their
relcache entry for the indexlist - and by extension rd_indexattr - in
all other transactions that could possibly write to the table is
up2date.

As a relation_open with a lock (which is done for every write) will
always drain the invalidations thats guaranteed if we wait that way.

> So the following should be perfectly fine:
> >
> > StartTransactionCommand();
> > BuildListOfIndexes();
> > foreach(index in indexes)
> > DefineNewIndex(index);
> > CommitTransactionCommand();
> >
> > StartTransactionCommand();
> > foreach(table in tables)
> > GetLockConflicts()
> > foreach(conflict in conflicts)
> > VirtualXactLocks()
> > CommitTransactionCommand();
> >
> > foreach(index in indexes)
> > StartTransactionCommand();
> > InitialIndexBuild(index)
> > CommitTransactionCommand();
> >
> So you're point is simply to wait for all the locks currently taken on each
> table in a different transaction only once and for all, independently from
> the build and validation phases. Correct?

Exactly. That will batch the wait for the transactions together and thus
will greatly decrease the overhead of doing a concurrent reindex
(wall, not cpu-clock wise).

> > > It looks that this feature has still too many disadvantages compared to the
> > > advantages it could bring in the current infrastructure (SnapshotNow
> > > problems, what to do with invalid toast indexes, etc.), so I would tend to
> > > agree with Tom and postpone this feature once infrastructure is more
> > > mature, one of the main things being the non-MVCC'ed catalogs.
> >
> > I think while catalog mvcc snapshots would make this easier, most
> > problems, basically all but the switching of relations, are pretty much
> > independent from that fact. All the waiting etc, will still be there.
> >
> > I can see an argument for pushing it to the next CF because its not
> > really there yet...
> >
> Even if we get this patch in a shape that you think is sufficient to make
> it reviewable by a committer within a couple of days, there are still many
> doubts from many people regarding this feature, so this is going to take
> far more time to put it in a shape that would satisfy a vast majority. So
> it is honestly wiser to work on that later.

I really haven't heard too many arguments from other after the initial
round.
Right now I "only" recall Tom and Robert doubting the usefulness, right?

I think most of the work in this patch is completely independent from
the snapshot stuff, so I really don't see much of an argument to make it
dependent on catalog snapshots.

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

Greetings,

Andres Freund

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