September 2019 - developers - lists.mariadb.org

Re: [Maria-developers] [Commits] cde9170709c: MDEV-18648: slave_parallel_mode= optimistic default in 10.5
by Kristian Nielsen 22 Sep '19

22 Sep '19

sujatha <sujatha.sivakumar(a)mariadb.com> writes: > Thank you for the review comments. You are right. Setting > rgi->worker_error=1 > for the 2 is the right way to handle. With this, upon reaching > 'finish_event_group' > 2nd will notify 3rd transaction that something went wrong during prior > commit > execution. Ok, great if this fix works. >> Also, this fix doesn't seem to belong with the other MDEV-18648 changes, it >> is unrelated to what the default parallel replication mode is. So please do >> it in a separate commit. >The missing transaction issue is possible, only in the case of >slave_parallel_mode='optimistic'. I *think* it is also possible in conservative mode. Not between different batches of transactions that group-committed together on the master, as you described. But between transactions in one gco. Though I did not check the code deeply for this. But my point was actually that this problem must exist also in earlier versions of MariaDB, where optimistic is not the default, but the user manually enables it. So it should be considered for fixing at least in 10.4 I would assume? - Kristian.

1 0

Re: [Maria-developers] c3d2998a038: MDEV-16130 wrong error message adding AS ROW START to versioned table
by Sergei Golubchik 21 Sep '19

21 Sep '19

Hi, Aleksey! On Aug 30, Aleksey Midenkov wrote: > revision-id: c3d2998a038 (versioning-1.0.3-71-gc3d2998a038) > parent(s): 611488e3d90 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2018-05-23 22:45:08 +0300 > message: > > MDEV-16130 wrong error message adding AS ROW START to versioned table > > Closes tempesta-tech/mariadb#494 > > --- > mysql-test/suite/versioning/r/alter.result | 12 ++++++++---- > mysql-test/suite/versioning/t/alter.test | 7 ++++++- > sql/handler.cc | 3 ++- > sql/share/errmsg-utf8.txt | 2 +- > sql/sql_table.cc | 6 ------ > 5 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/mysql-test/suite/versioning/r/alter.result b/mysql-test/suite/versioning/r/alter.result > index fafcf3c30b0..666420dc2e5 100644 > --- a/mysql-test/suite/versioning/r/alter.result > +++ b/mysql-test/suite/versioning/r/alter.result > @@ -76,13 +76,17 @@ t CREATE TABLE `t` ( > `a` int(11) DEFAULT NULL > ) ENGINE=MyISAM DEFAULT CHARSET=latin1 > alter table t add column trx_start timestamp(6) as row start; > -ERROR HY000: Table `t` is not system-versioned > +ERROR HY000: Can not add system property AS ROW START/END for field `trx_start` This is very strange wording. What is a "system property"? The standard has no such concept. Neither does MariaDB, it's not used anywhere in the manual, as far as I know. Here you can use, for example, ER_VERS_DUPLICATE_ROW_START_END: Duplicate ROW START column `trx_start` > alter table t add system versioning; > show create table t; > Table Create Table > t CREATE TABLE `t` ( > `a` int(11) DEFAULT NULL > ) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +alter table t add column trx_start timestamp(6) as row start; > +ERROR HY000: Can not add system property AS ROW START/END for field `trx_start` > +alter table t modify a int as row start; > +ERROR HY000: Can not add system property AS ROW START/END for field `a` > alter table t add column b int; > show create table t; > Table Create Table > @@ -527,6 +531,6 @@ use test; > # MDEV-15956 Strange ER_UNSUPPORTED_ACTION_ON_GENERATED_COLUMN upon ALTER on versioning column > create or replace table t1 (i int, j int as (i), s timestamp(6) as row start, e timestamp(6) as row end, period for system_time(s,e)) with system versioning; > alter table t1 modify s timestamp(6) as row start; > -ERROR HY000: Can not change system versioning field `s` > +ERROR HY000: Can not add system property AS ROW START/END for field `s` This doesn't look right either. The statement does not add anything, the field `s` is already AS ROW START. Why is it an error at all? > drop database test; > create database test; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] cde9170709c: MDEV-18648: slave_parallel_mode= optimistic default in 10.5
by Kristian Nielsen 20 Sep '19

