Skip to content
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

Support min_gtid annotation #2469

Merged
merged 3 commits into from
Jan 13, 2020
Merged

Support min_gtid annotation #2469

merged 3 commits into from
Jan 13, 2020

Conversation

kirs
Copy link
Contributor

@kirs kirs commented Jan 1, 2020

This PR is an attempt to get #2444 done. I'm probably missing a lot, and this is the first time I do C++, so I'd really appreciate the feedback.

How does it work?

  • This relies on ProxySQL tracking GTID of each backend, using https://github.com/sysown/proxysql_mysqlbinlog. That must be running on each MySQL host.
  • Each client keeps min_gtid of the data that it has written (maybe even leveraging session_track_gtids): in a cookie/session for web requests, and in Redis for batch jobs.
  • min_gtid is passed in a comment to ProxySQL, which lets it choose any read replica that satisfies that GTID.

#2444 (comment) has a diagram showing how that can be leveraged by a typical web app.

Questions:

  • What would be the best way to test this? I'm not sure I'm 100% following how ProxySQL test suite works.

I'm looking forward to your feedback ❤️

@kirs kirs changed the title Support min_gtid comment Support min_gtid annotation Jan 1, 2020
Copy link
Contributor

@alpes214 alpes214 left a comment

Choose a reason for hiding this comment

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

@kirs according to the logic the min_gtid takes precedence over gtid_from_hostgroup which is configured in query rules. It looks like correct behavior.

There several issues that should be resolved before merge.

lib/Query_Processor.cpp Outdated Show resolved Hide resolved
lib/MySQL_Session.cpp Outdated Show resolved Hide resolved
lib/MySQL_Session.cpp Outdated Show resolved Hide resolved
@kirs
Copy link
Contributor Author

kirs commented Jan 7, 2020

Thanks for review @Val214! I pushed another commit that adds GTID validation, to make sure that we only set qpo->min_gtid to legit value.

@pondix
Copy link
Contributor

pondix commented Jan 9, 2020

Automated message: PR pending admin approval for build testing

@alpes214
Copy link
Contributor

alpes214 commented Jan 9, 2020

@kirs, thank you for your changes but there is an issue that still should be resolved.

The is_valid_gtid() function handles now the situation when ':' is absent, but even in this case two parts of code Query_Processor::is_valid_gtid and MySQL_Session::handler___client_DSS_QUERY_SENT___server_DSS_NOT_INITIALIZED__get_connection can be changed independently in future and cause a crash of the proxysql. The NULL return value of the index() functions must be handled properly.

@kirs
Copy link
Contributor Author

kirs commented Jan 10, 2020

I somehow assumed that index() in handler___client_DSS_QUERY_SENT___server_DSS_NOT_INITIALIZED__get_connection was intentionally "unsafe" because at that point it's only supposed to accept valid GTIDs, but it makes sense to make it safe in both places in case it's changed in the future.

I pushed a commit that makes index() usage safe to NULL in that function. I hope I understand you correctly, but if not, I'm happy to follow up.

Thanks again for reviewing this.

in handler___client_DSS_QUERY_SENT___server_DSS_NOT_INITIALIZED__get_connection
@renecannao renecannao merged commit 3d2fd59 into sysown:v2.0.9 Jan 13, 2020
@renecannao
Copy link
Contributor

LGTM!
Thank you everybody!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants