Minor cleanup for search path cache
От | Tom Lane |
---|---|
Тема | Minor cleanup for search path cache |
Дата | |
Msg-id | 3972900.1704145107@sss.pgh.pa.us обсуждение исходный текст |
Ответы |
Re: Minor cleanup for search path cache
Re: Minor cleanup for search path cache |
Список | pgsql-hackers |
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
В списке pgsql-hackers по дате отправления: