Hi, After hearing from a couple of directions about systems spending too much time scanning the local lock hash table, I wrote the trivial patch to put them in a linked list, before learning that people have considered that before, so I should probably go and read some history on that and find out why it hasn't been done...
However, I noticed in passing that RemoveLocalLock() accesses *locallock after removing it from the hash table (in assertion builds only). So one question I have is whether it's actually a programming rule that you can't do that (at most you can compare the pointer against NULL), or whether it's supposed to be safe-if-you-know-what-you're-doing, as the existing comments hints. Here also is a patch that does wipe_mem on removed elements, as threatened last time this topic came up[1], which reveals the problem. I'm also not exactly sure why it's only a WARNING if your local lock table is out of sync, but perhaps that's in the archives too. [1] https://www.postgresql.org/message-id/flat/CAHut%2BPs-pL%2B%2Bf6CJwPx2%2BvUqXuew%3DXt-9Bi-6kCyxn%2BFwi2M7w%40mail.gmail.com
From d267c3c0cb26ea97616ca1184ab13664acb4feb8 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Tue, 11 May 2021 11:11:15 +1200 Subject: [PATCH 1/3] Clobber memory released by HASH_REMOVE. --- src/backend/utils/hash/dynahash.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c index 6546e3c7c7..b977d0395c 100644 --- a/src/backend/utils/hash/dynahash.c +++ b/src/backend/utils/hash/dynahash.c @@ -102,6 +102,7 @@ #include "storage/shmem.h" #include "storage/spin.h" #include "utils/dynahash.h" +#include "utils/memdebug.h" #include "utils/memutils.h" @@ -1076,10 +1077,12 @@ hash_search_with_hash_value(HTAB *hashp, SpinLockRelease(&hctl->freeList[freelist_idx].mutex); /* - * better hope the caller is synchronizing access to this - * element, because someone else is going to reuse it the next - * time something is added to the table + * Clobber the memory, to find bugs in code that accesses + * it after removing. */ +#ifdef CLOBBER_FREED_MEMORY + wipe_mem(ELEMENTKEY(currBucket), hctl->entrysize); +#endif return (void *) ELEMENTKEY(currBucket); } return NULL; -- 2.30.2
From 1cf44ab0f7bfc9d5221ce19f162b3e07ab42c567 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Tue, 11 May 2021 11:11:37 +1200 Subject: [PATCH 2/3] Don't access a local locks after freeing them. --- src/backend/storage/lmgr/lock.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c index 108b4d9023..8642afc275 100644 --- a/src/backend/storage/lmgr/lock.c +++ b/src/backend/storage/lmgr/lock.c @@ -1390,15 +1390,15 @@ RemoveLocalLock(LOCALLOCK *locallock) SpinLockRelease(&FastPathStrongRelationLocks->mutex); } - if (!hash_search(LockMethodLocalHash, - (void *) &(locallock->tag), - HASH_REMOVE, NULL)) - elog(WARNING, "locallock table corrupted"); - /* * Indicate that the lock is released for certain types of locks */ CheckAndSetLockHeld(locallock, false); + + if (!hash_search(LockMethodLocalHash, + (void *) &(locallock->tag), + HASH_REMOVE, NULL)) + elog(WARNING, "locallock table corrupted"); } /* -- 2.30.2
