Skip to content

Commit f4ece89

Browse files
nmischnmisch
committed
Assert lack of hazardous buffer locks before possible catalog read.
Commit 0bada39 fixed a bug of this kind, which existed in all branches for six days before detection. While the probability of reaching the trouble was low, the disruption was extreme. No new backends could start, and service restoration needed an immediate shutdown. Hence, add this to catch the next bug like it. The new check in RelationIdGetRelation() suffices to make autovacuum detect the bug in commit 243e9b4 that led to commit 0bada39. This also checks in a number of similar places. It replaces each Assert(IsTransactionState()) that pertained to a conditional catalog read. No back-patch for now, but a back-patch of commit 243e9b4 should back-patch this, too. A back-patch could omit the src/test/regress changes, since back branches won't gain new index columns. Reported-by: Alexander Lakhin <exclusion@gmail.com> Discussion: https://postgr.es/m/20250410191830.0e.nmisch@google.com Discussion: https://postgr.es/m/10ec0bc3-5933-1189-6bb8-5dec4114558e@gmail.com
1 parent b669293 commit f4ece89

File tree

15 files changed

+248
-24
lines changed

15 files changed

+248
-24
lines changed

‎src/backend/catalog/catalog.c

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
#include "catalog/pg_namespace.h"
3535
#include "catalog/pg_parameter_acl.h"
3636
#include "catalog/pg_replication_origin.h"
37+
#include "catalog/pg_seclabel.h"
3738
#include "catalog/pg_shdepend.h"
3839
#include "catalog/pg_shdescription.h"
3940
#include "catalog/pg_shseclabel.h"
@@ -135,6 +136,36 @@ IsCatalogRelationOid(Oid relid)
135136
return (relid < (Oid) FirstUnpinnedObjectId);
136137
}
137138

139+
/*
140+
* IsCatalogTextUniqueIndexOid
141+
* True iff the relation identified by this OID is a catalog UNIQUE index
142+
* having a column of type "text".
143+
*
144+
* The relcache must not use these indexes. Inserting into any UNIQUE
145+
* index compares index keys while holding BUFFER_LOCK_EXCLUSIVE.
146+
* bttextcmp() can search the COLLID catcache. Depending on concurrent
147+
* invalidation traffic, catcache can reach relcache builds. A backend
148+
* would self-deadlock on LWLocks if the relcache build read the
149+
* exclusive-locked buffer.
150+
*
151+
* To avoid being itself the cause of self-deadlock, this doesn't read
152+
* catalogs. Instead, it uses a hard-coded list with a supporting
153+
* regression test.
154+
*/
155+
bool
156+
IsCatalogTextUniqueIndexOid(Oid relid)
157+
{
158+
switch (relid)
159+
{
160+
case ParameterAclParnameIndexId:
161+
case ReplicationOriginNameIndex:
162+
case SecLabelObjectIndexId:
163+
case SharedSecLabelObjectIndexId:
164+
return true;
165+
}
166+
return false;
167+
}
168+
138169
/*
139170
* IsInplaceUpdateRelation
140171
* True iff core code performs inplace updates on the relation.

‎src/backend/storage/buffer/bufmgr.c

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,9 @@
4040
#include "access/tableam.h"
4141
#include "access/xloginsert.h"
4242
#include "access/xlogutils.h"
43+
#ifdef USE_ASSERT_CHECKING
44+
#include "catalog/pg_tablespace_d.h"
45+
#endif
4346
#include "catalog/storage.h"
4447
#include "catalog/storage_xlog.h"
4548
#include "executor/instrument.h"
@@ -541,6 +544,10 @@ static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
541544
ForkNumber forkNum, bool permanent);
542545
static void AtProcExit_Buffers(int code, Datum arg);
543546
static void CheckForBufferLeaks(void);
547+
#ifdef USE_ASSERT_CHECKING
548+
static void AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
549+
void *unused_context);
550+
#endif
544551
static int rlocator_comparator(const void *p1, const void *p2);
545552
static inline int buffertag_comparator(const BufferTag *ba, const BufferTag *bb);
546553
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
@@ -4097,6 +4104,69 @@ CheckForBufferLeaks(void)
40974104
#endif
40984105
}
40994106

4107+
#ifdef USE_ASSERT_CHECKING
4108+
/*
4109+
* Check for exclusive-locked catalog buffers. This is the core of
4110+
* AssertCouldGetRelation().
4111+
*
4112+
* A backend would self-deadlock on LWLocks if the catalog scan read the
4113+
* exclusive-locked buffer. The main threat is exclusive-locked buffers of
4114+
* catalogs used in relcache, because a catcache search on any catalog may
4115+
* build that catalog's relcache entry. We don't have an inventory of
4116+
* catalogs relcache uses, so just check buffers of most catalogs.
4117+
*
4118+
* It's better to minimize waits while holding an exclusive buffer lock, so it
4119+
* would be nice to broaden this check not to be catalog-specific. However,
4120+
* bttextcmp() accesses pg_collation, and non-core opclasses might similarly
4121+
* read tables. That is deadlock-free as long as there's no loop in the
4122+
* dependency graph: modifying table A may cause an opclass to read table B,
4123+
* but it must not cause a read of table A.
4124+
*/
4125+
void
4126+
AssertBufferLocksPermitCatalogRead(void)
4127+
{
4128+
ForEachLWLockHeldByMe(AssertNotCatalogBufferLock, NULL);
4129+
}
4130+
4131+
static void
4132+
AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
4133+
void *unused_context)
4134+
{
4135+
BufferDesc *bufHdr;
4136+
BufferTag tag;
4137+
Oid relid;
4138+
4139+
if (mode != LW_EXCLUSIVE)
4140+
return;
4141+
4142+
if (!((BufferDescPadded *) lock > BufferDescriptors &&
4143+
(BufferDescPadded *) lock < BufferDescriptors + NBuffers))
4144+
return; /* not a buffer lock */
4145+
4146+
bufHdr = (BufferDesc *)
4147+
((char *) lock - offsetof(BufferDesc, content_lock));
4148+
tag = bufHdr->tag;
4149+
4150+
/*
4151+
* This relNumber==relid assumption holds until a catalog experiences
4152+
* VACUUM FULL or similar. After a command like that, relNumber will be
4153+
* in the normal (non-catalog) range, and we lose the ability to detect
4154+
* hazardous access to that catalog. Calling RelidByRelfilenumber() would
4155+
* close that gap, but RelidByRelfilenumber() might then deadlock with a
4156+
* held lock.
4157+
*/
4158+
relid = tag.relNumber;
4159+
4160+
if (IsCatalogTextUniqueIndexOid(relid)) /* see comments at the callee */
4161+
return;
4162+
4163+
Assert(!IsCatalogRelationOid(relid));
4164+
/* Shared rels are always catalogs: detect even after VACUUM FULL. */
4165+
Assert(tag.spcOid != GLOBALTABLESPACE_OID);
4166+
}
4167+
#endif
4168+
4169+
41004170
/*
41014171
* Helper routine to issue warnings when a buffer is unexpectedly pinned
41024172
*/

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1961,6 +1961,21 @@ LWLockReleaseAll(void)
19611961
}
19621962

