-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
MDEV-32462: mysql_upgrade -s still checks for non system tables #2790
MDEV-32462: mysql_upgrade -s still checks for non system tables #2790
Conversation
f50d5ff
to
c1bd9ab
Compare
# Let's now load plugin first | ||
-- source include/have_utf8.inc | ||
# Don't load plugin in order to get ER_UNKNOWN_DATA_TYPE instead of ER_TABLE_NEEDS_REBUILD | ||
# -- source include/have_type_mysql_json.inc |
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.
why is this commented?
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.
In order to control the output for mariadb-dump
and error messages, it will not harm to stay or being removed or included.
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.
it looks like its ensuring that the plugin exists, so I'd say include it, at the top of the file.
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.
Please no commented out code. Either removed, or not commented. Looks like it should just be removed.
--copy_file std_data/mysql_json/mysql_json_test.frm $MYSQLD_DATADIR/mysql/mysql_json_test.frm | ||
--copy_file std_data/mysql_json/mysql_json_test.MYI $MYSQLD_DATADIR/mysql/mysql_json_test.MYI | ||
--copy_file std_data/mysql_json/mysql_json_test.MYD $MYSQLD_DATADIR/mysql/mysql_json_test.MYD | ||
--copy_file std_data/mysql_json/mysql_json_test.frm $MYSQLD_DATADIR/test/mysql_json_test.frm |
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.
Can you leave a small clarifying comment on why the test.mysql_json_test table is being placed there?
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.
There is comment in the --echo
above.
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.
mysql.mysql_json_test is examined, test.mysql_json_test doesn't appear to be anything done with it.
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.
Concur with @grooverdan.
Every line in a test file must be purposeful.
You don't actually check that test.mysql_json_test table is "upgraded" or not.
To make the test more readable:
- Don't use "use mysql".
- Run show create table test.mysql_json_test;
- Run show create table mysql.mysql_json_test;
- Do steps 2 and 3 both before and after the upgrade execution.
--remove_file $MYSQLD_DATADIR/mysql_upgrade_info | ||
|
||
select * from mysql.plugin; | ||
select * from mysql.general_log where argument like "%SELECT table_comment FROM information_schema.tables%"; |
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.
*
from general_log includes timestamp which is why bb fails. command_type, argument
would make the result repeatable.
@@ -0,0 +1,2 @@ | |||
--general_log |
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.
rather than set these as opts, I think you can set these around the --exec $MYSQL_UPGRADE
. And reset to previous value directly afterwards.
When doing so I think truncate mysql.general_log
will be required on cleanup.
This might prevent the warnings on shutdown:
2023-10-17 15:06:33 0 [Warning] Although a general log file was specified, log tables are used. To enable logging to files use the --log-output option.
2023-10-17 15:06:33 0 [Warning] Although a slow query log file was specified, log tables are used. To enable logging to files use the --log-output=file option.
client/mysql_upgrade.c
Outdated
@@ -1155,6 +1155,9 @@ static int install_used_plugin_data_types(void) | |||
DYNAMIC_STRING ds_result; | |||
const char *query = "SELECT table_comment FROM information_schema.tables" | |||
" WHERE table_comment LIKE 'Unknown data type: %'"; | |||
if (opt_systables_only) | |||
query= "SELECT table_comment FROM information_schema.tables" | |||
" WHERE table_schema='mysql' and table_comment LIKE 'Unknown data type: %'"; |
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.
good!
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.
Eh, not a big fan of repeating the query.
Suggested:
- const char *query = "SELECT table_comment FROM information_schema.tables"
- " WHERE table_comment LIKE 'Unknown data type: %'";
- if (opt_systables_only)
- query= "SELECT table_comment FROM information_schema.tables"
- " WHERE table_schema='mysql' and table_comment LIKE 'Unknown data type: %'";
+ char query[1024];
+ const char *extra_mysql_db_filter =
+ opt_systables_only ? "AND table_schema = 'mysql' " : "";
+ snprintf(query, sizeof(query),
+ "SELECT table_comment FROM information_schema.tables"
+ " WHERE table_comment LIKE 'Unknown data type: %%' %s",
+ extra_mysql_db_filter);
+
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.
IMHO it is readable and performance wise better to use string as in patch.
|
||
# Let's now load plugin first | ||
-- source include/have_utf8.inc | ||
# Don't load plugin in order to get ER_UNKNOWN_DATA_TYPE instead of ER_TABLE_NEEDS_REBUILD |
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.
To prevent such a requirement:
--error ER_UNKNOWN_DATA_TYPE,ER_TABLE_NEEDS_REBUILD
This would also make the select from mysql.plugin
unreliable, but I'm not sure that's a significant part of this test.
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.
Yes, not needed select
will remove.
c1bd9ab
to
aa3310d
Compare
Note
enable general log into tables only for |
aa3310d
to
d2e9dc7
Compare
client/mysql_upgrade.c
Outdated
@@ -1155,6 +1155,9 @@ static int install_used_plugin_data_types(void) | |||
DYNAMIC_STRING ds_result; | |||
const char *query = "SELECT table_comment FROM information_schema.tables" | |||
" WHERE table_comment LIKE 'Unknown data type: %'"; | |||
if (opt_systables_only) | |||
query= "SELECT table_comment FROM information_schema.tables" | |||
" WHERE table_schema='mysql' and table_comment LIKE 'Unknown data type: %'"; |
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.
Eh, not a big fan of repeating the query.
Suggested:
- const char *query = "SELECT table_comment FROM information_schema.tables"
- " WHERE table_comment LIKE 'Unknown data type: %'";
- if (opt_systables_only)
- query= "SELECT table_comment FROM information_schema.tables"
- " WHERE table_schema='mysql' and table_comment LIKE 'Unknown data type: %'";
+ char query[1024];
+ const char *extra_mysql_db_filter =
+ opt_systables_only ? "AND table_schema = 'mysql' " : "";
+ snprintf(query, sizeof(query),
+ "SELECT table_comment FROM information_schema.tables"
+ " WHERE table_comment LIKE 'Unknown data type: %%' %s",
+ extra_mysql_db_filter);
+
@@ -0,0 +1,92 @@ | |||
# |
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.
Not a big fan of the test name. Can we have two commits please of the form:
- Rename this test as mysql_upgrade_mysql_json_upgrade_system_tables (in this commit)
- Rename all mysql_json_mysql_upgrade_xxx tests to mysql_upgrade_mysql_json_xxx (in a separate commit)
This should make things consistent with the other test names in main suite related to mysql_upgrade.
# Let's now load plugin first | ||
-- source include/have_utf8.inc | ||
# Don't load plugin in order to get ER_UNKNOWN_DATA_TYPE instead of ER_TABLE_NEEDS_REBUILD | ||
# -- source include/have_type_mysql_json.inc |
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.
Please no commented out code. Either removed, or not commented. Looks like it should just be removed.
--copy_file std_data/mysql_json/mysql_json_test.frm $MYSQLD_DATADIR/mysql/mysql_json_test.frm | ||
--copy_file std_data/mysql_json/mysql_json_test.MYI $MYSQLD_DATADIR/mysql/mysql_json_test.MYI | ||
--copy_file std_data/mysql_json/mysql_json_test.MYD $MYSQLD_DATADIR/mysql/mysql_json_test.MYD | ||
--copy_file std_data/mysql_json/mysql_json_test.frm $MYSQLD_DATADIR/test/mysql_json_test.frm |
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.
Concur with @grooverdan.
Every line in a test file must be purposeful.
You don't actually check that test.mysql_json_test table is "upgraded" or not.
To make the test more readable:
- Don't use "use mysql".
- Run show create table test.mysql_json_test;
- Run show create table mysql.mysql_json_test;
- Do steps 2 and 3 both before and after the upgrade execution.
|
||
# Let's now load plugin first | ||
-- source include/have_utf8.inc | ||
--source include/not_embedded.inc |
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.
Please be consistent, either no spaces after --
or spaces after --
. I suggest no space after --
anywhere in the file.
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
d2e9dc7
to
9ca9e20
Compare
@cvicentiu , @grooverdan thanks, updated according to the review. |
|
||
# User table is not upgraded in `mysql\test` DB, so we cannot see it. | ||
--error ER_UNKNOWN_DATA_TYPE | ||
show create table mysql.mysql_json_test; |
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.
This is not good then. I thought that we'd be upgrading mysql database tables after loading the MYSQL_JSON plugin.
If mysql_upgrade is now run with -s parameter, it leads to this current logic that makes no sense:
- Try to check all tables in mysql database and upgrade them. This will fail for any tables with MYSQL_JSON datatype.
- Check if we need to load MYSQL_JSON plugin for any tables in mysql database.
- Don't do anything else.
Observing this, let's just skip loading MYSQL_JSON for mysql database, like you @an3l proposed initially,
static int install_used_plugin_data_types(void)
{
....
if (opt_systables_only)
return 0;
....
}
There's no point in actually going through all that logic, loading the plugin, unloading it, to actually not do anything in the end.
To make sure, I checked all MySQL's tables in 5.7:
mysql> select distinct(data_type) from information_schema.columns where table_schema='mysql' order by 1;
+------------+
| data_type |
+------------+
| bigint |
| blob |
| char |
| datetime |
| enum |
| float |
| int |
| longblob |
| mediumblob |
| mediumtext |
| set |
| smallint |
| text |
| time |
| timestamp |
| tinyint |
| varchar |
+------------+
So we should be safe ignoring systables in this case.
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.
Updated.
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
9ca9e20
to
61b158f
Compare
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
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.
Ok to push. Please check BB if we missed any corner case.
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR #2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR #2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
- Rename files as requested by Vicentiu: ``` mysql_json_mysql_upgrade.test -> mysql_upgrade_mysql_json.test mysql_json_mysql_upgrade_with_plugin_loaded.test -> mysql_upgrade_mysql_json_with_plugin_loaded.test mysql_json_mysql_upgrade_system_tables.test -> mysql_upgrade_mysql_json_system_tables.test ``` - Related to PR MariaDB#2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
61b158f
to
6d12330
Compare
- Prevent opening of any user tables in case `upgrade-system-table` option is used. - Still there may be uninstalled data types in `mysql` system table so allow it to perform. - Closes PR #2790 - Reviewer: <daniel@mariadb.org>, <vicentiu@mariadb.org>
How can this PR be tested?
$ cat ~/.my105.cnf [mariadb] datadir=/tmp/10.5 lc_messages_dir=/home/anel/GitHub/mariadb/server/build/10.5/sql/share general_log=1 general_log_file=patch.log plugin_load_add=type_mysql_json.so plugin_dir=/home/anel/GitHub/mariadb/server/build/10.5/plugin/type_mysql_json/
test
(user) andmysql
(system) databases in MariaDBmariadb_upgrade --force
mariadb_upgrade --force
general_log_file
(make sure proper query is executed only onmysql
)General log file
Basing the PR against the correct MariaDB version
PR quality check