From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael@paquier.xyz>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Noah Misch <noah@leadboat.com>
Cc: Xiaoran Wang <fanfuxiaoran@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org, Andres Freund <andres@anarazel.de>
Subject: Re: Recovering from detoast-related catcache invalidations
Date: Wed, 25 Dec 2024 11:27:38 +0200
Message-ID: <ea00edec-d649-42bf-8cfb-13bc83d4af8e@iki.fi> (raw)
In-Reply-To: <Z2plC4rkwdn52Q6n@paquier.xyz>
References: <329632.1705011663@sss.pgh.pa.us>
<CAGjhLkOFmzx9gbB8qudKxdg8UR0SApo-thKuPZ=1zJaZ6-ORXQ@mail.gmail.com>
<754826.1705090452@sss.pgh.pa.us>
<793469.1705092433@sss.pgh.pa.us>
<20240114201411.d0@rfd.leadboat.com>
<20240924212036.0a.nmisch@google.com>
<ea198f85-5df0-464d-8a0a-45bf88166c33@iki.fi>
<1066416.1734103822@sss.pgh.pa.us>
<2234dc98-06fe-42ed-b5db-ac17384dc880@iki.fi>
<1f11aacf-968c-4b8d-a917-31cb207b7a56@iki.fi>
<Z2plC4rkwdn52Q6n@paquier.xyz>
On 24/12/2024 09:38, Michael Paquier wrote:
> On Tue, Dec 24, 2024 at 12:18:09AM +0200, Heikki Linnakangas wrote:
>> My first attempt was to insert the CatCTup or CatCList entry to the catcache
>> before starting to build it, marked with a flag to indicate that the entry
>> isn't fully built yet. But when I started to write that it got pretty
>> invasive, and it felt easier to add another structure to hold the
>> in-progress entries instead.
>>
>> From ec248c69cb42a0747ecc6a63ac4e4682cce2ee93 Mon Sep 17 00:00:00 2001
>> From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
>> Date: Fri, 20 Dec 2024 18:37:44 +0200
>> Subject: [PATCH 1/1] Don't allow GetTransactionSnapshot() in logical decoding
>
> It seems to me that this is not what you intended to attach for the
> catcache inconsistency fix?
Right, sorry, here are the correct patches.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
[text/x-patch] v2-0001-Demonstrate-catcache-list-invalidation-bug.patch (5.0K, ../ea00edec-d649-42bf-8cfb-13bc83d4af8e@iki.fi/2-v2-0001-Demonstrate-catcache-list-invalidation-bug.patch)
download | inline diff:
From 9fb473ba6d252998967f6d69a566c2be1d8d7f79 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 24 Dec 2024 00:13:36 +0200
Subject: [PATCH v2 1/2] Demonstrate catcache list invalidation bug
---
src/backend/utils/cache/catcache.c | 3 +
src/test/modules/test_misc/meson.build | 1 +
src/test/modules/test_misc/t/007_bugs.pl | 96 ++++++++++++++++++++++++
3 files changed, 100 insertions(+)
create mode 100644 src/test/modules/test_misc/t/007_bugs.pl
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.cindex ee303dc501d..4f06b45239c 100644--- a/src/backend/utils/cache/catcache.c+++ b/src/backend/utils/cache/catcache.c@@ -34,6 +34,7 @@
#include "utils/catcache.h"
#include "utils/datum.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -1811,6 +1812,8 @@ SearchCatCacheList(CatCache *cache,
stale = false;
+ INJECTION_POINT("catcache-list-miss-systable-scan-started");+
while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
{
uint32 hashValue;
diff --git a/src/test/modules/test_misc/meson.build b/src/test/modules/test_misc/meson.buildindex 283ffa751aa..3abcd471c03 100644--- a/src/test/modules/test_misc/meson.build+++ b/src/test/modules/test_misc/meson.build@@ -15,6 +15,7 @@ tests += {
't/004_io_direct.pl',
't/005_timeouts.pl',
't/006_signal_autovacuum.pl',
+ 't/007_bugs.pl',
],
},
}
diff --git a/src/test/modules/test_misc/t/007_bugs.pl b/src/test/modules/test_misc/t/007_bugs.plnew file mode 100644index 00000000000..48adf2f7b65--- /dev/null+++ b/src/test/modules/test_misc/t/007_bugs.pl@@ -0,0 +1,96 @@++# Copyright (c) 2024, PostgreSQL Global Development Group++use strict;+use warnings FATAL => 'all';+use locale;++use PostgreSQL::Test::Cluster;+use PostgreSQL::Test::Utils;+use Time::HiRes qw(usleep);+use Test::More;++# Test timeouts that will cause FATAL errors. This test relies on injection+# points to await a timeout occurrence. Relying on sleep proved to be unstable+# on buildfarm. It's difficult to rely on the NOTICE injection point because+# the backend under FATAL error can behave differently.++if ($ENV{enable_injection_points} ne 'yes')+{+ plan skip_all => 'Injection points not supported by this build';+}++# Node initialization+my $node = PostgreSQL::Test::Cluster->new('master');+$node->init();+$node->start;++# Check if the extension injection_points is available, as it may be+# possible that this script is run with installcheck, where the module+# would not be installed by default.+if (!$node->check_extension('injection_points'))+{+ plan skip_all => 'Extension injection_points not installed';+}++$node->safe_psql('postgres', 'CREATE EXTENSION injection_points;');+++sub randStr {+ my $len = shift;+ my @chars = ("A".."Z", "a".."z", "0".."9");+ return join '', map { @chars[rand @chars] } 1 .. $len;+}++# XXX: If you make this shorter, the pg_proc tuple is not toasted, and the test passes+my $longtext = randStr(10000); # test fails+#my $longtext = randStr(10); # test passes++$node->safe_psql('postgres', qq[+ CREATE FUNCTION foofunc(dummy integer) RETURNS integer AS \$\$ SELECT 1; /* $longtext */ \$\$ LANGUAGE SQL+]);++my $psql_session = $node->background_psql('postgres');+my $psql_session2 = $node->background_psql('postgres');++# Set injection point in the session, to pause while populating the catcache list+$psql_session->query_safe(+ qq[+ SELECT injection_points_set_local();+ SELECT injection_points_attach('catcache-list-miss-systable-scan-started', 'wait');+]);++# This pauses on the injection point while populating catcache list for+# functions with name "foofunc"+$psql_session->query_until(+ qr/starting_bg_psql/, q(+ \echo starting_bg_psql+ SELECT foofunc(1);+));++# While the first session is building the catcache list, create a new+# function that overloads the same name. This sends a catcache invalidation.+$node->safe_psql('postgres', qq[+ CREATE FUNCTION foofunc() RETURNS integer AS \$\$ SELECT 123 \$\$ LANGUAGE SQL+]);++# Continue the paused session. It will continue to construct the catcache+# list, and will accept invalidations while doing that, but it misses the+# new function we just created. The "SELECT foofunc(1)" query will now+# finish.+$psql_session2->query_safe(qq[+ SELECT injection_points_wakeup('catcache-list-miss-systable-scan-started');+ SELECT injection_points_detach('catcache-list-miss-systable-scan-started');+]);++# Demonstrate that we can run other queries in the session. (Not necessary, but+# makes it more clear that the session is permanently missing the catcache entry)+$psql_session->query_safe("SELECT now();");++# This should find the new function, but it does not.+$psql_session->query_safe("SELECT foofunc();");++ok($psql_session->quit);+ok($psql_session2->quit);++done_testing();--
2.39.5
[text/x-patch] v2-0002-Fix-bug-in-catcache-invalidation.patch (11.3K, ../ea00edec-d649-42bf-8cfb-13bc83d4af8e@iki.fi/3-v2-0002-Fix-bug-in-catcache-invalidation.patch)
download | inline diff:
From 9ed223c36591b9ab777de48e8e3f28e321d3f9d4 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Tue, 24 Dec 2024 00:07:38 +0200
Subject: [PATCH v2 2/2] Fix bug in catcache invalidation
If a new entry is added that belongs to a catche list entry, and cache
invalidation happens while the list entry is being built, the new entry
can be missed.
To fix, change the way we detect concurrent invalidations while a
catcache entry is being built. Keep a stack of entries that are being
built, and apply cache invalidation to those entries in addition to the
real catcache entries.
Discussion: XXX
---
src/backend/utils/cache/catcache.c | 185 +++++++++++++++++------------
src/tools/pgindent/typedefs.list | 1 +
2 files changed, 112 insertions(+), 74 deletions(-)
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.cindex 4f06b45239c..2332c52c2bd 100644--- a/src/backend/utils/cache/catcache.c+++ b/src/backend/utils/cache/catcache.c@@ -41,6 +41,23 @@
#include "utils/resowner.h"
#include "utils/syscache.h"
+/*+ * If a catcache invalidation happens while we are in the middle of creating a+ * catcache entry (or list), it might apply to the entry we're creating,+ * making it invalid before it's inserted to the catcache. To catch such+ * cases, we have a stack of "create-in-progress" entries. Cache invalidation+ * marks any matching entries in the stack as dead, in addition to the actual+ * CatCTup and CatCList entries .+ */+typedef struct CatCCreating+{+ CatCache *cache;+ uint32 hash_value;+ bool list;+ bool dead;+ struct CatCCreating *next;+} CatCCreating;+static CatCCreating *catcache_creating_stack = NULL;
/* #define CACHEDEBUG */ /* turns DEBUG elogs on */
@@ -93,8 +110,7 @@ static void CatCacheRemoveCList(CatCache *cache, CatCList *cl);
static void RehashCatCache(CatCache *cp);
static void RehashCatCacheLists(CatCache *cp);
static void CatalogCacheInitializeCache(CatCache *cache);
-static CatCTup *CatalogCacheCreateEntry(CatCache *cache,- HeapTuple ntp, SysScanDesc scandesc,+static CatCTup *CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp,
Datum *arguments,
uint32 hashValue, Index hashIndex);
@@ -662,6 +678,16 @@ CatCacheInvalidate(CatCache *cache, uint32 hashValue)
/* could be multiple matches, so keep looking! */
}
}
++ /* Also invalidate any entries that are being built */+ for (CatCCreating *e = catcache_creating_stack; e != NULL; e = e->next)+ {+ if (e->cache == cache)+ {+ if (e->list || e->hash_value == hashValue)+ e->dead = true;+ }+ }
}
/* ----------------------------------------------------------------
@@ -744,6 +770,13 @@ ResetCatalogCache(CatCache *cache)
#endif
}
}
++ /* Also invalidate any entries that are being built */+ for (CatCCreating *e = catcache_creating_stack; e != NULL; e = e->next)+ {+ if (e->cache == cache)+ e->dead = true;+ }
}
/*
@@ -1493,7 +1526,7 @@ SearchCatCacheMiss(CatCache *cache,
while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
{
- ct = CatalogCacheCreateEntry(cache, ntp, scandesc, NULL,+ ct = CatalogCacheCreateEntry(cache, ntp, NULL,
hashValue, hashIndex);
/* upon failure, we must start the scan over */
if (ct == NULL)
@@ -1528,7 +1561,7 @@ SearchCatCacheMiss(CatCache *cache,
if (IsBootstrapProcessingMode())
return NULL;
- ct = CatalogCacheCreateEntry(cache, NULL, NULL, arguments,+ ct = CatalogCacheCreateEntry(cache, NULL, arguments,
hashValue, hashIndex);
/* Creating a negative cache entry shouldn't fail */
@@ -1665,6 +1698,8 @@ SearchCatCacheList(CatCache *cache,
HeapTuple ntp;
MemoryContext oldcxt;
int i;
+ volatile CatCCreating creating_list;+ bool first_iter = true;
/*
* one-time startup overhead for each cache
@@ -1779,12 +1814,25 @@ SearchCatCacheList(CatCache *cache,
*/
ctlist = NIL;
+ /*+ * Cache invalidation can happen while we're building the list.+ * CatalogCacheCreateEntry() handles concurrent invalidation of individual+ * tuples, but it's also possible that a new entry is concurrently added+ * that we might miss. Push an "in-progress" entry that will receive the+ * invalidation, until we have built the final list entry.+ */+ creating_list.cache = cache;+ creating_list.hash_value = lHashValue;+ creating_list.list = true;+ creating_list.dead = false;+ creating_list.next = catcache_creating_stack;+ catcache_creating_stack = (CatCCreating *) &creating_list;+
PG_TRY();
{
ScanKeyData cur_skey[CATCACHE_MAXKEYS];
Relation relation;
SysScanDesc scandesc;
- bool stale;
relation = table_open(cache->cc_reloid, AccessShareLock);
@@ -1810,9 +1858,10 @@ SearchCatCacheList(CatCache *cache,
/* The list will be ordered iff we are doing an index scan */
ordered = (scandesc->irel != NULL);
- stale = false;+ creating_list.dead = false;- INJECTION_POINT("catcache-list-miss-systable-scan-started");+ if (first_iter)+ INJECTION_POINT("catcache-list-miss-systable-scan-started");
while (HeapTupleIsValid(ntp = systable_getnext(scandesc)))
{
@@ -1856,30 +1905,13 @@ SearchCatCacheList(CatCache *cache,
if (!found)
{
/* We didn't find a usable entry, so make a new one */
- ct = CatalogCacheCreateEntry(cache, ntp, scandesc, NULL,+ ct = CatalogCacheCreateEntry(cache, ntp, NULL,
hashValue, hashIndex);
+
/* upon failure, we must start the scan over */
if (ct == NULL)
{
- /*- * Release refcounts on any items we already had. We- * dare not try to free them if they're now- * unreferenced, since an error while doing that would- * result in the PG_CATCH below doing extra refcount- * decrements. Besides, we'll likely re-adopt those- * items in the next iteration, so it's not worth- * complicating matters to try to get rid of them.- */- foreach(ctlist_item, ctlist)- {- ct = (CatCTup *) lfirst(ctlist_item);- Assert(ct->c_list == NULL);- Assert(ct->refcount > 0);- ct->refcount--;- }- /* Reset ctlist in preparation for new try */- ctlist = NIL;- stale = true;+ creating_list.dead = true;
break;
}
}
@@ -1891,7 +1923,32 @@ SearchCatCacheList(CatCache *cache,
}
systable_endscan(scandesc);
- } while (stale);++ /* upon failure, we must start the scan over */+ if (creating_list.dead)+ {+ /*+ * Release refcounts on any items we already had. We dare not+ * try to free them if they're now unreferenced, since an+ * error while doing that would result in the PG_CATCH below+ * doing extra refcount decrements. Besides, we'll likely+ * re-adopt those items in the next iteration, so it's not+ * worth complicating matters to try to get rid of them.+ */+ foreach(ctlist_item, ctlist)+ {+ ct = (CatCTup *) lfirst(ctlist_item);+ Assert(ct->c_list == NULL);+ Assert(ct->refcount > 0);+ ct->refcount--;+ }+ /* Reset ctlist in preparation for new try */+ ctlist = NIL;+ first_iter = false;+ continue;+ }+ break;+ } while (true);
table_close(relation, AccessShareLock);
@@ -1919,6 +1976,7 @@ SearchCatCacheList(CatCache *cache,
}
PG_CATCH();
{
+ catcache_creating_stack = creating_list.next;
foreach(ctlist_item, ctlist)
{
ct = (CatCTup *) lfirst(ctlist_item);
@@ -1937,6 +1995,8 @@ SearchCatCacheList(CatCache *cache,
PG_RE_THROW();
}
PG_END_TRY();
+ catcache_creating_stack = creating_list.next;+ Assert(!creating_list.dead);
cl->cl_magic = CL_MAGIC;
cl->my_cache = cache;
@@ -2009,23 +2069,6 @@ ReleaseCatCacheListWithOwner(CatCList *list, ResourceOwner resowner)
}
-/*- * equalTuple- * Are these tuples memcmp()-equal?- */-static bool-equalTuple(HeapTuple a, HeapTuple b)-{- uint32 alen;- uint32 blen;-- alen = a->t_len;- blen = b->t_len;- return (alen == blen &&- memcmp((char *) a->t_data,- (char *) b->t_data, blen) == 0);-}-
/*
* CatalogCacheCreateEntry
* Create a new CatCTup entry, copying the given HeapTuple and other
@@ -2033,17 +2076,16 @@ equalTuple(HeapTuple a, HeapTuple b)
*
* To create a normal cache entry, ntp must be the HeapTuple just fetched
* from scandesc, and "arguments" is not used. To create a negative cache
- * entry, pass NULL for ntp and scandesc; then "arguments" is the cache- * keys to use. In either case, hashValue/hashIndex are the hash values- * computed from the cache keys.+ * entry, pass NULL for ntp; then "arguments" is the cache keys to use.+ * In either case, hashValue/hashIndex are the hash values computed from+ * the cache keys.
*
* Returns NULL if we attempt to detoast the tuple and observe that it
* became stale. (This cannot happen for a negative entry.) Caller must
* retry the tuple lookup in that case.
*/
static CatCTup *
-CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, SysScanDesc scandesc,- Datum *arguments,+CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, Datum *arguments,
uint32 hashValue, Index hashIndex)
{
CatCTup *ct;
@@ -2076,38 +2118,33 @@ CatalogCacheCreateEntry(CatCache *cache, HeapTuple ntp, SysScanDesc scandesc,
*/
if (HeapTupleHasExternal(ntp))
{
- bool need_cmp = IsInplaceUpdateOid(cache->cc_reloid);- HeapTuple before = NULL;- bool matches = true;-- if (need_cmp)- before = heap_copytuple(ntp);- dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);+ volatile CatCCreating creating;
/*
* The tuple could become stale while we are doing toast table
- * access (since AcceptInvalidationMessages can run then).- * equalTuple() detects staleness from inplace updates, while- * systable_recheck_tuple() detects staleness from normal updates.- *- * While this equalTuple() follows the usual rule of reading with- * a pin and no buffer lock, it warrants suspicion since an- * inplace update could appear at any moment. It's safe because- * the inplace update sends an invalidation that can't reorder- * before the inplace heap change. If the heap change reaches- * this process just after equalTuple() looks, we've not missed- * its inval.+ * access (since AcceptInvalidationMessages can run then). The+ * invalidation will mark our 'creating' entry as dead.
*/
- if (need_cmp)+ creating.cache = cache;+ creating.hash_value = hashValue;+ creating.list = false;+ creating.dead = false;+ creating.next = catcache_creating_stack;+ catcache_creating_stack = (CatCCreating *) &creating;++ PG_TRY();
{
- matches = equalTuple(before, ntp);- heap_freetuple(before);+ dtp = toast_flatten_tuple(ntp, cache->cc_tupdesc);
}
- if (!matches || !systable_recheck_tuple(scandesc, ntp))+ PG_FINALLY();
{
- heap_freetuple(dtp);- return NULL;+ Assert(catcache_creating_stack == &creating);+ catcache_creating_stack = creating.next;
}
+ PG_END_TRY();++ if (creating.dead)+ return NULL;
}
else
dtp = ntp;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.listindex e1c4f913f84..e1d63776581 100644--- a/src/tools/pgindent/typedefs.list+++ b/src/tools/pgindent/typedefs.list@@ -383,6 +383,7 @@ CaseTestExpr
CaseWhen
Cash
CastInfo
+CatCCreating
CatCList
CatCTup
CatCache
--
2.39.5
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hlinnaka@iki.fi, michael@paquier.xyz, tgl@sss.pgh.pa.us, noah@leadboat.com, fanfuxiaoran@gmail.com, andres@anarazel.de
Subject: Re: Recovering from detoast-related catcache invalidations
In-Reply-To: <ea00edec-d649-42bf-8cfb-13bc83d4af8e@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox