- developers - lists.mariadb.org

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 26 Dec '14

26 Dec '14

Hi, Sergey! On Dec 26, svoj(a)mariadb.org wrote: > revision-id: f65901eef28728212a350f61569dc884c747bffc > parent(s): 8c616cd347aa7b3fcc2d8defb5e172ad316f432c > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-26 23:38:45 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > Autosize log-tc-size, so that it's min value= page size * 3, default > value= page size * 6, block size= page size. > > diff --git a/sql/log.cc b/sql/log.cc > index 66e1426..52c85d6 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8780,6 +8779,7 @@ int TC_LOG_MMAP::sync() > int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > { > pending_cookies *full_buffer= NULL; > + uint32 ncookies= tc_log_page_size / sizeof(my_xid); > DBUG_ASSERT(*(my_xid *)(data+cookie) == xid); > > /* > @@ -8793,7 +8793,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > mysql_mutex_lock(&LOCK_pending_checkpoint); > if (pending_checkpoint == NULL) > { > - uint32 size= sizeof(*pending_checkpoint); > + uint32 size= sizeof(*pending_checkpoint) + sizeof(ulong) * (ncookies - 1); > if (!(pending_checkpoint= > (pending_cookies *)my_malloc(size, MYF(MY_ZEROFILL)))) > { > @@ -8804,8 +8804,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > } > > pending_checkpoint->cookies[pending_checkpoint->count++]= cookie; > - if (pending_checkpoint->count == sizeof(pending_checkpoint->cookies) / > - sizeof(pending_checkpoint->cookies[0])) > + if (pending_checkpoint->count == ncookies) > { > full_buffer= pending_checkpoint; > pending_checkpoint= NULL; > @@ -8839,7 +8838,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > if (count == 0) > { > uint i; > - for (i= 0; i < sizeof(pending->cookies)/sizeof(pending->cookies[0]); ++i) > + for (i= 0; i < tc_log_page_size / sizeof(my_xid); ++i) I wonder why you didn't use ncookies here :) > delete_entry(pending->cookies[i]); > my_free(pending); > } Regards, Sergei

2 1

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 26 Dec '14

26 Dec '14

Hi, Sergey! Yes, that's what I had in mind, thanks! There are few rough edges, see below. Ok to push after you fix that, there's no need for another review round. On Dec 26, svoj(a)mariadb.org wrote: > revision-id: 72e58dc2b1a0bb859e918de0ff820d5a2a92dc32 > parent(s): db89dd3a8f7b0d868946d25ba98c6f88612d309a > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-26 17:03:45 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > Autosize log-tc-size, so that it's min value= page size * 6, default > value= page size * 6, block size= page size. > > --- > mysql-test/r/mysqld--help.result | 1 - > mysql-test/suite/sys_vars/r/log_tc_size_basic.result | 4 ++++ > .../suite/sys_vars/r/sysvars_server_notembedded.result | 14 ++++++++++++++ > mysql-test/suite/sys_vars/t/log_tc_size_basic.test | 5 +++++ > mysql-test/t/mysqld--help.test | 2 +- > sql/log.cc | 5 ++--- > sql/log.h | 3 +-- > sql/mysqld.cc | 7 +------ > sql/sys_vars.cc | 11 +++++++++++ > 9 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/mysql-test/suite/sys_vars/t/log_tc_size_basic.test b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test > new file mode 100644 > index 0000000..1fce212 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test > @@ -0,0 +1,5 @@ > +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE > +SET GLOBAL log_tc_size=1; > + > +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE do we ever build on platforms without mmap? if the answer is "no" - you don't need ER_UNKNOWN_SYSTEM_VARIABLE here > +SET SESSION log_tc_size=1; > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > index 006b9a9..5491169 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -1757,6 +1757,20 @@ NUMERIC_BLOCK_SIZE NULL > ENUM_VALUE_LIST innodb,query_plan,explain > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME LOG_TC_SIZE > +SESSION_VALUE NULL > +GLOBAL_VALUE 24576 > +GLOBAL_VALUE_ORIGIN AUTO > +DEFAULT_VALUE 24576 > +VARIABLE_SCOPE GLOBAL > +VARIABLE_TYPE BIGINT UNSIGNED > +VARIABLE_COMMENT Size of transaction coordinator log. > +NUMERIC_MIN_VALUE 24576 > +NUMERIC_MAX_VALUE 18446744073709551615 > +NUMERIC_BLOCK_SIZE 4096 > +ENUM_VALUE_LIST NULL > +READ_ONLY YES > +COMMAND_LINE_ARGUMENT REQUIRED if the answer is "yes" - this test will fail there. even if the answer is "no", the variable value is architecture dependent. But it's good to see all stable variable metadata (e.g. that GLOBAL_VALUE_ORIGIN is AUTO). > VARIABLE_NAME LOG_WARNINGS > SESSION_VALUE 1 > GLOBAL_VALUE 1 > diff --git a/sql/log.cc b/sql/log.cc > index 66e1426..6d6512e 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8793,7 +8792,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid) > mysql_mutex_lock(&LOCK_pending_checkpoint); > if (pending_checkpoint == NULL) > { > - uint32 size= sizeof(*pending_checkpoint); > + uint32 size= sizeof(*pending_checkpoint) + tc_log_page_size / sizeof(my_xid); that's not enough. There are further sizeof's, like if (pending_checkpoint->count == sizeof(pending_checkpoint->cookies) / sizeof(pending_checkpoint->cookies[0])) and for (i= 0; i < sizeof(pending->cookies)/sizeof(pending->cookies[0]); ++i) I've found these two by looking for "sizeof" in the Kristian's patch that introduced pending_checkpoint. There can be more sizeofs added later. > if (!(pending_checkpoint= > (pending_cookies *)my_malloc(size, MYF(MY_ZEROFILL)))) > { > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index fb11377..bfaf7d7 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -5202,3 +5202,14 @@ static Sys_var_mybool Sys_strict_password_validation( > "that cannot be validated (passwords specified as a hash)", > GLOBAL_VAR(strict_password_validation), > CMD_LINE(OPT_ARG), DEFAULT(TRUE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > + > +#ifdef HAVE_MMAP > +static Sys_var_ulong Sys_log_tc_size( > + "log_tc_size", > + "Size of transaction coordinator log.", > + READ_ONLY GLOBAL_VAR(opt_tc_log_size), > + CMD_LINE(REQUIRED_ARG), > + VALID_RANGE(my_getpagesize() * 6, ULONG_MAX), > + DEFAULT(my_getpagesize() * 6), > + BLOCK_SIZE(my_getpagesize())); I think, min value could be my_getpagesize()*3 ? > +#endif Regards, Sergei

2 1

Re: [Maria-developers] MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
by Sergei Golubchik 25 Dec '14

25 Dec '14

Hi, Sergey! On Dec 17, svoj(a)mariadb.org wrote: > revision-id: 4a469b0ed68bb341e1a1e1ba540a45a3b6ff3ef9 > parent(s): ea8862aa9ffa1cfac9a99bbe0fe9b539f12b9a19 > committer: Sergey Vojtovich > branch nick: 10.1 > timestamp: 2014-12-17 13:55:01 +0400 > message: > > MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64 > > log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size > must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open() > with a comment: to guarantee non-empty pool. > > This all makes server not startable in default configuration on PPC64. > > When creating tc log, adjust it's size to max(log-tc-size, page size * 6). > > --- > sql/log.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/sql/log.cc b/sql/log.cc > index 24e5283..a2fd70f 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -8316,7 +8316,7 @@ int TC_LOG_MMAP::open(const char *opt_name) > O_RDWR, MYF(MY_WME))) < 0) > goto err; > inited=1; > - file_length= opt_tc_log_size; > + file_length= MY_MAX(tc_log_page_size * 2 * 3, opt_tc_log_size); > if (mysql_file_chsize(fd, file_length, 0, MYF(MY_WME))) > goto err; > } I don't particularly like silent adjustments of user-specified values. And anyway, as you found out, this fix is not sufficient. I'd suggest the following: 1. Create a sysvar for log-tc-size 2. invoke my_getpagesize() not from log.cc, but from mysqld.cc, and set min and default values for log-tc-size appropriately, don't forget to use SYSVAR_AUTOSIZE() 3. change the code not to depend on the TC_LOG_PAGE_SIZE constant. - it's basically used in only two places now: to set a default for TC_LOG_MIN_SIZE (but you'll fix it step 2) and in kristian's code for group commit, cookies[] array. - don't mind TC_LOG_PAGE_SIZE being used in TC_LOG_BINLOG, it means nothing there, any reasonable number will do, I suppose. - so basically you only need to remove TC_LOG_PAGE_SIZE from cookies[] array. That is, you can define it as 'ulong cookies[1]' and take care to fix all sizeof() in the code to add the real size there. 4. Then you can remove that assert. The tc log page will be of my_getpagesize() size and log-tc-size will be N*my_getpagesize() where N>3. Regards, Sergei

1 0

[Maria-developers] JIRA ping
by Jonas Oreland 23 Dec '14

23 Dec '14

Hi Kristian, I light of recent "I did see any of your comments until by accident just now", I now mail you directly that I updated both MDEV-162 and MDEV-7257. I want (review) comments. /Jonas

2 9

