Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 0cecc90

Browse filesBrowse 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 f88cdb3 commit 0cecc90
Copy full SHA for 0cecc90

File tree

Expand file treeCollapse file tree

7 files changed

+61
-30
lines changed
Filter options
Expand file treeCollapse file tree

7 files changed

+61
-30
lines changed

‎src/backend/catalog/index.c

Copy file name to clipboardExpand all lines: src/backend/catalog/index.c
+1Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,7 @@ index_create(Relation heapRelation,
10581058
if (OidIsValid(parentIndexRelid))
10591059
{
10601060
StoreSingleInheritance(indexRelationId, parentIndexRelid, 1);
1061+
LockRelationOid(parentIndexRelid, ShareUpdateExclusiveLock);
10611062
SetRelationHasSubclass(parentIndexRelid, true);
10621063
}
10631064

‎src/backend/commands/indexcmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/indexcmds.c
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4355,7 +4355,10 @@ IndexSetParentIndex(Relation partitionIdx, Oid parentOid)
43554355

43564356
/* set relhassubclass if an index partition has been added to the parent */
43574357
if (OidIsValid(parentOid))
4358+
{
4359+
LockRelationOid(parentOid, ShareUpdateExclusiveLock);
43584360
SetRelationHasSubclass(parentOid, true);
4361+
}
43594362

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

‎src/backend/commands/tablecmds.c

Copy file name to clipboardExpand all lines: src/backend/commands/tablecmds.c
+14-2Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3483,8 +3483,15 @@ findAttrByName(const char *attributeName, const List *columns)
34833483
* SetRelationHasSubclass
34843484
* Set the value of the relation's relhassubclass field in pg_class.
34853485
*
3486-
* NOTE: caller must be holding an appropriate lock on the relation.
3487-
* ShareUpdateExclusiveLock is sufficient.
3486+
* It's always safe to set this field to true, because all SQL commands are
3487+
* ready to see true and then find no children. On the other hand, commands
3488+
* generally assume zero children if this is false.
3489+
*
3490+
* Caller must hold any self-exclusive lock until end of transaction. If the
3491+
* new value is false, caller must have acquired that lock before reading the
3492+
* evidence that justified the false value. That way, it properly waits if
3493+
* another backend is simultaneously concluding no need to change the tuple
3494+
* (new and old values are true).
34883495
*
34893496
* NOTE: an important side-effect of this operation is that an SI invalidation
34903497
* message is sent out to all backends --- including me --- causing plans
@@ -3499,6 +3506,11 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
34993506
HeapTuple tuple;
35003507
Form_pg_class classtuple;
35013508

3509+
Assert(CheckRelationOidLockedByMe(relationId,
3510+
ShareUpdateExclusiveLock, false) ||
3511+
CheckRelationOidLockedByMe(relationId,
3512+
ShareRowExclusiveLock, true));
3513+
35023514
/*
35033515
* Fetch a modifiable copy of the tuple, modify it, update pg_class.
35043516
*/

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

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lmgr.c
+13-23Lines changed: 13 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -335,32 +335,22 @@ CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger)
335335
relation->rd_lockInfo.lockRelId.dbId,
336336
relation->rd_lockInfo.lockRelId.relId);
337337

338-
if (LockHeldByMe(&tag, lockmode))
339-
return true;
338+
return LockHeldByMe(&tag, lockmode, orstronger);
339+
}
340340

341-
if (orstronger)
342-
{
343-
LOCKMODE slockmode;
341+
/*
342+
* CheckRelationOidLockedByMe
343+
*
344+
* Like the above, but takes an OID as argument.
345+
*/
346+
bool
347+
CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, bool orstronger)
348+
{
349+
LOCKTAG tag;
344350

345-
for (slockmode = lockmode + 1;
346-
slockmode <= MaxLockMode;
347-
slockmode++)
348-
{
349-
if (LockHeldByMe(&tag, slockmode))
350-
{
351-
#ifdef NOT_USED
352-
/* Sometimes this might be useful for debugging purposes */
353-
elog(WARNING, "lock mode %s substituted for %s on relation %s",
354-
GetLockmodeName(tag.locktag_lockmethodid, slockmode),
355-
GetLockmodeName(tag.locktag_lockmethodid, lockmode),
356-
RelationGetRelationName(relation));
357-
#endif
358-
return true;
359-
}
360-
}
361-
}
351+
SetLocktagRelationOid(&tag, relid);
362352

363-
return false;
353+
return LockHeldByMe(&tag, lockmode, orstronger);
364354
}
365355

366356
/*

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

Copy file name to clipboardExpand all lines: src/backend/storage/lmgr/lock.c
+26-4Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -578,11 +578,17 @@ DoLockModesConflict(LOCKMODE mode1, LOCKMODE mode2)
578578
}
579579

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

601-
return (locallock && locallock->nLocks > 0);
607+
if (locallock && locallock->nLocks > 0)
608+
return true;
609+
610+
if (orstronger)
611+
{
612+
LOCKMODE slockmode;
613+
614+
for (slockmode = lockmode + 1;
615+
slockmode <= MaxLockMode;
616+
slockmode++)
617+
{
618+
if (LockHeldByMe(locktag, slockmode, false))
619+
return true;
620+
}
621+
}
622+
623+
return false;
602624
}
603625

604626
#ifdef USE_ASSERT_CHECKING

‎src/include/storage/lmgr.h

Copy file name to clipboardExpand all lines: src/include/storage/lmgr.h
+2Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ extern bool ConditionalLockRelation(Relation relation, LOCKMODE lockmode);
4848
extern void UnlockRelation(Relation relation, LOCKMODE lockmode);
4949
extern bool CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode,
5050
bool orstronger);
51+
extern bool CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode,
52+
bool orstronger);
5153
extern bool LockHasWaitersRelation(Relation relation, LOCKMODE lockmode);
5254

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

‎src/include/storage/lock.h

Copy file name to clipboardExpand all lines: src/include/storage/lock.h
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,8 @@ extern void LockReleaseAll(LOCKMETHODID lockmethodid, bool allLocks);
567567
extern void LockReleaseSession(LOCKMETHODID lockmethodid);
568568
extern void LockReleaseCurrentOwner(LOCALLOCK **locallocks, int nlocks);
569569
extern void LockReassignCurrentOwner(LOCALLOCK **locallocks, int nlocks);
570-
extern bool LockHeldByMe(const LOCKTAG *locktag, LOCKMODE lockmode);
570+
extern bool LockHeldByMe(const LOCKTAG *locktag,
571+
LOCKMODE lockmode, bool orstronger);
571572
#ifdef USE_ASSERT_CHECKING
572573
extern HTAB *GetLockMethodLocalHash(void);
573574
#endif

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.