MDEV-28911: Add --init-command option to mariadb-{,admin,dump,binlog,check,import, show,slap,test}#4723
MDEV-28911: Add --init-command option to mariadb-{,admin,dump,binlog,check,import, show,slap,test}#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.
|
failed tests work fine on my machine |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Thank you for taking the extra effort to implement the common headers!
Stand by for the final review.
HNOONa-0
force-pushed
the
MDEV-28911
branch
5 times, most recently
from
3ac70a9 to
339a7fb
Compare
|
Hello @gkodinov, I've implemented some of the modifications you suggested. For now, I've only applied them to a single client (mysql.cc) as a proof of concept. If you're happy with this selection of variables, I will apply them to other client files and maybe add a couple of tests too. |
It looks good. |
HNOONa-0
force-pushed
the
MDEV-28911
branch
2 times, most recently
from
8a59c52 to
44c0456
Compare
grooverdan
changed the title
grooverdan
left a comment
grooverdan
left a comment
There was a problem hiding this comment.
Nice refactorization. Where did the test case(s) that where previously there go? Can you include some form of --init-command in existing tests, e.g. mysql-test/main/mysqladmin.test and determine that this --init-command did have an affect (create new table?). Take note of the need for --replace-results as Windows executables have the .exe in the output.
Updating the man pages to include this option would be good too.
This sets up a good framework for readding bind address (#2750 - unfortunately reverted) as an option, for now or later.
| static char *opt_init_command = 0, *opt_client_plugin_dir = 0, *opt_default_auth = 0; | ||
|
|
||
| /* Options that are safe before any mysql_real_connect (including db=NULL). */ | ||
| static inline void SET_COMMON_CLI_VARS_EXCEPT_INIT(MYSQL *mysql) |
There was a problem hiding this comment.
note even for a static inline we don't use upper case function names.
| */ | ||
|
|
||
| /* Connection options */ | ||
| static uint opt_mysql_port = 0; |
There was a problem hiding this comment.
static variables are 0 by default. And if there was a need, the style would be opt_mysql_port= 0
standard client options into a unified header. This ensures consistent behavior for shared flags across all MariaDB clients. The following changes were made: - Added --init-command support to mysqldump. - Refactored 10 client tools (mysql, mysqldump, etc.) to use common-cli-longopts.h for shared variable declarations. - Removed redundant/duplicate option definitions to prevent future inconsistencies.
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