[Maria-developers] Review of Debian packaging file listing?
by Otto Kekäläinen 22 Dec '14

22 Dec '14

Hello! I recently updated my build scripts to automatically generate a file listing for each commit in in the Debian packaging "pool" I have. Here is a recent one: http://labs.seravo.fi/~otto/mariadb-repo/mariadb-10.0-sid-amd64/filelist-ec… It is now very easy to review and track what files are in what package. Could somebody please quickly review that file and tell me if you spot: - any files in wrong package and should be moved? - any package that has too few files and maybe something missing? - any file attributes that might be wrong or insecure? - should any of the MariaDB plugins deserve to be shipped as a separate package? (apart from current connect and oqgraph) -- Check out our blog at http://seravo.fi/blog and follow @ottokekalainen

4 5

[Maria-developers] Report from Debian/Ubuntu MySQL packaging sprint in London in December
by Otto Kekäläinen 21 Dec '14

21 Dec '14

Hello! I thought I'd brief the readers of this mailing list about the packaging sprint in London last week. It was organized by Canonical and in addition to Canonical staff and me there where also people from Oracle and Percona. We mostly worked on syncing the MySQL/MariaDB/Percona server packaging and a little bit also on creating a unified Galera package. We made good progress and all results have been pushed to public repos. I also recently updated the pages https://wiki.debian.org/Teams/MySQL/MariaDB and https://wiki.debian.org/Teams/MySQL/Galera to reflect the current status. One big technical change that was discussed and planned is the decoupling of the config and data directories. The plan is that in the future no MySQL variant in particular would anymore own the /etc/mysql/my.cnf file but instead each variant would register their own config file at installation time by using the Debian update-alternatives mechanism. So in future there would be /etc/mysql/mariadb.conf, mysql.conf, percona.conf etc and some more snippets in /etc/mysql/mariadb.conf.d/.. etc. The plan is not complete nor tested, but there are preliminary changes in the mysql-common package to provide the config file registration hook and in a preliminary Percona Debian package to use the new config file registration facility. As the config file defines the data directory, successful decoupling of config files would make it easy to also decouple the data directories so that if the user installs/removes/upgrades any of the MySQL variants they would not mess up their data accidentally and the installation scripts would help users to dump/migrate/copy/move their data in the common scenarios. -- Check out our blog at http://seravo.fi/blog and follow @ottokekalainen

2 1

[Maria-developers] CREATE USER gotcha
by Alexander Barkov 19 Dec '14

19 Dec '14

Hi Ian, I think there is a CREATE USER related gotcha which is not described in the manual. CREATE USER allows to create multiple users at the same time: CREATE USER user1, user2; A reader of https://mariadb.com/kb/en/mariadb/documentation/sql-commands/account-manage… might think that the above query is a short synonym for: CREATE USER user1; CREATE USER user2; But in fact it is not always true. The difference can happen during replication. A "CREATE USER" statement is binlogged as a single shot, but only if at least one user was really created. Suppose (on master) user1 already exists, while user2 does not. "CREATE USER user1, user2" will be binary logged as is, while two independent "CREATE USER user1" and "CREARE USER user2" statements will be logged only as "CREATE USER user2". The CREATE statement for user1 won't be logged, because it failed on master! Now, suppose non of the users exist on slave. In case of "CREATE USER user1, user2;" both will be created on slave. In case of two independent queries, only "user2" will be created on slave.

4 6

[Maria-developers] Syntax for parallel replication
by Kristian Nielsen 17 Dec '14

17 Dec '14

