Skip to content

Commit bcb784e

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 d3e5d89 commit bcb784e

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
@@ -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"
@@ -535,6 +538,10 @@ static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
535538
ForkNumber forkNum, bool permanent);
536539
static void AtProcExit_Buffers(int code, Datum arg);
537540
static void CheckForBufferLeaks(void);
541+
#ifdef USE_ASSERT_CHECKING
542+
static void AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
543+
void *unused_context);
544+
#endif
538545
static int rlocator_comparator(const void *p1, const void *p2);
539546
static inline int buffertag_comparator(const BufferTag *ba, const BufferTag *bb);
540547
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
@@ -3647,6 +3654,66 @@ CheckForBufferLeaks(void)
36473654
#endif
36483655
}
36493656

3657+
#ifdef USE_ASSERT_CHECKING
3658+
/*
3659+
* Check for exclusive-locked catalog buffers. This is the core of
3660+
* AssertCouldGetRelation().
3661+
*
3662+
* A backend would self-deadlock on LWLocks if the catalog scan read the
3663+
* exclusive-locked buffer. The main threat is exclusive-locked buffers of
3664+
* catalogs used in relcache, because a catcache search on any catalog may
3665+
* build that catalog's relcache entry. We don't have an inventory of
3666+
* catalogs relcache uses, so just check buffers of most catalogs.
3667+
*
3668+
* It's better to minimize waits while holding an exclusive buffer lock, so it
3669+
* would be nice to broaden this check not to be catalog-specific. However,
3670+
* bttextcmp() accesses pg_collation, and non-core opclasses might similarly
3671+
* read tables. That is deadlock-free as long as there's no loop in the
3672+
* dependency graph: modifying table A may cause an opclass to read table B,
3673+
* but it must not cause a read of table A.
3674+
*/
3675+
void
3676+
AssertBufferLocksPermitCatalogRead(void)
3677+
{
3678+
ForEachLWLockHeldByMe(AssertNotCatalogBufferLock, NULL);
3679+
}
3680+
3681+
static void
3682+
AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
3683+
void *unused_context)
3684+
{
3685+
BufferDesc *bufHdr;
3686+
BufferTag tag;
3687+
Oid relid;
3688+
3689+
if (mode != LW_EXCLUSIVE)
3690+
return;
3691+
3692+
if (!((BufferDescPadded *) lock > BufferDescriptors &&
3693+
(BufferDescPadded *) lock < BufferDescriptors + NBuffers))
3694+
return; /* not a buffer lock */
3695+
3696+
bufHdr = (BufferDesc *)
3697+
((char *) lock - offsetof(BufferDesc, content_lock));
3698+
tag = bufHdr->tag;
3699+
3700+
/*
3701+
* This relNumber==relid assumption holds until a catalog experiences
3702+
* VACUUM FULL or similar. After a command like that, relNumber will be
3703+
* in the normal (non-catalog) range, and we lose the ability to detect
3704+
* hazardous access to that catalog. Calling RelidByRelfilenumber() would
3705+
* close that gap, but RelidByRelfilenumber() might then deadlock with a
3706+
* held lock.
3707+
*/
3708+
relid = tag.relNumber;
3709+
3710+
Assert(!IsCatalogRelationOid(relid));
3711+
/* Shared rels are always catalogs: detect even after VACUUM FULL. */
3712+
Assert(tag.spcOid != GLOBALTABLESPACE_OID);
3713+
}
3714+
#endif
3715+
3716+
36503717
/*
36513718
* Helper routine to issue warnings when a buffer is unexpectedly pinned
36523719
*/

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

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

18881888

