v1-0003-Add-cache-for-recomputeNamespacePath.patch

From 8152b3f279a6ca2f43c38633d4ff80afec0c6a8a Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Wed, 26 Jul 2023 12:55:09 -0700
Subject: [PATCH v1 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.
---
 src/backend/catalog/namespace.c               | 218 +++++++++++++++---
 .../expected/test_oat_hooks.out               |   6 -
 2 files changed, 183 insertions(+), 41 deletions(-)

diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index cb87f3385e..147b4308f9 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"
@@ -115,12 +116,14 @@
  * topmost stack entry, or 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 non-overridden lookup attempt.  Note that an
- * override spec is never subject to recomputation.
+ * 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 non-overridden
+ * lookup attempt.  Note that an override spec is never subject to
+ * recomputation.
  *
  * Any namespaces mentioned in namespace_search_path that are not readable
  * by the current user ID are simply left out of baseSearchPath; so
@@ -160,6 +163,23 @@ 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;
+	Oid					 firstNS;
+	bool				 temp_missing;
+	char				 status;
+} SearchPathCacheEntry;
 
 /* Override requests are remembered in a stack of OverrideStackEntry structs */
 
@@ -211,6 +231,44 @@ 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_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
@@ -3388,6 +3446,7 @@ SetTempNamespaceState(Oid tempNamespaceId, Oid tempToastNamespaceId)
 	 */
 
 	baseSearchPathValid = false;	/* may need to rebuild list */
+	searchPathCacheValid = false;
 }
 
 
@@ -3778,32 +3837,19 @@ FindDefaultConversionProc(int32 for_encoding, int32 to_encoding)
 }
 
 /*
- * recomputeNamespacePath - recompute path derived variables if needed.
+ * Full recomputation of search_path.
  */
-static void
-recomputeNamespacePath(void)
+static List *
+processNamespacePath(const char *searchPath, Oid roleid, Oid *firstNS,
+					 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 an override search spec is active. */
-	if (overrideStack)
-		return;
 
-	/* 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))
@@ -3820,7 +3866,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);
@@ -3860,7 +3906,7 @@ recomputeNamespacePath(void)
 			{
 				/* If it ought to be the creation namespace, set flag */
 				if (oidlist == NIL)
-					temp_missing = true;
+					*temp_missing = true;
 			}
 		}
 		else
@@ -3882,9 +3928,9 @@ recomputeNamespacePath(void)
 	 * explicit from implicit mention of pg_catalog.)
 	 */
 	if (oidlist == NIL)
-		firstNS = InvalidOid;
+		*firstNS = InvalidOid;
 	else
