Minor cleanup for search path cache

Started by Tom Laneabout 2 years ago5 messages
#1Tom Lane
tgl@sss.pgh.pa.us
1 attachment(s)

I happened to notice that there is a not-quite-theoretical crash
hazard in spcache_init(). If we see that SPCACHE_RESET_THRESHOLD
is exceeded and decide to reset the cache, but then nsphash_create
fails for some reason (perhaps OOM), an error will be thrown
leaving the SearchPathCache pointer pointing at already-freed
memory. Next time through, we'll try to dereference that dangling
pointer, potentially causing SIGSEGV, or worse we might find a
value less than SPCACHE_RESET_THRESHOLD and decide that the cache
is okay despite having been freed.

The fix of course is to make sure we reset the pointer variables
*before* the MemoryContextReset.

I also observed that the code seems to have been run through
pgindent without fixing typedefs.list, making various places
uglier than they should be.

The attached proposed cleanup patch fixes those things and in
passing improves (IMO anyway) some comments. I assume it wasn't
intentional to leave two copies of the same comment block in
check_search_path().

regards, tom lane

Attachments:

minor-search-path-cache-cleanup.patchtext/x-diff; charset=us-ascii; name=minor-search-path-cache-cleanup.patchDownload
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 37a69e9023..465a4e40bc 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -156,6 +156,11 @@ static Oid	namespaceUser = InvalidOid;
 
 /* The above four values are valid only if baseSearchPathValid */
 static bool baseSearchPathValid = true;
+
+/*
+ * Storage for search path cache.  Clear searchPathCacheValid as a simple
+ * way to invalidate *all* the cache entries, not just the active one.
+ */
 static bool searchPathCacheValid = false;
 static MemoryContext SearchPathCacheContext = NULL;
 
@@ -163,7 +168,7 @@ typedef struct SearchPathCacheKey
 {
 	const char *searchPath;
 	Oid			roleid;
-}			SearchPathCacheKey;
+} SearchPathCacheKey;
 
 typedef struct SearchPathCacheEntry
 {
@@ -176,7 +181,7 @@ typedef struct SearchPathCacheEntry
 
 	/* needed for simplehash */
 	char		status;
-}			SearchPathCacheEntry;
+} SearchPathCacheEntry;
 
 /*
  * myTempNamespace is InvalidOid until and unless a TEMP namespace is set up
@@ -281,8 +286,8 @@ spcachekey_equal(SearchPathCacheKey a, SearchPathCacheKey b)
  */
 #define SPCACHE_RESET_THRESHOLD		256
 
-static nsphash_hash * SearchPathCache = NULL;
-static SearchPathCacheEntry * LastSearchPathCacheEntry = NULL;
+static nsphash_hash *SearchPathCache = NULL;
+static SearchPathCacheEntry *LastSearchPathCacheEntry = NULL;
 
 /*
  * Create or reset search_path cache as necessary.
@@ -296,8 +301,11 @@ spcache_init(void)
 		SearchPathCache->members < SPCACHE_RESET_THRESHOLD)
 		return;
 
-	MemoryContextReset(SearchPathCacheContext);
+	/* make sure we don't leave dangling pointers if nsphash_create fails */
+	SearchPathCache = NULL;
 	LastSearchPathCacheEntry = NULL;
+
+	MemoryContextReset(SearchPathCacheContext);
 	/* arbitrary initial starting size of 16 elements */
 	SearchPathCache = nsphash_create(SearchPathCacheContext, 16, NULL);
 	searchPathCacheValid = true;
@@ -4267,7 +4275,7 @@ recomputeNamespacePath(void)
 {
 	Oid			roleid = GetUserId();
 	bool		pathChanged;
-	const		SearchPathCacheEntry *entry;
+	const SearchPathCacheEntry *entry;
 
 	/* Do nothing if path is already valid. */
 	if (baseSearchPathValid && namespaceUser == roleid)
@@ -4635,9 +4643,7 @@ check_search_path(char **newval, void **extra, GucSource source)
 	 * 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.
@@ -4667,19 +4673,10 @@ check_search_path(char **newval, void **extra, GucSource source)
 		list_free(namelist);
 		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);
 
-	/* create empty cache entry */
+	/* OK to create empty cache entry */
 	if (use_cache)
 		(void) spcache_insert(searchPath, roleid);
 
