On Fri, 2023-09-15 at 11:31 -0700, Jeff Davis wrote:
> On Tue, 2023-09-12 at 13:55 -0700, Jeff Davis wrote:
> > On Mon, 2023-08-07 at 15:39 -0700, Nathan Bossart wrote:
> > > 0003 is looking pretty good, too, but I think we
> > > should get some more eyes on it, given the complexity.
> >
> > Attached rebased version of 0003.
>
> Is someone else planning to look at 0003, or should I just proceed?
> It
> seems to be clearly wanted, and I think it's better to get it in this
> 'fest than to wait.
Attaching a new version, as well as some additional optimizations.
Changes:
* I separated it into more small functions, and generally refactored
quite a bit trying to make it easier to review.
* The new version is more careful about what might happen during an
OOM, or in weird cases such as a huge number of distinct search_path
strings.
0003: Cache for recomputeNamespacePath.
0004: Use the same cache to optimize check_search_path().
0005: Optimize cache for repeated lookups of the same value.
Applying the same tests as described in the first message[1], the new
numbers are:
baseline: 4.4s
test query:
without patch: 12.3s
0003: 8.8s
0003,0004: 7.4s
0003,0004,0005: 7.0s
This brings the slowdown from 180% on master down to about 60%. Still
not where we want to be exactly, but more tolerable.
The profile shows a few more areas worth looking at, so I suppose a bit
more effort could bring it down further. find_option(), for instance,
is annoyingly slow because it does case folding.
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
From 955cce952e8fbc8d117d4df1876e56feaec1d944 Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Wed, 26 Jul 2023 12:55:09 -0700
Subject: [PATCH v7 3/6] Add cache for recomputeNamespacePath().
When search_path is changed to something that was previously set, and
no invalidation happened in between, use the cached list of namespace
OIDs rather than recomputing them. This avoids syscache lookups and
ACL checks.
Important when the search_path changes frequently, such as when set in
proconfig.
Discussion: https://postgr.es/m/04c8592dbd694e4114a3ed87139a7a04e4363030.camel%40j-davis.com
---
src/backend/catalog/namespace.c | 440 ++++++++++++++++++++++++++------
1 file changed, 365 insertions(+), 75 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index ff1bfae1a3..258c97b590 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -41,6 +41,7 @@
#include "catalog/pg_ts_template.h"
#include "catalog/pg_type.h"
#include "commands/dbcommands.h"
+#include "common/hashfn.h"
#include "funcapi.h"
#include "mb/pg_wchar.h"
#include "miscadmin.h"
@@ -109,11 +110,12 @@
* activeSearchPath is always the actually active path; it points to
* to baseSearchPath which is the list derived from namespace_search_path.
*
- * If baseSearchPathValid is false, then baseSearchPath (and other
- * derived variables) need to be recomputed from namespace_search_path.
- * We mark it invalid upon an assignment to namespace_search_path or receipt
- * of a syscache invalidation event for pg_namespace. The recomputation
- * is done during the next lookup attempt.
+ * If baseSearchPathValid is false, then baseSearchPath (and other derived
+ * variables) need to be recomputed from namespace_search_path, or retrieved
+ * from the search path cache if there haven't been any syscache
+ * invalidations. We mark it invalid upon an assignment to
+ * namespace_search_path or receipt of a syscache invalidation event for
+ * pg_namespace. The recomputation is done during the next lookup attempt.
*
* Any namespaces mentioned in namespace_search_path that are not readable
* by the current user ID are simply left out of baseSearchPath; so
@@ -153,6 +155,27 @@ static Oid namespaceUser = InvalidOid;
/* The above four values are valid only if baseSearchPathValid */
static bool baseSearchPathValid = true;
+static bool searchPathCacheValid = false;
+static MemoryContext SearchPathCacheContext = NULL;
+
+typedef struct SearchPathCacheKey
+{
+ const char *searchPath;
+ Oid roleid;
+} SearchPathCacheKey;
+
+typedef struct SearchPathCacheEntry
+{
+ SearchPathCacheKey key;
+ List *oidlist; /* namespace OIDs that pass ACL checks */
+ List *finalPath; /* cached final computed search path */
+ Oid firstNS; /* first explicitly-listed namespace */
+ bool temp_missing;
+ bool forceRecompute; /* force recompute of finalPath */
+
+ /* needed for simplehash */
+ char status;
+} SearchPathCacheEntry;
/*
* myTempNamespace is InvalidOid until and unless a TEMP namespace is set up
@@ -206,6 +229,143 @@ static bool MatchNamedCall(HeapTuple proctup, int nargs, List *argnames,
bool include_out_arguments, int pronargs,
int **argnumbers);
+/*
+ * Recomputing the namespace path can be costly when done frequently, such as
+ * when a function has search_path set in proconfig. Add a search path cache
+ * that can be used by recomputeNamespacePath().
+ *
+ * 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.
+ */
+
+static inline uint32
+spcachekey_hash(SearchPathCacheKey key)
+{
+ const unsigned char *bytes = (const unsigned char *)key.searchPath;
+ int blen = strlen(key.searchPath);
+
+ return hash_combine(hash_bytes(bytes, blen),
+ hash_uint32(key.roleid));
+}
+
+static inline bool
+spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
+{
+ return a.roleid == b.roleid &&
+ strcmp(a.searchPath, b.searchPath) == 0;
+}
+
+#define SH_PREFIX nsphash
+#define SH_ELEMENT_TYPE SearchPathCacheEntry
+#define SH_KEY_TYPE SearchPathCacheKey
+#define SH_KEY key
+#define SH_HASH_KEY(tb, key) spcachekey_hash(key)
+#define SH_EQUAL(tb, a, b) spcachekey_equal(a, b)
+#define SH_SCOPE static inline
+#define SH_DECLARE
+#define SH_DEFINE
+#include "lib/simplehash.h"
+
+/*
+ * We only expect a small number of unique search_path strings to be used. If
+ * this cache grows to an unreasonable size, reset it to avoid steady-state
+ * memory growth. Most likely, only a few of those entries will benefit from
+ * the cache, and the cache will be quickly repopulated with such entries.
+ */
+#define SPCACHE_RESET_THRESHOLD 1024
+
+static nsphash_hash *SearchPathCache = NULL;
+
+/*
+ * Create search path cache.
+ */
+static void
+spcache_init(void)
+{
+ Assert(SearchPathCacheContext);
+
+ if (SearchPathCache)
+ return;
+
+ /* 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.
+ *
+ * Initialize key safely, so that OOM does not leave an entry without a valid
+ * key. Caller must ensure that non-key contents are properly initialized.
+ */
+static SearchPathCacheEntry *
+spcache_insert(const char *searchPath, Oid roleid)
+{
+ SearchPathCacheEntry *entry;
+ bool found;
+ SearchPathCacheKey cachekey = {
+ .searchPath = searchPath,
+ .roleid = roleid
+ };
+
+ entry = nsphash_insert(SearchPathCache, cachekey, &found);
+
+ /* ensure that key is initialized and the rest is zeroed */
+ if (!found)
+ {
+ size_t size = strlen(searchPath) + 1;
+ char *newstr;
+
+ /* do not touch entry->status, used by simplehash */
+ entry->oidlist = NIL;
+ entry->finalPath = NIL;
+ entry->firstNS = InvalidOid;
+ entry->temp_missing = false;
+
+ newstr = MemoryContextAllocExtended(SearchPathCacheContext, size,
+ MCXT_ALLOC_NO_OOM);
+
+ /* if we can't initialize the key due to OOM, delete the entry */
+ if (newstr == NULL)
+ {
+ nsphash_delete_item(SearchPathCache, entry);
+ MemoryContextStats(TopMemoryContext);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed on request of size %zu in memory context \"%s\".",
+ size, SearchPathCacheContext->name)));
+ }
+
+ memcpy(newstr, searchPath, size);
+ entry->key.searchPath = newstr;
+ entry->key.roleid = roleid;
+ }
+
+ return entry;
+}
/*
* RangeVarGetRelidExtended
@@ -3630,6 +3790,7 @@ SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId)
*/
baseSearchPathValid = false; /* may need to rebuild list */
+ searchPathCacheValid = false;
}
@@ -3893,28 +4054,19 @@ FindDefaultConversionProc(int32 for_encoding, int32 to_encoding)
}
/*
- * recomputeNamespacePath - recompute path derived variables if needed.
+ * Look up namespace IDs and perform ACL checks. Return newly-allocated list.
*/
-static void
-recomputeNamespacePath(void)
+static List *
+preprocessNamespacePath(const char *searchPath, Oid roleid,
+ bool *temp_missing)
{
- Oid roleid = GetUserId();
char *rawname;
List *namelist;
List *oidlist;
- List *newpath;
ListCell *l;
- bool temp_missing;
- Oid firstNS;
- bool pathChanged;
- MemoryContext oldcxt;
- /* Do nothing if path is already valid. */
- if (baseSearchPathValid && namespaceUser == roleid)
- return;
-
- /* Need a modifiable copy of namespace_search_path string */
- rawname = pstrdup(namespace_search_path);
+ /* Need a modifiable copy */
+ rawname = pstrdup(searchPath);
/* Parse string into list of identifiers */
if (!SplitIdentifierString(rawname, ',', &namelist))
@@ -3931,7 +4083,7 @@ recomputeNamespacePath(void)
* already been accepted.) Don't make duplicate entries, either.
*/
oidlist = NIL;
- temp_missing = false;
+ *temp_missing = false;
foreach(l, namelist)
{
char *curname = (char *) lfirst(l);
@@ -3951,10 +4103,8 @@ recomputeNamespacePath(void)
namespaceId = get_namespace_oid(rname, true);
ReleaseSysCache(tuple);
if (OidIsValid(namespaceId) &&
- !list_member_oid(oidlist, namespaceId) &&
object_aclcheck(NamespaceRelationId, namespaceId, roleid,
- ACL_USAGE) == ACLCHECK_OK &&
- InvokeNamespaceSearchHook(namespaceId, false))
+ ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
@@ -3962,16 +4112,12 @@ recomputeNamespacePath(void)
{
/* pg_temp --- substitute temp namespace, if any */
if (OidIsValid(myTempNamespace))
- {
- if (!list_member_oid(oidlist, myTempNamespace) &&
- InvokeNamespaceSearchHook(myTempNamespace, false))
- oidlist = lappend_oid(oidlist, myTempNamespace);
- }
+ oidlist = lappend_oid(oidlist, myTempNamespace);
else
{
/* If it ought to be the creation namespace, set flag */
if (oidlist == NIL)
- temp_missing = true;
+ *temp_missing = true;
}
}
else
@@ -3979,62 +4125,190 @@ recomputeNamespacePath(void)
/* normal namespace reference */
namespaceId = get_namespace_oid(curname, true);
if (OidIsValid(namespaceId) &&
- !list_member_oid(oidlist, namespaceId) &&
object_aclcheck(NamespaceRelationId, namespaceId, roleid,
- ACL_USAGE) == ACLCHECK_OK &&
- InvokeNamespaceSearchHook(namespaceId, false))
+ ACL_USAGE) == ACLCHECK_OK)
oidlist = lappend_oid(oidlist, namespaceId);
}
}
+ pfree(rawname);
+ list_free(namelist);
+
+ return oidlist;
+}
+
+/*
+ * Remove duplicates, run namespace search hooks, and prepend
+ * implicitly-searched namespaces. Return newly-allocated list.
+ *
+ * If an object_access_hook is present, this must always be recalculated. It
+ * may seem that duplicate elimination is not dependent on the result of the
+ * hook, but if a hook returns different results on different calls for the
+ * same namespace ID, then it could affect the order in which that namespace
+ * appears in the final list.
+ */
+static List *
+finalNamespacePath(List *oidlist, Oid *firstNS)
+{
+ List *finalPath = NIL;
+ ListCell *lc;
+
+ foreach(lc, oidlist)
+ {
+ Oid namespaceId = lfirst_oid(lc);
+ if (!list_member_oid(finalPath, namespaceId))
+ {
+ if (InvokeNamespaceSearchHook(namespaceId, false))
+ finalPath = lappend_oid(finalPath, namespaceId);
+ }
+ }
+
/*
* Remember the first member of the explicit list. (Note: this is
* nominally wrong if temp_missing, but we need it anyway to distinguish
* explicit from implicit mention of pg_catalog.)
*/
- if (oidlist == NIL)
- firstNS = InvalidOid;
+ if (finalPath == NIL)
+ *firstNS = InvalidOid;
else
- firstNS = linitial_oid(oidlist);
+ *firstNS = linitial_oid(finalPath);
/*
* Add any implicitly-searched namespaces to the list. Note these go on
* the front, not the back; also notice that we do not check USAGE
* permissions for these.
*/
- if (!list_member_oid(oidlist, PG_CATALOG_NAMESPACE))
- oidlist = lcons_oid(PG_CATALOG_NAMESPACE, oidlist);
+ if (!list_member_oid(finalPath, PG_CATALOG_NAMESPACE))
+ finalPath = lcons_oid(PG_CATALOG_NAMESPACE, finalPath);
if (OidIsValid(myTempNamespace) &&
- !list_member_oid(oidlist, myTempNamespace))
- oidlist = lcons_oid(myTempNamespace, oidlist);
+ !list_member_oid(finalPath, myTempNamespace))
+ finalPath = lcons_oid(myTempNamespace, finalPath);
+
+ return finalPath;
+}
+
+/*
+ * 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 different from 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 *changed)
+{
+ 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);
/*
- * We want to detect the case where the effective value of the base search
- * path variables didn't change. As long as we're doing so, we can avoid
- * copying the OID list unnecessarily.
+ * An OOM may have resulted in a cache entry with mising 'oidlist' or
+ * 'finalPath', so just compute whatever is missing.
*/
- if (baseCreationNamespace == firstNS &&
- baseTempCreationPending == temp_missing &&
- equal(oidlist, baseSearchPath))
+
+ if (entry->oidlist == NIL)
{
- pathChanged = false;
+ oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
+ entry->oidlist = preprocessNamespacePath(searchPath, roleid,
+ &entry->temp_missing);
+ MemoryContextSwitchTo(oldcxt);
}
- else
+
+ /*
+ * If a hook is set, we must recompute finalPath from the oidlist each
+ * time, because the hook may affect the result. This is still much faster
+ * than recomputing from the string (and doing catalog lookups and ACL
+ * checks).
+ */
+ if (entry->finalPath == NIL || object_access_hook ||
+ entry->forceRecompute)
{
- pathChanged = true;
+ /*
+ * 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;
- /* Must save OID list in permanent storage. */
- oldcxt = MemoryContextSwitchTo(TopMemoryContext);
- newpath = list_copy(oidlist);
+ oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
+ finalPath = finalNamespacePath(entry->oidlist,
+ &entry->firstNS);
MemoryContextSwitchTo(oldcxt);
- /* Now safe to assign to state variables. */
- list_free(baseSearchPath);
- baseSearchPath = newpath;
- baseCreationNamespace = firstNS;
- baseTempCreationPending = temp_missing;
+ *changed = !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
+ * finalPath the next time this cache entry is used, even if the
+ * object_access_hook is not set at that time.
+ */
+ entry->forceRecompute = object_access_hook ? true : false;
+ }
+ else
+ {
+ /* use cached version of finalPath */
+ *changed = !equal(prevPath, entry->finalPath);
+ }
+
+ list_free(prevPathCopy);
+
+ return entry;
+}
+
+/*
+ * recomputeNamespacePath - recompute path derived variables if needed.
+ */
+static void
+recomputeNamespacePath(void)
+{
+ Oid roleid = GetUserId();
+ bool newPathEqual;
+ bool pathChanged;
+ const SearchPathCacheEntry *entry;
+
+ /* Do nothing if path is already valid. */
+ if (baseSearchPathValid && namespaceUser == roleid)
+ return;
+
+ entry = cachedNamespacePath(namespace_search_path, roleid, baseSearchPath,
+ &newPathEqual);
+
+ if (baseCreationNamespace == entry->firstNS &&
+ baseTempCreationPending == entry->temp_missing &&
+ !newPathEqual)
+ {
+ pathChanged = false;
}
+ else
+ {
+ pathChanged = true;
+ }
+
+ /* Now safe to assign to state variables. */
+ baseSearchPath = entry->finalPath;
+ baseCreationNamespace = entry->firstNS;
+ baseTempCreationPending = entry->temp_missing;
/* Mark the path valid. */
baseSearchPathValid = true;
@@ -4051,11 +4325,6 @@ recomputeNamespacePath(void)
*/
if (pathChanged)
activePathGeneration++;
-
- /* Clean up. */
- pfree(rawname);
- list_free(namelist);
- list_free(oidlist);
}
/*
@@ -4210,6 +4479,7 @@ InitTempTableNamespace(void)
myTempNamespaceSubID = GetCurrentSubTransactionId();
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
}
/*
@@ -4235,6 +4505,7 @@ AtEOXact_Namespace(bool isCommit, bool parallel)
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
/*
* Reset the temporary namespace flag in MyProc. We assume that
@@ -4276,6 +4547,7 @@ AtEOSubXact_Namespace(bool isCommit, SubTransactionId mySubid,
myTempNamespace = InvalidOid;
myTempToastNamespace = InvalidOid;
baseSearchPathValid = false; /* need to rebuild list */
+ searchPathCacheValid = false;
/*
* Reset the temporary namespace flag in MyProc. We assume that
@@ -4361,11 +4633,23 @@ ResetTempTableNamespace(void)
bool
check_search_path(char **newval, void **extra, GucSource source)
{
- char *rawname;
- List *namelist;
+ const char *searchPath = *newval;
+ char *rawname;
+ List *namelist;
+
+ /*
+ * 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.
+ */
- /* Need a modifiable copy of string */
- rawname = pstrdup(*newval);
+ /*
+ * 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))
@@ -4377,14 +4661,6 @@ check_search_path(char **newval, void **extra, GucSource source)
return false;
}
- /*
- * 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.
- */
-
pfree(rawname);
list_free(namelist);
@@ -4399,6 +4675,10 @@ assign_search_path(const char *newval, void *extra)
* We mark the path as needing recomputation, but don't do anything until
* it's needed. This avoids trying to do database access during GUC
* initialization, or outside a transaction.
+ *
+ * This does not invalidate the search path cache, so if this value had
+ * been previously set and no syscache invalidations happened,
+ * recomputation may not be necessary.
*/
baseSearchPathValid = false;
}
@@ -4433,6 +4713,10 @@ InitializeSearchPath(void)
}
else
{
+ SearchPathCacheContext = AllocSetContextCreate(
+ TopMemoryContext, "search_path processing cache",
+ ALLOCSET_DEFAULT_SIZES);
+
/*
* In normal mode, arrange for a callback on any syscache invalidation
* of pg_namespace or pg_authid rows. (Changing a role name may affect
@@ -4446,6 +4730,7 @@ InitializeSearchPath(void)
(Datum) 0);
/* Force search path to be recomputed on next use */
baseSearchPathValid = false;
+ searchPathCacheValid = false;
}
}
@@ -4456,8 +4741,13 @@ InitializeSearchPath(void)
static void
NamespaceCallback(Datum arg, int cacheid, uint32 hashvalue)
{
- /* Force search path to be recomputed on next use */
+ /*
+ * Force search path to be recomputed on next use, also invalidating the
+ * search path cache (because namespace names, ACLs, or role names may
+ * have changed).
+ */
baseSearchPathValid = false;
+ searchPathCacheValid = false;
}
/*
--
2.34.1
From 46d51755859746789aa073625fd5570f2d01be0c 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 v7 4/6] 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 | 50 +++++++++++++++++++++++++++++++--
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 258c97b590..68deb86554 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -234,6 +234,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. In this case, the caller uses roleid=InvalidOid.
+ *
* 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.
@@ -314,6 +318,21 @@ spcache_members(void)
return SearchPathCache->members;
}
+/*
+ * 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.
*
@@ -4633,9 +4652,10 @@ ResetTempTableNamespace(void)
bool
check_search_path(char **newval, void **extra, GucSource source)
{
- const char *searchPath = *newval;
- char *rawname;
- List *namelist;
+ Oid roleid = InvalidOid;
+ const char *searchPath = *newval;
+ char *rawname;
+ List *namelist;
/*
* We used to try to check that the named schemas exist, but there are
@@ -4645,6 +4665,24 @@ check_search_path(char **newval, void **extra, GucSource source)
* 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;
+
+ if (spcache_members() >= SPCACHE_RESET_THRESHOLD)
+ spcache_reset();
+ }
+
/*
* Ensure validity check succeeds before creating cache entry.
*/
@@ -4664,6 +4702,12 @@ 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 8b5e905cbffa9eadd1e0d4fa6af7e7b77a93b7c7 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 v7 5/6] 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 | 107 ++++++++++++++++++++------------
1 file changed, 67 insertions(+), 40 deletions(-)
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 68deb86554..ff7169dc99 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -240,7 +240,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
@@ -280,6 +281,7 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
#define SPCACHE_RESET_THRESHOLD 1024
static nsphash_hash *SearchPathCache = NULL;
+static SearchPathCacheEntry *LastSearchPathCacheEntry = NULL;
/*
* Create search path cache.
@@ -308,6 +310,7 @@ spcache_reset(void)
MemoryContextReset(SearchPathCacheContext);
SearchPathCache = NULL;
+ LastSearchPathCacheEntry = NULL;
spcache_init();
}
@@ -325,12 +328,25 @@ spcache_members(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;
+ }
}
/*
@@ -342,48 +358,59 @@ spcache_lookup(const char *searchPath, Oid roleid)
static SearchPathCacheEntry *
spcache_insert(const char *searchPath, Oid roleid)
{
- SearchPathCacheEntry *entry;
- bool found;
- SearchPathCacheKey cachekey = {
- .searchPath = searchPath,
- .roleid = roleid
- };
+ if (LastSearchPathCacheEntry &&
+ LastSearchPathCacheEntry->key.roleid == roleid &&
+ strcmp(LastSearchPathCacheEntry->key.searchPath, searchPath) == 0)
+ {
+ return LastSearchPathCacheEntry;
+ }
+ else
+ {
+ SearchPathCacheEntry *entry;
+ bool found;
+ SearchPathCacheKey cachekey = {
+ .searchPath = searchPath,
+ .roleid = roleid
+ };
- entry = nsphash_insert(SearchPathCache, cachekey, &found);
+ entry = nsphash_insert(SearchPathCache, cachekey, &found);
- /* ensure that key is initialized and the rest is zeroed */
- if (!found)
- {
- size_t size = strlen(searchPath) + 1;
- char *newstr;
+ /* ensure that key is initialized and the rest is zeroed */
+ if (!found)
+ {
+ size_t size = strlen(searchPath) + 1;
+ char *newstr;
- /* do not touch entry->status, used by simplehash */
- entry->oidlist = NIL;
- entry->finalPath = NIL;
- entry->firstNS = InvalidOid;
- entry->temp_missing = false;
+ /* do not touch entry->status, used by simplehash */
+ entry->oidlist = NIL;
+ entry->finalPath = NIL;
+ entry->firstNS = InvalidOid;
+ entry->temp_missing = false;
- newstr = MemoryContextAllocExtended(SearchPathCacheContext, size,
- MCXT_ALLOC_NO_OOM);
+ newstr = MemoryContextAllocExtended(SearchPathCacheContext, size,
+ MCXT_ALLOC_NO_OOM);
- /* if we can't initialize the key due to OOM, delete the entry */
- if (newstr == NULL)
- {
- nsphash_delete_item(SearchPathCache, entry);
- MemoryContextStats(TopMemoryContext);
- ereport(ERROR,
- (errcode(ERRCODE_OUT_OF_MEMORY),
- errmsg("out of memory"),
- errdetail("Failed on request of size %zu in memory context \"%s\".",
- size, SearchPathCacheContext->name)));
+ /* if we can't initialize the key due to OOM, delete the entry */
+ if (newstr == NULL)
+ {
+ nsphash_delete_item(SearchPathCache, entry);
+ MemoryContextStats(TopMemoryContext);
+ ereport(ERROR,
+ (errcode(ERRCODE_OUT_OF_MEMORY),
+ errmsg("out of memory"),
+ errdetail("Failed on request of size %zu in memory context \"%s\".",
+ size, SearchPathCacheContext->name)));
+
+ }
+
+ memcpy(newstr, searchPath, size);
+ entry->key.searchPath = newstr;
+ entry->key.roleid = roleid;
}
- memcpy(newstr, searchPath, size);
- entry->key.searchPath = newstr;
- entry->key.roleid = roleid;
+ LastSearchPathCacheEntry = entry;
+ return entry;
}
-
- return entry;
}
/*
--
2.34.1
