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

On 2019-05-02 10:44:44 +0200, Peter Eisentraut wrote:
> 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

Hm? But that's a different callback from the one used from
reindex_index()? reindex_relation() uses the
RangeVarCallbackOwnsTable() callback and passes in NULL as the argument,
whereas reindex_index() passses in RangeVarCallbackForReindexIndex() and
passes in &heapOid?

And RangeVarCallbackForReindexIndex() pretty clearly sets it *heapOid:

* Lock level here should match reindex_index() heap lock. If the OID
* isn't valid, it means the index as concurrently dropped, which is
* not a problem for us; just return normally.
*/
*heapOid = IndexGetRelation(relId, true);

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

I might miss something here, and it's actually unused. But if so the fix
would be to make it being used, because it's actually
important. Otherwise ReindexIndex() becomes racy or has even more
deadlock hazards.

Greetings,

Andres Freund