Re: [Maria-developers] 673ea509e2c: MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
Hi, Marko, On Jan 20, Marko Mäkelä wrote:
revision-id: 673ea509e2c (mariadb-10.2.40-230-g673ea509e2c) parent(s): d28d3aee108 author: Marko Mäkelä committer: Marko Mäkelä timestamp: 2022-01-20 16:13:08 +0200 message:
MDEV-26870 --skip-symbolic-links does not disallow .isl file creation
The InnoDB DATA DIRECTORY attribute is not implemented via symbolic links but something similar, *.isl files that contain the names of data files.
InnoDB failed to reject the DATA DIRECTORY attribute even though the server was started with --skip-symbolic-links.
Because TRUNCATE TABLE cannot easily return an error, it will keep rebuilding tables even if they carry the DATA DIRECTORY attribute.
diff --git a/mysql-test/suite/encryption/t/innodb-first-page-read.test b/mysql-test/suite/encryption/t/innodb-first-page-read.test index d661e4565d2..a0b3ca3f0ff 100644 --- a/mysql-test/suite/encryption/t/innodb-first-page-read.test +++ b/mysql-test/suite/encryption/t/innodb-first-page-read.test @@ -1,6 +1,7 @@ -- source include/have_innodb.inc -- source include/have_file_key_management_plugin.inc -- source include/not_embedded.inc +-- source include/have_symlink.inc
when can have_symlink be false? Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files.
--disable_warnings SET GLOBAL innodb_file_format = `Barracuda`; diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 5d78e64a06b..a8b16ba1eb0 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -11710,9 +11710,15 @@ create_table_info_t::create_options_are_invalid() break; }
- if (m_create_info->data_file_name - && m_create_info->data_file_name[0] != '\0' - && !create_option_data_directory_is_valid()) { + if (!m_create_info->data_file_name + || !m_create_info->data_file_name[0]) { + } else if (!my_use_symdir) { + push_warning( + m_thd, Sql_condition::WARN_LEVEL_WARN, + ER_ILLEGAL_HA_CREATE_OPTION, + "InnoDB: DATA DIRECTORY requires HAVE_SYMLINK."); + ret = "DATA DIRECTORY";
MyISAM does push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, WARN_OPTION_IGNORED, ER_THD(thd, WARN_OPTION_IGNORED), "DATA DIRECTORY"); Archive does if (create_info->data_file_name) my_error(WARN_OPTION_IGNORED, MYF(ME_WARNING), "DATA DIRECTORY"); I suggest you do one of the above too.
+ } else if (!create_option_data_directory_is_valid()) { ret = "DATA DIRECTORY"; }
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei,
On Thu, Jan 20, 2022 at 5:35 PM Sergei Golubchik
Hi, Marko,
On Jan 20, Marko Mäkelä wrote:
revision-id: 673ea509e2c (mariadb-10.2.40-230-g673ea509e2c)
[snip]
+-- source include/have_symlink.inc
when can have_symlink be false?
The tests where I added that line would fail after this change if I ran them with ./mtr --mysqld=--skip-symbolic-links
Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files.
In https://jira.mariadb.org/browse/MDEV-26870 you suggested using my_use_symdir, which I did. That variable is bound to the Boolean start-up parameter symbolic_links. Changing InnoDB to use actual symlinks (and to stop creating "databasename" directories, to be similar to other storage engines) would be a file format change that could break downgrades to earlier minor versions. Therefore, it is only doable in a development release.
+ push_warning( + m_thd, Sql_condition::WARN_LEVEL_WARN, + ER_ILLEGAL_HA_CREATE_OPTION, + "InnoDB: DATA DIRECTORY requires HAVE_SYMLINK."); + ret = "DATA DIRECTORY";
MyISAM does
push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, WARN_OPTION_IGNORED, ER_THD(thd, WARN_OPTION_IGNORED), "DATA DIRECTORY"); Archive does
if (create_info->data_file_name) my_error(WARN_OPTION_IGNORED, MYF(ME_WARNING), "DATA DIRECTORY");
I suggest you do one of the above too.
I had copied the reporting style of create_table_info_t::create_option_data_directory_is_valid(). I agree that it is better to flag warnings for the --skip-symbolic-links rather than errors. It will cause fewer surprises to users as well. Best regards, Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation
Hi, Marko, On Jan 21, Marko Mäkelä wrote:
Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files.
In https://jira.mariadb.org/browse/MDEV-26870 you suggested using my_use_symdir, which I did. That variable is bound to the Boolean start-up parameter symbolic_links.
Changing InnoDB to use actual symlinks (and to stop creating "databasename" directories, to be similar to other storage engines) would be a file format change that could break downgrades to earlier minor versions. Therefore, it is only doable in a development release.
That's not what I meant. I was saying, --skip-symbolic-links and filesystem support of symbolic links are unrelated. If --skip-symbolic-links was not specified, a filesystem can support symbolic links or not support them. In the latter case, I think, InnoDB technically still could use DATA DIRECTORY, even if MyISAM cannot. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei,
On Fri, Jan 21, 2022 at 10:45 AM Sergei Golubchik
Hi, Marko,
On Jan 21, Marko Mäkelä wrote:
Why InnoDB tests depend on server's symlink support? InnoDB does not use symlinks, it uses isl files.
In https://jira.mariadb.org/browse/MDEV-26870 you suggested using my_use_symdir, which I did. That variable is bound to the Boolean start-up parameter symbolic_links.
Changing InnoDB to use actual symlinks (and to stop creating "databasename" directories, to be similar to other storage engines) would be a file format change that could break downgrades to earlier minor versions. Therefore, it is only doable in a development release.
That's not what I meant. I was saying, --skip-symbolic-links and filesystem support of symbolic links are unrelated.
If --skip-symbolic-links was not specified, a filesystem can support symbolic links or not support them. In the latter case, I think, InnoDB technically still could use DATA DIRECTORY, even if MyISAM cannot.
That is true, the InnoDB attribute DATA DIRECTORY can be technically supported on any file system. The ticket MDEV-26870 gradually evolved to a request for InnoDB to be able to disable its DATA DIRECTORY feature. Quoting your comments:
This is not what skip-symbolic-links affects. It disables DATA/INDEX DIRECTORY support. You won't be able to create tables with DATA DIRECTORY or INDEX DIRECTORY attributes. In other words, the server won't create symlinks. But you can, and the server will access symlinked files all right. [snip] But, I think, users expect skip-symbolic-links to affect isl files too. Shall we treat it as a bug?
I think that this reasoning makes sense. If you think that my way of avoiding test failures with ./mtr --mysqld=--skip-symbolic-links is confusing, I am open to suggestions. Please find a revised fix at https://github.com/MariaDB/server/commit/c1d7b4575e67bd0ef458457859cdf7de32b... It is a little different, in that native ALTER TABLE in InnoDB will always retain any DATA DIRECTORY attribute while TRUNCATE will silently discard it. Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation
participants (2)
-
Marko Mäkelä -
Sergei Golubchik