[MDEV-14978] Client programs to use $MARIADB_HOST consistently #2556

oceanli-hub · 2023-03-16T20:04:58Z

Description

Note: mysql_upgrade already uses environment variable as default. This feature was not added in for mysql_plugin and mysqltest

Only mysql client program was using $MYSQL_HOST as the default host.
Add the same feature in most other client programs but using $MARIADB_HOST instead.

How can this PR be tested?

Set up environment variable export MARIADB_HOST=your_test_server

Run each client program with the following commands:

client/mysqlslap -h another_test_server -P 3306 -u admin -p

client/mysqlslap -P 3306 -u admin -p

Verify that $MARIADB_HOST is used when -h is not specified. Also verify $MARIADB_HOST does not override command line argument.

Basing the PR against the correct MariaDB version

This is a new feature and the PR is based against the latest MariaDB development branch

Copyright

All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

CLAassistant · 2023-03-16T20:05:05Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

ottok · 2023-03-18T22:18:44Z

This is an improved take on #1067. Please review and merge, thanks!

grooverdan

If you're able to update the man pages for these client utilities too that would be appreciated.

client/mysqladmin.cc

grooverdan · 2023-03-24T02:44:22Z

mysql-test/suite/client/client-env-variable.test

+--let MYSQLDUMP = $MYSQL_BINDIR/client//mariadb-dump
+--let MYSQLIMPORT = $MYSQL_BINDIR/client//mariadb-import
+--let MYSQLSHOW = $MYSQL_BINDIR/client//mariadb-show
+--let MYSQLSLAP = $MYSQL_BINDIR/client//mariadb-slap


These are all set in mysql-test/mariadb-test-run.pl, though usually as with underscore like $MYSQL_CHECK.

The environment variables for the client programs have a default options file argument in them. The options file was stopping me from testing the client programs

In that case, they should probably be things like MARIABINLOG, etc...

mysql-test/suite/client/client-env-variable.test

grooverdan · 2023-03-24T02:47:23Z

mysql-test/suite/client/client-env-variable.test

+
+--echo ****************
+--echo # Setting MYSQL_HOST environment variable 
+--let MYSQL_HOST=nonexistent-sever


spelling "server"

grooverdan · 2023-03-24T02:49:28Z

mysql-test/suite/client/client-env-variable.test

+--exec $MYSQLIMPORT $options env_variable --local $MYSQL_TMP_DIR/pet > /dev/null 2>&1
+--exec $MYSQLSHOW $options env_variable > /dev/null 2>&1
+--exec $MYSQLSLAP $options > /dev/null 2>&1
+


Are there positive test examples using ~~MYSQL_HOST~~ MARIADB_HOST maybe with --protocol tcp?

oceanli-hub · 2023-04-18T17:22:47Z

Hi @grooverdan I updated my pull requested based off your feedback. I also updated the help function to include $MARIADB_HOST. In mysqlbinlog and mysqldump the memory is freed at the end during clean up so I was getting SIGSEV error when trying to use assignment. Assignments work for all the other client programs.

For mysql client program would you like me to change it to $MARIADB_HOST in this pr also?

Thanks for the review!

grooverdan · 2023-04-27T05:04:47Z

mysql-test/suite/client/client-env-variable.test

+# Clean up
+--echo Done
+DROP TABLE pet;
+--exec rm $MYSQL_TMP_DIR/tmp.sql


--remove_file for windows compatibility

grooverdan

For mysql client program would you like me to change it to $MARIADB_HOST in this pr also?

Sure, but in this case $MARIADB_HOST will be preferred over $MYSQL_HOST so:

if (!(tmp= getenv("MARIADB_HOST"))
  tmp= getenv("MYSQL_HOST")
...

grooverdan · 2023-04-27T05:16:39Z

client/mysqlcheck.c

@@ -437,6 +437,9 @@ static int get_options(int *argc, char ***argv)
 int ho_error;
 DBUG_ENTER("get_options");

+ if (current_host == NULL)
+ current_host = getenv("MARIADB_HOST");


Style fix (applies to all other code too),

current_host= getenv...

No space between var and =.

grooverdan · 2023-04-27T05:25:57Z

mysql-test/suite/client/client-env-variable.test

+EOF
+
+# Options for client program
+--let $options = --user=root --port=16000 --socket=$MYSQL_TMP_DIR/mysqld.1.sock


port and socket aren't fixed.

$MASTER_MYPORT and $MASTER_MYSOCK (from mysql-test/include/default_my.cnf - no include required - its a default).

Or, if you create a client-env-variable.cnf like:

!include include/default_my.cnf [ENV] MARIADB_HOST=localhost

This is a true environment variable.

Then $options to be --port=$MASTER_MYPORT (opmit socket so it doesn't get used) and use a positive test (no more --error) to ensure the connection is there.

no more --error

I am not sure if you want to remove the negative test case with --error. I added the positive test case but kept the negative test case in the new revision.

oceanli-hub · 2023-05-05T20:43:52Z

I pushed a new revision based off feedback. $MARIADB_HOST is added the mysql client program. For the MTR test, I created client-env-variable.cnf and added positive tests to show the connection is there.

LinuxJedi

Looking good, one more final cleanup pass and I think we are there.

LinuxJedi · 2024-02-19T11:20:49Z

client/mysqladmin.cc

@@ -374,6 +373,9 @@ int main(int argc,char *argv[])
 MYSQL mysql;
 char **commands, **save_argv, **temp_argv;

+ if (host==NULL)


Minor style thing, spaces around ==.

LinuxJedi · 2024-02-19T11:24:19Z

client/mysql.cc

@@ -1704,8 +1704,8 @@ static struct my_option my_long_options[] =
 &opt_local_infile, &opt_local_infile, 0, GET_BOOL, OPT_ARG, 0, 0, 0, 0, 0, 0},
 {"no-beep", 'b', "Turn off beep on error.", &opt_nobeep,
 &opt_nobeep, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
- {"host", 'h', "Connect to host.", &current_host,
- &current_host, 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
+ {"host", 'h', "Connect to host. If host is not specified, connect to localhost by default or $MARIADB_HOST",


I suspect it would read better if this said something along the lines of "Defaults in the following order: environment variables MARIADB_HOST, MYSQL_HOST, and then localhost."

LinuxJedi · 2024-02-19T11:25:12Z

client/mysql.cc

@@ -1704,8 +1704,8 @@ static struct my_option my_long_options[] =
 &opt_local_infile, &opt_local_infile, 0, GET_BOOL, OPT_ARG, 0, 0, 0, 0, 0, 0},
 {"no-beep", 'b', "Turn off beep on error.", &opt_nobeep,
 &opt_nobeep, 0, GET_BOOL, NO_ARG, 0, 0, 0, 0, 0, 0},
- {"host", 'h', "Connect to host.", &current_host,
- &current_host, 0, GET_STR_ALLOC, REQUIRED_ARG, 0, 0, 0, 0, 0, 0},
+ {"host", 'h', "Connect to host. If host is not specified, connect to localhost by default or $MARIADB_HOST",


Also, as a style thing, line is longer than 80 chars.

LinuxJedi · 2024-02-19T11:32:56Z

mysql-test/suite/client/client-env-variable.test

+--let MYSQLDUMP = $MYSQL_BINDIR/client//mariadb-dump
+--let MYSQLIMPORT = $MYSQL_BINDIR/client//mariadb-import
+--let MYSQLSHOW = $MYSQL_BINDIR/client//mariadb-show
+--let MYSQLSLAP = $MYSQL_BINDIR/client//mariadb-slap


In that case, they should probably be things like MARIABINLOG, etc...

Only `mysql` client program was using $MYSQL_HOST as the default host. Add the same feature in most other client programs but using $MARIADB_HOST instead. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.

oceanli-hub · 2024-04-10T00:01:47Z

@LinuxJedi thanks for the feedback, I pushed a new revision based off your comments. Please review again.

LinuxJedi

I think that is the bases covered now, many thanks.

grooverdan

looks good. Ready for 11.6 testing.

optional improvement of manual pages welcome.

oceanli-hub force-pushed the 11.0 branch 3 times, most recently from 138e8bf to 39dc8dd Compare March 16, 2023 23:42

grooverdan requested changes Mar 24, 2023

View reviewed changes

grooverdan self-assigned this Mar 29, 2023

grooverdan added license-bsd-new Contributed under BSD-new license need feedback Can the contributor please address the questions asked. labels Mar 29, 2023

oceanli-hub force-pushed the 11.0 branch from 39dc8dd to 5b535aa Compare April 18, 2023 00:03

oceanli-hub requested a review from ottok as a code owner April 18, 2023 00:03

oceanli-hub changed the base branch from 11.0 to 11.1 April 18, 2023 00:05

oceanli-hub requested a review from grooverdan April 18, 2023 00:07

oceanli-hub changed the title ~~[MDEV-14978] Client programs to use $MYSQL_HOST consistently~~ [MDEV-14978] Client programs to use $MARIADB_HOST consistently Apr 18, 2023

oceanli-hub force-pushed the 11.0 branch from 5b535aa to 645b308 Compare April 18, 2023 08:03

grooverdan reviewed Apr 27, 2023

View reviewed changes

oceanli-hub force-pushed the 11.0 branch from 645b308 to b6723c3 Compare May 4, 2023 07:21

oceanli-hub requested a review from grooverdan May 5, 2023 20:30

LinuxJedi mentioned this pull request Jul 25, 2023

MDEV-14978 Client programs to use $MYSQL_HOST consistently #1067

Closed

LinuxJedi requested changes Feb 19, 2024

View reviewed changes

oceanli-hub changed the base branch from 11.1 to 11.5 April 9, 2024 22:44

oceanli-hub force-pushed the 11.0 branch 2 times, most recently from 7ff6a6d to e9cc3dd Compare April 9, 2024 22:46

oceanli-hub force-pushed the 11.0 branch from e9cc3dd to c607c9d Compare April 9, 2024 22:46

oceanli-hub requested a review from LinuxJedi April 25, 2024 01:03

LinuxJedi approved these changes May 8, 2024

View reviewed changes

grooverdan approved these changes May 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MDEV-14978] Client programs to use $MARIADB_HOST consistently #2556

[MDEV-14978] Client programs to use $MARIADB_HOST consistently #2556

oceanli-hub commented Mar 16, 2023 •

edited

CLAassistant commented Mar 16, 2023 •

edited

ottok commented Mar 18, 2023

grooverdan left a comment

grooverdan Mar 24, 2023

oceanli-hub Mar 24, 2023

LinuxJedi Feb 19, 2024

grooverdan Mar 24, 2023

grooverdan Mar 24, 2023

oceanli-hub commented Apr 18, 2023

grooverdan Apr 27, 2023

grooverdan left a comment

grooverdan Apr 27, 2023

grooverdan Apr 27, 2023

oceanli-hub May 5, 2023 •

edited

oceanli-hub commented May 5, 2023

LinuxJedi left a comment

LinuxJedi Feb 19, 2024

LinuxJedi Feb 19, 2024

LinuxJedi Feb 19, 2024

LinuxJedi Feb 19, 2024

oceanli-hub commented Apr 10, 2024

LinuxJedi left a comment

grooverdan left a comment

[MDEV-14978] Client programs to use $MARIADB_HOST consistently #2556

Are you sure you want to change the base?

[MDEV-14978] Client programs to use $MARIADB_HOST consistently #2556

Conversation

oceanli-hub commented Mar 16, 2023 • edited

Description

How can this PR be tested?

Basing the PR against the correct MariaDB version

Copyright

CLAassistant commented Mar 16, 2023 • edited

ottok commented Mar 18, 2023

grooverdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oceanli-hub commented Apr 18, 2023

Choose a reason for hiding this comment

grooverdan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oceanli-hub May 5, 2023 • edited

Choose a reason for hiding this comment

oceanli-hub commented May 5, 2023

LinuxJedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oceanli-hub commented Apr 10, 2024

LinuxJedi left a comment

Choose a reason for hiding this comment

grooverdan left a comment

Choose a reason for hiding this comment

oceanli-hub commented Mar 16, 2023 •

edited

CLAassistant commented Mar 16, 2023 •

edited

oceanli-hub May 5, 2023 •

edited