Tom Lane wrote: > "Kevin Grittner" <kgri...@mail.com> writes: >> To put that another way, it should be done at a time when it is >> sure that no query sees indisvalid = true and no query has yet >> seen indisready = false. Patch attached. Will apply if nobody sees >> a problem with it. > > The above statement of the requirement doesn't seem to match what > you put in the comment: > >> + * All predicate locks on the index are about to be made invalid. Promote >> + * them to relation locks on the heap. For correctness this must be done >> + * after the index was last seen with indisready = true and before it is >> + * seen with indisvalid = false.
It took me a while to spot it, but yeah -- I reversed the field names in the comment. :-( The email was right; I fixed the comment. > and the comment is rather vaguely worded too (last seen by what?). > Please wordsmith that a bit more. I tried to leave nothing to the imagination this time. I think the README or function comments on the predicate locking should include more about this sort of thing. In retrospect, the documentation reads more like a maintainer's guide for SSI, and the user's guide is lacking. Something like that would make it easier for people working on other features (like DROP INDEX CONCURRENTLY) to know where to put which calls. That's more than I can do right at the moment, but I'll make a note of it. I suspect that if enough of this is in the comment above the function definition, less of it needs to be near each call site. -Kevin
*** a/src/backend/catalog/index.c --- b/src/backend/catalog/index.c *************** *** 1316,1321 **** index_drop(Oid indexId, bool concurrent) --- 1316,1333 ---- * table lock strong enough to prevent all queries on the table from * proceeding until we commit and send out a shared-cache-inval notice * that will make them update their index lists. + * + * All predicate locks on the index are about to be made invalid. Promote + * them to relation locks on the heap. For correctness the index must not + * be seen with indisvalid = true during query planning after the move + * starts, so that the index will not be used for a scan after the + * predicate lock move, as this could create new predicate locks on the + * index which would not ensure a heap relation lock. Also, the index must + * not be seen during execution of a heap tuple insert with indisready = + * false before the move is complete, since the conflict with the + * predicate lock on the index gap could be missed before the lock on the + * heap relation is in place to detect a conflict based on the heap tuple + * insert. */ heapId = IndexGetRelation(indexId, false); if (concurrent) *************** *** 1349,1354 **** index_drop(Oid indexId, bool concurrent) --- 1361,1368 ---- */ indexRelation = heap_open(IndexRelationId, RowExclusiveLock); + TransferPredicateLocksToHeapRelation(userIndexRelation); + tuple = SearchSysCacheCopy1(INDEXRELID, ObjectIdGetDatum(indexId)); if (!HeapTupleIsValid(tuple)) *************** *** 1438,1449 **** index_drop(Oid indexId, bool concurrent) userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock); userIndexRelation = index_open(indexId, AccessExclusiveLock); } ! ! /* ! * All predicate locks on the index are about to be made invalid. Promote ! * them to relation locks on the heap. ! */ ! TransferPredicateLocksToHeapRelation(userIndexRelation); /* * Schedule physical removal of the files --- 1452,1459 ---- userHeapRelation = heap_open(heapId, ShareUpdateExclusiveLock); userIndexRelation = index_open(indexId, AccessExclusiveLock); } ! else ! TransferPredicateLocksToHeapRelation(userIndexRelation); /* * Schedule physical removal of the files
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
