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
From 89db1445787b3fb676ecfa07964682e72a58e7bb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 2 May 2019 10:38:06 +0200
Subject: [PATCH] Remove unused argument of RangeVarCallbackForReindexIndex()
---
src/backend/commands/indexcmds.c | 22 ++++------------------
1 file changed, 4 insertions(+), 18 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index fabba3bacd..2800255d9d 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2304,7 +2304,6 @@ void
ReindexIndex(RangeVar *indexRelation, int options, bool concurrent)
{
Oid indOid;
- Oid heapOid = InvalidOid;
Relation irel;
char persistence;
@@ -2317,7 +2316,7 @@ ReindexIndex(RangeVar *indexRelation, int options, bool
concurrent)
concurrent ? ShareUpdateExclusiveLock : AccessExclusiveLock,
0,
RangeVarCallbackForReindexIndex,
- (void
*) &heapOid);
+ NULL);
/*
* Obtain the current persistence of the existing index. We already
hold
@@ -2350,19 +2349,6 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
Oid relId, Oid
oldRelId, void *arg)
{
char relkind;
- Oid *heapOid = (Oid *) arg;
-
- /*
- * If we previously locked some other index's heap, and the name we're
- * looking up no longer refers to that relation, release the now-useless
- * lock.
- */
- if (relId != oldRelId && OidIsValid(oldRelId))
- {
- /* lock level here should match reindex_index() heap lock */
- UnlockRelationOid(*heapOid, ShareLock);
- *heapOid = InvalidOid;
- }
/* If the relation does not exist, there's nothing more to do. */
if (!OidIsValid(relId))
@@ -2394,9 +2380,9 @@ RangeVarCallbackForReindexIndex(const RangeVar *relation,
* isn't valid, it means the index as concurrently dropped,
which is
* not a problem for us; just return normally.
*/
- *heapOid = IndexGetRelation(relId, true);
- if (OidIsValid(*heapOid))
- LockRelationOid(*heapOid, ShareLock);
+ Oid heapOid = IndexGetRelation(relId, true);
+ if (OidIsValid(heapOid))
+ LockRelationOid(heapOid, ShareLock);
}
}
--
2.21.0