@@ -4732,8 +4729,9 @@ InitializeSearchPath(void)
 	}
 	else
 	{
-		SearchPathCacheContext = AllocSetContextCreate(
-													   TopMemoryContext, "search_path processing cache",
+		/* Make the context we'll keep search path cache hashtable in */
+		SearchPathCacheContext = AllocSetContextCreate(TopMemoryContext,
+													   "search_path processing cache",
 													   ALLOCSET_DEFAULT_SIZES);
 
 		/*
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 3310726cdd..cf8c59a411 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2479,6 +2479,8 @@ ScanState
 ScanTypeControl
 ScannerCallbackState
 SchemaQuery
+SearchPathCacheEntry
+SearchPathCacheKey
 SearchPathMatcher
 SecBuffer
 SecBufferDesc
@@ -3513,6 +3515,7 @@ needs_fmgr_hook_type
 network_sortsupport_state
 nodeitem
 normal_rand_fctx
+nsphash_hash
 ntile_context
 numeric
 object_access_hook_type
#2Zhang Mingli
zmlpostgres@gmail.com
In reply to: Tom Lane (#1)
Re: Minor cleanup for search path cache

Hi,

Zhang Mingli
www.hashdata.xyz
On Jan 2, 2024 at 05:38 +0800, Tom Lane <tgl@sss.pgh.pa.us>, wrote:

I happened to notice that there is a not-quite-theoretical crash
hazard in spcache_init(). If we see that SPCACHE_RESET_THRESHOLD
is exceeded and decide to reset the cache, but then nsphash_create
fails for some reason (perhaps OOM), an error will be thrown
leaving the SearchPathCache pointer pointing at already-freed
memory. Next time through, we'll try to dereference that dangling
pointer, potentially causing SIGSEGV, or worse we might find a
value less than SPCACHE_RESET_THRESHOLD and decide that the cache
is okay despite having been freed.

The fix of course is to make sure we reset the pointer variables
*before* the MemoryContextReset.

I also observed that the code seems to have been run through
pgindent without fixing typedefs.list, making various places
uglier than they should be.

The attached proposed cleanup patch fixes those things and in
passing improves (IMO anyway) some comments. I assume it wasn't
intentional to leave two copies of the same comment block in
check_search_path().

regards, tom lane

Only me?

zml@localhashdata postgres % git apply minor-search-path-cache-cleanup.patch
error: patch failed: src/backend/catalog/namespace.c:156
error: src/backend/catalog/namespace.c: patch does not apply
error: patch failed: src/tools/pgindent/typedefs.list:2479
error: src/tools/pgindent/typedefs.list: patch does not apply

I’m in commit 9a17be1e24 Allow upgrades to preserve the full subscription's state

#3Tom Lane
tgl@sss.pgh.pa.us
In reply to: Zhang Mingli (#2)
Re: Minor cleanup for search path cache

Zhang Mingli <zmlpostgres@gmail.com> writes:

Only me?

zml@localhashdata postgres % git apply minor-search-path-cache-cleanup.patch
error: patch failed: src/backend/catalog/namespace.c:156
error: src/backend/catalog/namespace.c: patch does not apply
error: patch failed: src/tools/pgindent/typedefs.list:2479
error: src/tools/pgindent/typedefs.list: patch does not apply

Use patch(1). git-apply is extremely fragile.

regards, tom lane

#4Jeff Davis
pgsql@j-davis.com
In reply to: Tom Lane (#1)
Re: Minor cleanup for search path cache

On Mon, 2024-01-01 at 16:38 -0500, Tom Lane wrote:

I happened to notice that there is a not-quite-theoretical crash
hazard in spcache_init().  If we see that SPCACHE_RESET_THRESHOLD
is exceeded and decide to reset the cache, but then nsphash_create
fails for some reason (perhaps OOM), an error will be thrown
leaving the SearchPathCache pointer pointing at already-freed
memory.

Good catch, thank you. I tried to avoid OOM hazards (e.g. b282fa88df,
8efa301532), but I missed this one.

I also observed that the code seems to have been run through
pgindent without fixing typedefs.list, making various places
uglier than they should be.

The attached proposed cleanup patch fixes those things and in
passing improves (IMO anyway) some comments.  I assume it wasn't
intentional to leave two copies of the same comment block in
check_search_path().

Looks good to me.

Regards,
Jeff Davis

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Jeff Davis (#4)
Re: Minor cleanup for search path cache

Jeff Davis <pgsql@j-davis.com> writes:

Looks good to me.

Thanks for reviewing, will push shortly.

regards, tom lane