This is an automated email from the ASF dual-hosted git repository. maxyang pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit 1bf774ef22af1afe7a592887563218a724b710ec Author: Thomas Munro <[email protected]> AuthorDate: Mon Jul 11 14:47:16 2022 +1200 Fix lock assertions in dshash.c. dshash.c previously maintained flags to be able to assert that you didn't hold any partition lock. These flags could get out of sync with reality in error scenarios. Get rid of all that, and make assertions about the locks themselves instead. Since LWLockHeldByMe() loops internally, we don't want to put that inside another loop over all partition locks. Introduce a new debugging-only interface LWLockAnyHeldByMe() to avoid that. This problem was noted by Tom and Andres while reviewing changes to support the new shared memory stats system, and later showed up in reality while working on commit 389869af. Back-patch to 11, where dshash.c arrived. Reported-by: Tom Lane <[email protected]> Reported-by: Andres Freund <[email protected]> Reviewed-by: Kyotaro HORIGUCHI <[email protected]> Reviewed-by: Zhihong Yu <[email protected]> Reviewed-by: Andres Freund <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/CA%2BhUKGJ31Wce6HJ7xnVTKWjFUWQZPBngxfJVx4q0E98pDr3kAw%40mail.gmail.com --- src/backend/lib/dshash.c | 35 +++++++++-------------------------- src/backend/storage/lmgr/lwlock.c | 26 ++++++++++++++++++++++++++ src/include/storage/lwlock.h | 1 + 3 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 88ca9d62aa..99128b223a 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. */ @@ -186,6 +184,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 @@ -226,9 +228,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. @@ -277,8 +276,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); /* @@ -301,7 +298,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); @@ -392,7 +389,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); @@ -410,8 +407,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); } } @@ -441,7 +436,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), @@ -486,8 +481,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); } @@ -506,7 +499,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); @@ -543,14 +536,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)); } @@ -564,13 +553,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)); } @@ -603,7 +586,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 866b7ae95f..06d97fd38d 100644 --- a/src/backend/storage/lmgr/lwlock.c +++ b/src/backend/storage/lmgr/lwlock.c @@ -1934,6 +1934,32 @@ LWLockHeldByMe(LWLock *l) return false; } +/* + * LWLockHeldByMe - test whether my process holds any of an array of locks + * + * 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 06f6e9f81a..eaffe82d7c 100644 --- a/src/include/storage/lwlock.h +++ b/src/include/storage/lwlock.h @@ -123,6 +123,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 bool LWLockWaitForVar(LWLock *lock, uint64 *valptr, uint64 oldval, uint64 *newval); --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
