Percona Server moved to https://jira.percona.com/projects/PS

Merge lp://staging/~sergei.glushchenko/percona-server/ps51-128-max-index into lp://staging/percona-server/5.1

  1. ps51-128-max-index
  2. Merge into 5.1
Proposed by Sergei Glushchenko
Status: Merged
Approved by: Alexey Kopytov
Approved revision: no longer in the source branch.
Merged at revision: 488
Proposed branch: lp://staging/~sergei.glushchenko/percona-server/ps51-128-max-index
Merge into: lp://staging/percona-server/5.1
Diff against target: 172 lines (+64/-4)
10 files modified
Percona-Server/mysql-test/include/have_64_keys.inc (+12/-0)
Percona-Server/mysql-test/r/have_64_keys.require (+14/-0)
Percona-Server/mysql-test/t/create.test (+5/-0)
Percona-Server/mysql-test/t/ps_1general.test (+5/-0)
Percona-Server/mysql-test/t/ps_2myisam.test (+5/-0)
Percona-Server/mysql-test/t/ps_3innodb.test (+5/-0)
Percona-Server/mysql-test/t/ps_4heap.test (+5/-0)
Percona-Server/mysql-test/t/ps_5merge.test (+5/-0)
Percona-Server/mysys/my_bitmap.c (+1/-1)
Percona-Server/sql/sql_bitmap.h (+7/-3)
To merge this branch: bzr merge lp://staging/~sergei.glushchenko/percona-server/ps51-128-max-index
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Laurynas Biveinis (community) Approve
Review via email: mp+122520@code.staging.launchpad.net

Description of the change

This is a port from
http://bazaar.launchpad.net/~percona-core/percona-server/rnt-5.1/revision/166

Been tested locally and the only failed tests were
main.create
main.ps_1general
main.ps_2myisam
main.ps_4heap
main.ps_5merge
main.ps_3innodb

which are designed with the assumption that MAX_INDEXES=64.

To post a comment you must log in.
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

#25824

Revision history for this message
Alexey Kopytov (akopytov) wrote :

As discussed on IRC we should look into making the failing test cases pass (or be skipped) with all --with-max-indexes values.

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

include/have_64_keys.inc been added to check whether we have exactly 64 max indexes or not. If we have not
main.create
main.ps_1general
main.ps_2myisam
main.ps_4heap
main.ps_5merge
main.ps_3innodb
are skipped.

http://jenkins.percona.com/view/PS%205.1/job/percona-server-5.1-param/403/

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

Minor comments:

    Please add a comment to have_64_keys explaining what part of the
    metadata confirms that max_indexes = 64.
    Typos: line 39: s/check/checks, line 41: s/skipping/skip. These
    are repeated in other test headers.

    Bitmap::to_ulonglong() returns the first ulonglong-sized bytes of
    the bitmap. In case of default bitmap representation that's the
    whole bitmap, in case of the general bitmap that's a prefix only.
    It does suggest that the code in
    opt_range.cc:get_best_group_min_max() might produce suboptimal
    query plans whenever max_indexes > 64 and actual indexes (key
    parts > 64). I am not sure that we should do anything about it.

review: Needs Fixing
Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Laurynas,
> Bitmap::to_ulonglong() returns the first ulonglong-sized bytes of
> the bitmap. In case of default bitmap representation that's the
> whole bitmap, in case of the general bitmap that's a prefix only.
> It does suggest that the code in
> opt_range.cc:get_best_group_min_max() might produce suboptimal
> query plans whenever max_indexes > 64 and actual indexes (key
> parts > 64). I am not sure that we should do anything about it.

Code in opt_range.cc can be easily modified to compare prefix by using other operations of Bitmap<> class. More interesting is to construct a good testcase for this issue.

Revision history for this message
Sergei Glushchenko (sergei.glushchenko) wrote :

Comments been added and typos been fixed

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

    Diff line 12: "a synonym"
    Diff line 46 and friends: s/if it been done/in this case.

No need for another MP, just edit and re-push the branch. Thanks!

review: Approve
Revision history for this message
Alexey Kopytov (akopytov) wrote :

But we also need a 5.5 MP to be updated.

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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches