Skip to content

Commit 27e4fad

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. Back-patch to v14 - v17. This a back-patch of commit f4ece89 (from before v18 branched) to all supported branches, to accompany the back-patch of commits 243e9b4 and 0bada39. For catalog indexes, the bttextcmp() behavior that motivated IsCatalogTextUniqueIndexOid() was v18-specific. Hence, this back-patch doesn't need that or its correction from commit 4a4ee0c. 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 Backpatch-through: 14-17
1 parent 720e930 commit 27e4fad

File tree

10 files changed

+166
-20
lines changed

10 files changed

+166
-20
lines changed

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

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@
3737
#include "access/xloginsert.h"
3838
#include "access/xlogutils.h"
3939
#include "catalog/catalog.h"
40+
#ifdef USE_ASSERT_CHECKING
41+
#include "catalog/pg_tablespace_d.h"
42+
#endif
4043
#include "catalog/storage.h"
4144
#include "catalog/storage_xlog.h"
4245
#include "executor/instrument.h"
@@ -498,6 +501,10 @@ static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
498501
ForkNumber forkNum, bool permanent);
499502
static void AtProcExit_Buffers(int code, Datum arg);
500503
static void CheckForBufferLeaks(void);
504+
#ifdef USE_ASSERT_CHECKING
505+
static void AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
506+
void *unused_context);
507+
#endif
501508
static int rlocator_comparator(const void *p1, const void *p2);
502509
static inline int buffertag_comparator(const BufferTag *ba, const BufferTag *bb);
503510
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
@@ -3225,6 +3232,66 @@ CheckForBufferLeaks(void)
32253232
#endif
32263233
}
32273234

3235+
#ifdef USE_ASSERT_CHECKING
3236+
/*
3237+
* Check for exclusive-locked catalog buffers. This is the core of
3238+
* AssertCouldGetRelation().
3239+
*
3240+
* A backend would self-deadlock on LWLocks if the catalog scan read the
3241+
* exclusive-locked buffer. The main threat is exclusive-locked buffers of
3242+
* catalogs used in relcache, because a catcache search on any catalog may
3243+
* build that catalog's relcache entry. We don't have an inventory of
3244+
* catalogs relcache uses, so just check buffers of most catalogs.
3245+
*
3246+
* It's better to minimize waits while holding an exclusive buffer lock, so it
3247+
* would be nice to broaden this check not to be catalog-specific. However,
3248+
* bttextcmp() accesses pg_collation, and non-core opclasses might similarly
3249+
* read tables. That is deadlock-free as long as there's no loop in the
3250+
* dependency graph: modifying table A may cause an opclass to read table B,
3251+
* but it must not cause a read of table A.
3252+
*/
3253+
void
3254+
AssertBufferLocksPermitCatalogRead(void)
3255+
{
3256+
ForEachLWLockHeldByMe(AssertNotCatalogBufferLock, NULL);
3257+
}
3258+
3259+
static void
3260+
AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
3261+
void *unused_context)
3262+
{
3263+
BufferDesc *bufHdr;
3264+
BufferTag tag;
3265+
Oid relid;
3266+
3267+
if (mode != LW_EXCLUSIVE)
3268+
return;
3269+
3270+
if (!((BufferDescPadded *) lock > BufferDescriptors &&
3271+
(BufferDescPadded *) lock < BufferDescriptors + NBuffers))
3272+
return; /* not a buffer lock */
3273+
3274+
bufHdr = (BufferDesc *)
3275+
((char *) lock - offsetof(BufferDesc, content_lock));
3276+
tag = bufHdr->tag;
3277+
3278+
/*
3279+
* This relNumber==relid assumption holds until a catalog experiences
3280+
* VACUUM FULL or similar. After a command like that, relNumber will be
3281+
* in the normal (non-catalog) range, and we lose the ability to detect
3282+
* hazardous access to that catalog. Calling RelidByRelfilenumber() would
3283+
* close that gap, but RelidByRelfilenumber() might then deadlock with a
3284+
* held lock.
3285+
*/
3286+
relid = tag.relNumber;
3287+
3288+
Assert(!IsCatalogRelationOid(relid));
3289+
/* Shared rels are always catalogs: detect even after VACUUM FULL. */
3290+
Assert(tag.spcOid != GLOBALTABLESPACE_OID);
3291+
}
3292+
#endif
3293+
3294+
32283295
/*
32293296
* Helper routine to issue warnings when a buffer is unexpectedly pinned
32303297
*/

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

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

19121912

