Merge lp:~laurynas-biveinis/percona-server/26611-bug1059738-5.1 into lp:percona-server/5.1

[Laurynas Biveinis]

- Fixed } else { formatting.
- Added a comment explaining the sibling page unfix from the buffer.
- buf_read_page_low() is not used instead of btr_block_get() to avoid fixing/unfixing as it would make the code diverge too much in the prefetching case from the regular code path.
- No Jenkins, as the only changes are in formatting and comments.

Fix bug 1059738 (make innodb_fake_changes faster -- prefetch sibling
pages).

Fix adapted from lp:mysqlatfacebook/51, revision 3791, with the
difference that the sibling pages are not latched at all.

- Add new btr_latch_mode value BTR_SEARCH_TREE that is similar
  BTR_MODIFY_TREE except that it does not X-latch the sibling pages of
  the given leaf.
- btr_cur_latch_leaves(): handle BTR_SEARCH_TREE by fetching the
  sibling leaf pages by reading them without latching and then
  immediately unfixing them from the buffer.
- row_ins_index_entry_low(), row_upd_clust_rec(): use BTR_SEARCH_TREE
  instead of BTR_MODIFY_TREE in case of fake changes.

26611

http://jenkins.percona.com/job/percona-server-5.1-param/451/

The main.userstat_bug602047 failure is a bit worrying as it could implicate the stat fixes (prerequisite branch of this one). But I stared at the changes long and hard and so far am willing to take the risk that this is more of a Jenkins thing.

[Alexey Kopytov (akopytov)]

Laurynas,

- why did you remove "ut_ad(mtr->trx->fake_changes);" from the original patch? Looks like a good idea to me.

- what's the purpose of unfixing the sibling pages from the buffer? How about adding a comment?

- it must be "} else {" rather than "}<newline>else {"

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

> - why did you remove "ut_ad(mtr->trx->fake_changes);" from the original patch?
> Looks like a good idea to me.

Unintentionally and it is a good idea, will add back.

> - what's the purpose of unfixing the sibling pages from the buffer? How about
> adding a comment?

Because this is prefetch and fixing the pages is only a side effect of reading them without any latch, it does not serve any purpose besides increasing buffer pool pressure temporarily a tiny bit (the fixing is until the end of fake changes trx, so I expect it to be short).

In both cases the pages still might get evicted from LRU before the real transaction comes that uses those pages, which is a workload timing issue, not really fixable at the code level.

I will add a comment.

> - it must be "} else {" rather than "}<newline>else {"

Bad habit :)

[Laurynas Biveinis (laurynas-biveinis)]

> - why did you remove "ut_ad(mtr->trx->fake_changes);" from the original patch?

Sorry, I did it intentionally. There is no mtr->trx field in XtraDB.

[Laurynas Biveinis (laurynas-biveinis)]

The prefetch code should use buf_page_read_low() to avoid all this MTR fixing-unfixing business.

