MDEV-22110 InnoDB unnecessarily writes unmodified pages

At least since commit 6a7be48b1b0b4107bf6991240ae69fcec0b7189a
InnoDB appears to be invoking buf_flush_note_modification() on pages
that were exclusively latched but not modified in a mini-transaction.

MTR_MEMO_MODIFY, mtr_t::modify(): Define not only in debug code,
but also in release code. We will set the MTR_MEMO_MODIFY flag
on the earliest mtr_t::m_memo entry that we find.

MTR_LOG_NONE: Only use this mode in cases where the previous
mode will be restored before anything is modified in the mini-transaction.

MTR_MEMO_PAGE_X_MODIFY, MTR_MEMO_PAGE_SX_MODIFY: The allowed flag
combinations that include MTR_MEMO_MODIFY.

ReleaseBlocks: Only invoke buf_flush_note_modification()
on those buffer pool blocks on which mtr_t::set_modified()
and mtr_t::modify() were invoked.

1 files changed, 94 insertions, 84 deletions

diff --git a/storage/innobase/mtr/mtr0mtr.cc b/storage/innobase/mtr/mtr0mtr.cc
index 8a542a9f842..94a88bd3f83 100644
--- a/storage/innobase/mtr/mtr0mtr.cc
+++ b/storage/innobase/mtr/mtr0mtr.cc
@@ -205,13 +205,6 @@ private:
 static void memo_slot_release(mtr_memo_slot_t *slot)
 {
   switch (slot->type) {
-#ifdef UNIV_DEBUG
-  default:
-    ut_ad("invalid type" == 0);
-    break;
-  case MTR_MEMO_MODIFY:
-    break;
-#endif /* UNIV_DEBUG */
   case MTR_MEMO_S_LOCK:
     rw_lock_s_unlock(reinterpret_cast<rw_lock_t*>(slot->object));
     break;
@@ -228,12 +221,21 @@ static void memo_slot_release(mtr_memo_slot_t *slot)
   case MTR_MEMO_X_LOCK:
     rw_lock_x_unlock(reinterpret_cast<rw_lock_t*>(slot->object));
     break;
-  case MTR_MEMO_BUF_FIX:
-  case MTR_MEMO_PAGE_S_FIX:
-  case MTR_MEMO_PAGE_SX_FIX:
-  case MTR_MEMO_PAGE_X_FIX:
+  default:
+#ifdef UNIV_DEBUG
+    switch (slot->type & ~MTR_MEMO_MODIFY) {
+    case MTR_MEMO_BUF_FIX:
+    case MTR_MEMO_PAGE_S_FIX:
+    case MTR_MEMO_PAGE_SX_FIX:
+    case MTR_MEMO_PAGE_X_FIX:
+      break;
+    default:
+      ut_ad("invalid type" == 0);
+      break;
+    }
+#endif /* UNIV_DEBUG */
     buf_block_t *block= reinterpret_cast<buf_block_t*>(slot->object);
-    buf_page_release_latch(block, slot->type);
+    buf_page_release_latch(block, slot->type & ~MTR_MEMO_MODIFY);
     block->unfix();
     break;
   }
@@ -248,13 +250,6 @@ struct ReleaseLatches {
     if (!slot->object)
       return true;
     switch (slot->type) {
-#ifdef UNIV_DEBUG
-    default:
-      ut_ad("invalid type" == 0);
-      break;
-    case MTR_MEMO_MODIFY:
-      break;
-#endif /* UNIV_DEBUG */
     case MTR_MEMO_S_LOCK:
       rw_lock_s_unlock(reinterpret_cast<rw_lock_t*>(slot->object));
       break;
@@ -271,12 +266,21 @@ struct ReleaseLatches {
     case MTR_MEMO_SX_LOCK:
       rw_lock_sx_unlock(reinterpret_cast<rw_lock_t*>(slot->object));
       break;
-    case MTR_MEMO_BUF_FIX:
-    case MTR_MEMO_PAGE_S_FIX:
-    case MTR_MEMO_PAGE_SX_FIX:
-    case MTR_MEMO_PAGE_X_FIX:
+    default:
+#ifdef UNIV_DEBUG
+      switch (slot->type & ~MTR_MEMO_MODIFY) {
+      case MTR_MEMO_BUF_FIX:
+      case MTR_MEMO_PAGE_S_FIX:
+      case MTR_MEMO_PAGE_SX_FIX:
+      case MTR_MEMO_PAGE_X_FIX:
+        break;
+      default:
+        ut_ad("invalid type" == 0);
+        break;
+      }
+#endif /* UNIV_DEBUG */
       buf_block_t *block= reinterpret_cast<buf_block_t*>(slot->object);
-      buf_page_release_latch(block, slot->type);
+      buf_page_release_latch(block, slot->type & ~MTR_MEMO_MODIFY);
       block->unfix();
       break;
     }
@@ -308,50 +312,42 @@ struct DebugCheck {
 };
 #endif
 
-/** Release a resource acquired by the mini-transaction. */
-struct ReleaseBlocks {
-	/** Release specific object */
-	ReleaseBlocks(lsn_t start_lsn, lsn_t end_lsn)
-		:
-		m_end_lsn(end_lsn),
-		m_start_lsn(start_lsn)
-	{
-		/* Do nothing */
-	}
-
-	/** Add the modified page to the buffer flush list. */
-	void add_dirty_page_to_flush_list(mtr_memo_slot_t* slot) const
-	{
-		ut_ad(m_end_lsn > 0);
-		ut_ad(m_start_lsn > 0);
-
-		buf_block_t*	block;
-
-		block = reinterpret_cast<buf_block_t*>(slot->object);
-
-		buf_flush_note_modification(block, m_start_lsn, m_end_lsn);
-	}
-
-	/** @return true always. */
-	bool operator()(mtr_memo_slot_t* slot) const
-	{
-		if (slot->object != NULL) {
-
-			if (slot->type == MTR_MEMO_PAGE_X_FIX
-			    || slot->type == MTR_MEMO_PAGE_SX_FIX) {
-
-				add_dirty_page_to_flush_list(slot);
-			}
-		}
+/** Release page latches held by the mini-transaction. */
+struct ReleaseBlocks
+{
+  const lsn_t start, end;
+#ifdef UNIV_DEBUG
+  const mtr_buf_t &memo;
 
-		return(true);
-	}
+  ReleaseBlocks(lsn_t start, lsn_t end, const mtr_buf_t &memo) :
+    start(start), end(end), memo(memo)
+#else /* UNIV_DEBUG */
+  ReleaseBlocks(lsn_t start, lsn_t end, const mtr_buf_t&) :
+    start(start), end(end)
+#endif /* UNIV_DEBUG */
+  {
+    ut_ad(start);
+    ut_ad(end);
+  }
 
-	/** Mini-transaction REDO start LSN */
-	lsn_t		m_end_lsn;
+  /** @return true always */
+  bool operator()(mtr_memo_slot_t* slot) const
+  {
+    if (!slot->object)
+      return true;
+    switch (slot->type) {
+    case MTR_MEMO_PAGE_X_MODIFY:
+    case MTR_MEMO_PAGE_SX_MODIFY:
+      break;
+    default:
+      ut_ad(!(slot->type & MTR_MEMO_MODIFY));
+      return true;
+    }
 
-	/** Mini-transaction REDO end LSN */
-	lsn_t		m_start_lsn;
+    buf_flush_note_modification(static_cast<buf_block_t*>(slot->object),
+                                start, end);
+    return true;
+  }
 };
 
 /** Write the block contents to the REDO log */
@@ -457,7 +453,8 @@ void mtr_t::commit()
     }
 
     m_memo.for_each_block_in_reverse(CIterate<const ReleaseBlocks>
-                                     (ReleaseBlocks(start_lsn, m_commit_lsn)));
+                                     (ReleaseBlocks(start_lsn, m_commit_lsn,
+                                                    m_memo)));
     if (m_made_dirty)
       log_flush_order_mutex_exit();
 
@@ -834,19 +831,32 @@ mtr_t::memo_contains_page_flagged(
 		? NULL : iteration.functor.get_block();
 }
 
+/** Print info of an mtr handle. */
+void
+mtr_t::print() const
+{
+	ib::info() << "Mini-transaction handle: memo size "
+		<< m_memo.size() << " bytes log size "
+		<< get_log()->size() << " bytes";
+}
+
+#endif /* UNIV_DEBUG */
+
+
 /** Find a block, preferrably in MTR_MEMO_MODIFY state */
 struct FindModified
 {
-  const mtr_memo_slot_t *found= nullptr;
+  mtr_memo_slot_t *found= nullptr;
   const buf_block_t& block;
 
   FindModified(const buf_block_t &block) : block(block) {}
-  bool operator()(const mtr_memo_slot_t* slot)
+  bool operator()(mtr_memo_slot_t *slot)
   {
     if (slot->object != &block)
       return true;
     found= slot;
-    return slot->type != MTR_MEMO_MODIFY;
+    return !(slot->type & (MTR_MEMO_MODIFY |
+                           MTR_MEMO_PAGE_X_FIX | MTR_MEMO_PAGE_SX_FIX));
   }
 };
 
@@ -854,20 +864,20 @@ struct FindModified
 @param block   page that will be modified */
 void mtr_t::modify(const buf_block_t &block)
 {
-  Iterate<FindModified> iteration(block);
-  m_memo.for_each_block_in_reverse(iteration);
-  ut_ad(iteration.functor.found);
-  if (iteration.functor.found->type != MTR_MEMO_MODIFY)
-    memo_push(const_cast<buf_block_t*>(&block), MTR_MEMO_MODIFY);
-}
+  if (UNIV_UNLIKELY(m_memo.empty()))
+  {
+    /* This must be PageConverter::update_page() in IMPORT TABLESPACE. */
+    ut_ad(!block.page.in_LRU_list);
+    ut_ad(!buf_pool.is_uncompressed(&block));
+    return;
+  }
 
-/** Print info of an mtr handle. */
-void
-mtr_t::print() const
-{
-	ib::info() << "Mini-transaction handle: memo size "
-		<< m_memo.size() << " bytes log size "
-		<< get_log()->size() << " bytes";
+  Iterate<FindModified> iteration((FindModified(block)));
+  if (UNIV_UNLIKELY(m_memo.for_each_block(iteration)))
+  {
+    ut_ad("modifying an unlatched page" == 0);
+    return;
+  }
+  iteration.functor.found->type= static_cast<mtr_memo_type_t>
+    (iteration.functor.found->type | MTR_MEMO_MODIFY);
 }
-
-#endif /* UNIV_DEBUG */