Re: REINDEX CONCURRENTLY unexpectedly fails

On 09/01/2020 05:06, Michael Paquier wrote:
> On Wed, Jan 08, 2020 at 05:19:30PM +0900, Michael Paquier wrote:
>> 2) The handling of the patch within index_drop is too weak.  As
>> presented, the patch first locks the OID using a RangeVar.  However
>> for a temporary relation we would first take ShareUpdateExclusiveLock
>> RemoveRelations() and then upgrade to a AccessExclusiveLock in
>> index_drop().  I think that actually the check in index_drop() is not
>> necessary, and that instead we had better do three things:
>> a) In RangeVarCallbackForDropRelation(), if the relation is temporary,
>> use AccessExclusiveLock all the time, and we know the OID of the
>> relation here.
>> b) After locking the OID with the RangeVar, re-check if the relation
>> is temporary, and then remove PERFORM_DELETION_CONCURRENTLY is.
>> c) Add an assertion in index_drop() to be sure that this code path is
>> never invoked concurrently with a temporary relation.
>>
>> I am lacking of time today, I'll continue tomorrow.
> 
> Okay, so here is an updated patch fixing those issues, with several
> modifications done to the patch (docs, updates for the assertions,
> some redesign).  Considering again those aspects, I have come up with
> the same conclusion as what's stated above, though you actually need
> to make sure that it is RangeVarGetRelidExtended() that has to be
> careful about the lock to use on the temporary relation, before
> anything else is done.  The callback RangeVarCallbackForDropRelation()
> also needs to be careful about the relation it looks at and check if
> the relation supports concurrent indexing.  On the other hand, we
> could also say that we don't care about lock upgrade risks when
> working on temporary tables because these are not accessed by other
> sessions, though that's not a sane base to rely on IMO.  A solution
> involving RangeVarGetRelidExtended() feels also like a sledgehammer to
> smash a peanut, because it has a wider impact.

I'm not a fan of all those changes in RangeVarCallbackForDropRelation() 
to ensure that you get an AccessExclusiveLock to begin with. It gets 
pretty complicated, and it feels like you need to special-case temporary 
tables in dozen different places. I liked the v3 of this patch better. 
It's true that you're upgrading the ShareUpdateExclusiveLock to 
AccessExclusiveLock, but since it's a temporary table, there really 
should be no other backend holding a lock on it.

> If lock upgrade risks are not worth bothering, this needs to be
> clearly documented in the patch with more comments.
Yes, definitely.

- Heikki