Hi Serg, Can you help me with suggestions/comments for the following proposal for how to do configuration for MDEV-6676, speculative parallel replication? I am not too happy about how things are in 10.0. There, there is a single option --slave-parallel-threads=N. If N>0, then parallel replication is enabled, else not. One problem is that this makes it not configurable per multi-source master connection. Another problem is that there are two possible mechanisms for parallelisation, group-commit based and domain_id based; currently, one can enable none of them or both, but not just one of them. MDEV-6676 will introduce at least one other mechanism, which seems to make it essential to make a better way to configure this. Now, ideally, there would not be any configuration at all. The server would just run things in parallel when possible and to the degree desirable. However, I think there are some reasons that we need to allow the user this configurability: - Parallel replication is still a somewhat experimental feature, so it seems too risky to enable it by default. Also, it doesn't really seem possible for the server to automatically set the best number of threads to use, with current implementation (or possibly any implementation). - When replicating with non-transactional updates, or in non-gtid mode, slave state is not crash safe. This is true in non-parallel replication also, but in parallel replication, the problem seems amplified, as there may be multiple transactions in progress at the time of a crash, complicating possible manual recovery. This also suggests that parallel replication must be configurable. - When using domain-based parallel replication, the user is responsible for ensuring that independent domains are non-conflicting and can be replicated out-of-order wrt. each other. So if replication domains are used, but this property is not guaranteed, then domain-based parallel replication need to be configurable, or parallel replication cannot be used at all. - The new speculative replication feature in MDEV-6676 is not always guaranteed to be a win - in some workloads, where there are many conflicts between successive transactions, excessive rollback could cause it to be less efficient than not using it. Again, this suggests it needs to be configurable. So given this, I came up with the following idea for syntax: CHANGE MASTER TO PARALLEL_MODE=(domain,groupcommit,transactional,waiting) Each of the four keywords in the parenthesis is optional. "domain" enables domain-based parallelisation, where each replication domain is treated independently. "groupcommit" enables the non-speculative mode, where only transactions that group-committed together on the master are applied in parallel on the slave. "transactional" enables the speculative mode, where all transactional DML is optimistically tried in parallel, and then in case of conflict a rollback and retry is done. "groupcommit" and "transactional" are mutually exclusive, at most one of them can be specified. The default would be (domain,groupcommit) to be back-wards compatible with 10.0. If slave_parallel_thread=0, then no parallel apply will happen even if PARALLEL_MODE is non-empty. If slave_parallel_thread>0 but PARALLEL_MODE is empty (PARALLEL_MODE=()), then again no parallel apply will be done. The "waiting" option is not essential to add, we could remove it, I put it in because there were already a number of options so it seemed to cause no harm. The idea is that on the master, we detect if transaction T2 had to do a row lock wait on transaction T1. If so, it seems likely that a similar conflict could occur on the slave, so we will not run T2 in parallel with T1; instead we will let T2 wait for T1 to commit before T2 is started. The "waiting" option could be enabled by a user to disable this check, enabling even more aggressive parallelisation. I am not sure if it is worth it to have this configurable though, comments welcome. I have not checked how hard it will be to implement the new syntax. We do not have any similar multi-option CHANGE MASTER elements, as far as I know, but it is similar to ENUM system variables. And we already have a IGNORE_SERVER_IDS syntax with comma-separated list within parenthesis. So hopefully not too hard to do, and somewhat consistent with existing syntax. So what do you think? Is it a reasonable syntax? Any comments, or suggestions for better way to do it? Thanks, - Kristian.

5 23

Re: [Maria-developers] Rev 4383: MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views
by Sergei Golubchik 17 Dec '14

17 Dec '14

