MDEV-30134 Assertion failed in buf_page_t::unfix() in buf_pool_t::watch_unset()

buf_pool_t::watch_set(): Always buffer-fix a block if one was found,
no matter if it is a watch sentinel or a buffer page. The type of
the block descriptor will be rechecked in buf_page_t::watch_unset().
Do not expect the caller to acquire the page hash latch. Starting with
commit bd5a6403cace36c6ed428cde62e35adcd3f7e7d0 it is safe to release
buf_pool.mutex before acquiring a buf_pool.page_hash latch.

buf_page_get_low(): Adjust to the changed buf_pool_t::watch_set().

This simplifies the logic and fixes a bug that was reproduced when
using debug builds and the setting innodb_change_buffering_debug=1.

2 files changed, 33 insertions, 50 deletions

diff --git a/storage/innobase/buf/buf0buf.cc b/storage/innobase/buf/buf0buf.cc
index b17c49ab751..47cc915a11d 100644
--- a/storage/innobase/buf/buf0buf.cc
+++ b/storage/innobase/buf/buf0buf.cc
@@ -1951,28 +1951,22 @@ static void buf_relocate(buf_page_t *bpage, buf_page_t *dpage)
   buf_pool.page_hash.replace(chain, bpage, dpage);
 }
 
-/** Register a watch for a page identifier. The caller must hold an
-exclusive page hash latch. The *hash_lock may be released,
-relocated, and reacquired.
-@param id         page identifier
-@param chain      hash table chain with exclusively held page_hash
-@return a buffer pool block corresponding to id
-@retval nullptr   if the block was not present, and a watch was installed */
-inline buf_page_t *buf_pool_t::watch_set(const page_id_t id,
-                                         buf_pool_t::hash_chain &chain)
+buf_page_t *buf_pool_t::watch_set(const page_id_t id,
+                                  buf_pool_t::hash_chain &chain)
 {
   ut_ad(&chain == &page_hash.cell_get(id.fold()));
-  ut_ad(page_hash.lock_get(chain).is_write_locked());
+  page_hash.lock_get(chain).lock();
 
-retry:
-  if (buf_page_t *bpage= page_hash.get(id, chain))
+  buf_page_t *bpage= page_hash.get(id, chain);
+
+  if (bpage)
   {
-    if (!watch_is_sentinel(*bpage))
-      /* The page was loaded meanwhile. */
-      return bpage;
-    /* Add to an existing watch. */
+got_block:
     bpage->fix();
-    return nullptr;
+    if (watch_is_sentinel(*bpage))
+      bpage= nullptr;
+    page_hash.lock_get(chain).unlock();
+    return bpage;
   }
 
   page_hash.lock_get(chain).unlock();
@@ -2001,25 +1995,23 @@ retry:
     w->set_state(buf_page_t::UNFIXED + 1);
     w->id_= id;
 
-    buf_page_t *bpage= page_hash.get(id, chain);
+    page_hash.lock_get(chain).lock();
+    bpage= page_hash.get(id, chain);
     if (UNIV_LIKELY_NULL(bpage))
     {
       w->set_state(buf_page_t::NOT_USED);
-      page_hash.lock_get(chain).lock();
       mysql_mutex_unlock(&mutex);
-      goto retry;
+      goto got_block;
     }
 
-    page_hash.lock_get(chain).lock();
     ut_ad(w->state() == buf_page_t::UNFIXED + 1);
     buf_pool.page_hash.append(chain, w);
     mysql_mutex_unlock(&mutex);
+    page_hash.lock_get(chain).unlock();
     return nullptr;
   }
 
   ut_error;
-  mysql_mutex_unlock(&mutex);
-  return nullptr;
 }
 
 /** Stop watching whether a page has been read in.
@@ -2467,16 +2459,13 @@ loop:
 	case BUF_PEEK_IF_IN_POOL:
 		return nullptr;
 	case BUF_GET_IF_IN_POOL_OR_WATCH:
-		/* We cannot easily use a memory transaction here. */
-		hash_lock.lock();
+		/* Buffer-fixing inside watch_set() will prevent eviction */
 		block = reinterpret_cast<buf_block_t*>
 			(buf_pool.watch_set(page_id, chain));
-		/* buffer-fixing will prevent eviction */
-		state = block ? block->page.fix() : 0;
-		hash_lock.unlock();
 
 		if (block) {
-			goto got_block;
+			state = block->page.state();
+			goto got_block_fixed;
 		}
 
 		return nullptr;
@@ -2515,6 +2504,7 @@ loop:
 got_block:
 	ut_ad(!block->page.in_zip_hash);
 	state++;
+got_block_fixed:
 	ut_ad(state > buf_page_t::FREED);
 
 	if (state > buf_page_t::READ_FIX && state < buf_page_t::WRITE_FIX) {
@@ -2724,24 +2714,19 @@ re_evict:
 		const bool evicted = buf_LRU_free_page(&block->page, true);
 		space->release();
 
+		if (!evicted) {
+			block->fix();
+		}
+
+		mysql_mutex_unlock(&buf_pool.mutex);
+
 		if (evicted) {
-			page_hash_latch& hash_lock
-				= buf_pool.page_hash.lock_get(chain);
-			hash_lock.lock();
-			mysql_mutex_unlock(&buf_pool.mutex);
-			/* We may set the watch, as it would have
-			been set if the page were not in the
-			buffer pool in the first place. */
-			block= reinterpret_cast<buf_block_t*>(
-				mode == BUF_GET_IF_IN_POOL_OR_WATCH
-				? buf_pool.watch_set(page_id, chain)
-				: buf_pool.page_hash.get(page_id, chain));
-			hash_lock.unlock();
+			if (mode == BUF_GET_IF_IN_POOL_OR_WATCH) {
+				buf_pool.watch_set(page_id, chain);
+			}
 			return(NULL);
 		}
 
-		block->fix();
-		mysql_mutex_unlock(&buf_pool.mutex);
 		buf_flush_sync();
 
 		state = block->page.state();
diff --git a/storage/innobase/include/buf0buf.h b/storage/innobase/include/buf0buf.h
index b4d8c8b76ac..2dccdda8a2f 100644
--- a/storage/innobase/include/buf0buf.h
+++ b/storage/innobase/include/buf0buf.h
@@ -1470,14 +1470,12 @@ public:
     return !watch_is_sentinel(*page_hash.get(id, chain));
   }
 
-  /** Register a watch for a page identifier. The caller must hold an
-  exclusive page hash latch. The *hash_lock may be released,
-  relocated, and reacquired.
+  /** Register a watch for a page identifier.
   @param id         page identifier
-  @param chain      hash table chain with exclusively held page_hash
-  @return a buffer pool block corresponding to id
-  @retval nullptr   if the block was not present, and a watch was installed */
-  inline buf_page_t *watch_set(const page_id_t id, hash_chain &chain);
+  @param chain      page_hash.cell_get(id.fold())
+  @return a buffer page corresponding to id
+  @retval nullptr   if the block was not present in page_hash */
+  buf_page_t *watch_set(const page_id_t id, hash_chain &chain);
 
   /** Stop watching whether a page has been read in.
   watch_set(id) must have returned nullptr before.