Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(#1235): add an optimizer switch flag to migration context #1236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cyrinux wants to merge 5 commits into github:master
base: master
Choose a base branch
Loading
from cyrinux:feat/cle/add-optimizer-switch

Conversation

@cyrinux
Copy link

@cyrinux cyrinux commented Dec 27, 2022
edited
Loading

Description

This add a optimizer_switch string flag.

--optimizer-switch="prefer_ordering_index=on"

I don't check the value, easier to maintains, this will just fail if its a unknown value, depending the mysql/maria/percona version.

Related issue: #1235

@cyrinux cyrinux force-pushed the feat/cle/add-optimizer-switch branch from d499039 to 71df3e8 Compare December 27, 2022 09:55
@cyrinux cyrinux changed the title (削除) feat(#1235): add an optimizer switch params to migration context (削除ここまで) (追記) feat(#1235): add an optimizer switch param to migration context (追記ここまで) Dec 27, 2022
@cyrinux cyrinux changed the title (削除) feat(#1235): add an optimizer switch param to migration context (削除ここまで) (追記) feat(#1235): add an optimizer switch flag to migration context (追記ここまで) Dec 27, 2022
Copy link
Collaborator

@timvaillancourt timvaillancourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyrinux thanks for the PR! I have left a few comments/nits in line

Two more questions:

  1. For context/curiosity, in what situations or environments are you needing to override the optimizer_switch? This might be useful to know or document
  2. Can you please update doc/command-line-flags.md with a description of this new flag?

🙇

Copy link
Author

cyrinux commented Dec 27, 2022
edited
Loading

@cyrinux thanks for the PR! I have left a few comments/nits in line

Two more questions:

1. For context/curiosity, in what situations or environments are you needing to override the `optimizer_switch`? This might be useful to know or document
2. Can you please update [`doc/command-line-flags.md`](https://github.com/github/gh-ost/blob/master/doc/command-line-flags.md?rgh-link-date=2022年12月27日T18%3A27%3A27Z) with a description of this new flag?

🙇

I'm not DBA myself, but I understand sometimes we set those optimizer_switch on GLOBAL and then depending the value, this can slow by 10 in my case a simple count(*). It was the case here, unable to use the tools after some months and tweaking.
So we explicitly set by SESSION the optimizer_switch in SOME cases to revert the GLOBAL behavior.

I apply your recommandations, is-it better for you ? 🙏

@cyrinux cyrinux force-pushed the feat/cle/add-optimizer-switch branch from 462cc89 to 66211ee Compare December 27, 2022 19:13
to be able to pass for example:
`--optimizer-switch="prefer_ordering_index=on"`
Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com>
@cyrinux cyrinux force-pushed the feat/cle/add-optimizer-switch branch from 66211ee to eec7aa5 Compare December 27, 2022 19:21
@cyrinux cyrinux requested review from timvaillancourt and removed request for a user and rashiq December 27, 2022 19:55
Copy link
Collaborator

@timvaillancourt timvaillancourt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyrinux thanks for the changes! Two more suggestions/questions added on 2nd thought, my apologies.

@cyrinux cyrinux force-pushed the feat/cle/add-optimizer-switch branch from 4c9061b to 73bbd54 Compare December 28, 2022 14:39
Copy link

dland commented Jan 3, 2023

For people who stumble upon this later on and want to know the reason from a DBA point of view (I worked with Cyril to put this together)...

The backstory behind this is that we have a stable application that has been running for over 15 years on Mysql. When we upgraded from 5.7 to 8.0, we observed some severe performance impacts on a certain set of queries. It would have been particularly hard to rewrite them (since they are generated by a query builder that no-one is particularly keen to hack on in case the change introduces other side effects).

We worked out that we could recover the original performance (at the expense of marginally slowing a few other types of queries, which was deemed to be an acceptable tradeoff) by setting optimizer_switch='prefer_ordering_index=off'. And then we were able to completely forget about the issue.

Fast forward to a recent gh-ost session where an execute would repeatedly crash with

[mysql] 2022年12月21日 16:32:48 packets.go:37: unexpected EOF

Why? Consider first, a source table:

CREATE TABLE thing (
 thing_id int not null auto_increment primary key
);

The table I was trying to ghost was, roughly:

CREATE TABLE thing_ref (
 thing_id int not null,
 ref_id int not null,
 primary key (thing_id, ref_id),
 key (ref_id)
);

In production, the thing table contains 165 million rows and the thing_ref table contains 1.43 billion rows (essentially, each thing can have a set of many refs, and we want to answer efficiently how many things have a particular ref).

When explaining the query that gh-ost issued to find the min/max migration values, it became apparent that it was using the index on ref_id instead of thing_id, ref_id which meant that, instead of being more or less instantaneous, mysql was doing a table scan over 1.43 billion rows to locate the min/max of thing_id, far too long to be acceptable. Reverting our local optimizer_switch hack (hence the original patch) restored the speed of the query back to something that was acceptable, and gh-ost was able to complete the migration of the table (taking 27 hours to do so!)

So yeah tl;dr issuing a force index should result in the desired behavior.

cyrinux reacted with thumbs up emoji

@cyrinux cyrinux reopened this Jul 1, 2023
@cyrinux cyrinux requested a review from morgo July 1, 2023 20:56
Copy link
Collaborator

@cyrinux / @dland the "force index" fix was merged in this PR #1237

I think this is still a good change however 👍

dland reacted with thumbs up emoji

Copy link
Collaborator

@cyrinux this fails on 5.7 CI on:

2023年12月09日 02:26:19 FATAL Error 1231: Variable 'optimizer_switch' can't be set to the value of 'prefer_ordering_index=on'

Copy link
Contributor

morgo commented Dec 9, 2023 via email

As I mentioned above, this was introduced post-GA. So it won't work with Aurora 2.0. I think this is a bit of a problem because 5.7 is no longer supported, but Aurora 2.0 is.
...
On Fri, Dec 8, 2023, 7:28 p.m. Tim Vaillancourt ***@***.***> wrote: @cyrinux <https://github.com/cyrinux> this fails on 5.7 CI on: 2023年12月09日 02:26:19 FATAL Error 1231: Variable 'optimizer_switch' can't be set to the value of 'prefer_ordering_index=on' — Reply to this email directly, view it on GitHub <#1236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAOE7RZP3VMP55MQKBFAH3YIPEGJAVCNFSM6AAAAAATKIJBZOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBYGA4TKNRTGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

Copy link

dland commented Dec 12, 2023

I would not worry about a CI failure on a version of Mysql that is no longer receiving upstream updates. The last MySQL General Availability version was 5.7.44, released on 2023年10月25日. The 5.7 branch is has been End of Life since then.

On 8.x you will get a similar error message if you provide a garbage value, so perhaps the best approach is to remind the user to consult the Mysql documentation for the list of values that --optimizer-switch accepts.

cyrinux reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timvaillancourt timvaillancourt Awaiting requested review from timvaillancourt timvaillancourt is a code owner

@morgo morgo Awaiting requested review from morgo

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /