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 <tgl@sss.pgh.pa.us>
Reported-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
Reviewed-by: Zhihong Yu <zyu@yugabyte.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Discussion: https://postgr.es/m/20220311012712.botrpsikaufzteyt@alap3.anarazel.de
Discussion: https://postgr.es/m/CA%2BhUKGJ31Wce6HJ7xnVTKWjFUWQZPBngxfJVx4q0E98pDr3kAw%40mail.gmail.com

1 files changed, 10 insertions, 34 deletions

diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c
index ec454b4d655..c5c032a593b 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)
 	{