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