Bug #70228 Is buf_LRU_free_page() really supposed to make non-zip block sticky at the end?
Submitted: 3 Sep 2013 15:17 Modified: 16 Oct 2015 8:27
Reporter: Laurynas Biveinis (OCA) Email Updates:
Status: Closed Impact on me:
None 
Category:MySQL Server: InnoDB storage engine Severity:S3 (Non-critical)
Version:5.6.13 OS:Any
Assigned to: CPU Architecture:Any

[3 Sep 2013 15:17] Laurynas Biveinis 
Description:
buf_LRU_free_page() appears to either have redundant code to needlessly make the removed page sticky to the buffer pool, or at least a misleading comment.

How to repeat:
The relevant bits are:

bool
buf_LRU_free_page(...)
{
...
	ut_ad(buf_pool_mutex_own(buf_pool));
...
	if (!buf_LRU_block_remove_hashed(bpage, zip)) {
		return(true);
	}
...
	if (b) {
...
	} else {

		/* There can be multiple threads doing an LRU scan to
		free a block. The page_cleaner thread can be doing an
		LRU batch whereas user threads can potentially be doing
		multiple single page flushes. As we release
		buf_pool->mutex below we need to make sure that no one
		else considers this block as a victim for page
		replacement. This block is already out of page_hash
		and we are about to remove it from the LRU list and put
		it on the free list. */
		mutex_enter(block_mutex);
		buf_page_set_sticky(bpage);
		mutex_exit(block_mutex);
	}

	buf_pool_mutex_exit(buf_pool);
...
}

buf_LRU_block_remove_hashed will remove bpage from the LRU list. Thus it appears that
1) The comment "we need sure that no one else considers this block as a victim for page replacement" is false, as the block is not on the LRU list anymore where the victims are picked from;
2) There is no need to make the page sticky here and non-sticky below (not shown in the code snippet).

Suggested fix:
Remove else branch, make the final buf_page_unset_sticky() handle the b != NULL case only.
[3 Sep 2013 15:18] Laurynas Biveinis 
Adding "non-zip" to title to indicate that this applies to non-compressed blocks only.
[4 Sep 2013 9:33] MySQL Verification Team 
Hello Laurynas,

Thank you for the report.

Thanks,
Umesh
[4 Sep 2013 15:10] Inaam Rana 
Laurynas,

You are right. I originally wrote this code and comment. At this point in code a non-zip block is not in page_hash, LRU, flush_list or free_list. Setting the sticky bit is redundant. Good catch!
[18 Sep 2014 18:42] Daniel Price 
Fixed as of the upcoming 5.7.6 release, and here's the changelog entry:

"buf_LRU_free_page()" would call "buf_page_set_sticky(bpage)", needlessly
making removed pages sticky in some cases. 

Thank you for the bug report.
[16 Oct 2015 8:27] Laurynas Biveinis 
$ git show 0f01e35 --no-patch
commit 0f01e359aab44e7cfb2f6bd8c3ef26decf291b36
Author: Yasufumi Kinoshita <yasufumi.kinoshita@oracle.com>
Date:   Tue Sep 16 16:56:06 2014 +0900

    Bug#17407091 : IS BUF_LRU_FREE_PAGE() REALLY SUPPOSED TO MAKE NON-ZIP BLOCK STICKY AT THE END? (Bug#70228)
    
    buf_LRU_free_page() used buf_page_set_sticky(bpage) redundantly for some cases.
    
    Approved by Vasil in rb#6258