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

Proposed by Laurynas Biveinis
Status: Merged
Approved by: Alexey Kopytov
Approved revision: 494
Merged at revision: 500
Proposed branch: lp:~laurynas-biveinis/percona-server/26611-bug1059738-5.1
Merge into: lp:percona-server/5.1
Prerequisite: lp:~laurynas-biveinis/percona-server/26611-bug1064333-5.1
Diff against target: 141 lines (+43/-9)
4 files modified
Percona-Server/storage/innodb_plugin/btr/btr0cur.c (+28/-6)
Percona-Server/storage/innodb_plugin/include/btr0btr.h (+4/-1)
Percona-Server/storage/innodb_plugin/row/row0ins.c (+9/-1)
Percona-Server/storage/innodb_plugin/row/row0upd.c (+2/-1)
To merge this branch: bzr merge lp:~laurynas-biveinis/percona-server/26611-bug1059738-5.1
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Laurynas Biveinis Pending
Review via email: mp+131831@code.launchpad.net

This proposal supersedes a proposal from 2012-10-18.

Description of the change

- 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.

To post a comment you must log in.
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

> - 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) wrote : Posted in a previous version of this proposal

> - 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) wrote : Posted in a previous version of this proposal

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/storage/innodb_plugin/btr/btr0cur.c'
2--- Percona-Server/storage/innodb_plugin/btr/btr0cur.c 2012-10-29 08:56:24 +0000
3+++ Percona-Server/storage/innodb_plugin/btr/btr0cur.c 2012-10-29 08:56:24 +0000
4@@ -228,6 +228,7 @@
5 mtr_t* mtr) /*!< in: mtr */
6 {
7 ulint mode;
8+ ulint sibling_mode;
9 ulint left_page_no;
10 ulint right_page_no;
11 buf_block_t* get_block;
12@@ -251,14 +252,21 @@
13 #endif /* UNIV_BTR_DEBUG */
14 get_block->check_index_page_at_flush = TRUE;
15 return;
16+ case BTR_SEARCH_TREE:
17 case BTR_MODIFY_TREE:
18- /* x-latch also brothers from left to right */
19+ if (UNIV_UNLIKELY(latch_mode == BTR_SEARCH_TREE)) {
20+ mode = RW_S_LATCH;
21+ sibling_mode = RW_NO_LATCH;
22+ } else {
23+ mode = sibling_mode = RW_X_LATCH;
24+ }
25+ /* Fetch and possibly latch also brothers from left to right */
26 left_page_no = btr_page_get_prev(page, mtr);
27
28 if (left_page_no != FIL_NULL) {
29 get_block = btr_block_get(
30 space, zip_size, left_page_no,
31- RW_X_LATCH, cursor->index, mtr);
32+ sibling_mode, cursor->index, mtr);
33
34 if (srv_pass_corrupt_table && !get_block) {
35 return;
36@@ -270,12 +278,21 @@
37 ut_a(btr_page_get_next(get_block->frame, mtr)
38 == page_get_page_no(page));
39 #endif /* UNIV_BTR_DEBUG */
40- get_block->check_index_page_at_flush = TRUE;
41+ if (sibling_mode == RW_NO_LATCH) {
42+ /* btr_block_get() called with RW_NO_LATCH will
43+ fix the read block in the buffer. This serves
44+ no purpose for the fake changes prefetching,
45+ thus we unfix the sibling blocks immediately.*/
46+ mtr_memo_release(mtr, get_block,
47+ MTR_MEMO_BUF_FIX);
48+ } else {
49+ get_block->check_index_page_at_flush = TRUE;
50+ }
51 }
52
53 get_block = btr_block_get(
54 space, zip_size, page_no,
55- RW_X_LATCH, cursor->index, mtr);
56+ mode, cursor->index, mtr);
57
58 if (srv_pass_corrupt_table && !get_block) {
59 return;
60@@ -291,7 +308,7 @@
61 if (right_page_no != FIL_NULL) {
62 get_block = btr_block_get(
63 space, zip_size, right_page_no,
64- RW_X_LATCH, cursor->index, mtr);
65+ sibling_mode, cursor->index, mtr);
66
67 if (srv_pass_corrupt_table && !get_block) {
68 return;
69@@ -303,7 +320,12 @@
70 ut_a(btr_page_get_prev(get_block->frame, mtr)
71 == page_get_page_no(page));
72 #endif /* UNIV_BTR_DEBUG */
73- get_block->check_index_page_at_flush = TRUE;
74+ if (sibling_mode == RW_NO_LATCH) {
75+ mtr_memo_release(mtr, get_block,
76+ MTR_MEMO_BUF_FIX);
77+ } else {
78+ get_block->check_index_page_at_flush = TRUE;
79+ }
80 }
81
82 return;
83
84=== modified file 'Percona-Server/storage/innodb_plugin/include/btr0btr.h'
85--- Percona-Server/storage/innodb_plugin/include/btr0btr.h 2012-05-09 04:14:12 +0000
86+++ Percona-Server/storage/innodb_plugin/include/btr0btr.h 2012-10-29 08:56:24 +0000
87@@ -65,7 +65,10 @@
88 /** Search the previous record. */
89 BTR_SEARCH_PREV = 35,
90 /** Modify the previous record. */
91- BTR_MODIFY_PREV = 36
92+ BTR_MODIFY_PREV = 36,
93+ /** Weaker BTR_MODIFY_TREE that does not lock the leaf page siblings,
94+ used for fake changes. */
95+ BTR_SEARCH_TREE = 37 /* BTR_MODIFY_TREE | 4 */
96 };
97
98 /** If this is ORed to btr_latch_mode, it means that the search tuple
99
100=== modified file 'Percona-Server/storage/innodb_plugin/row/row0ins.c'
101--- Percona-Server/storage/innodb_plugin/row/row0ins.c 2012-08-20 03:14:02 +0000
102+++ Percona-Server/storage/innodb_plugin/row/row0ins.c 2012-10-29 08:56:24 +0000
103@@ -2016,6 +2016,7 @@
104 big_rec_t* big_rec = NULL;
105 mtr_t mtr;
106 mem_heap_t* heap = NULL;
107+ ulint search_mode;
108
109 log_free_check();
110
111@@ -2031,8 +2032,15 @@
112 ignore_sec_unique = BTR_IGNORE_SEC_UNIQUE;
113 }
114
115+ if (UNIV_UNLIKELY(thr_get_trx(thr)->fake_changes)) {
116+ search_mode = (mode & BTR_MODIFY_TREE)
117+ ? BTR_SEARCH_TREE : BTR_SEARCH_LEAF;
118+ } else {
119+ search_mode = mode | BTR_INSERT | ignore_sec_unique;
120+ }
121+
122 btr_cur_search_to_nth_level(index, 0, entry, PAGE_CUR_LE,
123- thr_get_trx(thr)->fake_changes ? BTR_SEARCH_LEAF : (mode | BTR_INSERT | ignore_sec_unique),
124+ search_mode,
125 &cursor, 0, __FILE__, __LINE__, &mtr);
126
127 if (cursor.flag == BTR_CUR_INSERT_TO_IBUF) {
128
129=== modified file 'Percona-Server/storage/innodb_plugin/row/row0upd.c'
130--- Percona-Server/storage/innodb_plugin/row/row0upd.c 2012-04-02 02:09:15 +0000
131+++ Percona-Server/storage/innodb_plugin/row/row0upd.c 2012-10-29 08:56:24 +0000
132@@ -1994,7 +1994,8 @@
133 the same transaction do not modify the record in the meantime.
134 Therefore we can assert that the restoration of the cursor succeeds. */
135
136- ut_a(btr_pcur_restore_position(thr_get_trx(thr)->fake_changes ? BTR_SEARCH_LEAF : BTR_MODIFY_TREE,
137+ ut_a(btr_pcur_restore_position(thr_get_trx(thr)->fake_changes
138+ ? BTR_SEARCH_TREE : BTR_MODIFY_TREE,
139 pcur, mtr));
140
141 ut_ad(!rec_get_deleted_flag(btr_pcur_get_rec(pcur),

Subscribers

People subscribed via source and target branches