MDEV-28911: Add --init-command option to mysqldump#4723
MDEV-28911: Add --init-command option to mysqldump#4723HNOONa-0 wants to merge 1 commit intoMariaDB:mainfrom
Conversation
grooverdan
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
Please consider extending the MDEV text with a full functional description of the new option so it could be documented well.
Also, please make sure the text of your commit message complies with CODING_STANDARDS.md.
Some answers to your questions below.
| --exec $MYSQL_DUMP --compact --skip-comments --skip-tz-utc --init-command="SET SESSION time_zone='+05:00'" 28911_mysqldump | ||
|
|
||
| DROP TABLE t1; | ||
| DROP DATABASE 28911_mysqldump; |
There was a problem hiding this comment.
Please consider adding a "--echo End of 13.0 tests" at the end. It's a convention we try to follow.
| if (opt_default_auth && *opt_default_auth) | ||
| mysql_options(con, MYSQL_DEFAULT_AUTH, opt_default_auth); | ||
|
|
||
| if (opt_init_command && *opt_init_command) |
There was a problem hiding this comment.
This is very much in line with the mysql CLI and other users of MYSQL_INIT_COMMAND do with it.
By itself, the option allows multiple init commands to be added. But none of the existing tools do that.
FWIW, I would consider adding a set of include files (call it e.g. common-cli-options) similar to sslopt-*.h, implementing this option in these and then replacing the implementation for individual tools with the right set of includes and calls. This will make it easier to include support for this option in all CLI tools and will ensure the functionality of that support will remain the same. But this is optional and the final reviewer will have a say on it.
3 participants
Description
A quick note on the implementation: I set it up so the command runs before mariadb-dump initializes its own default session settings. I originally thought it made more sense to put it after (so users could override the tool's internal defaults), but looking at the rest of the codebase, doing it before seems to be the established pattern. I decided to stick with the existing convention to keep things consistent. I also added a small MTR test file to verify the flag works as expected. Do you agree with this order of execution?
Right now, only the last
init-commandoption will run. Is it desirable to allow users to specify multipleinitcommands?Release Notes
Add --init-command option to
mysqldumpHow can this PR be tested?
Basing the PR against the correct MariaDB version
mainbranch.PR quality check