Hi, Sanja! Comments about the code, see below. Comments about the whole approach - see my other email. On Dec 07, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/ > > ------------------------------------------------------------ > revno: 4383 > revision-id: sanja(a)askmonty.org-20141207094825-tx163unk3mm4gkql > parent: sanja(a)askmonty.org-20141203234414-63zkpeq9eb0n9lc1 > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Sun 2014-12-07 10:48:25 +0100 > message: > MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views > (SQL-level commands to support view update) > === modified file 'mysql-test/r/view.result' > --- a/mysql-test/r/view.result 2014-12-03 22:37:59 +0000 > +++ b/mysql-test/r/view.result 2014-12-07 09:48:25 +0000 > @@ -5398,6 +5398,29 @@ DROP VIEW v1; > DROP TABLE t1, t2; > create view v1 as select 1; > drop view v1; > +# > +#MDEV-6916:Upgrade from MySQL to MariaDB breaks already created views > +# (SQL command for upgrade check) > +# > +create table t1 (a int); > +create algorithm=temptable view v1 as select a from t1; > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci > +check table v1 force mariadb upgrade; > +Table Op Msg_type Msg_text > +test.v1 check note View algorithm swap > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=MERGE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci No, I don't like that. FORCE or not, upgrading of a view should not blindly change the alorithm. This functionality is created to solve a specific problem. So when a view has mariadb-version field we know with certainty that the algorithm is correct and it should *not* be changed with or without FORCE. In the test you'll need to copy mysql-created view frm (from std_data). Addendum: ok, I've read till the end. See my comment in view_check() function. > +check table v1 force mariadb upgrade; Also, I don't like that CHECK changes the algorithm of the view. For tables CHECK only performs the *check* (and updates the version in the frm). It's REPAIR that does the change. To be consistent with the behavior for tables, CHECK ... FOR UPGRADE should only report an error, like "view needs upgrading" and a user should then use REPAIR to actually fix the view. > +Table Op Msg_type Msg_text > +test.v1 check note View algorithm swap > +show create view v1; > +View Create View character_set_client collation_connection > +v1 CREATE ALGORITHM=TEMPTABLE DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select `t1`.`a` AS `a` from `t1` latin1 latin1_swedish_ci > +drop view v1; > +drop table t1; > # ----------------------------------------------------------------- > # -- End of 5.5 tests. > # ----------------------------------------------------------------- > > === modified file 'sql/share/errmsg-utf8.txt' > --- a/sql/share/errmsg-utf8.txt 2014-05-27 06:45:01 +0000 > +++ b/sql/share/errmsg-utf8.txt 2014-12-07 09:48:25 +0000 > @@ -6565,3 +6565,5 @@ ER_QUERY_EXCEEDED_ROWS_EXAMINED_LIMIT > ER_NO_SUCH_TABLE_IN_ENGINE 42S02 > eng "Table '%-.192s.%-.192s' doesn't exist in engine" > swe "Det finns ingen tabell som heter '%-.192s.%-.192s' i handlern" > +ER_VIEW_UPGRADE_IS_DONE > + eng "View algorithm swap" No way, you cannot add a new error message in 5.5 because the next major branch (10.0) is already GA. > === modified file 'sql/sql_admin.cc' > --- a/sql/sql_admin.cc 2014-05-03 16:12:17 +0000 > +++ b/sql/sql_admin.cc 2014-12-07 09:48:25 +0000 > @@ -314,7 +314,8 @@ static bool mysql_admin_table(THD* thd, > HA_CHECK_OPT *), > int (handler::*operator_func)(THD *, > HA_CHECK_OPT *), > - int (view_operator_func)(THD *, TABLE_LIST*)) > + int (view_operator_func)(THD *, TABLE_LIST*, > + HA_CHECK_OPT *)) > { > TABLE_LIST *table; > SELECT_LEX *select= &thd->lex->select_lex; > @@ -521,7 +522,7 @@ static bool mysql_admin_table(THD* thd, > ER_CHECK_NO_SUCH_TABLE, ER(ER_CHECK_NO_SUCH_TABLE)); > /* if it was a view will check md5 sum */ > if (table->view && > - view_checksum(thd, table) == HA_ADMIN_WRONG_CHECKSUM) > + view_check(thd, table, check_opt) == HA_ADMIN_WRONG_CHECKSUM) > push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN, > ER_VIEW_CHECKSUM, ER(ER_VIEW_CHECKSUM)); > if (thd->stmt_da->is_error() && > @@ -536,7 +537,7 @@ static bool mysql_admin_table(THD* thd, > if (table->view) > { > DBUG_PRINT("admin", ("calling view_operator_func")); > - result_code= (*view_operator_func)(thd, table); > + result_code= (*view_operator_func)(thd, table, check_opt); > goto send_result; > } > > @@ -849,6 +850,14 @@ send_result_message: > system_charset_info); > break; > } > + case HA_ADMIN_VIEW_UPGRADE_IS_DONE: > + { > + protocol->store(STRING_WITH_LEN("note"), system_charset_info); > + protocol->store(ER(ER_VIEW_UPGRADE_IS_DONE), > + strlen(ER(ER_VIEW_UPGRADE_IS_DONE)), > + system_charset_info); > + break; > + } Don't do that. First, you cannot have ER_VIEW_UPGRADE_IS_DONE. Second, it's simply unnecessary. Return HA_ADMIN_OK from the view_operator_func() and use push_warning inside view_operator_func() to say that the algorithm was changed. > case HA_ADMIN_NEEDS_UPGRADE: > case HA_ADMIN_NEEDS_ALTER: > @@ -1071,7 +1080,7 @@ bool Check_table_statement::execute(THD > > res= mysql_admin_table(thd, first_table, &m_lex->check_opt, "check", > lock_type, 0, 0, HA_OPEN_FOR_REPAIR, 0, > - &handler::ha_check, &view_checksum); > + &handler::ha_check, &view_check); > > m_lex->select_lex.table_list.first= first_table; > m_lex->query_tables= first_table; > > === modified file 'sql/sql_view.cc' > --- a/sql/sql_view.cc 2014-12-03 23:44:14 +0000 > +++ b/sql/sql_view.cc 2014-12-07 09:48:25 +0000 > @@ -729,9 +729,29 @@ err: > DBUG_RETURN(res || thd->is_error()); > } > > +static void make_view_filename(LEX_STRING *dir, char *dir_buff, > + size_t dir_buff_len, > + LEX_STRING *path, char *path_buff, > + size_t path_buff_len, > + LEX_STRING *file, > + TABLE_LIST *view) > +{ > + /* print file name */ > + dir->length= build_table_filename(dir_buff, dir_buff_len - 1, > + view->db, "", "", 0); > + dir->str= dir_buff; > + > + path->length= build_table_filename(path_buff, path_buff_len - 1, > + view->db, view->table_name, reg_ext, 0); > + path->str= path_buff; > + > + file->str= path->str + dir->length; > + file->length= path->length - dir->length; > +} > + > > /* number of required parameters for making view */ > -static const int required_view_parameters= 15; > +static const int required_view_parameters= 16; eh? why? > /* > table of VIEW .frm field descriptors > @@ -795,6 +815,65 @@ static File_option view_parameters[]= > static LEX_STRING view_file_type[]= {{(char*) STRING_WITH_LEN("VIEW") }}; > > > +int mariadb_fix_view(THD *thd, TABLE_LIST *view) > +{ > + char dir_buff[FN_REFLEN + 1], path_buff[FN_REFLEN + 1]; > + LEX_STRING dir, file, path; > + DBUG_ENTER("mariadb_fix_view"); > + > + if (view->algorithm == VIEW_ALGORITHM_UNDEFINED) > + DBUG_RETURN(HA_ADMIN_OK); > + > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > + /* init timestamp */ > + if (!view->timestamp.str) > + view->timestamp.str= view->timestamp_buffer; > + > + /* check old .frm */ > + { > + char path_buff[FN_REFLEN]; > + LEX_STRING path; > + File_parser *parser; > + > + path.str= path_buff; > + fn_format(path_buff, file.str, dir.str, "", MY_UNPACK_FILENAME); > + path.length= strlen(path_buff); > + if (access(path.str, F_OK)) > + DBUG_RETURN(HA_ADMIN_INVALID); > + > + if (!(parser= sql_parse_prepare(&path, thd->mem_root, 0))) > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + > + if (!parser->ok() || !is_equal(&view_type, parser->type())) > + DBUG_RETURN(HA_ADMIN_INVALID); > + } > + > + DBUG_ASSERT(view->algorithm == VIEW_ALGORITHM_MERGE || > + view->algorithm == VIEW_ALGORITHM_TMPTABLE); if() instead of an assert, please. while unlikely, it's possible that the file would be changed between two opens. > + if (view->algorithm == VIEW_ALGORITHM_MERGE) > + view->algorithm= VIEW_ALGORITHM_TMPTABLE; > + else > + view->algorithm= VIEW_ALGORITHM_MERGE; > + > + if (sql_create_definition_file(&dir, &file, view_file_type, > + (uchar*)view, view_parameters)) > + { > + sql_print_error("View '%-.192s'.'%-.192s': algorithm swap error.", "View %`s.%`s: cannot change the algorithm from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information. > + view->db, view->table_name); > + DBUG_RETURN(HA_ADMIN_INTERNAL_ERROR); > + } > + sql_print_information("View '%-.192s'.'%-.192s': algorithm swapped to '%s'", "View %`s.%`s: algorithm changed from %s to %s" And push_warning_printf() with the same. Or, rather, only push_warning_printf without sql_print_information. > + view->db, view->table_name, > + (view->algorithm == VIEW_ALGORITHM_MERGE)? > + "MERGE":"TEMPTABLE"); > + > + > + DBUG_RETURN(HA_ADMIN_VIEW_UPGRADE_IS_DONE); > +} > + > + > /* > Register VIEW (write .frm & process .frm's history backups) > > @@ -931,16 +1010,9 @@ static int mysql_register_view(THD *thd, > } > loop_out: > /* print file name */ > - dir.length= build_table_filename(dir_buff, sizeof(dir_buff) - 1, > - view->db, "", "", 0); > - dir.str= dir_buff; > - > - path.length= build_table_filename(path_buff, sizeof(path_buff) - 1, > - view->db, view->table_name, reg_ext, 0); > - path.str= path_buff; > - > - file.str= path.str + dir.length; > - file.length= path.length - dir.length; > + make_view_filename(&dir, dir_buff, sizeof(dir_buff), > + &path, path_buff, sizeof(path_buff), > + &file, view); > > /* init timestamp */ > if (!view->timestamp.str) > @@ -1953,6 +2025,33 @@ int view_checksum(THD *thd, TABLE_LIST * > HA_ADMIN_OK); > } you can make view_checksum() static now. > > +/** > + Check view > + > + @param thd thread handle > + @param view view for check > + @param check_opt check options > + > + @retval HA_ADMIN_OK OK > + @retval HA_ADMIN_NOT_IMPLEMENTED it is not VIEW > + @retval HA_ADMIN_WRONG_CHECKSUM check sum is wrong > +*/ > +int view_check(THD *thd, TABLE_LIST *view, HA_CHECK_OPT *check_opt) > +{ > + int res; > + DBUG_ENTER("view_check"); > + if ((res= view_checksum(thd, view)) != HA_ADMIN_OK) > + DBUG_RETURN(res); > + if (((check_opt->sql_flags & TT_MARIADB_UPGRADE) && > + !view->mariadb_version) || > + ((check_opt->sql_flags & (TT_MARIADB_UPGRADE | TT_FORCE)))) What is the point? Why would one ever want to change the algorithm of the provably created-in-mariadb view and pretend that it's an "upgrade"? To change the algorithm one should use ALTER. Fixing the view after an upgrade is not altering it, it's *fixing after an upgrade*. And views that were created in mariadb don't need to be fixed after an upgrade. No FORCE please. > + { > + DBUG_PRINT("XXX", ("Upgrade")); please either remove this DBUG_PRINT or change it to use a reasonable keyword. > + DBUG_RETURN(mariadb_fix_view(thd, view)); > + } > + DBUG_RETURN(HA_ADMIN_OK); > +} > + > === modified file 'sql/sql_yacc.yy' > --- a/sql/sql_yacc.yy 2014-11-08 18:54:42 +0000 > +++ b/sql/sql_yacc.yy 2014-12-07 09:48:25 +0000 > @@ -7299,6 +7300,10 @@ mi_check_type: > | EXTENDED_SYM { Lex->check_opt.flags|= T_EXTEND; } > | CHANGED { Lex->check_opt.flags|= T_CHECK_ONLY_CHANGED; } > | FOR_SYM UPGRADE_SYM { Lex->check_opt.sql_flags|= TT_FOR_UPGRADE; } > + | MARIADB_SYM UPGRADE_SYM > + { Lex->check_opt.sql_flags|= TT_MARIADB_UPGRADE; } > + | FORCE_SYM MARIADB_SYM UPGRADE_SYM > + { Lex->check_opt.sql_flags|= (TT_MARIADB_UPGRADE | TT_FORCE); } > ; > > optimize: Regards, Sergei