review: Needs Fixing

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'Percona-Server/storage/innodb_plugin/btr/btr0cur.c'
--- Percona-Server/storage/innodb_plugin/btr/btr0cur.c	2012-10-29 08:56:24 +0000
+++ Percona-Server/storage/innodb_plugin/btr/btr0cur.c	2012-10-29 08:56:24 +0000
@@ -228,6 +228,7 @@
 	mtr_t*		mtr)		/*!< in: mtr */
 {
 	ulint		mode;
+	ulint		sibling_mode;
 	ulint		left_page_no;
 	ulint		right_page_no;
 	buf_block_t*	get_block;
@@ -251,14 +252,21 @@
 #endif /* UNIV_BTR_DEBUG */
 		get_block->check_index_page_at_flush = TRUE;
 		return;
+	case BTR_SEARCH_TREE:
 	case BTR_MODIFY_TREE:
-		/* x-latch also brothers from left to right */
+		if (UNIV_UNLIKELY(latch_mode == BTR_SEARCH_TREE)) {
+			mode = RW_S_LATCH;
+			sibling_mode = RW_NO_LATCH;
+		} else {
+			mode = sibling_mode = RW_X_LATCH;
+		}
+		/* Fetch and possibly latch also brothers from left to right */
 		left_page_no = btr_page_get_prev(page, mtr);
 
 		if (left_page_no != FIL_NULL) {
 			get_block = btr_block_get(
 				space, zip_size, left_page_no,
-				RW_X_LATCH, cursor->index, mtr);
+				sibling_mode, cursor->index, mtr);
 
 			if (srv_pass_corrupt_table && !get_block) {
 				return;
@@ -270,12 +278,21 @@
 			ut_a(btr_page_get_next(get_block->frame, mtr)
 			     == page_get_page_no(page));
 #endif /* UNIV_BTR_DEBUG */
-			get_block->check_index_page_at_flush = TRUE;
+			if (sibling_mode == RW_NO_LATCH) {
+				/* btr_block_get() called with RW_NO_LATCH will
+				fix the read block in the buffer.  This serves
+				no purpose for the fake changes prefetching,
+				thus we unfix the sibling blocks immediately.*/
+				mtr_memo_release(mtr, get_block,
+						 MTR_MEMO_BUF_FIX);
+			} else {
+				get_block->check_index_page_at_flush = TRUE;
+			}
 		}
 
 		get_block = btr_block_get(
 			space, zip_size, page_no,
-			RW_X_LATCH, cursor->index, mtr);
+			mode, cursor->index, mtr);
 
 		if (srv_pass_corrupt_table && !get_block) {
 			return;
@@ -291,7 +308,7 @@
 		if (right_page_no != FIL_NULL) {
 			get_block = btr_block_get(
 				space, zip_size, right_page_no,
-				RW_X_LATCH, cursor->index, mtr);
+				sibling_mode, cursor->index, mtr);
 
 			if (srv_pass_corrupt_table && !get_block) {
 				return;
@@ -303,7 +320,12 @@
 			ut_a(btr_page_get_prev(get_block->frame, mtr)
 			     == page_get_page_no(page));
 #endif /* UNIV_BTR_DEBUG */
-			get_block->check_index_page_at_flush = TRUE;
+			if (sibling_mode == RW_NO_LATCH) {
+				mtr_memo_release(mtr, get_block,
+						 MTR_MEMO_BUF_FIX);
+			} else {
+				get_block->check_index_page_at_flush = TRUE;
+			}
 		}
 
 		return;

=== modified file 'Percona-Server/storage/innodb_plugin/include/btr0btr.h'
--- Percona-Server/storage/innodb_plugin/include/btr0btr.h	2012-05-09 04:14:12 +0000
+++ Percona-Server/storage/innodb_plugin/include/btr0btr.h	2012-10-29 08:56:24 +0000
@@ -65,7 +65,10 @@
 	/** Search the previous record. */
 	BTR_SEARCH_PREV = 35,
 	/** Modify the previous record. */
-	BTR_MODIFY_PREV = 36
+	BTR_MODIFY_PREV = 36,
+	/** Weaker BTR_MODIFY_TREE that does not lock the leaf page siblings,
+	used for fake changes.  */
+	BTR_SEARCH_TREE = 37 /* BTR_MODIFY_TREE | 4 */
 };
 
 /** If this is ORed to btr_latch_mode, it means that the search tuple

=== modified file 'Percona-Server/storage/innodb_plugin/row/row0ins.c'
--- Percona-Server/storage/innodb_plugin/row/row0ins.c	2012-08-20 03:14:02 +0000
+++ Percona-Server/storage/innodb_plugin/row/row0ins.c	2012-10-29 08:56:24 +0000
@@ -2016,6 +2016,7 @@
 	big_rec_t*	big_rec			= NULL;
 	mtr_t		mtr;
 	mem_heap_t*	heap			= NULL;
+	ulint		search_mode;
 
 	log_free_check();
 
@@ -2031,8 +2032,15 @@
 		ignore_sec_unique = BTR_IGNORE_SEC_UNIQUE;
 	}
 
+	if (UNIV_UNLIKELY(thr_get_trx(thr)->fake_changes)) {
+		search_mode = (mode & BTR_MODIFY_TREE)
+			? BTR_SEARCH_TREE : BTR_SEARCH_LEAF;
+	} else {
+		search_mode = mode | BTR_INSERT | ignore_sec_unique;
+	}
+
 	btr_cur_search_to_nth_level(index, 0, entry, PAGE_CUR_LE,
-				    thr_get_trx(thr)->fake_changes ? BTR_SEARCH_LEAF : (mode | BTR_INSERT | ignore_sec_unique),
+				    search_mode,
 				    &cursor, 0, __FILE__, __LINE__, &mtr);
 
 	if (cursor.flag == BTR_CUR_INSERT_TO_IBUF) {

=== modified file 'Percona-Server/storage/innodb_plugin/row/row0upd.c'
--- Percona-Server/storage/innodb_plugin/row/row0upd.c	2012-04-02 02:09:15 +0000
+++ Percona-Server/storage/innodb_plugin/row/row0upd.c	2012-10-29 08:56:24 +0000
@@ -1994,7 +1994,8 @@
 	the same transaction do not modify the record in the meantime.
 	Therefore we can assert that the restoration of the cursor succeeds. */
 
-	ut_a(btr_pcur_restore_position(thr_get_trx(thr)->fake_changes ? BTR_SEARCH_LEAF : BTR_MODIFY_TREE,
+	ut_a(btr_pcur_restore_position(thr_get_trx(thr)->fake_changes
+				       ? BTR_SEARCH_TREE : BTR_MODIFY_TREE,
 				       pcur, mtr));
 
 	ut_ad(!rec_get_deleted_flag(btr_pcur_get_rec(pcur),