1913+
/*
1914+
* ForEachLWLockHeldByMe - run a callback for each held lock
1915+
*
1916+
* This is meant as debug support only.
1917+
*/
1918+
void
1919+
ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
1920+
void *context)
1921+
{
1922+
int i;
1923+
1924+
for (i = 0; i < num_held_lwlocks; i++)
1925+
callback(held_lwlocks[i].lock, held_lwlocks[i].mode, context);
1926+
}
1927+
19131928
/*
19141929
* LWLockHeldByMe - test whether my process holds a lock in any mode
19151930
*

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@
6767
#include "utils/lsyscache.h"
6868
#include "utils/memutils.h"
6969
#include "utils/pg_locale.h"
70+
#include "utils/relcache.h"
7071
#include "utils/syscache.h"
7172

7273
#ifdef USE_ICU
@@ -1222,6 +1223,8 @@ lookup_collation_cache(Oid collation, bool set_flags)
12221223
Assert(OidIsValid(collation));
12231224
Assert(collation != DEFAULT_COLLATION_OID);
12241225

1226+
AssertCouldGetRelation();
1227+
12251228
if (collation_cache == NULL)
12261229
{
12271230
/* First time through, initialize the hash table */

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

Lines changed: 36 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -999,12 +999,41 @@ RehashCatCacheLists(CatCache *cp)
999999
cp->cc_lbucket = newbucket;
10001000
}
10011001