20 Sep '19

sujatha <sujatha.sivakumar(a)mariadb.com> writes: Hi Sujutha, > @sql/sql_class.h > Moved 'wait_for_prior_commit(THD *thd)' method inside sql_class.cc > > @sql/sql_class.cc > Added code to check for 'stop_on_error_sub_id' for event groups which get skipped > and don't have any preceding group to wait for. This looks like the wrong fix. The wait_for_commit mechanism is a low-level API, it should not deal with things like stop_on_error_sub_id. > 1,2,3 are scheduled in parallel. > Since the above is true 'skip_event_group=true' is set. Simply call > 'wait_for_prior_commit' to wakeup all waiters. Group '2' didn't had any > waitee and its execution is skipped. Hence its wakeup_error=0.It sends a > positive wakeup signal to '3'. Which commits. This results in a missed > transaction. i.e 33 is missed. I think this the the real problem. 2 is stopping because of error, it must not call wakeup_subsequent_commits() without propagating the error, that breaks the whole semantics of wait_for_commit. Maybe it's enough just to also set rgi->worker_error here, when skip_event_group is set due to error in an earlier group? if (unlikely(entry->stop_on_error_sub_id <= rgi->wait_commit_sub_id)) skip_event_group= true; Then wakeup_subsequent_commits() will correctly inform the following transaction about the error so it doesn't try to commit on its own. There is already similar code in the retry_event_group() function: if (entry->stop_on_error_sub_id == (uint64) ULONGLONG_MAX || rgi->gtid_sub_id < entry->stop_on_error_sub_id) ... else { err= rgi->worker_error= 1; Then all of the changes to sql_class.h and sql_class.cc can be omittet. Also, this fix doesn't seem to belong with the other MDEV-18648 changes, it is unrelated to what the default parallel replication mode is. So please do it in a separate commit. Hope this helps, - Kristian. > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 4eab241232b..5ba9c5fe456 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7365,6 +7365,33 @@ wait_for_commit::register_wait_for_prior_commit(wait_for_commit *waitee) > } > > > +int wait_for_commit::wait_for_prior_commit(THD *thd) > +{ > + /* > + Quick inline check, to avoid function call and locking in the common case > + where no wakeup is registered, or a registered wait was already signalled. > + */ > + if (waitee) > + return wait_for_prior_commit2(thd); > + else > + { > + if (wakeup_error) > + my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > + else > + { > + rpl_group_info* rgi= thd->rgi_slave; > + if (rgi && rgi->is_parallel_exec && > + rgi->parallel_entry->stop_on_error_sub_id < (uint64)ULONGLONG_MAX && > + rgi->gtid_sub_id >= rgi->parallel_entry->stop_on_error_sub_id) > + { > + my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > + wakeup_error= ER_PRIOR_COMMIT_FAILED; > + } > + } > + return wakeup_error; > + } > +} > + > /* > Wait for commit of another transaction to complete, as already registered > with register_wait_for_prior_commit(). If the commit already completed, > @@ -7387,6 +7414,17 @@ wait_for_commit::wait_for_prior_commit2(THD *thd) > { > if (wakeup_error) > my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > + else > + { > + rpl_group_info* rgi= thd->rgi_slave; > + if (rgi && rgi->is_parallel_exec && > + rgi->parallel_entry->stop_on_error_sub_id < (uint64)ULONGLONG_MAX && > + rgi->gtid_sub_id >= rgi->parallel_entry->stop_on_error_sub_id) > + { > + my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > + wakeup_error= ER_PRIOR_COMMIT_FAILED; > + } > + } > goto end; > } > /* > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 72cb8148895..0a0a1aa9fa1 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -2062,21 +2062,6 @@ struct wait_for_commit > bool commit_started; > > void register_wait_for_prior_commit(wait_for_commit *waitee); > - int wait_for_prior_commit(THD *thd) > - { > - /* > - Quick inline check, to avoid function call and locking in the common case > - where no wakeup is registered, or a registered wait was already signalled. > - */ > - if (waitee) > - return wait_for_prior_commit2(thd); > - else > - { > - if (wakeup_error) > - my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > - return wakeup_error; > - } > - } > void wakeup_subsequent_commits(int wakeup_error_arg) > { > /* > @@ -2123,6 +2108,7 @@ struct wait_for_commit > > void wakeup(int wakeup_error); > > + int wait_for_prior_commit(THD *thd); > int wait_for_prior_commit2(THD *thd); > void wakeup_subsequent_commits2(int wakeup_error); > void unregister_wait_for_prior_commit2();

1 0

[Maria-developers] mdev-19956 - error messages
by Alexander Barkov 19 Sep '19

19 Sep '19

Hi Igor, We discussed the problem with error messages with Sergei and both agreed that a new error message like: Subquery is not allowed in '%s' will be more informative, and it's easy to do. Why not replace: bool expr_allows_subselect; to const char *clause_that_disallows_subselect; ? So we can change the grammar to something like this: | opt_generated_always AS { Lex->clause_that_disallows_subselect= "GENERATED ALWAYS AS"; } virtual_column_func { Lex->clause_that_disallows_subselect= NULL; } The same for other places where we disallow subselects. This can be done in 10.4 before your patch, as a separate commit.

1 0

Re: [Maria-developers] Please review mdev-19956
by Alexander Barkov 19 Sep '19

19 Sep '19

Hi Igor, I have the following suggestions so far. Can you please fix them and replace the commit in bb-10.4-igor? 1. In statements where subquery is not allowed, the error message become confusing: EXECUTE stmt USING (SELECT 1); < ERROR 1064 (42000): ... syntax to use near 'SELECT 1)' at line 1 > ERROR 1064 (42000): ... syntax to use near ')' at line 1 Please change SELECT and WITH to have the '<kwd>' type, like this: %token <kwd> SELECT_SYM /* SQL-2003-R */ %token <kwd> WITH /* SQL-2003-R */ Then in the grammar like this: query_specification_start: SELECT_SYM { SELECT_LEX *sel; LEX *lex= Lex; if (!(sel= lex->alloc_select(TRUE)) || lex->push_select(sel)) MYSQL_YYABORT; sel->init_select(); sel->braces= FALSE; you will be able to use $1.pos() to get the position of the keyword, and later use this position to generate the error properly. 2. The change in sql_cte.cc is not relevant to this task. Please make a separate MDEV for this and push it to 10.4, then rebase MDEV-19956. 3. There are a few mistakes in comments: + <query expression body> :: + <query term> + | <query expression body> UNION [ ALL | DISTINCT ] <query term> + | <query expression body> EXCEPT [ DISTINCT ] + Consider the production rule of the SQL Standard + subquery: + '(' query_expression_no_with_clause ')' + The latter can be re-written into + subquery: + '(' query_expression_body_ext_parens ')' + | '(' with_clause query_expression_no_with_clause ')' Please fix them as discussed on slack. Thanks.

1 0

[Maria-developers] Implementation of Threadpool
by GUESNET, ETIENNE (ext) 19 Sep '19

19 Sep '19

Hi, I am implementing a threadpool system to AIX. The AIX equivalent of epoll / kqueue on AIX is pollset (and IOCP, but partial implementation only). However, pollset has only a level-trigger mode and MariaDB needs edge-trigger (see comments of sql/threadpoll_generic.h file). Adding a pollset support in MariaDB would be difficult, and probably not so efficient, as we need to simulate the edge-trigger behavior. Obviously, AIX has poll and select support. MariaDB has not. Is there a reason to don't implement threadpoll through poll or select? No interest? Performance issues? MariaDB currently works on AIX without threadpool; in term of efficiency, do you know what can be obtained using threadpool with poll/select or a more modern solution? As far I know, SunOS/Solaris/Illumos threadpoll system (called "port") is also level-trigger only, but I do not find specific functions to manage this. Thanks! Etienne Guesnet.

4 6

[Maria-developers] Congratulations on a successful GSoC!
by Vicențiu Ciorbaru 05 Sep '19

05 Sep '19

Hi! By now, all of you have probably received the results of this year's Google Summer of Code. I want to congratulate you all for the work that you have done this year, students and mentors alike. We have had 4 projects this year: 1. EXCEPT ALL and INTERSECT ALL operations - Ruihang Xia https://github.com/MariaDB/server/pull/1378 2. INSERT ... RETURNING - Rucha Deodar https://medium.com/@ruchad1998/google-summer-of-code-2019-add-returning-to-… 3. UPDATE ... RETURNING - Miroslav Koberskii https://gist.github.com/Mup0c/43c781e2135e55bf5126e0f8c60e2a24 4. Support for indexes on expressions - Alexey Mogilyovkin https://gist.github.com/ZeroICQ/f0c35a20cae7065368dae31d7ad9b001 Implementing these projects certainly took a lot of effort. The first one is already merged within MariaDB and the rest will probably follow, after a bit more polishing. The MariaDB community would be grateful if you stuck around to watch over your projects, extend them and improve them over time. I am sure there was a lot to learn from this experience. Now that you have gotten a feel to what it means to contribute significant features to MariaDB you are in a great position to help others! In this sense, I encourage you to keep being active in the community and share what you have learned. :) Thank you for a great summer! Vicențiu Ciorbaru GSoC MariaDB Foundation Admin

1 0

[Maria-developers] Review for MDEV-19708 RBR replication loses data silently ignoring important column attributes
by Alexander Barkov 02 Sep '19

02 Sep '19

Hi Sachin, I've reviewed the code in bb-10.5-19708. It looks good. I suggest some cleanups. - Remove Field::binlog_type_info_slave, it's not used. - Remove Field::binlog_type_info_master. Please don't cache Binlog_type_info inside the Field itself. Let the caller code cache it in an array of Binlog_type_info pointers. - Change this method in Field: virtual Binlog_type_info *binlog_type_info(); to virtual Binlog_type_info *binlog_type_info(MEM_ROOT *) const; Let's pass mem_root as a parameter instead of using table->mem_root. Please add the 'const' qualifier! - Restore the 'const' qualifier to Field::enum_conv_type rpl_conv_type_from - Instead of doing this: Binlog_type_info * Field_new_decimal::binlog_type_info() { Field_num::binlog_type_info(); this->binlog_type_info_master->m_metadata= precision + (decimals() << 8); this->binlog_type_info_master->m_metadata_size= 2; return this->binlog_type_info_master; } Please add a number of Binlog_type_info constructor helpers, in addition to the current one. For example, for numeric types this would be good: Binlog_type_info(uchar type_code, uint16 metadata, uint8 metadata_size, binlog_signess_t signess) :m_type_code(type_code), m_metadata(metadata), m_metadata_size(metadata_size), m_signess(signess), m_cs(NULL), m_enum_typelib(NULL), m_set_typelib(NULL), m_geom_type(GEOM_GEOMETRY) { }; so you can do something simple like this: Binlog_type_info Field_new_decimal::binlog_type_info(MEM_ROOT *root) const { return new (root) Binlog_type_info(type(), precision + (decimals() << 8), 2, signess_arg); } - It seems you always set signess to SIGNESS_NOT_RELEVANT. Where is signess set to SIGNED or UNSIGNED? - This is something we've never used before: +#include <vector> +#include <string> +#include <functional> +#include <memory> We need to discuss this with Serg. Is it possible to avoid this? - There is a new test file mysql-test/suite/rpl/t/rpl_mdev_19708.test.diff but I could not find a corresponding .result.diff. Where is it? - Please use error names instead of numbers in tests: Instead of: --error 1231 SET GLOBAL binlog_row_metadata = -1; it should be: --error ER_WRONG_VALUE_FOR_VAR SET GLOBAL binlog_row_metadata = -1; You can find error names in include/mysqld_ername.h Thanks.

2 1