Re: Heap lock levels for REINDEX INDEX CONCURRENTLY not quite right?

On 2019-04-30 17:17, Andres Freund wrote:
>     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.

Confirmed.

What seems weird to me is that the existing callback argument heapOid
isn't used at all.  It seems to have been like that since the original
commit of the callback infrastructure.  Therefore also, this code

    if (relId != oldRelId && OidIsValid(oldRelId))
    {
        /* lock level here should match reindex_index() heap lock */
        UnlockRelationOid(*heapOid, ShareLock);

in RangeVarCallbackForReindexIndex() can't ever do anything useful.

Patch to remove the unused code attached; but needs some checking for
this dubious conditional block.

Thoughts?


-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services