Skip to content

Commit 1a6d65b

Browse files
nmischnmisch
committed
Lock before setting relhassubclass on RELKIND_PARTITIONED_INDEX.
Commit 5b56264 added a comment that SetRelationHasSubclass() callers must hold this lock. When commit 17f206f extended use of this column to partitioned indexes, it didn't take the lock. As the latter commit message mentioned, we currently never reset a partitioned index to relhassubclass=f. That largely avoids harm from the lock omission. The cause for fixing this now is to unblock introducing a rule about locks required to heap_update() a pg_class row. This might cause more deadlocks. It gives minor user-visible benefits: - If an ALTER INDEX SET TABLESPACE runs concurrently with ALTER TABLE ATTACH PARTITION or CREATE PARTITION OF, one transaction blocks instead of failing with "tuple concurrently updated". (Many cases of DDL concurrency still fail that way.) - Match ALTER INDEX ATTACH PARTITION in choosing to lock the index. While not user-visible today, we'll need this if we ever make something set the flag to false for a partitioned index, like ANALYZE does today for tables. Back-patch to v12 (all supported versions), the plan for the commit relying on the new rule. In back branches, add LockOrStrongerHeldByMe() instead of adding a LockHeldByMe() parameter. Reviewed (in an earlier version) by Robert Haas. Discussion: https://postgr.es/m/20240611024525.9f.nmisch@google.com
1 parent c3437fb commit 1a6d65b

File tree

7 files changed

+77
-30
lines changed

7 files changed

+77
-30
lines changed

‎src/backend/catalog/index.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,6 +1010,7 @@ index_create(Relation heapRelation,
10101010
if (OidIsValid(parentIndexRelid))
10111011
{
10121012
StoreSingleInheritance(indexRelationId, parentIndexRelid, 1);
1013+
LockRelationOid(parentIndexRelid, ShareUpdateExclusiveLock);
10131014
SetRelationHasSubclass(parentIndexRelid, true);
10141015
}
10151016

‎src/backend/commands/indexcmds.c

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3762,7 +3762,10 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
37623762

37633763
/* set relhassubclass if an index partition has been added to the parent */
37643764
if (OidIsValid(parentOid))
3765+
{
3766+
LockRelationOid(parentOid, ShareUpdateExclusiveLock);
37653767
SetRelationHasSubclass(parentOid, true);
3768+
}
37663769

37673770
/* set relispartition correctly on the partition */
37683771
update_relispartition(partRelid, OidIsValid(parentOid));

‎src/backend/commands/tablecmds.c

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3017,8 +3017,15 @@ findAttrByName(const char *attributeName, List *schema)
30173017
* SetRelationHasSubclass
30183018
* Set the value of the relation's relhassubclass field in pg_class.
30193019
*
3020-
* NOTE: caller must be holding an appropriate lock on the relation.
3021-
* ShareUpdateExclusiveLock is sufficient.
3020+
* It's always safe to set this field to true, because all SQL commands are
3021+
* ready to see true and then find no children. On the other hand, commands
3022+
* generally assume zero children if this is false.
3023+
*
3024+
* Caller must hold any self-exclusive lock until end of transaction. If the
3025+
* new value is false, caller must have acquired that lock before reading the
3026+
* evidence that justified the false value. That way, it properly waits if
3027+
* another backend is simultaneously concluding no need to change the tuple
3028+
* (new and old values are true).
30223029
*
30233030
* NOTE: an important side-effect of this operation is that an SI invalidation
30243031
* message is sent out to all backends --- including me --- causing plans
@@ -3033,6 +3040,11 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
30333040
HeapTuple tuple;
30343041
Form_pg_class classtuple;
30353042

3043+
Assert(CheckRelationOidLockedByMe(relationId,
3044+
ShareUpdateExclusiveLock, false) ||
3045+
CheckRelationOidLockedByMe(relationId,
3046+
ShareRowExclusiveLock, true));
3047+
30363048
/*
30373049
* Fetch a modifiable copy of the tuple, modify it, update pg_class.
30383050
*/

‎src/backend/storage/lmgr/lmgr.c

Lines changed: 17 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -307,32 +307,26 @@ CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger)
307307
relation->rd_lockInfo.lockRelId.dbId,
308308
relation->rd_lockInfo.lockRelId.relId);
309309

