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
Bug1646100 5.7 #1305
Bug1646100 5.7 #1305
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -949,6 +948,8 @@ class MYSQL_BIN_LOG: public TC_LOG | |||
@retval !=0 Error | |||
*/ | |||
int get_gtid_executed(Sid_map *sid_map, Gtid_set *gtid_set); | |||
private: | |||
void publish_global_status_variables(void) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe I would just changed the name of this function to publish_global_snapshot_status_variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm indeed not the best name. publish_position_for_global_status ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is better than the previous one. Just note that apart from position you are also publishing binlog_global_snapshot_file. Maybe publish_position_for_global_snapshot_status, but publish_position_for_global_status is good too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, "coordinates" then (a pair of name, position), making it publish_coordinates_for_global_status ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
I added only one comment. LGTM |
@robgolebiowski, please take a look at #1304 too for 5.6 |
In the existing binary log status variables implementation, MYSQL_BIN_LOG::set_status_variables is called with locked LOCK_status and then it attempts to lock LOCK_log. This means that e.g. a long-running commit blocks any SHOW STATUS, and the latter then blocks e.g. client connects/disconnects. While MySQL has not sorted the core server locks by the required locking order, there is no precedent in the core server of LOCK_status owners taking any other mutexes, and most LOCK_status critical sections there are short. This suggests that LOCK_status should come near the end of locking order, and so fix the issue by locking LOCK_log before LOCK_status. For that, create two new static variables binlog_global_snapshot_file and binlog_global_snapshot_position to binlog.cc that hold last "published" global binary log position. Add a new method MYSQL_BIN_LOG::publish_global_status_variables, that, under LOCK_log taken already, locks LOCK_status, and sets the global position variables. Call it in the group commit after flush phase, and optionally after binlog rotate. Change show_binlog_vars to read the global position variables if required (no snapshot; binlog open), and avoid taking LOCK_log there.
In 5.7 this also fixes bug 1657128 (Deadlock by concurrent SHOW BINLOGS, performance_schema.global_status query, and binlog purge). The merge adds the testcase and its required PFS debug sync point.
873e734
to
9a4a7a3
Compare
Updated method name |
LGTM |
http://jenkins.percona.com/job/mysql-5.7-param/584/
@robgolebiowski, please review as well