-		firstNS = linitial_oid(oidlist);
+		*firstNS = linitial_oid(oidlist);
 
 	/*
 	 * Add any implicitly-searched namespaces to the list.  Note these go on
@@ -3898,6 +3944,95 @@ recomputeNamespacePath(void)
 		!list_member_oid(oidlist, myTempNamespace))
 		oidlist = lcons_oid(myTempNamespace, oidlist);
 
+	pfree(rawname);
+	list_free(namelist);
+
+	return oidlist;
+}
+
+/*
+ * Retrieve previously-computed search path information from the cache; or if
+ * not present, fully recompute it and add it to the cache.
+ */
+static List *
+cachedNamespacePath(const char *searchPath, Oid roleid, Oid *pfirstNS,
+					bool *p_temp_missing)
+{
+	SearchPathCacheKey		 cachekey;
+	SearchPathCacheEntry	*entry;
+	bool					 found;
+
+	/* clear the cache and start over when invalidated */
+	if (!searchPathCacheValid)
+	{
+		MemoryContextReset(SearchPathCacheContext);
+		/* arbitrary initial starting size of 16 elements */
+		SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
+		searchPathCacheValid = true;
+	}
+
+	cachekey.searchPath = searchPath;
+	cachekey.roleid = roleid;
+
+	entry = nsphash_insert(SearchPathCache, &cachekey, &found);
+
+	if (!found)
+	{
+		MemoryContext		 oldcxt;
+		SearchPathCacheKey	*newkey;
+		List				*oidlist;
+		Oid					 firstNS;
+		bool				 temp_missing;
+
+		oidlist = processNamespacePath(searchPath, roleid, &firstNS,
+									   &temp_missing);
+
+		oldcxt = MemoryContextSwitchTo(SearchPathCacheContext);
+
+		newkey = palloc(sizeof(SearchPathCacheKey));
+		newkey->searchPath = pstrdup(searchPath);
+		newkey->roleid = roleid;
+
+		entry->key = newkey;
+		entry->oidlist = list_copy(oidlist);
+		entry->temp_missing = temp_missing;
+		entry->firstNS = firstNS;
+
+		MemoryContextSwitchTo(oldcxt);
+
+		/* oidlist already copied into cache entry */
+		list_free(oidlist);
+	}
+
+	*pfirstNS = entry->firstNS;
+	*p_temp_missing = entry->temp_missing;
+	return entry->oidlist;
+}
+
+/*
+ * recomputeNamespacePath - recompute path derived variables if needed.
+ */
+static void
+recomputeNamespacePath(void)
+{
+	Oid			roleid = GetUserId();
+	List	   *oidlist;
+	List	   *newpath;
+	bool		temp_missing;
+	Oid			firstNS;
+	bool		pathChanged;
+
+	/* Do nothing if an override search spec is active. */
+	if (overrideStack)
+		return;
+
+	/* Do nothing if path is already valid. */
+	if (baseSearchPathValid && namespaceUser == roleid)
+		return;
+
+	oidlist = cachedNamespacePath(namespace_search_path, roleid, &firstNS,
+								  &temp_missing);
+
 	/*
 	 * 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
@@ -3911,6 +4046,8 @@ recomputeNamespacePath(void)
 	}
 	else
 	{
+		MemoryContext oldcxt;
+
 		pathChanged = true;
 
 		/* Must save OID list in permanent storage. */
@@ -3943,11 +4080,6 @@ recomputeNamespacePath(void)
 	 */
 	if (pathChanged)
 		activePathGeneration++;
-
-	/* Clean up. */
-	pfree(rawname);
-	list_free(namelist);
-	list_free(oidlist);
 }
 
 /*
@@ -4102,6 +4234,7 @@ InitTempTableNamespace(void)
 	myTempNamespaceSubID = GetCurrentSubTransactionId();
 
 	baseSearchPathValid = false;	/* need to rebuild list */
+	searchPathCacheValid = false;
 }
 
 /*
@@ -4127,6 +4260,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
@@ -4192,6 +4326,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
@@ -4351,6 +4486,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;
 }
@@ -4385,6 +4524,9 @@ 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 rows.
@@ -4394,6 +4536,7 @@ InitializeSearchPath(void)
 									  (Datum) 0);
 		/* Force search path to be recomputed on next use */
 		baseSearchPathValid = false;
+		searchPathCacheValid = false;
 	}
 }
 
@@ -4404,8 +4547,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;
 }
 
 /*
diff --git a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
index f80373aecc..7f76ebecf9 100644
--- a/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
+++ b/src/test/modules/test_oat_hooks/expected/test_oat_hooks.out
@@ -249,12 +249,6 @@ NOTICE:  in process utility: superuser attempting SET
 NOTICE:  in object_access_hook_str: non-superuser attempting alter (subId=0x1000, set) [session_authorization]
 ERROR:  permission denied: set session_authorization
 SELECT * FROM regress_test_table;
-NOTICE:  in object access: superuser attempting namespace search (subId=0x0) [no report on violation, allowed]
-LINE 1: SELECT * FROM regress_test_table;
-                      ^
-NOTICE:  in object access: superuser finished namespace search (subId=0x0) [no report on violation, allowed]
-LINE 1: SELECT * FROM regress_test_table;
-                      ^
 NOTICE:  in executor check perms: superuser attempting execute
 NOTICE:  in executor check perms: superuser finished execute
  t 
-- 
2.34.1