Thread: Minor cleanup for search path cache

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

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

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

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

Jeff Davis <pgsql@j-davis.com> writes:
> Looks good to me.

Thanks for reviewing, will push shortly.

            regards, tom lane