1002+
/*
1003+
* ConditionalCatalogCacheInitializeCache
1004+
*
1005+
* Call CatalogCacheInitializeCache() if not yet done.
1006+
*/
1007+
pg_attribute_always_inline
1008+
static void
1009+
ConditionalCatalogCacheInitializeCache(CatCache *cache)
1010+
{
1011+
#ifdef USE_ASSERT_CHECKING
1012+
/*
1013+
* TypeCacheRelCallback() runs outside transactions and relies on TYPEOID
1014+
* for hashing. This isn't ideal. Since lookup_type_cache() both
1015+
* registers the callback and searches TYPEOID, reaching trouble likely
1016+
* requires OOM at an unlucky moment.
1017+
*
1018+
* InvalidateAttoptCacheCallback() runs outside transactions and likewise
1019+
* relies on ATTNUM. InitPostgres() initializes ATTNUM, so it's reliable.
1020+
*/
1021+
if (!(cache->id == TYPEOID || cache->id == ATTNUM) ||
1022+
IsTransactionState())
1023+
AssertCouldGetRelation();
1024+
else
1025+
Assert(cache->cc_tupdesc != NULL);
1026+
#endif
1027+
1028+
if (unlikely(cache->cc_tupdesc == NULL))
1029+
CatalogCacheInitializeCache(cache);
1030+
}
1031+
10021032
/*
10031033
* CatalogCacheInitializeCache
10041034
*
10051035
* This function does final initialization of a catcache: obtain the tuple
1006-
* descriptor and set up the hash and equality function links. We assume
1007-
* that the relcache entry can be opened at this point!
1036+
* descriptor and set up the hash and equality function links.
10081037
*/
10091038
#ifdef CACHEDEBUG
10101039
#define CatalogCacheInitializeCache_DEBUG1 \
@@ -1139,8 +1168,7 @@ CatalogCacheInitializeCache(CatCache *cache)
11391168
void
11401169
InitCatCachePhase2(CatCache *cache, bool touch_index)
11411170
{
1142-
if (cache->cc_tupdesc == NULL)
1143-
CatalogCacheInitializeCache(cache);
1171+
ConditionalCatalogCacheInitializeCache(cache);
11441172

11451173
if (touch_index &&
11461174
cache->id != AMOID &&
@@ -1319,16 +1347,12 @@ SearchCatCacheInternal(CatCache *cache,
13191347
dlist_head *bucket;
13201348
CatCTup *ct;
13211349

1322-
/* Make sure we're in an xact, even if this ends up being a cache hit */
1323-
Assert(IsTransactionState());
1324-
13251350
Assert(cache->cc_nkeys == nkeys);
13261351

13271352
/*
13281353
* one-time startup overhead for each cache
13291354
*/
1330-
if (unlikely(cache->cc_tupdesc == NULL))
1331-
CatalogCacheInitializeCache(cache);
1355+
ConditionalCatalogCacheInitializeCache(cache);
13321356

13331357
#ifdef CATCACHE_STATS
13341358
cache->cc_searches++;
@@ -1607,8 +1631,7 @@ GetCatCacheHashValue(CatCache *cache,
16071631
/*
16081632
* one-time startup overhead for each cache
16091633
*/
1610-
if (cache->cc_tupdesc == NULL)
1611-
CatalogCacheInitializeCache(cache);
1634+
ConditionalCatalogCacheInitializeCache(cache);
16121635

16131636
/*
16141637
* calculate the hash value
@@ -1659,8 +1682,7 @@ SearchCatCacheList(CatCache *cache,
16591682
/*
16601683
* one-time startup overhead for each cache
16611684
*/
1662-
if (unlikely(cache->cc_tupdesc == NULL))
1663-
CatalogCacheInitializeCache(cache);
1685+
ConditionalCatalogCacheInitializeCache(cache);
16641686

16651687
Assert(nkeys > 0 && nkeys < cache->cc_nkeys);
16661688

@@ -2314,8 +2336,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
23142336
continue;
23152337

23162338
/* Just in case cache hasn't finished initialization yet... */
2317-
if (ccp->cc_tupdesc == NULL)
2318-
CatalogCacheInitializeCache(ccp);
2339+
ConditionalCatalogCacheInitializeCache(ccp);
23192340

23202341
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
23212342
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
@@ -631,7 +631,8 @@ PrepareInvalidationState(void)
631631
{
632632
TransInvalidationInfo *myInfo;
633633

634-
Assert(IsTransactionState());
634+
/* PrepareToInvalidateCacheTuple() needs relcache */
635+
AssertCouldGetRelation();
635636
/* Can't queue transactional message while collecting inplace messages. */
636637
Assert(inplaceInvalInfo == NULL);
637638

@@ -700,7 +701,7 @@ PrepareInplaceInvalidationState(void)
700701
{
701702
InvalidationInfo *myInfo;
702703

703-
Assert(IsTransactionState());
704+
AssertCouldGetRelation();
704705
/* limit of one inplace update under assembly */
705706
Assert(inplaceInvalInfo == NULL);
706707

@@ -863,6 +864,12 @@ InvalidateSystemCaches(void)
863864
void
864865
AcceptInvalidationMessages(void)
865866
{
867+
#ifdef USE_ASSERT_CHECKING
868+
/* message handlers shall access catalogs only during transactions */
869+
if (IsTransactionState())
870+
AssertCouldGetRelation();
871+
#endif
872+
866873
ReceiveSharedInvalidMessages(LocalExecuteInvalidationMessage,
867874
InvalidateSystemCaches);
868875

@@ -1326,6 +1333,9 @@ CacheInvalidateHeapTupleCommon(Relation relation,
13261333
Oid databaseId;
13271334
Oid relationId;
13281335

1336+
/* PrepareToInvalidateCacheTuple() needs relcache */
1337+
AssertCouldGetRelation();
1338+
13291339
/* Do nothing during bootstrap */
13301340
if (IsBootstrapProcessingMode())
13311341
return;

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2028,6 +2028,23 @@ formrdesc(const char *relationName, Oid relationReltype,
20282028
relation->rd_isvalid = true;
20292029
}
20302030

2031+
#ifdef USE_ASSERT_CHECKING
2032+
/*
2033+
* AssertCouldGetRelation
2034+
*
2035+
* Check safety of calling RelationIdGetRelation().
2036+
*
2037+
* In code that reads catalogs in the event of a cache miss, call this
2038+
* before checking the cache.
2039+
*/
2040+
void
2041+
AssertCouldGetRelation(void)
2042+
{
2043+
Assert(IsTransactionState());
2044+
AssertBufferLocksPermitCatalogRead();
2045+
}
2046+
#endif
2047+
20312048

20322049
/* ----------------------------------------------------------------
20332050
* Relation Descriptor Lookup Interface
@@ -2055,8 +2072,7 @@ RelationIdGetRelation(Oid relationId)
20552072
{
20562073
Relation rd;
20572074

2058-
/* Make sure we're in an xact, even if this ends up being a cache hit */
2059-
Assert(IsTransactionState());
2075+
AssertCouldGetRelation();
20602076

20612077
/*
20622078
* first try to find reldesc in the cache

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
#include "mb/pg_wchar.h"
4040
#include "utils/builtins.h"
4141
#include "utils/memutils.h"
42+
#include "utils/relcache.h"
4243
#include "utils/syscache.h"
4344
#include "varatt.h"
4445

@@ -311,7 +312,7 @@ InitializeClientEncoding(void)
311312
{
312313
Oid utf8_to_server_proc;
313314

314-
Assert(IsTransactionState());
315+
AssertCouldGetRelation();
315316
utf8_to_server_proc =
316317
FindDefaultConversionProc(PG_UTF8,
317318
current_server_encoding);

‎src/include/storage/bufmgr.h‎

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

206206
extern void InitBufferPoolAccess(void);
207207
extern void AtEOXact_Buffers(bool isCommit);
208+
#ifdef USE_ASSERT_CHECKING
209+
extern void AssertBufferLocksPermitCatalogRead(void);
210+
#endif
208211
extern void PrintBufferLeakWarning(Buffer buffer);
209212
extern void CheckPointBuffers(int flags);
210213
extern BlockNumber BufferGetBlockNumber(Buffer buffer);

‎src/include/storage/lwlock.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,8 @@ extern bool LWLockAcquireOrWait(LWLock *lock, LWLockMode mode);
131131
extern void LWLockRelease(LWLock *lock);
132132
extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val);
133133
extern void LWLockReleaseAll(void);
134+
extern void ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
135+
void *context);
134136
extern bool LWLockHeldByMe(LWLock *lock);
135137
extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride);
136138
extern bool LWLockHeldByMeInMode(LWLock *lock, LWLockMode mode);

‎src/include/utils/relcache.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,14 @@ typedef Relation *RelationPtr;
3737
/*
3838
* Routines to open (lookup) and close a relcache entry
3939
*/
40+
#ifdef USE_ASSERT_CHECKING
41+
extern void AssertCouldGetRelation(void);
42+
#else
43+
static inline void
44+
AssertCouldGetRelation(void)
45+
{
46+
}
47+
#endif
4048
extern Relation RelationIdGetRelation(Oid relationId);
4149
extern void RelationClose(Relation relation);
4250

0 commit comments

Comments
 (0)