Skip to content

Commit 0dfbd19

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 e4b1986 commit 0dfbd19

File tree

10 files changed

+165
-20
lines changed

10 files changed

+165
-20
lines changed

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,9 @@
3636
#include "access/tableam.h"
3737
#include "access/xlog.h"
3838
#include "catalog/catalog.h"
39+
#ifdef USE_ASSERT_CHECKING
40+
#include "catalog/pg_tablespace_d.h"
41+
#endif
3942
#include "catalog/storage.h"
4043
#include "executor/instrument.h"
4144
#include "lib/binaryheap.h"
@@ -487,6 +490,10 @@ static void FindAndDropRelFileNodeBuffers(RelFileNode rnode,
487490
BlockNumber firstDelBlock);
488491
static void AtProcExit_Buffers(int code, Datum arg);
489492
static void CheckForBufferLeaks(void);
493+
#ifdef USE_ASSERT_CHECKING
494+
static void AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
495+
void *unused_context);
496+
#endif
490497
static int rnode_comparator(const void *p1, const void *p2);
491498
static inline int buffertag_comparator(const BufferTag *a, const BufferTag *b);
492499
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
@@ -2697,6 +2704,65 @@ CheckForBufferLeaks(void)
26972704
#endif
26982705
}
26992706

2707+
#ifdef USE_ASSERT_CHECKING
2708+
/*
2709+
* Check for exclusive-locked catalog buffers. This is the core of
2710+
* AssertCouldGetRelation().
2711+
*
2712+
* A backend would self-deadlock on LWLocks if the catalog scan read the
2713+
* exclusive-locked buffer. The main threat is exclusive-locked buffers of
2714+
* catalogs used in relcache, because a catcache search on any catalog may
2715+
* build that catalog's relcache entry. We don't have an inventory of
2716+
* catalogs relcache uses, so just check buffers of most catalogs.
2717+
*
2718+
* It's better to minimize waits while holding an exclusive buffer lock, so it
2719+
* would be nice to broaden this check not to be catalog-specific. However,
2720+
* bttextcmp() accesses pg_collation, and non-core opclasses might similarly
2721+
* read tables. That is deadlock-free as long as there's no loop in the
2722+
* dependency graph: modifying table A may cause an opclass to read table B,
2723+
* but it must not cause a read of table A.
2724+
*/
2725+
void
2726+
AssertBufferLocksPermitCatalogRead(void)
2727+
{
2728+
ForEachLWLockHeldByMe(AssertNotCatalogBufferLock, NULL);
2729+
}
2730+
2731+
static void
2732+
AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
2733+
void *unused_context)
2734+
{
2735+
BufferDesc *bufHdr;
2736+
BufferTag tag;
2737+
Oid relid;
2738+
2739+
if (mode != LW_EXCLUSIVE)
2740+
return;
2741+
2742+
if (!((BufferDescPadded *) lock > BufferDescriptors &&
2743+
(BufferDescPadded *) lock < BufferDescriptors + NBuffers))
2744+
return; /* not a buffer lock */
2745+
2746+
bufHdr = (BufferDesc *)
2747+
((char *) lock - offsetof(BufferDesc, content_lock));
2748+
tag = bufHdr->tag;
2749+
2750+
/*
2751+
* This relNode==relid assumption holds until a catalog experiences VACUUM
2752+
* FULL or similar. After a command like that, relNode will be in the
2753+
* normal (non-catalog) range, and we lose the ability to detect hazardous
2754+
* access to that catalog. Calling RelidByRelfilenode() would close that
2755+
* gap, but RelidByRelfilenode() might then deadlock with a held lock.
2756+
*/
2757+
relid = tag.rnode.relNode;
2758+
2759+
Assert(!IsCatalogRelationOid(relid));
2760+
/* Shared rels are always catalogs: detect even after VACUUM FULL. */
2761+
Assert(tag.rnode.spcNode != GLOBALTABLESPACE_OID);
2762+
}
2763+
#endif
2764+
2765+
27002766
/*
27012767
* Helper routine to issue warnings when a buffer is unexpectedly pinned
27022768
*/

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

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

19231923