310-
if (LockHeldByMe(&tag, lockmode))
311-
return true;
310+
return (orstronger ?
311+
LockOrStrongerHeldByMe(&tag, lockmode) :
312+
LockHeldByMe(&tag, lockmode));
313+
}
312314

313-
if (orstronger)
314-
{
315-
LOCKMODE slockmode;
315+
/*
316+
* CheckRelationOidLockedByMe
317+
*
318+
* Like the above, but takes an OID as argument.
319+
*/
320+
bool
321+
CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, bool orstronger)
322+
{
323+
LOCKTAG tag;
316324

317-
for (slockmode = lockmode + 1;
318-
slockmode <= MaxLockMode;
319-
slockmode++)
320-
{
321-
if (LockHeldByMe(&tag, slockmode))
322-
{
323-
#ifdef NOT_USED
324-
/* Sometimes this might be useful for debugging purposes */
325-
elog(WARNING, "lock mode %s substituted for %s on relation %s",
326-
GetLockmodeName(tag.locktag_lockmethodid, slockmode),
327-
GetLockmodeName(tag.locktag_lockmethodid, lockmode),
328-
RelationGetRelationName(relation));
329-
#endif
330-
return true;
331-
}
332-
}
333-
}
325+
SetLocktagRelationOid(&tag, relid);
334326

335-
return false;
327+
return (orstronger ?
328+
LockOrStrongerHeldByMe(&tag, lockmode) :
329+
LockHeldByMe(&tag, lockmode));
336330
}
337331

338332
/*

‎src/backend/storage/lmgr/lock.c

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -579,11 +579,17 @@ DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)
579579
}
580580

581581
/*
582-
* LockHeldByMe -- test whether lock 'locktag' is held with mode 'lockmode'
583-
* by the current transaction
582+
* LockHeldByMeExtended -- test whether lock 'locktag' is held by the current
583+
* transaction
584+
*
585+
* Returns true if current transaction holds a lock on 'tag' of mode
586+
* 'lockmode'. If 'orstronger' is true, a stronger lockmode is also OK.
587+
* ("Stronger" is defined as "numerically higher", which is a bit
588+
* semantically dubious but is OK for the purposes we use this for.)
584589
*/
585-
bool
586-
LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode)
590+
static bool
591+
LockHeldByMeExtended(const LOCKTAG *locktag,
592+
LOCKMODE lockmode, bool orstronger)
587593
{
588594
LOCALLOCKTAG localtag;
589595
LOCALLOCK *locallock;
@@ -599,7 +605,35 @@ LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode)
599605
(void *) &localtag,
600606
HASH_FIND, NULL);
601607

602-
return (locallock && locallock->nLocks > 0);
608+
if (locallock && locallock->nLocks > 0)
609+
return true;
610+
611+
if (orstronger)
612+
{
613+
LOCKMODE slockmode;
614+
615+
for (slockmode = lockmode + 1;
616+
slockmode <= MaxLockMode;
617+
slockmode++)
618+
{
619+
if (LockHeldByMeExtended(locktag, slockmode, false))
620+
return true;
621+
}
622+
}
623+
624+
return false;
625+
}
626+
627+
bool
628+
LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode)
629+
{
630+
return LockHeldByMeExtended(locktag, lockmode, false);
631+
}
632+
633+
bool
634+
LockOrStrongerHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode)
635+
{
636+
return LockHeldByMeExtended(locktag, lockmode, true);
603637
}
604638

605639
#ifdef USE_ASSERT_CHECKING

‎src/include/storage/lmgr.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ extern bool ConditionalLockRelation(Relation relation, LOCKMODE lockmode);
4747
extern void UnlockRelation(Relation relation, LOCKMODE lockmode);
4848
extern bool CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode,
4949
bool orstronger);
50+
extern bool CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode,
51+
bool orstronger);
5052
extern bool LockHasWaitersRelation(Relation relation, LOCKMODE lockmode);
5153

5254
extern void LockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode);

‎src/include/storage/lock.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,7 @@ extern void LockReleaseSession(LOCKMETHODID lockmethodid);
558558
extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks);
559559
extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks);
560560
extern bool LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode);
561+
extern bool LockOrStrongerHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode);
561562
#ifdef USE_ASSERT_CHECKING
562563
extern HTAB *GetLockMethodLocalHash(void);
563564
#endif

0 commit comments

Comments
 (0)