Heap lock levels for REINDEX INDEX CONCURRENTLY not quite right?

[Andres Freund]

Hi,

While looking at 
https://www.postgresql.org/message-id/20190430070552.jzqgcy4ihalx7nur%40alap3.anarazel.de
I noticed that

/*
 * ReindexIndex
 *              Recreate a specific index.
 */
void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{
        Oid                     indOid;
        Oid                     heapOid = InvalidOid;
        Relation        irel;
        char            persistence;

        /*
         * Find and lock index, and check permissions on table; use callback to
         * obtain lock on table first, to avoid deadlock hazard.  The lock level
         * used here must match the index lock obtained in reindex_index().
         */
        indOid = RangeVarGetRelidExtended(indexRelation,
                                                                          
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
                                                                          0,
                                                                          
RangeVarCallbackForReindexIndex,
                                                                          (void 
*) &heapOid);

doesn't pass concurrent-ness to RangeVarCallbackForReindexIndex(). Which
then goes on to lock the table

static void
RangeVarCallbackForReindexIndex(const RangeVar *relation,
                                                                Oid relId, Oid 
oldRelId, void *arg)

                if (OidIsValid(*heapOid))
                        LockRelationOid(*heapOid, ShareLock);

without knowing that it should use ShareUpdateExclusive. Which
e.g. ReindexTable knows:

        /* The lock level used here should match reindex_relation(). */
        heapOid = RangeVarGetRelidExtended(relation,
                                                                           
concurrent ? ShareUpdateExclusiveLock : ShareLock,
                                                                           0,
                                                                           
RangeVarCallbackOwnsTable, NULL);

so there's a lock upgrade hazard.

Creating a table
CREATE TABLE blarg(id serial primary key);
and then using pgbench to reindex it:
REINDEX INDEX CONCURRENTLY blarg_pkey;

indeed proves that there's a problem:

2019-04-30 08:12:58.679 PDT [30844][7/925] ERROR:  40P01: deadlock detected
2019-04-30 08:12:58.679 PDT [30844][7/925] DETAIL:  Process 30844 waits for 
ShareUpdateExclusiveLock on relation 50661 of database 13408; blocked by 
process 30848.
        Process 30848 waits for ShareUpdateExclusiveLock on relation 50667 of 
database 13408; blocked by process 30844.
        Process 30844: REINDEX INDEX CONCURRENTLY blarg_pkey;
        Process 30848: REINDEX INDEX CONCURRENTLY blarg_pkey;
2019-04-30 08:12:58.679 PDT [30844][7/925] HINT:  See server log for query 
details.
2019-04-30 08:12:58.679 PDT [30844][7/925] LOCATION:  DeadLockReport, 
deadlock.c:1140
2019-04-30 08:12:58.679 PDT [30844][7/925] STATEMENT:  REINDEX INDEX 
CONCURRENTLY blarg_pkey;

I assume the fix woudl be to pass a struct {LOCKMODE lockmode; Oid
heapOid;} to RangeVarCallbackForReindexIndex().

Greetings,

Andres Freund