MDEV-22110 InnoDB unnecessarily writes unmodified pages

At least since commit 6a7be48b
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.

storage/innobase/btr/btr0btr.cc

@@ -1344,7 +1344,7 @@ btr_write_autoinc(dict_index_t* index, ib_uint64_t autoinc, bool reset)
 static void btr_page_reorganize_low(page_cur_t *cursor, dict_index_t *index,
                                     mtr_t *mtr)
 {
-  const mtr_log_t log_mode= mtr->set_log_mode(MTR_LOG_NONE);
+  const mtr_log_t log_mode= mtr->set_log_mode(MTR_LOG_NO_REDO);
 
   buf_block_t *const block= cursor->block;
 

storage/innobase/include/ibuf0ibuf.ic

@@ -46,7 +46,7 @@ ibuf_mtr_start(
 	mtr->enter_ibuf();
 
 	if (high_level_read_only || srv_read_only_mode) {
-		mtr_set_log_mode(mtr, MTR_LOG_NONE);
+		mtr_set_log_mode(mtr, MTR_LOG_NO_REDO);
 	}
 
 }

storage/innobase/include/mtr0mtr.h

@@ -141,10 +141,15 @@ struct mtr_t {
     return static_cast<mtr_log_t>(m_log_mode);
   }
 
-	/** Change the logging mode.
-	@param mode	 logging mode
-	@return	old mode */
-	inline mtr_log_t set_log_mode(mtr_log_t mode);
+  /** Change the logging mode.
+  @param mode	 logging mode
+  @return	old mode */
+  mtr_log_t set_log_mode(mtr_log_t mode)
+  {
+    const mtr_log_t old_mode= get_log_mode();
+    m_log_mode= mode & 3;
+    return old_mode;
+  }
 
 	/** Copy the tablespaces associated with the mini-transaction
 	(needed for generating FILE_MODIFY records)
@@ -281,19 +286,13 @@ struct mtr_t {
 private:
   /** Note that the mini-transaction will modify data. */
   void flag_modified() { m_modifications = true; }
-#ifdef UNIV_DEBUG
   /** Mark the given latched page as modified.
   @param block   page that will be modified */
   void modify(const buf_block_t& block);
 public:
   /** Note that the mini-transaction will modify a block. */
   void set_modified(const buf_block_t &block)
-  { flag_modified(); if (m_log_mode == MTR_LOG_ALL) modify(block); }
-#else /* UNIV_DEBUG */
-public:
-  /** Note that the mini-transaction will modify a block. */
-  void set_modified(const buf_block_t &) { flag_modified(); }
-#endif /* UNIV_DEBUG */
+  { flag_modified(); if (m_log_mode != MTR_LOG_NONE) modify(block); }
 
   /** Set the state to not-modified. This will not log the changes.
   This is only used during redo log apply, to avoid logging the changes. */

storage/innobase/include/mtr0mtr.ic

@@ -171,39 +171,3 @@ mtr_t::release_block_at_savepoint(
 
 	slot->object = NULL;
 }
-
-/**
-Changes the logging mode of a mini-transaction.
-@return	old mode */
-
-mtr_log_t
-mtr_t::set_log_mode(mtr_log_t mode)
-{
-	ut_ad(mode >= MTR_LOG_ALL);
-	ut_ad(mode <= MTR_LOG_NO_REDO);
-
-	const mtr_log_t	old_mode = get_log_mode();
-
-	switch (old_mode) {
-	case MTR_LOG_NO_REDO:
-		/* Once this mode is set, it must not be changed. */
-		ut_ad(mode == MTR_LOG_NO_REDO || mode == MTR_LOG_NONE);
-		return(old_mode);
-	case MTR_LOG_NONE:
-		if (mode == old_mode) {
-			/* Keep MTR_LOG_NONE. */
-			return(old_mode);
-		}
-		ut_ad(mode == MTR_LOG_ALL);
-		/* fall through */
-	case MTR_LOG_ALL:
-		/* MTR_LOG_NO_REDO can only be set before generating
-		any redo log records. */
-		ut_ad(mode != MTR_LOG_NO_REDO || m_log.empty());
-		m_log_mode = mode & 3;
-		return(old_mode);
-	}
-
-	ut_ad(0);
-	return(old_mode);
-}

storage/innobase/include/mtr0types.h

@@ -41,8 +41,7 @@ enum mtr_log_t {
 	MTR_LOG_ALL = 0,
 
 	/** Log no operations and dirty pages are not added to the flush list.
-	Set when applying log in crash recovery or when a modification of a
-	ROW_FORMAT=COMPRESSED page is attempted. */
+	Set for attempting modification of a ROW_FORMAT=COMPRESSED page. */
 	MTR_LOG_NONE,
 
 	/** Don't generate REDO log but add dirty pages to flush list */
@@ -329,9 +328,10 @@ enum mtr_memo_type_t {
 
 	MTR_MEMO_BUF_FIX = RW_NO_LATCH,
 
-#ifdef UNIV_DEBUG
 	MTR_MEMO_MODIFY = 16,
-#endif /* UNIV_DEBUG */
+
+	MTR_MEMO_PAGE_X_MODIFY = MTR_MEMO_PAGE_X_FIX | MTR_MEMO_MODIFY,
+	MTR_MEMO_PAGE_SX_MODIFY = MTR_MEMO_PAGE_SX_FIX | MTR_MEMO_MODIFY,
 
 	MTR_MEMO_S_LOCK = RW_S_LATCH << 5,
 

storage/innobase/log/log0recv.cc

@@ -2492,7 +2492,7 @@ void recv_recover_page(fil_space_t* space, buf_page_t* bpage)
 {
 	mtr_t mtr;
 	mtr.start();
-	mtr.set_log_mode(MTR_LOG_NONE);
+	mtr.set_log_mode(MTR_LOG_NO_REDO);
 
 	ut_ad(bpage->state() == BUF_BLOCK_FILE_PAGE);
 	buf_block_t* block = reinterpret_cast<buf_block_t*>(bpage);
@@ -2576,7 +2576,7 @@ inline buf_block_t *recv_sys_t::recover_low(const page_id_t page_id,
   else if (fil_space_t *space= fil_space_acquire_for_io(page_id.space()))
   {
     mtr.start();
-    mtr.set_log_mode(MTR_LOG_NONE);
+    mtr.set_log_mode(MTR_LOG_NO_REDO);
     block= buf_page_create(space, page_id.page_no(), space->zip_size(), &mtr);
     p= recv_sys.pages.find(page_id);
     if (p == recv_sys.pages.end())
@@ -2693,7 +2693,7 @@ void recv_sys_t::apply(bool last_batch)
         continue;
       case page_recv_t::RECV_NOT_PROCESSED:
         mtr.start();
-        mtr.set_log_mode(MTR_LOG_NONE);
+        mtr.set_log_mode(MTR_LOG_NO_REDO);
         if (buf_block_t *block= buf_page_get_low(page_id, 0, RW_X_LATCH,
                                                  nullptr, BUF_GET_IF_IN_POOL,
                                                  __FILE__, __LINE__,

storage/innobase/mtr/mtr0mtr.cc

@@ -205,13 +205,6 @@ struct FindPage
 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 */

storage/innobase/row/row0import.cc

@@ -234,7 +234,7 @@ class RecIterator {
 		memset(&m_cur, 0x0, sizeof(m_cur));
 		/* Make page_cur_delete_rec() happy. */
 		m_mtr.start();
-		m_mtr.set_log_mode(MTR_LOG_NONE);
+		m_mtr.set_log_mode(MTR_LOG_NO_REDO);
 	}
 
 	/** Position the cursor on the first user record. */