On Tue, Jul 5, 2022 at 11:25 AM Andres Freund <and...@anarazel.de> wrote: > On 2022-07-05 11:20:54 +1200, Thomas Munro wrote: > > Since there were 6 places with I-hold-no-lock assertions, I shoved the > > loop into a function so I could do: > > > > - Assert(!status->hash_table->find_locked); > > + assert_no_lock_held_by_me(hash_table); > > I am a *bit* wary about the costs of that, even in assert builds - each of the > partition checks in the loop will in turn need to iterate through > held_lwlocks. But I guess we can also just later weaken them if it turns out > to be a problem.
Maybe we should add assertion support for arrays of locks, so we don't need two levels of loop? Something like the attached?
From d98874ab15fc568b3da8a6395e0a0ef02855a880 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Mon, 4 Jul 2022 11:56:02 +1200 Subject: [PATCH v3] Fix lock assertions in dshash.c. dshash.c maintained flags to track locking, in order to make assertions that the dshash API was being used correctly. Unfortunately the interaction with ereport's non-local exits was not thought through carefully enough and the flag could get out of sync with reality. Since these flags were primarily used for assertions that no lock was held, we can get most of the intended effect by making an assertion about the partition locks directly. To avoid N*M complexity while asserting that we hold no partition locks, add a new debugging-only interface LWLockAnyHeldByMe(), which can check if any lock in an array is held without the need for a loop within a loop. This problem was noted by Tom and Andres while reviewing changes to support the new shared memory stats system in release 15, the second user of dshash.c. On closer inspection, even the earlier typcache.c code was not guaranteed to meet the asserted conditions, now that it's been highlighted that even dsa_get_address() can throw (albeit in unlikely circumstances). Back-patch to 11, where dshash.c arrived. Reported-by: Tom Lane <t...@sss.pgh.pa.us> Reported-by: Andres Freund <and...@anarazel.de> Reviewed-by: Kyotaro HORIGUCHI <horiguchi.kyot...@lab.ntt.co.jp> Reviewed-by: Zhihong Yu <z...@yugabyte.com> Reviewed-by: Andres Freund <and...@anarazel.de> Discussion: https://postgr.es/m/20220311012712.botrpsikaufzt...@alap3.anarazel.de --- src/backend/lib/dshash.c | 44 +++++++------------------------ src/backend/storage/lmgr/lwlock.c | 26 ++++++++++++++++++ src/include/storage/lwlock.h | 1 + 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index ec454b4d65..c5c032a593 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -110,8 +110,6 @@ struct dshash_table dshash_table_control *control; /* Control object in DSM. */ dsa_pointer *buckets; /* Current bucket pointers in DSM. */ size_t size_log2; /* log2(number of buckets) */ - bool find_locked; /* Is any partition lock held by 'find'? */ - bool find_exclusively_locked; /* ... exclusively? */ }; /* Given a pointer to an item, find the entry (user data) it holds. */ @@ -194,6 +192,10 @@ static inline bool equal_keys(dshash_table *hash_table, #define PARTITION_LOCK(hash_table, i) \ (&(hash_table)->control->partitions[(i)].lock) +#define ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table) \ + Assert(!LWLockAnyHeldByMe(&(hash_table)->control->partitions[0].lock, \ + DSHASH_NUM_PARTITIONS, sizeof(dshash_partition))) + /* * Create a new hash table backed by the given dynamic shared area, with the * given parameters. The returned object is allocated in backend-local memory @@ -234,9 +236,6 @@ dshash_create(dsa_area *area, const dshash_parameters *params, void *arg) } } - hash_table->find_locked = false; - hash_table->find_exclusively_locked = false; - /* * Set up the initial array of buckets. Our initial size is the same as * the number of partitions. @@ -285,8 +284,6 @@ dshash_attach(dsa_area *area, const dshash_parameters *params, hash_table->params = *params; hash_table->arg = arg; hash_table->control = dsa_get_address(area, control); - hash_table->find_locked = false; - hash_table->find_exclusively_locked = false; Assert(hash_table->control->magic == DSHASH_MAGIC); /* @@ -309,7 +306,7 @@ dshash_attach(dsa_area *area, const dshash_parameters *params, void dshash_detach(dshash_table *hash_table) { - Assert(!hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table); /* The hash table may have been destroyed. Just free local memory. */ pfree(hash_table); @@ -400,7 +397,7 @@ dshash_find(dshash_table *hash_table, const void *key, bool exclusive) partition = PARTITION_FOR_HASH(hash); Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table); LWLockAcquire(PARTITION_LOCK(hash_table, partition), exclusive ? LW_EXCLUSIVE : LW_SHARED); @@ -418,8 +415,6 @@ dshash_find(dshash_table *hash_table, const void *key, bool exclusive) else { /* The caller will free the lock by calling dshash_release_lock. */ - hash_table->find_locked = true; - hash_table->find_exclusively_locked = exclusive; return ENTRY_FROM_ITEM(item); } } @@ -449,7 +444,7 @@ dshash_find_or_insert(dshash_table *hash_table, partition = &hash_table->control->partitions[partition_index]; Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table); restart: LWLockAcquire(PARTITION_LOCK(hash_table, partition_index), @@ -494,8 +489,6 @@ restart: } /* The caller must release the lock with dshash_release_lock. */ - hash_table->find_locked = true; - hash_table->find_exclusively_locked = true; return ENTRY_FROM_ITEM(item); } @@ -514,7 +507,7 @@ dshash_delete_key(dshash_table *hash_table, const void *key) bool found; Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table); hash = hash_key(hash_table, key); partition = PARTITION_FOR_HASH(hash); @@ -551,14 +544,10 @@ dshash_delete_entry(dshash_table *hash_table, void *entry) size_t partition = PARTITION_FOR_HASH(item->hash); Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(hash_table->find_locked); - Assert(hash_table->find_exclusively_locked); Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition), LW_EXCLUSIVE)); delete_item(hash_table, item); - hash_table->find_locked = false; - hash_table->find_exclusively_locked = false; LWLockRelease(PARTITION_LOCK(hash_table, partition)); } @@ -572,13 +561,7 @@ dshash_release_lock(dshash_table *hash_table, void *entry) size_t partition_index = PARTITION_FOR_HASH(item->hash); Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(hash_table->find_locked); - Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition_index), - hash_table->find_exclusively_locked - ? LW_EXCLUSIVE : LW_SHARED)); - hash_table->find_locked = false; - hash_table->find_exclusively_locked = false; LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); } @@ -644,7 +627,7 @@ dshash_seq_next(dshash_seq_status *status) if (status->curpartition == -1) { Assert(status->curbucket == 0); - Assert(!status->hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(status->hash_table); status->curpartition = 0; @@ -702,8 +685,6 @@ dshash_seq_next(dshash_seq_status *status) status->curitem = dsa_get_address(status->hash_table->area, next_item_pointer); - status->hash_table->find_locked = true; - status->hash_table->find_exclusively_locked = status->exclusive; /* * The caller may delete the item. Store the next item in case of @@ -722,9 +703,6 @@ dshash_seq_next(dshash_seq_status *status) void dshash_seq_term(dshash_seq_status *status) { - status->hash_table->find_locked = false; - status->hash_table->find_exclusively_locked = false; - if (status->curpartition >= 0) LWLockRelease(PARTITION_LOCK(status->hash_table, status->curpartition)); } @@ -743,8 +721,6 @@ dshash_delete_current(dshash_seq_status *status) Assert(status->exclusive); Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(hash_table->find_locked); - Assert(hash_table->find_exclusively_locked); Assert(LWLockHeldByMeInMode(PARTITION_LOCK(hash_table, partition), LW_EXCLUSIVE)); @@ -762,7 +738,7 @@ dshash_dump(dshash_table *hash_table) size_t j; Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + ASSERT_NO_PARTITION_LOCKS_HELD_BY_ME(hash_table); for (i = 0; i < DSHASH_NUM_PARTITIONS; ++i) { diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c index 8aef909037..1ebb6c7464 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -1925,6 +1925,32 @@ LWLockHeldByMe(LWLock *l) return false; } +/* + * LWLockHeldByMe - test whether my process holds any of n locks in any mode + * + * This is meant as debug support only. + */ +bool +LWLockAnyHeldByMe(LWLock *l, int nlocks, size_t stride) +{ + char *held_lock_addr; + char *begin; + char *end; + int i; + + begin = (char *) l; + end = begin + nlocks * stride; + for (i = 0; i < num_held_lwlocks; i++) + { + held_lock_addr = (char *) held_lwlocks[i].lock; + if (held_lock_addr >= begin && + held_lock_addr < end && + (held_lock_addr - begin) % stride == 0) + return true; + } + return false; +} + /* * LWLockHeldByMeInMode - test whether my process holds a lock in given mode * diff --git a/src/include/storage/lwlock.h b/src/include/storage/lwlock.h index e8c91139f8..aeddd80987 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -120,6 +120,7 @@ extern void LWLockRelease(LWLock *lock); extern void LWLockReleaseClearVar(LWLock *lock, uint64 *valptr, uint64 val); extern void LWLockReleaseAll(void); extern bool LWLockHeldByMe(LWLock *lock); +extern bool LWLockAnyHeldByMe(LWLock *lock, int nlocks, size_t stride); extern bool LWLockHeldByMeInMode(LWLock *lock, LWLockMode mode); extern int LWLockHeldCount(void); -- 2.36.1
