Hi, On 2013-01-28 20:31:48 +0900, Michael Paquier wrote: > On Mon, Jan 28, 2013 at 7:39 PM, Andres Freund <and...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers