Merge lp:~laurynas-biveinis/percona-server/bug1186974 into lp:percona-server/5.6

Proposed by Laurynas Biveinis
Status: Merged
Approved by: Alexey Kopytov
Approved revision: 415
Merged at revision: 415
Proposed branch: lp:~laurynas-biveinis/percona-server/bug1186974
Merge into: lp:percona-server/5.6
Diff against target: 40 lines (+6/-2)
3 files modified
Percona-Server/storage/innobase/buf/buf0buf.cc (+2/-0)
Percona-Server/storage/innobase/include/buf0buf.h (+3/-0)
Percona-Server/storage/innobase/include/buf0rea.h (+1/-2)
To merge this branch: bzr merge lp:~laurynas-biveinis/percona-server/bug1186974
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Review via email: mp+182268@code.launchpad.net

This proposal supersedes a proposal from 2013-07-20.

Description of the change

2nd MP:

Reimplemented according to Alexey's review comment.

http://jenkins.percona.com/job/percona-server-5.6-param/235/#showFailuresLink

Fix bug 1186974 (Constant BUF_READ_AHEAD_AREA patch dropped from 5.6
by mistake) by precomputing read ahead area size once per buffer pool
instance initialization instead of every time BUF_READ_AHEAD_AREA.
For that, introduce new field in struct buf_pool_t, read_ahead_area,
that is set in buf_pool_init_instance().

1st MP:

http://jenkins.percona.com/job/percona-server-5.6-param/208/

Fix bug 1186974 (Constant BUF_READ_AHEAD_AREA patch dropped from 5.6
by mistake) by porting the dropped bug 606811 (Make
BUF_READ_AHEAD_AREA a constant) from 5.5.

- Redefine BUF_READ_AHEAD_AREA to be constant 64.
- Raise the low buffer pool size limit to 32 MB.
- Adjust the default buffer pool size for the testsuite to be 32 MB,
  remove buffer pool size options from innodb-index-online,
  innodb-table-online, and innodb_cmp_drop_table testcases.
- Remove the "Small buffer pool size"-printing warning as the new
  low limit is too high for it to apply. Remove corresponding
  suppressions from innodb-index-online, innodb-table-online
- Adjust innodb-wl6445-1 not to change buffer pool size in one of the
  restarts, drop supporting code.
- Adjust innodb, innodb_16k, and innodb_cmp_drop_table testcases for
  the new possible buffer page count values.

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

I could never understand the reason to implement constant readahead with changing the supported minimum buffer pool size and the resulting test suite woes.

So it depends on the buffer pool size, which is static. Well, calculate the corresponding readahead value and store it in a global variable on buffer pool initialization?

review: Needs Fixing
Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

This patch is something I'd like upstream to do. Increasing the buffer pool size lower limit to 32M is reasonable, the toasters and watches can run SQLite, the testsuite changes are one-time thing, and the "small buffer pool size" warnings disappear.

I am considering to implement what you suggest and submit this one under OCA.

Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Hi Laurynas,

On Thu, 01 Aug 2013 14:27:31 -0000, Laurynas Biveinis wrote:
> This patch is something I'd like upstream to do. Increasing the buffer pool size lower limit to 32M is reasonable, the toasters and watches can run SQLite, the testsuite changes are one-time thing, and the "small buffer pool size" warnings disappear.
>

It's not about toasters and watches, it's about implementing things
properly instead of copy-pasting crappy code blindly, spending effort
and time to support it and then exercising humor to defend it :)

> I am considering to implement what you suggest and submit this one under OCA.
>

As you like, that's secondary in this context.

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote : Posted in a previous version of this proposal

Alexey, but my point was that raising the buffer pool size low limit and hardcoding the readahead constant _is_ the proper implementation here in general. Whether it's the proper implementation for _us_, and whether my testsuite changes are OK is a bit different matter and I agreed to redo it in your suggested way.

Revision history for this message
Alexey Kopytov (akopytov) wrote : Posted in a previous version of this proposal

Hi Laurynas,

On Thu, 01 Aug 2013 16:07:36 -0000, Laurynas Biveinis wrote:
> Alexey, but my point was that raising the buffer pool size low limit and hardcoding the readahead constant _is_ the proper implementation here in general. Whether it's the proper implementation for _us_, and whether my testsuite changes are OK is a bit different matter and I agreed to redo it in your suggested way.
>

My point was that raising the buffer pool size low limit is OK, but can
be avoided.
Changing more than one test case to support such a trivial change is
_not_ OK. So it's better avoided.

Revision history for this message
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/innobase/buf/buf0buf.cc'
2--- Percona-Server/storage/innobase/buf/buf0buf.cc 2013-08-16 09:11:51 +0000
3+++ Percona-Server/storage/innobase/buf/buf0buf.cc 2013-08-27 05:04:49 +0000
4@@ -1324,6 +1324,8 @@
5 buf_pool->instance_no = instance_no;
6 buf_pool->old_pool_size = buf_pool_size;
7 buf_pool->curr_size = chunk->size;
8+ buf_pool->read_ahead_area
9+ = ut_min(64, ut_2_power_up(buf_pool->curr_size / 32));
10 buf_pool->curr_pool_size = buf_pool->curr_size * UNIV_PAGE_SIZE;
11
12 /* Number of locks protecting page_hash must be a
13
14=== modified file 'Percona-Server/storage/innobase/include/buf0buf.h'
15--- Percona-Server/storage/innobase/include/buf0buf.h 2013-08-16 09:11:51 +0000
16+++ Percona-Server/storage/innobase/include/buf0buf.h 2013-08-27 05:04:49 +0000
17@@ -1820,6 +1820,9 @@
18 ulint n_chunks; /*!< number of buffer pool chunks */
19 buf_chunk_t* chunks; /*!< buffer pool chunks */
20 ulint curr_size; /*!< current pool size in pages */
21+ ulint read_ahead_area;/*!< size in pages of the area which
22+ the read-ahead algorithms read if
23+ invoked */
24 hash_table_t* page_hash; /*!< hash table of buf_page_t or
25 buf_block_t file pages,
26 buf_page_in_file() == TRUE,
27
28=== modified file 'Percona-Server/storage/innobase/include/buf0rea.h'
29--- Percona-Server/storage/innobase/include/buf0rea.h 2013-08-06 15:16:34 +0000
30+++ Percona-Server/storage/innobase/include/buf0rea.h 2013-08-27 05:04:49 +0000
31@@ -164,8 +164,7 @@
32
33 /** The size in pages of the area which the read-ahead algorithms read if
34 invoked */
35-#define BUF_READ_AHEAD_AREA(b) \
36- ut_min(64, ut_2_power_up((b)->curr_size / 32))
37+#define BUF_READ_AHEAD_AREA(b) ((b)->read_ahead_area)
38
39 /** @name Modes used in read-ahead @{ */
40 /** read only pages belonging to the insert buffer tree */

Subscribers

People subscribed via source and target branches