1 0

Re: [Maria-developers] Rev 4384: MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views
by Sergei Golubchik 17 Dec '14

17 Dec '14

Hi, Sanja! Ok, I've seen all four patches. Specific comments about the code are below and in other emails. But the main question is - why are you doing it, what problem does it solve. As you said, the use case is - a user upgrades *from MySQL* and *runs mysql_upgrade*. To cover this use case mysql_upgrade needs to 1) detect whether it's an upgrade from MySQL, and to 2) run a command to change the algorithm of all views. You've done a lot of changes that aren't related to that. If this is, indeed, the use case, than you only need to: 1. add MySQL detection in mysql_upgrade. You have it, is_mysql() function. 2. add the code to toggle the algorithm of all views. See? No command line options, no FORCE, no CHECK, nothing. Now, let's consider extensions to this minimal functionality. 3. mariadb-version in the view frm file. this is a good idea, makes views a lot more future-proof. ok. I don't see why other extensions might be needed. Now, how to toggle the algorithm. The most natural solution would be to use ALTER VIEW. Like ALTER VIEW v1 ALGORITHM=MERGE. Unfortunately, ALTER VIEW doesn't work that way, I don't know why it was implemented in that crazy way in MySQL, but it's mostly useless. CHECK should not change its arguments. REPAIR? May be. I'd still prefer ALTER, but I don't know whether it can be extended in 5.5 in a safe way. If not then REPAIR ... AFTER UPGRADE is probably the second best. Or even REPAIR VIEW ... AFTER UPGRADE. It won't conflict with anything, doesn't add new keywords. Drawback - we'll be stuck with that hack for quite a while :( Even better would be to make mysql_upgrade to scan the data directory and rewrite view frms. No hacks or new grammar in the server. No new features in 5.5 GA. Nothing to maintain for years. Almost perfect. But it requires mysql_upgrade to have write access to the datadir. It kind of needs it already, but it's optional. With this approach it'll be less optional than before. So, I don't know... The most reasonable and future-proof solution would be to use ALTER TABLE. But I don't know whether we'll be able to do it in 5.5, looks a bit risky. Then there are reasonably safe hacks like REPAIR VIEW. And most safe and quite clean mysql_upgrade only approach, that needs write access to the datadir. Opinions? Anyway, here are comments about the code, below: On Dec 11, sanja(a)askmonty.org wrote: > At file:///home/bell/maria/bzr/work-maria-5.5-MDEV-6916/ > > ------------------------------------------------------------ > revno: 4384 > revision-id: sanja(a)askmonty.org-20141211180458-j8ajlibnvvydmj40 > parent: sanja(a)askmonty.org-20141207094825-tx163unk3mm4gkql > committer: sanja(a)askmonty.org > branch nick: work-maria-5.5-MDEV-6916 > timestamp: Thu 2014-12-11 19:04:58 +0100 > message: > MDEV-6916: Upgrade from MySQL to MariaDB breaks already created views > > mysql_upgrade now fix views from mysql. > > (+ fix new view detection) > === modified file 'client/mysql_upgrade.c' > --- a/client/mysql_upgrade.c 2014-03-27 21:26:58 +0000 > +++ b/client/mysql_upgrade.c 2014-12-11 18:04:58 +0000 > @@ -40,7 +40,8 @@ static char mysql_path[FN_REFLEN]; > static char mysqlcheck_path[FN_REFLEN]; > > static my_bool opt_force, opt_verbose, debug_info_flag, debug_check_flag, > - opt_systables_only, opt_version_check; > + opt_systables_only, opt_version_check, > + opt_mysql_upgrade= 0, opt_skip_mysql_upgrade= 0; > static my_bool opt_not_used, opt_silent; > static uint my_end_arg= 0; > static char *opt_user= (char*)"root"; > @@ -150,6 +151,14 @@ static struct my_option my_long_options[ > &opt_not_used, &opt_not_used, 0, GET_BOOL, NO_ARG, 1, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, > GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"mysql-upgrade", 'y', > + "Skip automatic detection MySQL and assume that we upgrade it", > + &opt_mysql_upgrade, &opt_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, > + {"skip-mysql-upgrade", 'Y', > + "Skip view algorithm upgrade from MySQL", > + &opt_skip_mysql_upgrade, &opt_skip_mysql_upgrade, 0, GET_BOOL, NO_ARG, > + 0, 0, 0, 0, 0, 0}, Those two are very bad option names. First, $ mysql_upgrade --mysql-upgrade looks plain weird. $ mysql_upgrade --skip-mysql-upgrade is even worse. And what about $ mysql_upgrade --mysql-upgrade --skip-mysql-upgrade Second, "skip" is the prefix automatically recognized by my_getopt. Normally, --skip-xxx is the same as --xxx=0. But now you're introducing --skip-mysql-upgrade which is very different from --skip-mysql-upgrade. While simultaneously adding support for --skip-skip-mysql-upgrade. A better option would be --fix-view-algorithm = { AUTO | ALWAYS | NEVER } > {"version-check", 'k', "Run this program only if its \'server version\' " > "matches the version of the server to which it's connecting, (enabled by " > "default); use --skip-version-check to avoid this check. Note: the \'server " > @@ -754,6 +771,23 @@ static int run_mysqlcheck_upgrade(void) > NULL); > } > > +static int run_mysqlcheck_views(void) > +{ > + if (!opt_mysql_upgrade) > + return 0; > + verbose("Phase 0: Fixing views"); No "Phase 0" please, renumber all phases to start from 1. > + print_conn_args("mysqlcheck"); > + return run_tool(mysqlcheck_path, > + NULL, /* Send output from mysqlcheck directly to screen */ > + "--no-defaults", > + ds_args.str, > + "--all-databases", > + "--view-upgrade", > + opt_verbose ? "--verbose": "", > + opt_silent ? "--silent": "", > + "2>&1", > + NULL); > +} > > === modified file 'client/mysqlcheck.c' > --- a/client/mysqlcheck.c 2014-02-17 10:00:51 +0000 > +++ b/client/mysqlcheck.c 2014-12-11 18:04:58 +0000 > @@ -42,7 +42,8 @@ static my_bool opt_alldbs = 0, opt_check > opt_medium_check = 0, opt_quick = 0, opt_all_in_1 = 0, > opt_silent = 0, opt_auto_repair = 0, ignore_errors = 0, > tty_password= 0, opt_frm= 0, debug_info_flag= 0, debug_check_flag= 0, > - opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0; > + opt_fix_table_names= 0, opt_fix_db_names= 0, opt_upgrade= 0, > + opt_mariadb_upgrade= 0, opt_f_mariadb_upgrade= 0; > static my_bool opt_write_binlog= 1, opt_flush_tables= 0; > static uint verbose = 0, opt_mysql_port=0; > static int my_end_arg; > @@ -196,6 +197,12 @@ static struct my_option my_long_options[ > NO_ARG, 0, 0, 0, 0, 0, 0}, > {"version", 'V', "Output version information and exit.", 0, 0, 0, GET_NO_ARG, > NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"view-upgrade", 'f', > + "Fix view algorithm view field if it is not new MariaDB view.", "Repair algorithm value for views created by MySQL (or MariaDB before 5.5.41)" > + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > + {"force-view-upgrade", 'F', > + "Swap view algorithm view field.", "Always change view algorithm" > + 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0}, > {0, 0, 0, 0, 0, 0, GET_NO_ARG, NO_ARG, 0, 0, 0, 0, 0, 0} > }; Regards, Sergei

1 0