On Tue, 2023-11-14 at 20:13 -0800, Jeff Davis wrote:
> On Thu, 2023-10-19 at 19:01 -0700, Jeff Davis wrote:
> > 0003: Cache for recomputeNamespacePath.
>
> Committed with some further simplification around the OOM handling.
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.
> > 0004: Use the same cache to optimize check_search_path().
> > 0005: Optimize cache for repeated lookups of the same value.
Also attached new versions of these patches.
Regards,
Jeff Davis
From 6ed0785c29a43ccbd36aa3f9bd3be751131a6bf2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Thu, 19 Oct 2023 15:48:16 -0700
Subject: [PATCH v9 5/5] 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 0785f2b797..f540da7ebc 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
From e7eb616cea01f575417f56615edb9bd24bfc2453 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Thu, 19 Oct 2023 15:19:05 -0700
Subject: [PATCH v9 4/5] Optimize check_search_path() by using SearchPathCache.
A hash lookup is faster than re-validating the string, particularly
because we use SplitIdentifierString() for validation.
Important when search_path changes frequently.
Discussion: https://postgr.es/m/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
---
src/backend/catalog/namespace.c | 55 +++++++++++++++++++++++++++++++--
1 file changed, 53 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 1a75b50807..0785f2b797 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -235,6 +235,10 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
* when a function has search_path set in proconfig. Add a search path cache
* that can be used by recomputeNamespacePath().
*
+ * The cache is also used to remember already-validated strings in
+ * check_search_path() to avoid the need to call SplitIdentifierString()
+ * repeatedly.
+ *
* 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.
@@ -296,6 +300,21 @@ spcache_init(void)
searchPathCacheValid = true;
}
+/*
+ * Look up entry in search path cache without inserting. Returns NULL if not
+ * present.
+ */
+static SearchPathCacheEntry *
+spcache_lookup(const char *searchPath, Oid roleid)
+{
+ SearchPathCacheKey cachekey = {
+ .searchPath = searchPath,
+ .roleid = roleid
+ };
+
+ return nsphash_lookup(SearchPathCache, cachekey);
+}
+
/*
* Look up or insert entry in search path cache.
*
@@ -4577,11 +4596,39 @@ ResetTempTableNamespace(void)
bool
check_search_path(char **newval, void **extra, GucSource source)
{
+ Oid roleid = InvalidOid;
+ const char *searchPath = *newval;
char *rawname;
List *namelist;
- /* Need a modifiable copy of string */
- rawname = pstrdup(*newval);
+ /*
+ * We used to try to check that the named schemas exist, but there are
+ * many valid use-cases for having search_path settings that include
+ * schemas that don't exist; and often, we are not inside a transaction
+ * here and so can't consult the system catalogs anyway. So now, the only
+ * requirement is syntactic validity of the identifier list.
+ */
+
+ /*
+ * Checking only the syntactic validity also allows us to use the search
+ * path cache (if available) to avoid calling SplitIdentifierString() on
+ * the same string repeatedly.
+ */
+ if (SearchPathCacheContext != NULL)
+ {
+ spcache_init();
+
+ roleid = GetUserId();
+
+ if (spcache_lookup(searchPath, roleid) != NULL)
+ return true;
+ }
+
+ /*
+ * Ensure validity check succeeds before creating cache entry.
+ */
+
+ rawname = pstrdup(searchPath); /* need a modifiable copy */
/* Parse string into list of identifiers */
if (!SplitIdentifierString(rawname, ',', &namelist))
@@ -4604,6 +4651,10 @@ check_search_path(char **newval, void **extra, GucSource source)
pfree(rawname);
list_free(namelist);
+ /* create empty cache entry */
+ if (SearchPathCacheContext != NULL)
+ (void) spcache_insert(searchPath, roleid);
+
return true;
}
--
2.34.1
From 8d870755b638a193a402292f265cb7e2f78ae432 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Thu, 16 Nov 2023 12:12:49 -0800
Subject: [PATCH v9 3/5] Be more paranoid about OOM when changing search_path.
Commit f26c2368dc introduced a search_path cache, and left some room
for potential problems with out-of-memory conditions.
This change re-introduces one list_copy(), but simplifies things
substantially in addition to better handling OOM.
---
src/backend/catalog/namespace.c | 118 ++++++++++----------------------
1 file changed, 35 insertions(+), 83 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 69dbe4500f..1a75b50807 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -279,42 +279,23 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
static nsphash_hash * SearchPathCache = NULL;
/*
- * Create search path cache.
+ * Create or reset search_path cache as necessary.
*/
static void
spcache_init(void)
{
Assert(SearchPathCacheContext);
- if (SearchPathCache)
- return;
+ if (SearchPathCache && searchPathCacheValid &&
+ SearchPathCache->members < SPCACHE_RESET_THRESHOLD)
+ return;
+ MemoryContextReset(SearchPathCacheContext);
/* arbitrary initial starting size of 16 elements */
SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
searchPathCacheValid = true;
}
-/*
- * Reset and reinitialize search path cache.
- */
-static void
-spcache_reset(void)
-{
- Assert(SearchPathCacheContext);
- Assert(SearchPathCache);
-
- MemoryContextReset(SearchPathCacheContext);
- SearchPathCache = NULL;
-
- spcache_init();
-}
-
-static uint32
-spcache_members(void)
-{
- return SearchPathCache->members;
-}
-
/*
* Look up or insert entry in search path cache.
*
@@ -325,27 +306,25 @@ static SearchPathCacheEntry *
spcache_insert(const char *searchPath, Oid roleid)
{
SearchPathCacheEntry *entry;
- bool found;
SearchPathCacheKey cachekey = {
.searchPath = searchPath,
.roleid = roleid
};
/*
- * If a new entry is created, we must ensure that it's properly
- * initialized. Set the cache invalid temporarily, so that if the
- * MemoryContextStrdup() below raises an OOM, the cache will be reset on
- * the next use, clearing the uninitialized entry.
+ * searchPath is not saved in SearchPathCacheContext. First perform a
+ * lookup, and copy searchPath only if we need to create a new entry.
*/
- searchPathCacheValid = false;
+ entry = nsphash_lookup(SearchPathCache, cachekey);
- entry = nsphash_insert(SearchPathCache, cachekey, &found);
-
- /* ensure that key is initialized and the rest is zeroed */
- if (!found)
+ if (!entry)
{
- entry->key.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
- entry->key.roleid = roleid;
+ bool found;
+
+ cachekey.searchPath = MemoryContextStrdup(SearchPathCacheContext, searchPath);
+ entry = nsphash_insert(SearchPathCache, cachekey, &found);
+ Assert(!found);
+
entry->oidlist = NIL;
entry->finalPath = NIL;
entry->firstNS = InvalidOid;
@@ -354,7 +333,6 @@ spcache_insert(const char *searchPath, Oid roleid)
/* do not touch entry->status, used by simplehash */
}
- searchPathCacheValid = true;
return entry;
}
@@ -4183,31 +4161,15 @@ finalNamespacePath(List *oidlist, Oid *firstNS)
/*
* Retrieve search path information from the cache; or if not there, fill
* it. The returned entry is valid only until the next call to this function.
- *
- * We also determine if the newly-computed finalPath is the same as the
- * prevPath passed by the caller (i.e. a no-op or a real change?). It's more
- * efficient to check for a change in this function than the caller, because
- * we can avoid unnecessary temporary copies of the previous path.
*/
static const SearchPathCacheEntry *
-cachedNamespacePath(const char *searchPath, Oid roleid, List *prevPath,
- bool *same)
+cachedNamespacePath(const char *searchPath, Oid roleid)
{
MemoryContext oldcxt;
SearchPathCacheEntry *entry;
- List *prevPathCopy = NIL;
spcache_init();
- /* invalidate cache if necessary */
- if (!searchPathCacheValid || spcache_members() >= SPCACHE_RESET_THRESHOLD)
- {
- /* prevPath will be destroyed; make temp copy for later comparison */
- prevPathCopy = list_copy(prevPath);
- prevPath = prevPathCopy;
- spcache_reset();
- }
-
entry = spcache_insert(searchPath, roleid);
/*
@@ -4232,23 +4194,14 @@ cachedNamespacePath(const char *searchPath, Oid roleid, List *prevPath,
if (entry->finalPath == NIL || object_access_hook ||
entry->forceRecompute)
{
- /*
- * Do not free the stale value of entry->finalPath until we've
- * performed the comparison, in case it's aliased by prevPath (which
- * can only happen when recomputing due to an object_access_hook).
- */
- List *finalPath;
+ list_free(entry->finalPath);
+ entry->finalPath = NIL;
oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
- finalPath = finalNamespacePath(entry->oidlist,
- &entry->firstNS);
+ entry->finalPath = finalNamespacePath(entry->oidlist,
+ &entry->firstNS);
MemoryContextSwitchTo(oldcxt);
- *same = equal(prevPath, finalPath);
-
- list_free(entry->finalPath);
- entry->finalPath = finalPath;
-
/*
* If an object_access_hook set when finalPath is calculated, the
* result may be affected by the hook. Force recomputation of
@@ -4257,13 +4210,6 @@ cachedNamespacePath(const char *searchPath, Oid roleid, List *prevPath,
*/
entry->forceRecompute = object_access_hook ? true : false;
}
- else
- {
- /* use cached version of finalPath */
- *same = equal(prevPath, entry->finalPath);
- }
-
- list_free(prevPathCopy);
return entry;
}
@@ -4275,7 +4221,6 @@ static void
recomputeNamespacePath(void)
{
Oid roleid = GetUserId();
- bool newPathEqual;
bool pathChanged;
const SearchPathCacheEntry *entry;
@@ -4283,24 +4228,31 @@ recomputeNamespacePath(void)
if (baseSearchPathValid && namespaceUser == roleid)
return;
- entry = cachedNamespacePath(namespace_search_path, roleid, baseSearchPath,
- &newPathEqual);
+ entry = cachedNamespacePath(namespace_search_path, roleid);
if (baseCreationNamespace == entry->firstNS &&
baseTempCreationPending == entry->temp_missing &&
- newPathEqual)
+ equal(entry->finalPath, baseSearchPath))
{
pathChanged = false;
}
else
{
+ MemoryContext oldcxt;
+ List *newpath;
+
pathChanged = true;
- }
- /* Now safe to assign to state variables. */
- baseSearchPath = entry->finalPath;
- baseCreationNamespace = entry->firstNS;
- baseTempCreationPending = entry->temp_missing;
+ oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+ newpath = list_copy(entry->finalPath);
+ MemoryContextSwitchTo(oldcxt);
+
+ /* Now safe to assign to state variables. */
+ list_free(baseSearchPath);
+ baseSearchPath = newpath;
+ baseCreationNamespace = entry->firstNS;
+ baseTempCreationPending = entry->temp_missing;
+ }
/* Mark the path valid. */
baseSearchPathValid = true;
--
2.34.1
