Improve included mysql configuration files

[morgo]

Fixes #4990
Fixes #3866
Fixes #5358

I recommend that we merge this after branching Vitess 4.0, since config files can break things.

This still requires SBR + only STRICT_TRANS_TABLES - two things I was hoping to eliminate, but are required for now so the test suite can pass.

Improvements:

• Rely more on auto-inclusion (there was a problem cleaning up the 5.6 file, because many 5.7 systems also included this file. With this turned off, the targeting is more specific.)
• Only sets a setting once (so if it is in global.cnf it won't be in a per-version file and vice versa). This helps with debugging. For example, previously you might remove the line binlog-format=STATEMENT, only to find there was another place it was set!
• Stops setting settings to the default value (so if a default value changes to be what was intended in 8.0, I will not put the value in default.cnf. It will be specified in each per-version config except 8.0). This can be important because the meaning or best value for a setting can change (for example MySQL 8.0 should use utf8mb4 because it is faster. The sort buffer allocates differently through versions.)
• If MySQL 8.0 prefers binlog_expire_logs_seconds, make sure it doesn't set expire_logs_days (causes a warning)

I have added myself as a CODEOWNER for the MySQL configuration files and mysqlctl/mysqld.go.

Important notes: This moves the worker test to shard 5. Which means it is no longer tested by CI. See comment by Sugu that this is deprecated.

Signed-off-by: Morgan Tocker tocker@gmail.com

[sougou]

Cleanup is looking really good.

[gaogaolulu]

Hello, I submitted one issue and one feature , hope it is helpful .

[gaogaolulu]

/mycnf/default.cnf 👍
binlog_format = ROW
basedir = /usr
lc_messages_dir=/usr/share/mysql
bind_address = 0.0.0.0

rpl_semi_sync_master_enabled = 1
rpl_semi_sync_master_timeout = 1000
rpl_semi_sync_slave_enabled = 1

/mycnf/master_mysql57.cnf
gtid_mode = ON
enforce_gtid_consistency = ON
log_bin= mysql-bin
log_slave_updates = ON
innodb_use_native_aio = 0

/mycnf/replica.cnf
log_slave_updates = ON

[morgo]

Note to reviewers:

I discovered this introduces a behavior change to Vitess, because the default sql-mode becomes more strict, in-line with MySQL's default behavior. The difference from MySQL however, is that changing the SQL mode in a user's session has no effect ( #5397 ). This means that some applications will break as they attempt to set the sql-mode to something non-strict and then create tables.

I am uneasy about both scenarios (differing defaults from MySQL, and breaking compatibility with packaged applications). What we could potentially do is revert back to a less strict sql-mode for now (move the sql-mode setting from default-fast.cnf back to default.cnf) and then implement a quick hack to always wrap set sql_mode='STRICT_TRANS_TABLES,NO_ENGINE_SUBSTITUTION' around DDL).

Suggestions welcome.

[derekperkins]

I'm also curious if an 8.0 config is going to be sufficient since they're adding a lot of functionality in patch releases. @msolters just found that MySQL 8.0.18 doesn't work, though I'm not sure if he root caused it

[derekperkins]

I think I'm also a little confused as to the purpose of these config files. Are we trying to be opinionated and try to force users to run MySQL the "correct" way or are we trying to make as few assumptions as possible and only setting what Vitess absolutely need to run? I feel like we're somewhere in the middle of those two now.

[morgo]

I think I'm also a little confused as to the purpose of these config files. Are we trying to be opinionated and try to force users to run MySQL the "correct" way or are we trying to make as few assumptions as possible and only setting what Vitess absolutely need to run? I feel like we're somewhere in the middle of those two now.

I would say we are closer to having no opinions/deferring opinions to upstream. That is the plan I intend to state in the docs vitessio/website#337

There are exceptions to that which are hard requirements (binary logging, gtids, utf8). The non-compulsory areas are mostly pre-existing:

long_query_time = 2
slow-query-log
skip-name-resolve
connect_timeout = 30
innodb_lock_wait_timeout = 20
max_allowed_packet = 64M

The rest is just (a) turning on a setting that becomes the default in a later MySQL release or (b) a re-organization to make sure that we don't set any setting twice, or set a setting to the default value for a version. Thus; every line should be doing something.

w.r.t. (a) You can see that certain settings disappear as MySQL defaults get better, such as 5.7 removing slave_net_timeout and 8.0 removing almost everything.

[morgo]

I'm also curious if an 8.0 config is going to be sufficient since they're adding a lot of functionality in patch releases. @msolters just found that MySQL 8.0.18 doesn't work, though I'm not sure if he root caused it

In the case that Vitess has fewer opinions about configuration, it is more likely to succeed with future unknown unknowns. But I'm curious what the breakage was all the same :-)

[sougou]

There is one test failing. The test basically wants to test if the retry vars are getting correctly exported if vtworker finds that mysql is down. However, the new changes have caused the worker to finish the job too quickly, before mysql is shutdown. So, the test fails because those retry vars don't get set.

I don't think this is a critical test. And, we're going to deprecate this. So, I'm ok to just delete the test, unless you can find an easy to slow down the worker enough to make it pass.