Skip to content

Commit 8609120

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 20a48c1 commit 8609120

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
@@ -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"
@@ -492,6 +495,10 @@ static void RelationCopyStorageUsingBuffer(RelFileNode srcnode,
492495
ForkNumber forkNum, bool permanent);
493496
static void AtProcExit_Buffers(int code, Datum arg);
494497
static void CheckForBufferLeaks(void);
498+
#ifdef USE_ASSERT_CHECKING
499+
static void AssertNotCatalogBufferLock(LWLock *lock, LWLockMode mode,
500+
void *unused_context);
501+
#endif
495502
static int rnode_comparator(const void *p1, const void *p2);
496503
static inline int buffertag_comparator(const BufferTag *a, const BufferTag *b);
497504
static inline int ckpt_buforder_comparator(const CkptSortItem *a, const CkptSortItem *b);
@@ -2691,6 +2698,65 @@ CheckForBufferLeaks(void)
26912698
#endif
26922699
}
26932700

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

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

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

19161916

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

‎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
@@ -1265,6 +1266,8 @@ lookup_collation_cache(Oid collation, bool set_flags)
12651266
Assert(OidIsValid(collation));
12661267
Assert(collation != DEFAULT_COLLATION_OID);
12671268

1269+
AssertCouldGetRelation();
1270+
12681271
if (collation_cache == NULL)
12691272
{
12701273
/* 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
@@ -937,12 +937,41 @@ RehashCatCache(CatCache *cp)
937937
cp->cc_bucket = newbucket;
938938
}
939939

940+
/*
941+
* ConditionalCatalogCacheInitializeCache
942+
*
943+
* Call CatalogCacheInitializeCache() if not yet done.
944+
*/
945+
pg_attribute_always_inline
946+
static void
947+
ConditionalCatalogCacheInitializeCache(CatCache *cache)
948+
{
949+
#ifdef USE_ASSERT_CHECKING
950+
/*
951+
* TypeCacheRelCallback() runs outside transactions and relies on TYPEOID
952+
* for hashing. This isn't ideal. Since lookup_type_cache() both
953+
* registers the callback and searches TYPEOID, reaching trouble likely
954+
* requires OOM at an unlucky moment.
955+
*
956+
* InvalidateAttoptCacheCallback() runs outside transactions and likewise
957+
* relies on ATTNUM. InitPostgres() initializes ATTNUM, so it's reliable.
958+
*/
959+
if (!(cache->id == TYPEOID || cache->id == ATTNUM) ||
960+
IsTransactionState())
961+
AssertCouldGetRelation();
962+
else
963+
Assert(cache->cc_tupdesc != NULL);
964+
#endif
965+
966+
if (unlikely(cache->cc_tupdesc == NULL))
967+
CatalogCacheInitializeCache(cache);
968+
}
969+
940970
/*
941971
* CatalogCacheInitializeCache
942972
*
943973
* This function does final initialization of a catcache: obtain the tuple
944-
* descriptor and set up the hash and equality function links. We assume
945-
* that the relcache entry can be opened at this point!
974+
* descriptor and set up the hash and equality function links.
946975
*/
947976
#ifdef CACHEDEBUG
948977
#define CatalogCacheInitializeCache_DEBUG1 \
@@ -1077,8 +1106,7 @@ CatalogCacheInitializeCache(CatCache *cache)
10771106
void
10781107
InitCatCachePhase2(CatCache *cache, bool touch_index)
10791108
{
1080-
if (cache->cc_tupdesc == NULL)
1081-
CatalogCacheInitializeCache(cache);
1109+
ConditionalCatalogCacheInitializeCache(cache);
10821110

10831111
if (touch_index &&
10841112
cache->id != AMOID &&
@@ -1257,16 +1285,12 @@ SearchCatCacheInternal(CatCache *cache,
12571285
dlist_head *bucket;
12581286
CatCTup *ct;
12591287

1260-
/* Make sure we're in an xact, even if this ends up being a cache hit */
1261-
Assert(IsTransactionState());
1262-
12631288
Assert(cache->cc_nkeys == nkeys);
12641289

12651290
/*
12661291
* one-time startup overhead for each cache
12671292
*/
1268-
if (unlikely(cache->cc_tupdesc == NULL))
1269-
CatalogCacheInitializeCache(cache);
1293+
ConditionalCatalogCacheInitializeCache(cache);
12701294

12711295
#ifdef CATCACHE_STATS
12721296
cache->cc_searches++;
@@ -1545,8 +1569,7 @@ GetCatCacheHashValue(CatCache *cache,
15451569
/*
15461570
* one-time startup overhead for each cache
15471571
*/
1548-
if (cache->cc_tupdesc == NULL)
1549-
CatalogCacheInitializeCache(cache);
1572+
ConditionalCatalogCacheInitializeCache(cache);
15501573

15511574
/*
15521575
* calculate the hash value
@@ -1595,8 +1618,7 @@ SearchCatCacheList(CatCache *cache,
15951618
/*
15961619
* one-time startup overhead for each cache
15971620
*/
1598-
if (cache->cc_tupdesc == NULL)
1599-
CatalogCacheInitializeCache(cache);
1621+
ConditionalCatalogCacheInitializeCache(cache);
16001622

16011623
Assert(nkeys > 0 && nkeys < cache->cc_nkeys);
16021624

@@ -2219,8 +2241,7 @@ PrepareToInvalidateCacheTuple(Relation relation,
22192241
continue;
22202242

22212243
/* Just in case cache hasn't finished initialization yet... */
2222-
if (ccp->cc_tupdesc == NULL)
2223-
CatalogCacheInitializeCache(ccp);
2244+
ConditionalCatalogCacheInitializeCache(ccp);
22242245

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

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 Buffer ReleaseAndReadBuffer(Buffer buffer, Relation relation,
196196
extern void InitBufferPool(void);
197197
extern void InitBufferPoolAccess(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)