- developers - lists.mariadb.org

[Maria-developers] GSoC: On completing the MDEV-6017 (Add support for Indexes on Expressions)
by Alexey Mogilyovkin 01 Oct '19

01 Oct '19

Hello, I would like to finish the task I was working on during GSoC. Nikita told me that there are some architectural questions to my code. Can you tell about them in more detail?

2 1

Re: [Maria-developers] 3f38da9145c: MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN
by Sergei Golubchik 27 Sep '19

27 Sep '19

Hi, Alexander! On Sep 16, Alexander Barkov wrote: > revision-id: 3f38da9145c (mariadb-10.4.4-251-g3f38da9145c) > parent(s): e6ff3f9d1c8 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2019-07-12 07:53:55 +0400 > message: > > MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN > diff --git a/include/mysql/plugin.h b/include/mysql/plugin.h > index 85e52a247af..92703b626ac 100644 > --- a/include/mysql/plugin.h > +++ b/include/mysql/plugin.h > @@ -611,6 +612,22 @@ struct handlerton; > int interface_version; > }; > > +/* > + API for data type plugin. (MariaDB_DATA_TYPE_PLUGIN) > +*/ > +#define MariaDB_DATA_TYPE_INTERFACE_VERSION (MYSQL_VERSION_ID << 8) > + > +/** > + Data type plugin descriptor > +*/ > +#ifdef __cplusplus > +struct st_mariadb_data_type > +{ > + int interface_version; > + const class Type_handler *type_handler; > +}; > +#endif Plugin-wise it'd be better to have a separate plugin_data_type.h and put Type_handler definition there. So that as much of the API as possible gets into the .pp file and we could detect when it changes. > + > /************************************************************************* > st_mysql_value struct for reading values from mysqld. > Used by server variables framework to parse user-provided values. > diff --git a/plugin/type_test/CMakeLists.txt b/plugin/type_test/CMakeLists.txt > new file mode 100644 > index 00000000000..b85168d1bd2 > --- /dev/null > +++ b/plugin/type_test/CMakeLists.txt > @@ -0,0 +1,17 @@ > +# Copyright (c) 2016, MariaDB corporation. All rights reserved. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; version 2 of the License. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write to the Free Software > +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA > + > +MYSQL_ADD_PLUGIN(type_test plugin.cc RECOMPILE_FOR_EMBEDDED > + MODULE_ONLY COMPONENT Test) Add at least two new plugins, please. May be in separate commits, as you like. But at least two. And practical, not just to test the API > diff --git a/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test b/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test > new file mode 100644 > index 00000000000..30e313481d6 > --- /dev/null > +++ b/plugin/type_test/mysql-test/type_test/type_test_int8-debug.test > @@ -0,0 +1,11 @@ > +--echo # > +--echo # MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN > +--echo # > + > +SET SESSION debug_dbug="+d,frm_data_type_info"; > + > +CREATE TABLE t1 (a TEST_INT8); > +SHOW CREATE TABLE t1; > +DROP TABLE t1; > + > +SET SESSION debug_dbug="-d,frm_data_type_info"; don't do that, always save old debug_dbug value instead and restore it later. SET @old_debug_dbug=@@debug_dbug; SET @@debug_dbug="+d,frm_data_type_info"; ... SET @@debug_dbug=@old_debug_dbug; because after "+d,xxx" you have one keyword enabled, like in @@debug_dbug="d,xxx". and after "-d,xxx" you have no keywords enabled, like in @debug_dbug="d" which means "all keywords enabled" for dbug. > diff --git a/plugin/type_test/mysql-test/type_test/type_test_int8.result b/plugin/type_test/mysql-test/type_test/type_test_int8.result > new file mode 100644 > index 00000000000..758f94904c1 > --- /dev/null > +++ b/plugin/type_test/mysql-test/type_test/type_test_int8.result > @@ -0,0 +1,144 @@ > +# > +# MDEV-20016 Add MariaDB_DATA_TYPE_PLUGIN > +# > +SELECT > +PLUGIN_NAME, > +PLUGIN_VERSION, > +PLUGIN_STATUS, > +PLUGIN_TYPE, > +PLUGIN_AUTHOR, > +PLUGIN_DESCRIPTION, > +PLUGIN_LICENSE, > +PLUGIN_MATURITY, > +PLUGIN_AUTH_VERSION > +FROM INFORMATION_SCHEMA.PLUGINS > +WHERE PLUGIN_TYPE='DATA TYPE' > + AND PLUGIN_NAME='TEST_INT8'; > +PLUGIN_NAME TEST_INT8 > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE DATA TYPE > +PLUGIN_AUTHOR MariaDB > +PLUGIN_DESCRIPTION Data type TEST_INT8 > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Alpha > +PLUGIN_AUTH_VERSION 1.0 > +CREATE TABLE t1 (a TEST_INT8); > +SHOW CREATE TABLE t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `a` test_int8 DEFAULT NULL is the type name a reserved word? or an arbitrary identifier? should we support (and print as) `a` `test_int8` DEFAULT NULL ? > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t1; > +SELECT CAST('100' AS TEST_INT8) AS cast; > +cast > +100 > +BEGIN NOT ATOMIC > +DECLARE a TEST_INT8 DEFAULT 256; > +SELECT HEX(a), a; > +END; > +$$ > +HEX(a) a > +100 256 > +CREATE FUNCTION f1(p TEST_INT8) RETURNS TEST_INT8 RETURN 1; > +SHOW CREATE FUNCTION f1; > +Function sql_mode Create Function character_set_client collation_connection Database Collation > +f1 STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION CREATE DEFINER=`root`@`localhost` FUNCTION `f1`(p TEST_INT8) RETURNS test_int8 > +RETURN 1 latin1 latin1_swedish_ci latin1_swedish_ci use enable_metadata for a part (or for a whole) of this test and show how mysql --column-type-info handles it. > +SELECT f1(10); > +f1(10) > +1 > +DROP FUNCTION f1; > +CREATE TABLE t1 (a TEST_INT8); > +CREATE TABLE t2 AS SELECT a FROM t1; > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `a` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +CREATE TABLE t2 AS SELECT COALESCE(a,a) FROM t1; > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `COALESCE(a,a)` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +CREATE TABLE t2 AS SELECT LEAST(a,a) FROM t1; > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `LEAST(a,a)` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +INSERT INTO t1 VALUES (1),(2); > +CREATE TABLE t2 AS SELECT MIN(a), MAX(a) FROM t1; > +SELECT * FROM t2; > +MIN(a) MAX(a) > +1 2 > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `MIN(a)` test_int8 DEFAULT NULL, > + `MAX(a)` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (id INT, a TEST_INT8); > +INSERT INTO t1 VALUES (1,1),(1,2),(2,1),(2,2); > +CREATE TABLE t2 AS SELECT id, MIN(a), MAX(a) FROM t1 GROUP BY id; > +SELECT * FROM t2; > +id MIN(a) MAX(a) > +1 1 2 > +2 1 2 > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `id` int(11) DEFAULT NULL, > + `MIN(a)` test_int8 DEFAULT NULL, > + `MAX(a)` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +INSERT INTO t1 VALUES (1),(2); > +CREATE TABLE t2 AS SELECT DISTINCT a FROM t1; > +SELECT * FROM t2; > +a > +1 > +2 > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `a` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +INSERT INTO t1 VALUES (1); > +CREATE TABLE t2 AS SELECT (SELECT a FROM t1) AS c1; > +SELECT * FROM t2; > +c1 > +1 > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `c1` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; > +CREATE TABLE t1 (a TEST_INT8); > +CREATE TABLE t2 AS SELECT a FROM t1 UNION SELECT a FROM t1; > +SHOW CREATE TABLE t2; > +Table Create Table > +t2 CREATE TABLE `t2` ( > + `a` test_int8 DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +DROP TABLE t2; > +DROP TABLE t1; CREATE LIKE, ALTER TABLE, SHOW COLUMNS, I_S.COLUMNS Does it support indexing? > diff --git a/plugin/type_test/plugin.cc b/plugin/type_test/plugin.cc > new file mode 100644 > index 00000000000..ea70c70f786 > --- /dev/null > +++ b/plugin/type_test/plugin.cc > @@ -0,0 +1,120 @@ > +/* > + Copyright (c) 2000, 2015, Oracle and/or its affiliates. > + Copyright (c) 2009, 2019, MariaDB > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ > + > +#include <my_global.h> > +#include <sql_class.h> // THD > +#include <mysql/plugin.h> > +#include "sql_type.h" > + > + > +class Field_test_int8 :public Field_longlong > +{ > +public: > + Field_test_int8(const LEX_CSTRING &name, const Record_addr &addr, > + enum utype unireg_check_arg, > + uint32 len_arg, bool zero_arg, bool unsigned_arg) > + :Field_longlong(addr.ptr(), len_arg, addr.null_ptr(), addr.null_bit(), > + Field::NONE, &name, zero_arg, unsigned_arg) > + {} > + void sql_type(String &res) const > + { > + CHARSET_INFO *cs= res.charset(); > + res.length(cs->cset->snprintf(cs,(char*) res.ptr(),res.alloced_length(), > + "test_int8")); isn't there an easier way of setting a String to a specific value? Like, res.copy() or something? By the way, why do you need to do it at all, if the parent's Field::sql_type method could set the value from type_handler()->name() ? > + // UNSIGNED and ZEROFILL flags are not supported by the parser yet. > + // add_zerofill_and_unsigned(res); > + } > + const Type_handler *type_handler() const; > +}; > + > + > +class Type_handler_test_int8: public Type_handler_longlong > +{ > +public: > + const Name name() const override > + { > + static Name name(STRING_WITH_LEN("test_int8")); I'd prefer this being picked up automatically from the plugin name. Like it's done for engines, I_S tables, auth plugins, ft parsers, etc. > + return name; > + } > + bool Column_definition_data_type_info_image(Binary_string *to, > + const Column_definition &def) > + const override > + { > + return to->append(Type_handler_test_int8::name().lex_cstring()); > + } Not sure, why this has to be overridden by the derived class. > + Field *make_table_field(MEM_ROOT *root, > + const LEX_CSTRING *name, > + const Record_addr &addr, > + const Type_all_attributes &attr, > + TABLE *table) const override > + { > + return new (root) > + Field_test_int8(*name, addr, Field::NONE, > + attr.max_char_length(), > + 0/*zerofill*/, > + attr.unsigned_flag); > + } > + > + Field *make_table_field_from_def(TABLE_SHARE *share, MEM_ROOT *root, > + const LEX_CSTRING *name, > + const Record_addr &rec, const Bit_addr &bit, > + const Column_definition_attributes *attr, > + uint32 flags) const override > + { > + return new (root) > + Field_test_int8(*name, rec, attr->unireg_check, > + (uint32) attr->length, > + f_is_zerofill(attr->pack_flag) != 0, > + f_is_dec(attr->pack_flag) == 0); > + } > +}; Why do you need two different methods? I'd expect one that does new Field_test_int8() to be enough. The second can be in the parent class, calling make_table_field(). Or both could be in the parent class, calling the only virtual method that actually does new (root) Field_test_int8(...) > + > +static Type_handler_test_int8 type_handler_test_int8; > + > + > +const Type_handler *Field_test_int8::type_handler() const > +{ > + return &type_handler_test_int8; > +} > + > + > +/*************************************************************************/ > + > +static struct st_mariadb_data_type data_type_test_plugin= > +{ > + MariaDB_DATA_TYPE_INTERFACE_VERSION, > + &type_handler_test_int8 > +}; It's be more interesting to have a distinct type, not just an alias for BIGINT. E.g. 7-byte integer. As an example, it's a pretty empty plugin, it doesn't show why this API was ever created. Add something non-trivial to it please. And some real, non-test, plugin in a separate commit. > + > + > +maria_declare_plugin(type_geom) > +{ > + MariaDB_DATA_TYPE_PLUGIN, // the plugin type (see include/mysql/plugin.h) > + &data_type_test_plugin, // pointer to type-specific plugin descriptor > + "TEST_INT8", // plugin name > + "MariaDB", // plugin author MariaDB Corporation ? > + "Data type TEST_INT8", // the plugin description > + PLUGIN_LICENSE_GPL, // the plugin license (see include/mysql/plugin.h) > + 0, // Pointer to plugin initialization function > + 0, // Pointer to plugin deinitialization function > + 0x0100, // Numeric version 0xAABB means AA.BB veriosn > + NULL, // Status variables > + NULL, // System variables > + "1.0", // String version representation > + MariaDB_PLUGIN_MATURITY_ALPHA // Maturity (see include/mysql/plugin.h)*/ EXPERIMENTAL (for test plugins. ALPHA kind of implies it'll be BETA, GAMMA and STABLE eventually) > +} > +maria_declare_plugin_end; > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 5cecd9f50f7..988619cb5f9 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -121,8 +121,23 @@ bool Type_handler_data::init() > > > const Type_handler * > -Type_handler::handler_by_name(const LEX_CSTRING &name) > +Type_handler::handler_by_name(THD *thd, const LEX_CSTRING &name) > { > + plugin_ref plugin; > + if ((plugin= my_plugin_lock_by_name(thd, &name, MariaDB_DATA_TYPE_PLUGIN))) > + { > + /* > + INSTALL PLUGIN is not fully supported for data type plugins yet. Why? What's not supported? > + Fow now we have only mandatory built-in plugins > + and dynamic plugins for test purposes, > + Should be safe to unlock the plugin immediately. > + */ > + const Type_handler *ph= reinterpret_cast<st_mariadb_data_type*> > + (plugin_decl(plugin)->info)->type_handler; > + plugin_unlock(thd, plugin); > + return ph; > + } > + > #ifdef HAVE_SPATIAL > const Type_handler *ha= type_collection_geometry.handler_by_name(name); > if (ha) > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 8f2d4d0c49d..b01330f30e4 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -3221,9 +3221,9 @@ class Information_schema_character_attributes > class Type_handler > { > protected: > - static const Name m_version_default; > - static const Name m_version_mysql56; > - static const Name m_version_mariadb53; > + static const MYSQL_PLUGIN_IMPORT Name m_version_default; > + static const MYSQL_PLUGIN_IMPORT Name m_version_mysql56; > + static const MYSQL_PLUGIN_IMPORT Name m_version_mariadb53; Why do you need that? Parent's behavior should be always fine for plugins. I don't think plugins should know about this at all. > String *print_item_value_csstr(THD *thd, Item *item, String *str) const; > String *print_item_value_temporal(THD *thd, Item *item, String *str, > const Name &type_name, String *buf) const; > @@ -5129,9 +5130,9 @@ class Type_handler_bool: public Type_handler_long > > class Type_handler_longlong: public Type_handler_general_purpose_int > { > - static const Name m_name_longlong; > - static const Type_limits_int m_limits_sint64; > - static const Type_limits_int m_limits_uint64; > + static const MYSQL_PLUGIN_IMPORT Name m_name_longlong; > + static const MYSQL_PLUGIN_IMPORT Type_limits_int m_limits_sint64; > + static const MYSQL_PLUGIN_IMPORT Type_limits_int m_limits_uint64; same here > public: > virtual ~Type_handler_longlong() {} > const Name name() const { return m_name_longlong; } > diff --git a/sql/table.cc b/sql/table.cc > index ea333cb2ecd..70b3814df6c 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -2291,12 +2291,20 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write, > > if (field_data_type_info_array.count()) > { > + const LEX_CSTRING &info= field_data_type_info_array. > + element(i).type_info(); > DBUG_EXECUTE_IF("frm_data_type_info", > push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, > ER_UNKNOWN_ERROR, "DBUG: [%u] name='%s' type_info='%.*s'", > i, share->fieldnames.type_names[i], > - (uint) field_data_type_info_array.element(i).type_info().length, > - field_data_type_info_array.element(i).type_info().str);); > + (uint) info.length, info.str);); > + > + if (info.length) > + { > + const Type_handler *h= Type_handler::handler_by_name(thd, info); > + if (h) > + handler= h; > + } I don't see where you handle the error that "unknown type" > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

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