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

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