From: Jeff Davis <pgsql@j-davis.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Isaac Morland <isaac.morland@gmail.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: Faster "SET search_path"
Date: Thu, 16 Nov 2023 16:46:10 -0800
Message-ID: <e6fded24cb8a2c53d4ef069d9f69cc7baaafe9ef.camel@j-davis.com> (raw)
In-Reply-To: <695668acc40b937258985669f3d55b94cce82c3c.camel@j-davis.com>
References: <04c8592dbd694e4114a3ed87139a7a04e4363030.camel@j-davis.com>
<CAMsGm5fZ2xiVDLYH4-_Ld8bfe1qwY23ZLgPc7zBpH8d1WUw70A@mail.gmail.com>
<f555c41554c7f5ff2699c085836ea94beac09bad.camel@j-davis.com>
<20230802050720.GB866741@nathanxps13>
<CAMsGm5dt6szoTe5oAzk5nfbHHOEpMDVhLTOGWCJGCQ3Wq8+J0w@mail.gmail.com>
<fec7f3bea50112323d9b6e700b5e2762a14af809.camel@j-davis.com>
<20230807223908.GB1284531@nathanxps13>
<3b2d57ead15798adcbf6a53a15e35cdbadb2c147.camel@j-davis.com>
<5bb4d9c21cc918e5d05021bf0d5e0e7d34a0377c.camel@j-davis.com>
<abf4ce8804e0e05dff8c1725ae6a8ed28b7d66e0.camel@j-davis.com>
<695668acc40b937258985669f3d55b94cce82c3c.camel@j-davis.com>
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
Attachments:download all (3 files, .tar.gz)v9-0005-Optimize-SearchPathCache-by-saving-the-last-entry.patch (text/x-patch, 4.4K) download
From 6ed0785c29a43ccbd36aa3f9bd3be751131a6bf2 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@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.cindex 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
v9-0004-Optimize-check_search_path-by-using-SearchPathCac.patch (text/x-patch, 3.4K) download
From e7eb616cea01f575417f56615edb9bd24bfc2453 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@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.cindex 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
v9-0003-Be-more-paranoid-about-OOM-when-changing-search_p.patch (text/x-patch, 6.9K) download
From 8d870755b638a193a402292f265cb7e2f78ae432 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@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.cindex 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
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: pgsql@j-davis.com, nathandbossart@gmail.com, isaac.morland@gmail.com
Subject: Re: Faster "SET search_path"
In-Reply-To: <e6fded24cb8a2c53d4ef069d9f69cc7baaafe9ef.camel@j-davis.com>
* 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