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. -- Jeff Davis PostgreSQL Contributor Team - AWS
From f5b055aea1bf08928de1bffcfd7b202e28847595 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 v5 3/3] 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 | 349 ++++++++++++++++++++++++++------ 1 file changed, 286 insertions(+), 63 deletions(-) diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c index 4ceae038ec..8b97b3b529 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,35 @@ 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; + + /* + * If true, some namespaces were previously filtered out of finalPath by a + * namespace search hook. The next time it's read from the cache, we need + * to recreate finalPath from the oidlist. + */ + bool hook_filtered; + + /* needed for simplehash */ + char status; +} SearchPathCacheEntry; + +static SearchPathCacheEntry *CurrentSearchPathCacheEntry = NULL; /* * myTempNamespace is InvalidOid until and unless a TEMP namespace is set up @@ -193,6 +224,45 @@ 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 cache that can be + * used when the search_path changes to a value that was previously set, and + * no invalidation intervened. + * + * The cache is a simple hash table in its own memory context, with a key of + * search_path and role ID. + */ + +static inline uint32 +nspcachekey_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 +nspcachekey_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) nspcachekey_hash(key) +#define SH_EQUAL(tb, a, b) nspcachekey_equal(a, b) +#define SH_SCOPE static inline +#define SH_DECLARE +#define SH_DEFINE +#include "lib/simplehash.h" + +static nsphash_hash *SearchPathCache = NULL; /* * RangeVarGetRelidExtended @@ -3370,6 +3440,7 @@ SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId) */ baseSearchPathValid = false; /* may need to rebuild list */ + searchPathCacheValid = false; } @@ -3633,28 +3704,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)) @@ -3671,7 +3733,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); @@ -3691,10 +3753,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); } } @@ -3702,16 +3762,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 @@ -3719,63 +3775,218 @@ 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. + * + * All of this must happen after retrieving from cache, because we don't know + * what might invalidate the result from the last time the hook was + * invoked. 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, bool *hook_filtered) +{ + List *finalPath = NIL; + ListCell *lc; + + *hook_filtered = false; + foreach(lc, oidlist) + { + Oid namespaceId = lfirst_oid(lc); + if (!list_member_oid(finalPath, namespaceId)) + { + if (InvokeNamespaceSearchHook(namespaceId, false)) + finalPath = lappend_oid(finalPath, namespaceId); + else + *hook_filtered = true; + } + } + /* * 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 until the next call to this function. + * + * Invalidation takes two forms: The first is ordinary syscache invalidation + * clears the whole cache, and every entry must be rebuilt from the string + * form (finding OIDs from the namespace names and performing ACL checks). The + * second form of invalidation is when an object_access_hook is present, in + * which case we rebuild the entry by calling the hook for each OID in the + * oidlist, which is much faster than rebuilding from the string. + * + * We also determine if the new entry's finalPath (which is a list of OIDs) is equal + * to the previous one. It's more efficient to do so in this function, because + * we can avoid copying the previous list unless there is a syscache + * invalidation (in which case we must copy it before resetting + * SearchPathCacheContext). + */ +static SearchPathCacheEntry * +cachedNamespacePath(const char *searchPath, Oid roleid, bool *prevPathEqual) +{ + MemoryContext oldcxt; + SearchPathCacheKey cachekey; + SearchPathCacheEntry *entry; + bool found; + SearchPathCacheEntry *prevEntry = CurrentSearchPathCacheEntry; + List *prevPath = prevEntry ? prevEntry->finalPath : NIL; + List *prevPathCopy = NIL; + + /* clear the cache and start over when invalidated */ + if (!searchPathCacheValid) + { + /* temporarily save prevPath for comparison after reset */ + prevPathCopy = list_copy(prevPath); + prevPath = prevPathCopy; + + MemoryContextReset(SearchPathCacheContext); + + /* arbitrary initial starting size of 16 elements */ + SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL); + searchPathCacheValid = true; + prevEntry = NULL; + } + + if (prevEntry && roleid == prevEntry->key->roleid && + strcmp(searchPath, prevEntry->key->searchPath) == 0) + { + /* + * If search_path has changed and then changed back before + * recomputeNamespacePath is called, then the hash lookup will just + * find the very same entry that we already have. Optimize this case. + */ + entry = prevEntry; + found = true; + } + else + { + /* otherwise do a quick lookup */ + cachekey.searchPath = searchPath; + cachekey.roleid = roleid; + + entry = nsphash_insert(SearchPathCache, &cachekey, &found); + } + + /* initialize entry, though don't set finalPath (etc.) */ + if (!found) + { + SearchPathCacheKey *newkey; + + oldcxt = MemoryContextSwitchTo(SearchPathCacheContext); + + newkey = palloc(sizeof(SearchPathCacheKey)); + newkey->searchPath = pstrdup(searchPath); + newkey->roleid = roleid; + + entry->key = newkey; + entry->oidlist = preprocessNamespacePath(searchPath, roleid, + &entry->temp_missing); + entry->finalPath = finalNamespacePath(entry->oidlist, + &entry->firstNS, + &entry->hook_filtered); + MemoryContextSwitchTo(oldcxt); + } /* - * 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. + * If a hook is set now, or if a previously-set hook filtered out some + * namespaces from finalPath, we must re-finalize. */ - if (baseCreationNamespace == firstNS && - baseTempCreationPending == temp_missing && - equal(oidlist, baseSearchPath)) + if (found && (object_access_hook || entry->hook_filtered)) + { + /* + * Don't free the existing entry->finalPath yet, as prevPath may be an + * alias for it. Treat it as a copy that will be freed later. + */ + Assert(prevPathCopy == NIL); + prevPathCopy = entry->finalPath; + + oldcxt = MemoryContextSwitchTo(SearchPathCacheContext); + entry->finalPath = finalNamespacePath(entry->oidlist, + &entry->firstNS, + &entry->hook_filtered); + MemoryContextSwitchTo(oldcxt); + } + + *prevPathEqual = equal(prevPath, entry->finalPath); + list_free(prevPathCopy); + + CurrentSearchPathCacheEntry = entry; + + return entry; +} + +/* + * recomputeNamespacePath - recompute path derived variables if needed. + */ +static void +recomputeNamespacePath(void) +{ + Oid roleid = GetUserId(); + bool prevPathEqual; + bool pathChanged; + SearchPathCacheEntry *entry; + + /* Do nothing if path is already valid. */ + if (baseSearchPathValid && namespaceUser == roleid) + return; + + entry = cachedNamespacePath(namespace_search_path, + roleid, &prevPathEqual); + + if (baseCreationNamespace == entry->firstNS && + baseTempCreationPending == entry->temp_missing && + prevPathEqual) { pathChanged = false; } else { pathChanged = true; - - /* Must save OID list in permanent storage. */ - oldcxt = MemoryContextSwitchTo(TopMemoryContext); - newpath = list_copy(oidlist); - MemoryContextSwitchTo(oldcxt); - - /* Now safe to assign to state variables. */ - list_free(baseSearchPath); - baseSearchPath = newpath; - baseCreationNamespace = firstNS; - baseTempCreationPending = temp_missing; } + /* Now safe to assign to state variables. */ + baseSearchPath = entry->finalPath; + baseCreationNamespace = entry->firstNS; + baseTempCreationPending = entry->temp_missing; + /* Mark the path valid. */ baseSearchPathValid = true; namespaceUser = roleid; @@ -3791,11 +4002,6 @@ recomputeNamespacePath(void) */ if (pathChanged) activePathGeneration++; - - /* Clean up. */ - pfree(rawname); - list_free(namelist); - list_free(oidlist); } /* @@ -3950,6 +4156,7 @@ InitTempTableNamespace(void) myTempNamespaceSubID = GetCurrentSubTransactionId(); baseSearchPathValid = false; /* need to rebuild list */ + searchPathCacheValid = false; } /* @@ -3975,6 +4182,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 @@ -4016,6 +4224,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 @@ -4139,6 +4348,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; } @@ -4173,6 +4386,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 @@ -4186,6 +4403,7 @@ InitializeSearchPath(void) (Datum) 0); /* Force search path to be recomputed on next use */ baseSearchPathValid = false; + searchPathCacheValid = false; } } @@ -4196,8 +4414,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