1889+
/*
1890+
* ForEachLWLockHeldByMe - run a callback for each held lock
1891+
*
1892+
* This is meant as debug support only.
1893+
*/
1894+
void
1895+
ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
1896+
void *context)
1897+
{
1898+
int i;
1899+
1900+
for (i = 0; i < num_held_lwlocks; i++)
1901+
callback(held_lwlocks[i].lock, held_lwlocks[i].mode, context);
1902+
}
1903+
18891904
/*
18901905
* LWLockHeldByMe - test whether my process holds a lock in any mode
18911906
*

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

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

7172
#ifdef USE_ICU
@@ -1258,6 +1259,8 @@ lookup_collation_cache(Oid collation, bool set_flags)
12581259
Assert(OidIsValid(collation));
12591260
Assert(collation != DEFAULT_COLLATION_OID);
12601261

1262+
AssertCouldGetRelation();
1263+
12611264
if (collation_cache == NULL)
12621265
{
12631266
/* 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
@@ -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++;
@@ -1669,8 +1693,7 @@ GetCatCacheHashValue(CatCache *cache,
16691693
/*
16701694
* one-time startup overhead for each cache
16711695
*/
1672-
if (cache->cc_tupdesc == NULL)
1673-
CatalogCacheInitializeCache(cache);
1696+
ConditionalCatalogCacheInitializeCache(cache);
16741697

16751698
/*
16761699
* calculate the hash value
@@ -1721,8 +1744,7 @@ SearchCatCacheList(CatCache *cache,
17211744
/*
17221745
* one-time startup overhead for each cache
17231746
*/
1724-
if (unlikely(cache->cc_tupdesc == NULL))
1725-
CatalogCacheInitializeCache(cache);
1747+
ConditionalCatalogCacheInitializeCache(cache);
17261748

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

@@ -2392,8 +2414,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
23922414
continue;
23932415

23942416
/* Just in case cache hasn't finished initialization yet... */
2395-
if (ccp->cc_tupdesc == NULL)
2396-
CatalogCacheInitializeCache(ccp);
2417+
ConditionalCatalogCacheInitializeCache(ccp);
23972418

23982419
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
23992420
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

@@ -1327,6 +1334,9 @@ CacheInvalidateHeapTupleCommon(Relation relation,
13271334
Oid databaseId;
13281335
Oid relationId;
13291336

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

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

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

2040+
#ifdef USE_ASSERT_CHECKING
2041+
/*
2042+
* AssertCouldGetRelation
2043+
*
2044+
* Check safety of calling RelationIdGetRelation().
2045+
*
2046+
* In code that reads catalogs in the event of a cache miss, call this
2047+
* before checking the cache.
2048+
*/
2049+
void
2050+
AssertCouldGetRelation(void)
2051+
{
2052+
Assert(IsTransactionState());
2053+
AssertBufferLocksPermitCatalogRead();
2054+
}
2055+
#endif
2056+
20402057

20412058
/* ----------------------------------------------------------------
20422059
* Relation Descriptor Lookup Interface
@@ -2064,8 +2081,7 @@ RelationIdGetRelation(Oid relationId)
20642081
{
20652082
Relation rd;
20662083

2067-
/* Make sure we're in an xact, even if this ends up being a cache hit */
2068-
Assert(IsTransactionState());
2084+
AssertCouldGetRelation();
20692085

20702086
/*
20712087
* 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/fmgrprotos.h"
4141
#include "utils/memutils.h"
42+
#include "utils/relcache.h"
4243
#include "varatt.h"
4344

4445
/*
@@ -310,7 +311,7 @@ InitializeClientEncoding(void)
310311
{
311312
Oid utf8_to_server_proc;
312313

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

‎src/include/storage/bufmgr.h‎

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

256256
extern void InitBufferPoolAccess(void);
257257
extern void AtEOXact_Buffers(bool isCommit);
258+
#ifdef USE_ASSERT_CHECKING
259+
extern void AssertBufferLocksPermitCatalogRead(void);
260+
#endif
258261
extern char *DebugPrintBufferRefcount(Buffer buffer);
259262
extern void CheckPointBuffers(int flags);
260263
extern BlockNumber BufferGetBlockNumber(Buffer buffer);

‎src/include/storage/lwlock.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,8 @@ extern bool LWLockAcquireOrWait(LWLock *lock, LWLockMode mode);
129129
extern void LWLockRelease(LWLock *lock);
130130
extern void LWLockReleaseClearVar(LWLock *lock, pg_atomic_uint64 *valptr, uint64 val);
131131
extern void LWLockReleaseAll(void);
132+
extern void ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
133+
void *context);
132134
extern bool LWLockHeldByMe(LWLock *lock);
133135
extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride);
134136
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)