19631963

1964+
/*
1965+
* ForEachLWLockHeldByMe - run a callback for each held lock
1966+
*
1967+
* This is meant as debug support only.
1968+
*/
1969+
void
1970+
ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
1971+
void *context)
1972+
{
1973+
int i;
1974+
1975+
for (i = 0; i < num_held_lwlocks; i++)
1976+
callback(held_lwlocks[i].lock, held_lwlocks[i].mode, context);
1977+
}
1978+
19641979
/*
19651980
* LWLockHeldByMe - test whether my process holds a lock in any mode
19661981
*

‎src/backend/utils/adt/pg_locale.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1196,6 +1196,8 @@ pg_newlocale_from_collation(Oid collid)
11961196
if (!OidIsValid(collid))
11971197
elog(ERROR, "cache lookup failed for collation %u", collid);
11981198

1199+
AssertCouldGetRelation();
1200+
11991201
if (last_collation_cache_oid == collid)
12001202
return last_collation_cache_locale;
12011203

‎src/backend/utils/cache/catcache.c

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1054,12 +1054,41 @@ RehashCatCacheLists(CatCache *cp)
10541054
cp->cc_lbucket = newbucket;
10551055
}
10561056

1057+
/*
1058+
* ConditionalCatalogCacheInitializeCache
1059+
*
1060+
* Call CatalogCacheInitializeCache() if not yet done.
1061+
*/
1062+
pg_attribute_always_inline
1063+
static void
1064+
ConditionalCatalogCacheInitializeCache(CatCache *cache)
1065+
{
1066+
#ifdef USE_ASSERT_CHECKING
1067+
/*
1068+
* TypeCacheRelCallback() runs outside transactions and relies on TYPEOID
1069+
* for hashing. This isn't ideal. Since lookup_type_cache() both
1070+
* registers the callback and searches TYPEOID, reaching trouble likely
1071+
* requires OOM at an unlucky moment.
1072+
*
1073+
* InvalidateAttoptCacheCallback() runs outside transactions and likewise
1074+
* relies on ATTNUM. InitPostgres() initializes ATTNUM, so it's reliable.
1075+
*/
1076+
if (!(cache->id == TYPEOID || cache->id == ATTNUM) ||
1077+
IsTransactionState())
1078+
AssertCouldGetRelation();
1079+
else
1080+
Assert(cache->cc_tupdesc != NULL);
1081+
#endif
1082+
1083+
if (unlikely(cache->cc_tupdesc == NULL))
1084+
CatalogCacheInitializeCache(cache);
1085+
}
1086+
10571087
/*
10581088
* CatalogCacheInitializeCache
10591089
*
10601090
* This function does final initialization of a catcache: obtain the tuple
1061-
* descriptor and set up the hash and equality function links. We assume
1062-
* that the relcache entry can be opened at this point!
1091+
* descriptor and set up the hash and equality function links.
10631092
*/
10641093
#ifdef CACHEDEBUG
10651094
#define CatalogCacheInitializeCache_DEBUG1 \
@@ -1194,8 +1223,7 @@ CatalogCacheInitializeCache(CatCache *cache)
11941223
void
11951224
InitCatCachePhase2(CatCache *cache, bool touch_index)
11961225
{
1197-
if (cache->cc_tupdesc == NULL)
1198-
CatalogCacheInitializeCache(cache);
1226+
ConditionalCatalogCacheInitializeCache(cache);
11991227

12001228
if (touch_index &&
12011229
cache->id != AMOID &&
@@ -1374,16 +1402,12 @@ SearchCatCacheInternal(CatCache *cache,
13741402
dlist_head *bucket;
13751403
CatCTup *ct;
13761404

1377-
/* Make sure we're in an xact, even if this ends up being a cache hit */
1378-
Assert(IsTransactionState());
1379-
13801405
Assert(cache->cc_nkeys == nkeys);
13811406

13821407
/*
13831408
* one-time startup overhead for each cache
13841409
*/
1385-
if (unlikely(cache->cc_tupdesc == NULL))
1386-
CatalogCacheInitializeCache(cache);
1410+
ConditionalCatalogCacheInitializeCache(cache);
13871411

13881412
#ifdef CATCACHE_STATS
13891413
cache->cc_searches++;
@@ -1668,8 +1692,7 @@ GetCatCacheHashValue(CatCache *cache,
16681692
/*
16691693
* one-time startup overhead for each cache
16701694
*/
1671-
if (cache->cc_tupdesc == NULL)
1672-
CatalogCacheInitializeCache(cache);
1695+
ConditionalCatalogCacheInitializeCache(cache);
16731696

16741697
/*
16751698
* calculate the hash value
@@ -1720,8 +1743,7 @@ SearchCatCacheList(CatCache *cache,
17201743
/*
17211744
* one-time startup overhead for each cache
17221745
*/
1723-
if (unlikely(cache->cc_tupdesc == NULL))
1724-
CatalogCacheInitializeCache(cache);
1746+
ConditionalCatalogCacheInitializeCache(cache);
17251747

17261748
Assert(nkeys > 0 && nkeys < cache->cc_nkeys);
17271749

@@ -2390,8 +2412,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
23902412
continue;
23912413

23922414
/* Just in case cache hasn't finished initialization yet... */
2393-
if (ccp->cc_tupdesc == NULL)
2394-
CatalogCacheInitializeCache(ccp);
2415+
ConditionalCatalogCacheInitializeCache(ccp);
23952416

23962417
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
23972418
dbid = ccp->cc_relisshared ? (Oid) 0 : MyDatabaseId;

‎src/backend/utils/cache/inval.c

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -683,7 +683,8 @@ PrepareInvalidationState(void)
683683
{
684684
TransInvalidationInfo *myInfo;
685685

686-
Assert(IsTransactionState());
686+
/* PrepareToInvalidateCacheTuple() needs relcache */
687+
AssertCouldGetRelation();
687688
/* Can't queue transactional message while collecting inplace messages. */
688689
Assert(inplaceInvalInfo == NULL);
689690

@@ -752,7 +753,7 @@ PrepareInplaceInvalidationState(void)
752753
{
753754
InvalidationInfo *myInfo;
754755

755-
Assert(IsTransactionState());
756+
AssertCouldGetRelation();
756757
/* limit of one inplace update under assembly */
757758
Assert(inplaceInvalInfo == NULL);
758759

@@ -928,6 +929,12 @@ InvalidateSystemCaches(void)
928929
void
929930
AcceptInvalidationMessages(void)
930931
{
932+
#ifdef USE_ASSERT_CHECKING
933+
/* message handlers shall access catalogs only during transactions */
934+
if (IsTransactionState())
935+
AssertCouldGetRelation();
936+
#endif
937+
931938
ReceiveSharedInvalidMessages(LocalExecuteInvalidationMessage,
932939
InvalidateSystemCaches);
933940

@@ -1436,6 +1443,9 @@ CacheInvalidateHeapTupleCommon(Relation relation,
14361443
Oid databaseId;
14371444
Oid relationId;
14381445

1446+
/* PrepareToInvalidateCacheTuple() needs relcache */
1447+
AssertCouldGetRelation();
1448+
14391449
/* Do nothing during bootstrap */
14401450
if (IsBootstrapProcessingMode())
14411451
return;

‎src/backend/utils/cache/relcache.c

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2056,6 +2056,23 @@ formrdesc(const char *relationName, Oid relationReltype,
20562056
relation->rd_isvalid = true;
20572057
}
20582058

2059+
#ifdef USE_ASSERT_CHECKING
2060+
/*
2061+
* AssertCouldGetRelation
2062+
*
2063+
* Check safety of calling RelationIdGetRelation().
2064+
*
2065+
* In code that reads catalogs in the event of a cache miss, call this
2066+
* before checking the cache.
2067+
*/
2068+
void
2069+
AssertCouldGetRelation(void)
2070+
{
2071+
Assert(IsTransactionState());
2072+
AssertBufferLocksPermitCatalogRead();
2073+
}
2074+
#endif
2075+
20592076

20602077
/* ----------------------------------------------------------------
20612078
* Relation Descriptor Lookup Interface
@@ -2083,8 +2100,7 @@ RelationIdGetRelation(Oid relationId)
20832100
{
20842101
Relation rd;
20852102

2086-
/* Make sure we're in an xact, even if this ends up being a cache hit */
2087-
Assert(IsTransactionState());
2103+
AssertCouldGetRelation();
20882104

20892105
/*
20902106
* first try to find reldesc in the cache
@@ -2373,8 +2389,7 @@ RelationReloadNailed(Relation relation)
23732389
Assert(relation->rd_isnailed);
23742390
/* nailed indexes are handled by RelationReloadIndexInfo() */
23752391
Assert(relation->rd_rel->relkind == RELKIND_RELATION);
2376-
/* can only reread catalog contents in a transaction */
2377-
Assert(IsTransactionState());
2392+
AssertCouldGetRelation();
23782393

23792394
/*
23802395
* Redo RelationInitPhysicalAddr in case it is a mapped relation whose
@@ -2570,8 +2585,7 @@ static void
25702585
RelationRebuildRelation(Relation relation)
25712586
{
25722587
Assert(!RelationHasReferenceCountZero(relation));
2573-
/* rebuilding requires access to the catalogs */
2574-
Assert(IsTransactionState());
2588+
AssertCouldGetRelation();
25752589
/* there is no reason to ever rebuild a dropped relation */
25762590
Assert(relation->rd_droppedSubid == InvalidSubTransactionId);
25772591

‎src/backend/utils/mb/mbutils.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ InitializeClientEncoding(void)
310310
{
311311
Oid utf8_to_server_proc;
312312

313-
Assert(IsTransactionState());
313+
AssertCouldGetRelation();
314314
utf8_to_server_proc =
315315
FindDefaultConversionProc(PG_UTF8,
316316
current_server_encoding);

‎src/include/catalog/catalog.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ extern bool IsSystemClass(Oid relid, Form_pg_class reltuple);
2727
extern bool IsToastClass(Form_pg_class reltuple);
2828

2929
extern bool IsCatalogRelationOid(Oid relid);
30+
extern bool IsCatalogTextUniqueIndexOid(Oid relid);
3031
extern bool IsInplaceUpdateOid(Oid relid);
3132

3233
extern bool IsCatalogNamespace(Oid namespaceId);

‎src/include/storage/bufmgr.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,9 @@ extern Buffer ExtendBufferedRelTo(BufferManagerRelation bmr,
258258

259259
extern void InitBufferManagerAccess(void);
260260
extern void AtEOXact_Buffers(bool isCommit);
261+
#ifdef USE_ASSERT_CHECKING
262+
extern void AssertBufferLocksPermitCatalogRead(void);
263+
#endif
261264
extern char *DebugPrintBufferRefcount(Buffer buffer);
262265
extern void CheckPointBuffers(int flags);
263266
extern BlockNumber BufferGetBlockNumber(Buffer buffer);

0 commit comments

Comments
 (0)