- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 673e2537249: Fix missing memory barrier in wait_for_commit
by Kristian Nielsen 26 Sep '19

26 Sep '19

Hi Andrei, > > I noticed another thing with the wait_for_commit error handling while > > looking at the MDEV-18648 patch. > So practically, we seem have to turn the types of the pair into > std::atomic<> to store into, in the right order, and load from (the way > it is). Yes. Something like this? There is a bit of noise in the patch due to changing the type of waitee to atomic and adjusting all code that accesses it. But it's basically this in wakeup: this->wakeup_error= wakeup_error; waitee.store(NULL, std::memory_order_release); and this in the waiter fast path (one place in sql_class.h and one in log.cc): if (waitee.load(std::memory_order_acquire)) return wait_for_prior_commit2(thd); else return wakeup_error; Other access are protected by lock or happen within the waiting thread only, so they can just use the memory_order_relaxed access semantics without barriers. Unfortunately I don't see how to do a useful test case. It is not really possible to force load or store reordering (on x86 I believe the CPU in fact enforces orderring between loads and stores). I couldn't think of a test that would fail without the patch and succeed with it. If you agree with the patch, do you have an opinion on which version it should go into? I'd suggest 10.4, since (IIUC) that version started the use of std::atomic. (Patch also available here: https://github.com/knielsen/server/commit/673e253724979fd9fe43a4a22bd7e1b2c…) - Kristian. Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > revision-id: 673e253724979fd9fe43a4a22bd7e1b2c3a5269e (mariadb-10.4.4-333-g673e2537249) > parent(s): 8887effe13ad87ba0460d4d3068fb5696f089bb0 > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2019-09-26 17:43:26 +0200 > message: > > Fix missing memory barrier in wait_for_commit > > The function wait_for_commit::wait_for_prior_commit() has a fast path > where it checks without locks if wakeup_subsequent_commits() has > already been called. This check was missing a memory barrier. The > waitee thread does two writes to variables `waitee' and > `wakeup_error', and if the waiting thread sees the first write it > _must_ also see the second or incorrect behaviour will occur. This > requires memory barriers between both the writes (release semantics) > and the reads (acquire semantics) of those two variables. > > Other accesses to these variables are done under lock or where only > one thread will be accessing them, and can be done without barriers > (relaxed sematics). > > --- > sql/log.cc | 19 +++++++++++++------ > sql/sql_class.cc | 25 ++++++++++++++----------- > sql/sql_class.h | 17 ++++++++++++----- > 3 files changed, 39 insertions(+), 22 deletions(-) > > diff --git a/sql/log.cc b/sql/log.cc > index 4f51a9a9c17..a88d5147898 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -7467,8 +7467,10 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) > */ > wfc= orig_entry->thd->wait_for_commit_ptr; > orig_entry->queued_by_other= false; > - if (wfc && wfc->waitee) > + if (wfc && wfc->waitee.load(std::memory_order_acquire)) > { > + wait_for_commit *loc_waitee; > + > mysql_mutex_lock(&wfc->LOCK_wait_commit); > /* > Do an extra check here, this time safely under lock. > @@ -7480,10 +7482,10 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) > before setting the flag, so there is no risk that we can queue ahead of > it. > */ > - if (wfc->waitee && !wfc->waitee->commit_started) > + if ((loc_waitee= wfc->waitee.load(std::memory_order_relaxed)) && > + !loc_waitee->commit_started) > { > PSI_stage_info old_stage; > - wait_for_commit *loc_waitee; > > /* > By setting wfc->opaque_pointer to our own entry, we mark that we are > @@ -7505,7 +7507,8 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) > &wfc->LOCK_wait_commit, > &stage_waiting_for_prior_transaction_to_commit, > &old_stage); > - while ((loc_waitee= wfc->waitee) && !orig_entry->thd->check_killed(1)) > + while ((loc_waitee= wfc->waitee.load(std::memory_order_relaxed)) && > + !orig_entry->thd->check_killed(1)) > mysql_cond_wait(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit); > wfc->opaque_pointer= NULL; > DBUG_PRINT("info", ("After waiting for prior commit, queued_by_other=%d", > @@ -7523,14 +7526,18 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) > do > { > mysql_cond_wait(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit); > - } while (wfc->waitee); > + } while (wfc->waitee.load(std::memory_order_relaxed)); > } > else > { > /* We were killed, so remove us from the list of waitee. */ > wfc->remove_from_list(&loc_waitee->subsequent_commits_list); > mysql_mutex_unlock(&loc_waitee->LOCK_wait_commit); > - wfc->waitee= NULL; > + /* > + This is the thread clearing its own status, it is no longer on > + the list of waiters. So no memory barriers are needed here. > + */ > + wfc->waitee.store(NULL, std::memory_order_relaxed); > > orig_entry->thd->EXIT_COND(&old_stage); > /* Interrupted by kill. */ > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 4eab241232b..ca179a39dc1 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7230,7 +7230,7 @@ wait_for_commit::reinit() > { > subsequent_commits_list= NULL; > next_subsequent_commit= NULL; > - waitee= NULL; > + waitee.store(NULL, std::memory_order_relaxed); > opaque_pointer= NULL; > wakeup_error= 0; > wakeup_subsequent_commits_running= false; > @@ -7308,8 +7308,9 @@ wait_for_commit::wakeup(int wakeup_error) > > */ > mysql_mutex_lock(&LOCK_wait_commit); > - waitee= NULL; > this->wakeup_error= wakeup_error; > + /* Memory barrier to make wakeup_error visible to the waiter thread. */ > + waitee.store(NULL, std::memory_order_release); > /* > Note that it is critical that the mysql_cond_signal() here is done while > still holding the mutex. As soon as we release the mutex, the waiter might > @@ -7340,9 +7341,10 @@ wait_for_commit::wakeup(int wakeup_error) > void > wait_for_commit::register_wait_for_prior_commit(wait_for_commit *waitee) > { > - DBUG_ASSERT(!this->waitee /* No prior registration allowed */); > + DBUG_ASSERT(!this->waitee.load(std::memory_order_relaxed) > + /* No prior registration allowed */); > wakeup_error= 0; > - this->waitee= waitee; > + this->waitee.store(waitee, std::memory_order_relaxed); > > mysql_mutex_lock(&waitee->LOCK_wait_commit); > /* > @@ -7351,7 +7353,7 @@ wait_for_commit::register_wait_for_prior_commit(wait_for_commit *waitee) > see comments on wakeup_subsequent_commits2() for details. > */ > if (waitee->wakeup_subsequent_commits_running) > - this->waitee= NULL; > + this->waitee.store(NULL, std::memory_order_relaxed); > else > { > /* > @@ -7381,7 +7383,8 @@ wait_for_commit::wait_for_prior_commit2(THD *thd) > thd->ENTER_COND(&COND_wait_commit, &LOCK_wait_commit, > &stage_waiting_for_prior_transaction_to_commit, > &old_stage); > - while ((loc_waitee= this->waitee) && likely(!thd->check_killed(1))) > + while ((loc_waitee= this->waitee.load(std::memory_order_relaxed)) && > + likely(!thd->check_killed(1))) > mysql_cond_wait(&COND_wait_commit, &LOCK_wait_commit); > if (!loc_waitee) > { > @@ -7404,14 +7407,14 @@ wait_for_commit::wait_for_prior_commit2(THD *thd) > do > { > mysql_cond_wait(&COND_wait_commit, &LOCK_wait_commit); > - } while (this->waitee); > + } while (this->waitee.load(std::memory_order_relaxed)); > if (wakeup_error) > my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); > goto end; > } > remove_from_list(&loc_waitee->subsequent_commits_list); > mysql_mutex_unlock(&loc_waitee->LOCK_wait_commit); > - this->waitee= NULL; > + this->waitee.store(NULL, std::memory_order_relaxed); > > wakeup_error= thd->killed_errno(); > if (!wakeup_error) > @@ -7513,7 +7516,7 @@ wait_for_commit::unregister_wait_for_prior_commit2() > wait_for_commit *loc_waitee; > > mysql_mutex_lock(&LOCK_wait_commit); > - if ((loc_waitee= this->waitee)) > + if ((loc_waitee= this->waitee.load(std::memory_order_relaxed))) > { > mysql_mutex_lock(&loc_waitee->LOCK_wait_commit); > if (loc_waitee->wakeup_subsequent_commits_running) > @@ -7526,7 +7529,7 @@ wait_for_commit::unregister_wait_for_prior_commit2() > See comments on wakeup_subsequent_commits2() for more details. > */ > mysql_mutex_unlock(&loc_waitee->LOCK_wait_commit); > - while (this->waitee) > + while (this->waitee.load(std::memory_order_relaxed)) > mysql_cond_wait(&COND_wait_commit, &LOCK_wait_commit); > } > else > @@ -7534,7 +7537,7 @@ wait_for_commit::unregister_wait_for_prior_commit2() > /* Remove ourselves from the list in the waitee. */ > remove_from_list(&loc_waitee->subsequent_commits_list); > mysql_mutex_unlock(&loc_waitee->LOCK_wait_commit); > - this->waitee= NULL; > + this->waitee.store(NULL, std::memory_order_relaxed); > } > } > wakeup_error= 0; > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 72cb8148895..1c81739865b 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -20,6 +20,7 @@ > > /* Classes in mysql */ > > +#include <atomic> > #include "dur_prop.h" > #include <waiting_threads.h> > #include "sql_const.h" > @@ -2018,7 +2019,7 @@ struct wait_for_commit > /* > The LOCK_wait_commit protects the fields subsequent_commits_list and > wakeup_subsequent_commits_running (for a waitee), and the pointer > - waiterr and associated COND_wait_commit (for a waiter). > + waitee and associated COND_wait_commit (for a waiter). > */ > mysql_mutex_t LOCK_wait_commit; > mysql_cond_t COND_wait_commit; > @@ -2032,8 +2033,14 @@ struct wait_for_commit > > When this is cleared for wakeup, the COND_wait_commit condition is > signalled. > + > + This pointer is protected by LOCK_wait_commit. But there is also a "fast > + path" where the waiter compares this to NULL without holding the lock. > + Such read must be done with acquire semantics (and all corresponding > + writes done with release semantics). This ensures that a wakeup with error > + is reliably detected as (waitee==NULL && wakeup_error != 0). > */ > - wait_for_commit *waitee; > + std::atomic<wait_for_commit *> waitee; > /* > Generic pointer for use by the transaction coordinator to optimise the > waiting for improved group commit. > @@ -2068,7 +2075,7 @@ struct wait_for_commit > 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) > + if (waitee.load(std::memory_order_acquire)) > return wait_for_prior_commit2(thd); > else > { > @@ -2096,7 +2103,7 @@ struct wait_for_commit > } > void unregister_wait_for_prior_commit() > { > - if (waitee) > + if (waitee.load(std::memory_order_relaxed)) > unregister_wait_for_prior_commit2(); > else > wakeup_error= 0; > @@ -2118,7 +2125,7 @@ struct wait_for_commit > } > next_ptr_ptr= &cur->next_subsequent_commit; > } > - waitee= NULL; > + waitee.store(NULL, std::memory_order_relaxed); > } > > void wakeup(int wakeup_error); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] e07caf401c2: MDEV-20645: Replication consistency is broken as workers miss the error notification from an earlier failed group.
by Kristian Nielsen 23 Sep '19

23 Sep '19

sujatha <sujatha.sivakumar(a)mariadb.com> writes: > revision-id: e07caf401c26cf8144899336d103e4c7aafd3d7a (mariadb-10.1.41-45-ge07caf401c2) > MDEV-20645: Replication consistency is broken as workers miss the error notification from an earlier failed group. Great that you could come up with a testcase like this to trigger the error! Also, thanks for the detailed description of the issue in the commit mail, that made it much easier to understand and comment on. - Kristian.

1 0

[Maria-developers] Missing memory barrier in parallel replication error handler in wait_for_prior_commit()?
by Kristian Nielsen 22 Sep '19

22 Sep '19

Hi Andrei (Cc: Sujatha), I noticed another thing with the wait_for_commit error handling while looking at the MDEV-18648 patch. We have code like this: // Wakeup code in wait_for_commit::wakeup(): mysql_mutex_lock(&LOCK_wait_commit); waitee= NULL; this->wakeup_error= wakeup_error; // Wait code in wait_for_prior_commit(): if (waitee) return wait_for_prior_commit2(thd); else { if (wakeup_error) my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); So the waiter code runs a "fast path" without locks. It is ok if we race on the assignment of NULL to wait_for_commit::waitee variable, because then the waiter will take the slow path and do proper locking. But it looks like there is a race as follows: 1. wakeup() sets waitee= NULL 2. wait_for_prior_commit() sees waitee==NULL _and_ wakeup_error==0, and incorrectly returns without error. 3. wakeup() too late sets wait_for_commit::wakeup_error. It is not enough of course to swap the assignments in wakeup(). A write-write memory barrier is needed between them in wakeup(), and a corresponding read-read barrier is needed in wait_for_prior_commit(). With proper barriers, the waiter cannot see the write of waitee=NULL without also seeing the write to wakeup_error. So it will either return with non-zero wakeup_error or take the slow path with proper locking. Both of which are fine. Andrei, what do you think? Can you see something in the above analysis that I missed? It seems like such an obvious miss in the code that I wonder how it remained undetected for so long (or how I could have written that in the first place...). But I suppose the race is very unlikely to hit in practice, especially in a code path that only triggers in the error case... - Kristian.

1 0

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) 18 Sep '19

18 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