MySQL Bugs: #61240: Unused functions mtr_rollback_to_savepoint() and btr_pcur_release

Bug #61240	Unused functions mtr_rollback_to_savepoint() and btr_pcur_release_leaf()
Submitted:	20 May 2011 5:41	Modified:	7 Oct 2011 1:16
Reporter:	Yasufumi Kinoshita	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB Plugin storage engine	Severity:	S4 (Feature request)
Version:	5.1.57	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
Though the following path might not used at all (? I cannot trace completely, but it is possible enough, I think), it seems wrong code and might cause problem about flushing and to make bad for durability of InnoDB.

It is not only cause just useless flushing, it might cause lost data at crash.

During the mtr_memo_slot_release(),
buf_flush_note_modification() can be called.
It updates
block->page.newest_modification = mtr->end_lsn;
and
possibly updates
block->page.oldest_modification = mtr->start_lsn;

However, mtr->end_lsn and mtr->start_lsn is set at mtr_log_reserve_and_write() which is called from mtr_commit().

So the existing path
mtr_memo_release() -> mtr_memo_slot_release() -> buf_page_release() -> buf_flush_note_modification()
is wrong, because mtr->start_lsn and mtr->end_lsn are undefined value yet.
(0 or the value of the previous mtr remained)

block->page.newest_modification and block->page.oldest_modification might be set wrong old value, and for example, buf_flush_batch() might stop flush by the wrong older value and not enough pages might be flushed.

How to repeat:
Please review the source code

Suggested fix:
We also need to add something assertion code "the released page is not modified at all".

Adding new argument
"ibool need_flush"
for buf_page_release() and mtr_memo_slot_release()

and
calling from mtr_memo_pop_all(), need_flush==TRUE
calling from mtr_memo_release(), need_flush==FALSE
(calling from mtr_rollback_to_savepoint(), need_flush==FALSE?? (not used))

buf_page_release():

-        if (rw_latch == RW_X_LATCH && mtr->modifications) {
+        if (need_flush && rw_latch == RW_X_LATCH && mtr->modifications) {
                 mutex_enter(&buf_pool->mutex);
                 buf_flush_note_modification(block, mtr);
                 mutex_exit(&buf_pool->mutex);
         }

Though the code is a little obscure but it is not a bug. The only direct caller to mtr_memo_release() that tries to release a block has the assertion that the block is not modified.

285 btr_leaf_page_release(
286 /*==================*/
287         buf_block_t*    block,          /*!< in: buffer block */
288         ulint           latch_mode,     /*!< in: BTR_SEARCH_LEAF or
289                                         BTR_MODIFY_LEAF */
290         mtr_t*          mtr)            /*!< in: mtr */
291 {
292         ut_ad(latch_mode == BTR_SEARCH_LEAF || latch_mode == BTR_MODIFY_LEAF);
293         ut_ad(!mtr_memo_contains(mtr, block, MTR_MEMO_MODIFY));
294 
295         mtr_memo_release(mtr, block,
296                          latch_mode == BTR_SEARCH_LEAF
297                          ? MTR_MEMO_PAGE_S_FIX
298                          : MTR_MEMO_PAGE_X_FIX);

Note the debug assertion at line 293 which ensures that the block being released is not modified. Perhaps we can move the debug assertion to mtr_memo_release(). I am classifying it as 'not a bug'.

Thank you for your analysis.

But, I am saying about "when modifies the another blocks in the mtr already when calling from btr_leaf_page_release() or btr_page_split_and_insert()".

If the other blocks in the mtr were already modified, mtr->modifications == TRUE and buf_flush_note_modification() is called for the "not modified" block, and it might be before mtr_log_reserve_and_write() which decides mtr->start_lsn and mtr->end_lsn.

I think we should add

"ut_ad(!mtr->modified);"

to mtr_memo_release()
and test carefully.

The only caller of buf_flush_note_modification() in InnoDB Plugin 5.1 is buf_page_release(rw_latch=RW_X_LATCH). Pages can only be modified if they are x-latched. For reads, we would s-latch the page.

We could have provisionally x-latched a page that has not been written yet. Is there a possible bug? The only caller of buf_page_release() is mtr_memo_slot_release(). When does it pass rw_latch=RW_X_LATCH? It is a little obscure:

#define	MTR_MEMO_PAGE_S_FIX	RW_S_LATCH /* 1 */
#define	MTR_MEMO_PAGE_X_FIX	RW_X_LATCH /* 2 */
#define	MTR_MEMO_BUF_FIX	RW_NO_LATCH /* 3 */

		if (type <= MTR_MEMO_BUF_FIX) {
			buf_page_release((buf_block_t*)object, type, mtr);

Only if the page was x-latched in the mini-transaction, we would invoke buf_flush_note_modification() on it, on mtr_commit(), mtr_rollback_to_savepoint() or mtr_memo_release().

The function mtr_rollback_to_savepoint() turns out to be unused. In the built-in InnoDB in 5.1, it is only called by the unused function buf_page_get_release_on_io(). Savepoints and rollbacks are concepts of user transactions; mini-transactions are never rolled back.

The only calls to mtr_memo_release() are on index->lock or (like Inaam noted) on a non-modified block in btr_leaf_page_release(). I see no bug there. Note that the MTR_MEMO_MODIFY pseudo-record will be added in UNIV_DEBUG builds whenever starting to buffer redo log to the mini-transaction object, in mlog_write_initial_log_record_fast().

mtr_commit() seems to do things in the right order. I see no bug here, except the unused function mtr_rollback_to_savepoint() that I should have removed together with buf_page_get_release_on_io().

OK. Thank you.
I confirmed that there are no problematic path for now, too.
(The mtr which calls mtr_memo_release() for X_LOCK of block after some modifications.)

We should remember the problem for future.

In addition to mtr_rollback_to_savepoint(), the function btr_pcur_release_leaf() is unused as well.

Added to changelog:

Unused functions were removed from the internal InnoDB code related
to mini-transactions, to clarify the logic.