1924+
/*
1925+
* ForEachLWLockHeldByMe - run a callback for each held lock
1926+
*
1927+
* This is meant as debug support only.
1928+
*/
1929+
void
1930+
ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
1931+
void *context)
1932+
{
1933+
int i;
1934+
1935+
for (i = 0; i < num_held_lwlocks; i++)
1936+
callback(held_lwlocks[i].lock, held_lwlocks[i].mode, context);
1937+
}
1938+
19241939
/*
19251940
* LWLockHeldByMe - test whether my process holds a lock in any mode
19261941
*

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@
6464
#include "utils/lsyscache.h"
6565
#include "utils/memutils.h"
6666
#include "utils/pg_locale.h"
67+
#include "utils/relcache.h"
6768
#include "utils/syscache.h"
6869

6970
#ifdef USE_ICU
@@ -1262,6 +1263,8 @@ lookup_collation_cache(Oid collation, bool set_flags)
12621263
Assert(OidIsValid(collation));
12631264
Assert(collation != DEFAULT_COLLATION_OID);
12641265

1266+
AssertCouldGetRelation();
1267+
12651268
if (collation_cache == NULL)
12661269
{
12671270
/* 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
@@ -942,12 +942,41 @@ RehashCatCache(CatCache *cp)
942942
cp->cc_bucket = newbucket;
943943
}
944944

945+
/*
946+
* ConditionalCatalogCacheInitializeCache
947+
*
948+
* Call CatalogCacheInitializeCache() if not yet done.
949+
*/
950+
pg_attribute_always_inline
951+
static void
952+
ConditionalCatalogCacheInitializeCache(CatCache *cache)
953+
{
954+
#ifdef USE_ASSERT_CHECKING
955+
/*
956+
* TypeCacheRelCallback() runs outside transactions and relies on TYPEOID
957+
* for hashing. This isn't ideal. Since lookup_type_cache() both
958+
* registers the callback and searches TYPEOID, reaching trouble likely
959+
* requires OOM at an unlucky moment.
960+
*
961+
* InvalidateAttoptCacheCallback() runs outside transactions and likewise
962+
* relies on ATTNUM. InitPostgres() initializes ATTNUM, so it's reliable.
963+
*/
964+
if (!(cache->id == TYPEOID || cache->id == ATTNUM) ||
965+
IsTransactionState())
966+
AssertCouldGetRelation();
967+
else
968+
Assert(cache->cc_tupdesc != NULL);
969+
#endif
970+
971+
if (unlikely(cache->cc_tupdesc == NULL))
972+
CatalogCacheInitializeCache(cache);
973+
}
974+
945975
/*
946976
* CatalogCacheInitializeCache
947977
*
948978
* This function does final initialization of a catcache: obtain the tuple
949-
* descriptor and set up the hash and equality function links. We assume
950-
* that the relcache entry can be opened at this point!
979+
* descriptor and set up the hash and equality function links.
951980
*/
952981
#ifdef CACHEDEBUG
953982
#define CatalogCacheInitializeCache_DEBUG1 \
@@ -1082,8 +1111,7 @@ CatalogCacheInitializeCache(CatCache *cache)
10821111
void
10831112
InitCatCachePhase2(CatCache *cache, bool touch_index)
10841113
{
1085-
if (cache->cc_tupdesc == NULL)
1086-
CatalogCacheInitializeCache(cache);
1114+
ConditionalCatalogCacheInitializeCache(cache);
10871115

10881116
if (touch_index &&
10891117
cache->id != AMOID &&
@@ -1262,16 +1290,12 @@ SearchCatCacheInternal(CatCache *cache,
12621290
dlist_head *bucket;
12631291
CatCTup *ct;
12641292

1265-
/* Make sure we're in an xact, even if this ends up being a cache hit */
1266-
Assert(IsTransactionState());
1267-
12681293
Assert(cache->cc_nkeys == nkeys);
12691294

12701295
/*
12711296
* one-time startup overhead for each cache
12721297
*/
1273-
if (unlikely(cache->cc_tupdesc == NULL))
1274-
CatalogCacheInitializeCache(cache);
1298+
ConditionalCatalogCacheInitializeCache(cache);
12751299

12761300
#ifdef CATCACHE_STATS
12771301
cache->cc_searches++;
@@ -1550,8 +1574,7 @@ GetCatCacheHashValue(CatCache *cache,
15501574
/*
15511575
* one-time startup overhead for each cache
15521576
*/
1553-
if (cache->cc_tupdesc == NULL)
1554-
CatalogCacheInitializeCache(cache);
1577+
ConditionalCatalogCacheInitializeCache(cache);
15551578

15561579
/*
15571580
* calculate the hash value
@@ -1600,8 +1623,7 @@ SearchCatCacheList(CatCache *cache,
16001623
/*
16011624
* one-time startup overhead for each cache
16021625
*/
1603-
if (cache->cc_tupdesc == NULL)
1604-
CatalogCacheInitializeCache(cache);
1626+
ConditionalCatalogCacheInitializeCache(cache);
16051627

16061628
Assert(nkeys > 0 && nkeys < cache->cc_nkeys);
16071629

@@ -2226,8 +2248,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
22262248
continue;
22272249

22282250
/* Just in case cache hasn't finished initialization yet... */
2229-
if (ccp->cc_tupdesc == NULL)
2230-
CatalogCacheInitializeCache(ccp);
2251+
ConditionalCatalogCacheInitializeCache(ccp);
22312252

22322253
hashvalue = CatalogCacheComputeTupleHashValue(ccp, ccp->cc_nkeys, tuple);
22332254
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
@@ -720,6 +720,12 @@ InvalidateSystemCachesExtended(bool debug_discard)
720720
void
721721
AcceptInvalidationMessages(void)
722722
{
723+
#ifdef USE_ASSERT_CHECKING
724+
/* message handlers shall access catalogs only during transactions */
725+
if (IsTransactionState())
726+
AssertCouldGetRelation();
727+
#endif
728+
723729
ReceiveSharedInvalidMessages(LocalExecuteInvalidationMessage,
724730
InvalidateSystemCaches);
725731

@@ -771,7 +777,8 @@ PrepareInvalidationState(void)
771777
{
772778
TransInvalidationInfo *myInfo;
773779

774-
Assert(IsTransactionState());
780+
/* PrepareToInvalidateCacheTuple() needs relcache */
781+
AssertCouldGetRelation();
775782
/* Can't queue transactional message while collecting inplace messages. */
776783
Assert(inplaceInvalInfo == NULL);
777784

@@ -807,7 +814,7 @@ PrepareInplaceInvalidationState(void)
807814
{
808815
InvalidationInfo *myInfo;
809816

810-
Assert(IsTransactionState());
817+
AssertCouldGetRelation();
811818
/* limit of one inplace update under assembly */
812819
Assert(inplaceInvalInfo == NULL);
813820

@@ -1248,6 +1255,9 @@ CacheInvalidateHeapTupleCommon(Relation relation,
12481255
Oid databaseId;
12491256
Oid relationId;
12501257

1258+
/* PrepareToInvalidateCacheTuple() needs relcache */
1259+
AssertCouldGetRelation();
1260+
12511261
/* Do nothing during bootstrap */
12521262
if (IsBootstrapProcessingMode())
12531263
return;

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

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

2023+
#ifdef USE_ASSERT_CHECKING
2024+
/*
2025+
* AssertCouldGetRelation
2026+
*
2027+
* Check safety of calling RelationIdGetRelation().
2028+
*
2029+
* In code that reads catalogs in the event of a cache miss, call this
2030+
* before checking the cache.
2031+
*/
2032+
void
2033+
AssertCouldGetRelation(void)
2034+
{
2035+
Assert(IsTransactionState());
2036+
AssertBufferLocksPermitCatalogRead();
2037+
}
2038+
#endif
2039+
20232040

20242041
/* ----------------------------------------------------------------
20252042
* Relation Descriptor Lookup Interface
@@ -2047,8 +2064,7 @@ RelationIdGetRelation(Oid relationId)
20472064
{
20482065
Relation rd;
20492066

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

20532069
/*
20542070
* 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

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
@@ -196,6 +196,9 @@ extern void InitBufferPool(void);
196196
extern void InitBufferPoolAccess(void);
197197
extern void InitBufferPoolBackend(void);
198198
extern void AtEOXact_Buffers(bool isCommit);
199+
#ifdef USE_ASSERT_CHECKING
200+
extern void AssertBufferLocksPermitCatalogRead(void);
201+
#endif
199202
extern void PrintBufferLeakWarning(Buffer buffer);
200203
extern void CheckPointBuffers(int flags);
201204
extern BlockNumber BufferGetBlockNumber(Buffer buffer);

‎src/include/storage/lwlock.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,8 @@ extern bool LWLockAcquireOrWait(LWLock *lock, LWLockMode mode);
127127
extern void LWLockRelease(LWLock *lock);
128128
extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val);
129129
extern void LWLockReleaseAll(void);
130+
extern void ForEachLWLockHeldByMe(void (*callback) (LWLock *, LWLockMode, void *),
131+
void *context);
130132
extern bool LWLockHeldByMe(LWLock *lock);
131133
extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride);
132134
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
@@ -36,6 +36,14 @@ typedef Relation *RelationPtr;
3636
/*
3737
* Routines to open (lookup) and close a relcache entry
3838
*/
39+
#ifdef USE_ASSERT_CHECKING
40+
extern void AssertCouldGetRelation(void);
41+
#else
42+
static inline void
43+
AssertCouldGetRelation(void)
44+
{
45+
}
46+
#endif
3947
extern Relation RelationIdGetRelation(Oid relationId);
4048
extern void RelationClose(Relation relation);
4149

0 commit comments

Comments
 (0)