On Thu, 2023-11-16 at 16:46 -0800, Jeff Davis wrote:
> While I considered OOM during hash key initialization, I missed some
> other potential out-of-memory hazards. Attached a fixup patch 0003,
> which re-introduces one list copy but it simplifies things
> substantially in addition to being safer around OOM conditions.
Committed 0003 fixup.
> > > 0004: Use the same cache to optimize check_search_path().
Committed 0004.
> > > 0005: Optimize cache for repeated lookups of the same value.
Will commit 0005 soon.
I also attached a trivial 0006 patch that uses SH_STORE_HASH. I wasn't
able to show much benefit, though, even when there's a bucket
collision. Perhaps there just aren't enough elements to matter -- I
suppose there would be a benefit if there are lots of unique
search_path strings, but that doesn't seem very plausible to me. If
someone thinks it's worth committing, then I will, but I don't see any
real upside or downside.
Regards,
Jeff Davis
From 5f41c0ecc602dd183b7f6e2f23cd28c9338b3c5b Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Sun, 19 Nov 2023 14:47:04 -0800
Subject: [PATCH v10 2/2] Use SH_STORE_HASH for search_path cache.
Does not change performance in expected cases, but makes performance
more resilient in case of bucket collisions.
Discussion: https://postgr.es/m/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/namespace.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 37a69e9023..0b6e0d711c 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -171,11 +171,10 @@ typedef struct SearchPathCacheEntry
List *oidlist; /* namespace OIDs that pass ACL checks */
List *finalPath; /* cached final computed search path */
Oid firstNS; /* first explicitly-listed namespace */
+ uint32 hash; /* needed for simplehash */
bool temp_missing;
bool forceRecompute; /* force recompute of finalPath */
-
- /* needed for simplehash */
- char status;
+ char status; /* needed for simplehash */
} SearchPathCacheEntry;
/*
@@ -270,6 +269,8 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
#define SH_EQUAL(tb, a, b) spcachekey_equal(a, b)
#define SH_SCOPE static inline
#define SH_DECLARE
+#define SH_GET_HASH(tb, a) a->hash
+#define SH_STORE_HASH
#define SH_DEFINE
#include "lib/simplehash.h"
--
2.34.1
From 46e09b225217bef79a57cc4f8450ed19be8f21ba Mon Sep 17 00:00:00 2001
From: Jeff Davis <[email protected]>
Date: Thu, 19 Oct 2023 15:48:16 -0700
Subject: [PATCH v10 1/2] Optimize SearchPathCache by saving the last entry.
Repeated lookups are common, so it's worth it to check the last entry
before doing another hash lookup.
Discussion: https://postgr.es/m/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
---
src/backend/catalog/namespace.c | 88 +++++++++++++++++++++------------
1 file changed, 57 insertions(+), 31 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 5027efc91d..37a69e9023 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -241,7 +241,8 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
*
* The search path cache is based on a wrapper around a simplehash hash table
* (nsphash, defined below). The spcache wrapper deals with OOM while trying
- * to initialize a key, and also offers a more convenient API.
+ * to initialize a key, optimizes repeated lookups of the same key, and also
+ * offers a more convenient API.
*/
static inline uint32
@@ -281,6 +282,7 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
#define SPCACHE_RESET_THRESHOLD 256
static nsphash_hash * SearchPathCache = NULL;
+static SearchPathCacheEntry * LastSearchPathCacheEntry = NULL;
/*
* Create or reset search_path cache as necessary.
@@ -295,6 +297,7 @@ spcache_init(void)
return;
MemoryContextReset(SearchPathCacheContext);
+ LastSearchPathCacheEntry = NULL;
/* arbitrary initial starting size of 16 elements */
SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
searchPathCacheValid = true;
@@ -307,12 +310,25 @@ spcache_init(void)
static SearchPathCacheEntry *
spcache_lookup(const char *searchPath, Oid roleid)
{
- SearchPathCacheKey cachekey = {
- .searchPath = searchPath,
- .roleid = roleid
- };
+ if (LastSearchPathCacheEntry &&
+ LastSearchPathCacheEntry->key.roleid == roleid &&
+ strcmp(LastSearchPathCacheEntry->key.searchPath, searchPath) == 0)
+ {
+ return LastSearchPathCacheEntry;
+ }
+ else
+ {
+ SearchPathCacheEntry *entry;
+ SearchPathCacheKey cachekey = {
+ .searchPath = searchPath,
+ .roleid = roleid
+ };
+
+ entry = nsphash_lookup(SearchPathCache, cachekey);
- return nsphash_lookup(SearchPathCache, cachekey);
+ LastSearchPathCacheEntry = entry;
+ return entry;
+ }
}
/*
@@ -324,35 +340,45 @@ spcache_lookup(const char *searchPath, Oid roleid)
static SearchPathCacheEntry *
spcache_insert(const char *searchPath, Oid roleid)
{
- SearchPathCacheEntry *entry;
- SearchPathCacheKey cachekey = {
- .searchPath = searchPath,
- .roleid = roleid
- };
-
- /*
- * searchPath is not saved in SearchPathCacheContext. First perform a
- * lookup, and copy searchPath only if we need to create a new entry.
- */
- entry = nsphash_lookup(SearchPathCache, cachekey);
-
- if (!entry)
+ if (LastSearchPathCacheEntry &&
+ LastSearchPathCacheEntry->key.roleid == roleid &&
+ strcmp(LastSearchPathCacheEntry->key.searchPath, searchPath) == 0)
{
- bool found;
+ return LastSearchPathCacheEntry;
+ }
+ else
+ {
+ SearchPathCacheEntry *entry;
+ SearchPathCacheKey cachekey = {
+ .searchPath = searchPath,
+ .roleid = roleid
+ };
- cachekey.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
- entry = nsphash_insert(SearchPathCache, cachekey, &found);
- Assert(!found);
+ /*
+ * searchPath is not saved in SearchPathCacheContext. First perform a
+ * lookup, and copy searchPath only if we need to create a new entry.
+ */
+ entry = nsphash_lookup(SearchPathCache, cachekey);
- entry->oidlist = NIL;
- entry->finalPath = NIL;
- entry->firstNS = InvalidOid;
- entry->temp_missing = false;
- entry->forceRecompute = false;
- /* do not touch entry->status, used by simplehash */
- }
+ if (!entry)
+ {
+ bool found;
+
+ cachekey.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
+ entry = nsphash_insert(SearchPathCache, cachekey, &found);
+ Assert(!found);
+
+ entry->oidlist = NIL;
+ entry->finalPath = NIL;
+ entry->firstNS = InvalidOid;
+ entry->temp_missing = false;
+ entry->forceRecompute = false;
+ /* do not touch entry->status, used by simplehash */
+ }
- return entry;
+ LastSearchPathCacheEntry = entry;
+ return entry;
+ }
}
/*
--
2.34.1
