- developers - lists.mariadb.org

Re: [Maria-developers] b745ea489f0: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir
by Sergei Golubchik 13 Feb '17

13 Feb '17

Hi, Vicentiu! Pretty nice. I like the how it looks now. Thanks! Few small comments below, really minor. On Feb 13, vicentiu wrote: > revision-id: b745ea489f0698c328383d9b8ec20d2f9777b1b8 (mariadb-10.2.3-197-gb745ea489f0) > parent(s): 52e0b585aaedd0f8f7191e0c0768d4be3c1c3496 > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2017-02-13 12:13:36 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir > > PART 1 of the fix requires a bit of refactoring to not use hard-coded > field indices any more. Create classes that express the grant tables structure, > without exposing the underlying field indices. > > Most of the code is converted to use these classes, except parts which > are not directly affected by the MDEV-11170. These however are TODO > items for subsequent refactoring. > > --- > sql/sql_acl.cc | 1457 ++++++++++++++++++++++++++++++++++++++------------------ > sql/structs.h | 2 +- > 2 files changed, 996 insertions(+), 463 deletions(-) > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index b8a06c22b62..568aff9ac92 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > +#ifdef HAVE_REPLICATION > + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) > + { > + /* > + GRANT and REVOKE are applied the slave in/exclusion rules as they are > + some kind of updates to the mysql.% tables. > + */ > + // TODO(remove-after-review) Note: there was a bug here in previous > + // open_grant_tables version. The linking of tables to be opened was > + // done according to a mask, starting from the end of the TABLES_LIST[TABLES_MAX] > + // array. rpl_filter->tables_ok(0, tables) would always start from > + // TABLES_LIST[0] and lookup the chain from there. If we would not open > + // the User table (first table in the array) > + // (as it happened in grant_reload()), the chain would stop there, hence > + // the rpl_filter check would not lookup all the tables opened I see. Right, it's a bug. > + // > + // This is the reason for the first_table_in_list member variable. > + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; > + if (rpl_filter->is_on() && > + !rpl_filter->tables_ok(0, &first_table_in_list->tl)) > + DBUG_RETURN(1); > + } > +#endif > + if (open_and_lock_tables(thd, &first_table_in_list->tl, FALSE, > + MYSQL_LOCK_IGNORE_TIMEOUT)) > + DBUG_RETURN(-1); > + > + /* The privilge columns vary based on MariaDB version. Figure out > + how many we have after we've opened the table. */ > + if (which_tables & Table_user) > + m_user_table.compute_num_privilege_cols(); > + if (which_tables & Table_db) > + m_db_table.compute_num_privilege_cols(); > + if (which_tables & Table_tables_priv) > + m_tables_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_columns_priv) > + m_columns_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_host) > + m_host_table.compute_num_privilege_cols(); > + if (which_tables & Table_procs_priv) > + m_procs_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_proxies_priv) > + m_proxies_priv_table.compute_num_privilege_cols(); > + if (which_tables & Table_roles_mapping) > + m_roles_mapping_table.compute_num_privilege_cols(); couldn't you just walk the list (from first_table_in_list) and do compute_num_privilege_cols() on every table? Like ((Grant_tables*)tl)->compute_num_privilege_cols(); > + DBUG_RETURN(0); > + } > + > + inline const User_table& user_table() const > + { > + return m_user_table; > + } I personally would just make user_table/etc members public, instead of creating accessors like that. > + > + inline const Db_table& db_table() const > + { > + return m_db_table; > + } > + > + > + inline const Tables_priv_table& tables_priv_table() const > + { > + return m_tables_priv_table; > + } > + > + inline const Columns_priv_table& columns_priv_table() const > + { > + return m_columns_priv_table; > + } > + > + inline const Host_table& host_table() const > + { > + return m_host_table; > + } > + > + inline const Procs_priv_table& procs_priv_table() const > + { > + return m_procs_priv_table; > + } > + > + inline const Proxies_priv_table& proxies_priv_table() const > + { > + return m_proxies_priv_table; > + } > + > + inline const Roles_mapping_table& roles_mapping_table() const > + { > + return m_roles_mapping_table; > + } > + > + private: > + User_table m_user_table; > + Db_table m_db_table; > + Tables_priv_table m_tables_priv_table; > + Columns_priv_table m_columns_priv_table; > + Host_table m_host_table; > + Procs_priv_table m_procs_priv_table; > + Proxies_priv_table m_proxies_priv_table; > + Roles_mapping_table m_roles_mapping_table; > + > + /* Bitmap of which tables we want to open. */ > + const int which_tables; you don't need it if you walk the list in open_and_lock() > + /* Type of lock we want to acquire for the tables we are oppening. */ > + const enum thr_lock_type lock_type; not needed here, it's stored in every TABLE_LIST anyway > + /* The grant tables are set-up in a linked list. We keep the head of it. */ > + Grant_table_base *first_table_in_list; > + /** > + Chain two grant tables' TABLE_LIST members. > + */ > + static void link_tables(Grant_table_base *from, Grant_table_base *to) > + { > + DBUG_ASSERT(from); > + if (to) > + from->tl.next_local= from->tl.next_global= &to->tl; > + else > + from->tl.next_local= from->tl.next_global= NULL; > + } > +}; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Server crashes in inline_mysql_mutex_lock
by Michael Widenius 10 Feb '17

10 Feb '17

Hi! > Thread 1 (Thread 0x7fa0f7549700 (LWP 21346)): > #0 0x00007fa0f9511101 in __pthread_kill (threadid=<optimized out>, > signo=11) at ../nptl/sysdeps/unix/sysv/linux/pthread_kill.c:61 > #1 0x00000000013f4d28 in my_write_core (sig=11) at > /data/src/bb-10.0-monty/mysys/stacktrace.c:478 > #2 0x00000000009f6fc3 in handle_fatal_signal (sig=11) at > /data/src/bb-10.0-monty/sql/signal_handler.cc:285 > #3 <signal handler called> > #4 0x000000000093123c in inline_mysql_mutex_lock (that=0x0, > src_file=0x14df108 "/data/src/bb-10.0-monty/sql/rpl_parallel.cc", > src_line=1278) at > /data/src/bb-10.0-monty/include/mysql/psi/mysql_thread.h:662 > #5 0x0000000000936842 in handle_rpl_parallel_thread (arg=0x7fa0df2d3ee8) at > /data/src/bb-10.0-monty/sql/rpl_parallel.cc:1278 > #6 0x00007fa0f950c0a4 in start_thread (arg=0x7fa0f7549700) at > pthread_create.c:309 > #7 0x00007fa0f76c487d in clone () at > ../sysdeps/unix/sysv/linux/x86_64/clone.S:111 The problem is flush tables with read locks combined with a stop slave of the only replication thread that forces a resize of thread pool. This can get the replication thread into a wrong state where it tries to delete itself before waiting for the flush tables, which the thread pool doesn't like. Fixed by adding a check that we are not in flush tables with read lock when we delete ourselves from the pool. Regards, Monty

1 0

Re: [Maria-developers] [Commits] fb49b98: MDEV-10436 non-deterministic vcol does not force rbr
by Sergei Golubchik 10 Feb '17

10 Feb '17

Hi, Sachin! On Jan 24, sachin wrote: > revision-id: fb49b98a85dc28846c1fdd893ff9791803873aad (mariadb-10.2.3-91-gfb49b98) > parent(s): 0d107a85b3dd6969e66cc9cb4bd29e1cc92a7d18 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-24 17:58:23 +0530 > message: > > MDEV-10436 non-deterministic vcol does not force rbr > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index f78382d..50d767d 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5824,6 +5826,16 @@ int THD::decide_logging_format(TABLE_LIST *tables) > table->table->file->ht) > multi_access_engine= TRUE; > > + /* > + Loop through each table field if it is virtual field check if it is deterministic > + or not. > + */ > + for (field_ptr= table->table->field; *field_ptr; field_ptr++) > + { > + field= *field_ptr; > + if (field->vcol_info && field->vcol_info->flags & VCOL_NOT_STRICTLY_DETERMINISTIC) > + lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_NON_DETER_VCOL); > + } > prev_access_table= table->table; > } 1. don't iterate table->table->field, use table->table->vfield - it only contains generated columns. 2. your check is too strong, in INSERT t1 VALUES (...) it does not matter if some of virtual columns are not strictly deterministic, so BINLOG_STMT_UNSAFE_NON_DETER_VCOL should not be raised. It only should be used when such a table is selected from (or in a WHERE condition of an UPDATE or DELETE). And only when such a column is used in a statement. There is nothing non-deterministing in INSERT t1 SELECT 1 FROM vcol_table 3. It might be not possible to do a per-column check, if read_set bitmaps are not ready yet. In that case, ok, do it per table, but then the result for the table is always the same, I mean where a table has not strictly deterministic columns is a property of a table. So, don't calculate it every time here, do it once and store in the TABLE_SHARE structure. Much like non_determinstic_insert is. But I'm removing non_determinstic_insert now (it's always false, dead code) so add another flag. Or - better - add a 'uint vcol_flags' member which is OR of vcol_info->flag for all fields. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 1

[Maria-developers] Doublewrite block number in XtraDB
by Hank Lyu 10 Feb '17

10 Feb '17

Hello: In XtraDB, if we adjust the size of doublewrite buffer and relative variable (i.e. srv_doublewrite_batch_size) , in theory, we can get better throughput when buffer pool flush to disk. I wonder that why doublewrite buffer size is 2 block and each flush page number is 120 (decided by srv_doublewrite_batch_size), instead doublewrite size is 8 and each flush page number is 500 or more? Is it worry about that when doing flush will occupy too much resource or other reason? Best Regard Hank Lyu

3 4

Re: [Maria-developers] [Commits] 15fde78: MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242)
by Sergey Petrunia 09 Feb '17

09 Feb '17

Hi Varun, On Thu, Feb 09, 2017 at 10:37:17PM +0530, Varun wrote: > revision-id: 15fde78ce812c8db91340936941b986fb28ce1ad (mariadb-10.1.20-82-g15fde78) > parent(s): 99b2de92c6214ddd73eba35c935f490eedf87a26 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-02-09 22:36:03 +0530 > message: > > MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242) > > Post review fixes As far as I understand it's not Post-review fixes, it's a complete patch. Please use an appropriate comment. > > --- > mysql-test/r/update_innodb.result | 26 ++++++++++++++++++++++++++ > mysql-test/t/update_innodb.test | 26 ++++++++++++++++++++++++++ > sql/sql_select.cc | 19 +++++++++++-------- > 3 files changed, 63 insertions(+), 8 deletions(-) > > diff --git a/mysql-test/r/update_innodb.result b/mysql-test/r/update_innodb.result > index 88c86c5..9ba6699 100644 > --- a/mysql-test/r/update_innodb.result > +++ b/mysql-test/r/update_innodb.result > @@ -29,3 +29,29 @@ CREATE ALGORITHM=UNDEFINED VIEW `v1` AS select `t4`.`c1` AS `c1`,`t4`.`c2` AS `c > UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON vw.c2 = del.c2 SET a.c2 = ( SELECT max(t.c1) FROM t3 t, v1 i WHERE del.c2 = t.c2 AND vw.c3 = i.c3 AND t.c3 = 4 ) WHERE a.c2 IS NULL OR a.c2 < '2011-05-01'; > drop view v1; > drop table t1,t2,t3,t4; > +# > +# MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +# > +CREATE TABLE t1 ( > +a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +b_id INT(20) UNSIGNED NULL DEFAULT NULL, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (a_id))COLLATE = 'utf8_general_ci' ENGINE = InnoDB; > +CREATE TABLE t2 ( > +b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (b_id), > +INDEX idx_c_id (c_id))COLLATE = 'utf8_general_ci' ENGINE = InnoDB; > +INSERT INTO t1 (b_id, c_id) VALUES (NULL, NULL); > +INSERT INTO t2 (c_id) VALUES (NULL); > +INSERT INTO t2 (c_id) VALUES (NULL); > +SELECT * FROM T1; > +a_id b_id c_id > +1 NULL NULL > +SELECT t2.b_id FROM t1,t2 WHERE t2.c_id = t1.c_id; > +b_id > +UPDATE t1 SET b_id = (SELECT t2.b_id FROM t2 t2 WHERE t2.c_id = t1.c_id); > +SELECT * FROM T1; > +a_id b_id c_id > +1 NULL NULL > +drop table t1,t2; > diff --git a/mysql-test/t/update_innodb.test b/mysql-test/t/update_innodb.test > index 67c356c..d4fb8b7 100644 > --- a/mysql-test/t/update_innodb.test > +++ b/mysql-test/t/update_innodb.test > @@ -37,3 +37,29 @@ UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON v > > drop view v1; > drop table t1,t2,t3,t4; > + > +--echo # > +--echo # MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +--echo # > + > +CREATE TABLE t1 ( > + a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + b_id INT(20) UNSIGNED NULL DEFAULT NULL, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (a_id))COLLATE = 'utf8_general_ci' ENGINE = InnoDB; > + > +CREATE TABLE t2 ( > + b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (b_id), > + INDEX idx_c_id (c_id))COLLATE = 'utf8_general_ci' ENGINE = InnoDB; > + > +INSERT INTO t1 (b_id, c_id) VALUES (NULL, NULL); > +INSERT INTO t2 (c_id) VALUES (NULL); > +INSERT INTO t2 (c_id) VALUES (NULL); > + > +SELECT * FROM T1; > +SELECT t2.b_id FROM t1,t2 WHERE t2.c_id = t1.c_id; > +UPDATE t1 SET b_id = (SELECT t2.b_id FROM t2 t2 WHERE t2.c_id = t1.c_id); > +SELECT * FROM T1; > +drop table t1,t2; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index e37c1b8..bc9a80f 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -9367,8 +9367,6 @@ static void add_not_null_conds(JOIN *join) > UPDATE t1 SET t1.f2=(SELECT MAX(t2.f4) FROM t2 WHERE t2.f3=t1.f1); > not_null_item is the t1.f1, but it's referred_tab is 0. > */ > - if (!referred_tab) > - continue; > if (!(notnull= new (join->thd->mem_root) > Item_func_isnotnull(join->thd, item))) > DBUG_VOID_RETURN; > @@ -9380,16 +9378,21 @@ static void add_not_null_conds(JOIN *join) > */ > if (notnull->fix_fields(join->thd, &notnull)) > DBUG_VOID_RETURN; > - DBUG_EXECUTE("where",print_where(notnull, > - referred_tab->table->alias.c_ptr(), > - QT_ORDINARY);); > + > + if(referred_tab) > + DBUG_EXECUTE("where",print_where(notnull, > + referred_tab->table->alias.c_ptr(), > + QT_ORDINARY);); > + else > + DBUG_EXECUTE("where",print_where(notnull, > + "outer_ref_cond",QT_ORDINARY);); Please take the re-factoring further and have one DBUG_EXECUTE call with referred_tab? referred_tab->table->alias.c_ptr(): "outer_ref_cond" expression as the parameter. > if (!tab->first_inner) > - { > - COND *new_cond= referred_tab->join == join ? > + { Does the above break indentation or it has a tab character? > + COND *new_cond= (referred_tab && referred_tab->join == join) ? > referred_tab->select_cond : > join->outer_ref_cond; > add_cond_and_fix(join->thd, &new_cond, notnull); > - if (referred_tab->join == join) > + if (referred_tab && referred_tab->join == join) > referred_tab->set_select_cond(new_cond, __LINE__); > else > join->outer_ref_cond= new_cond; OK to push after the above is addressed. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] bb-10.2-compatibility
by jerome brauge 09 Feb '17

09 Feb '17

Hello, I come back with a question on functions. On Oracle, there is only one difference between a function and a stored procedure : the return code. On Mariadb, stored functions are more limited : - Recursive stored functions are not allowed - Explicit or implicit commit is not allowed in stored function - in out parameter are not allowed (MDEV-10654) - no dynamic sql (execute immediate) Have you planned to remove these limits? If not, we'll have to find the best workarounds to use stored procedures (and find a solution to mimic a return code) Best regards, Jérôme.

2 3

Re: [Maria-developers] [Commits] cb918231a25: MDEV-11582 InnoDB: Failing assertion: !((field)->vcol_info && !(field)->stored_in_db())
by Alexander Barkov 09 Feb '17

09 Feb '17

Hello Sergei, The patch looks fine. I suggest one improvement. Currently SERIAL can have various redundant, ignored, non-applicable attributes, e.g.: CREATE TABLE t1 (a SERIAL NULL); CREATE TABLE t1 (a SERIAL AUTO_INCREMENT); CREATE TABLE t1 (a SERIAL SERIAL DEFAULT VALUE); CREATE OR REPLACE TABLE t1 (a SERIAL COLLATE `binary`); Can we split the "attribute:" rule to disallow these for SERIAL? Thanks! On 02/09/2017 03:06 PM, serg(a)mariadb.org wrote: > revision-id: cb918231a25ee792749d7e64ebc97e19c0faaf09 (mariadb-10.2.3-192-gcb918231a25) > parent(s): d1cc58f55ad516453aed42b65d71810e1392f745 > author: Sergei Golubchik > committer: Sergei Golubchik > timestamp: 2017-02-09 12:06:15 +0100 > message: > > MDEV-11582 InnoDB: Failing assertion: !((field)->vcol_info && !(field)->stored_in_db()) > > change the parser not to allow SERIAL as a normal data type. > make a special rule for it, where it could be used for define > fields, but not generated fields, not return type of a stored function, etc. > > --- > mysql-test/r/parser.result | 12 ++++++++++++ > mysql-test/t/parser.test | 11 +++++++++++ > sql/sql_yacc.yy | 22 ++++++++++++++-------- > 3 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/mysql-test/r/parser.result b/mysql-test/r/parser.result > index 7a0a8667350..1292b0bd2e5 100644 > --- a/mysql-test/r/parser.result > +++ b/mysql-test/r/parser.result > @@ -1289,3 +1289,15 @@ SELECT 1; > ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'UNION > SELECT 1' at line 2 > DROP TABLE t1; > +create table t1 (i int, vc serial as (i)); > +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'as (i))' at line 1 > +create function fs() returns serial return 1; > +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'serial return 1' at line 1 > +create table t1 ( id serial ); > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT, > + UNIQUE KEY `id` (`id`) > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 > +drop table t1; > diff --git a/mysql-test/t/parser.test b/mysql-test/t/parser.test > index 1fa7df7fc3d..dc538db6073 100644 > --- a/mysql-test/t/parser.test > +++ b/mysql-test/t/parser.test > @@ -1321,3 +1321,14 @@ UNION > SELECT 1; > > DROP TABLE t1; > + > +# > +# MDEV-11582 InnoDB: Failing assertion: !((field)->vcol_info && !(field)->stored_in_db()) > +# > +--error ER_PARSE_ERROR > +create table t1 (i int, vc serial as (i)); > +--error ER_PARSE_ERROR > +create function fs() returns serial return 1; > +create table t1 ( id serial ); > +show create table t1; > +drop table t1; > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy > index 12423e71f1f..db22708db43 100644 > --- a/sql/sql_yacc.yy > +++ b/sql/sql_yacc.yy > @@ -6145,8 +6145,7 @@ field_spec: > lex->init_last_field(f, $1.str, NULL); > $<create_field>$= f; > } > - field_type { Lex->set_last_field_type($3); } > - field_def > + field_type_or_serial > { > LEX *lex=Lex; > $$= $<create_field>2; > @@ -6164,6 +6163,19 @@ field_spec: > } > ; > > +field_type_or_serial: > + field_type { Lex->set_last_field_type($1); } field_def > + | SERIAL_SYM > + { > + Lex_field_type_st type; > + type.set(MYSQL_TYPE_LONGLONG); > + Lex->set_last_field_type(type); > + Lex->last_field->flags|= AUTO_INCREMENT_FLAG | NOT_NULL_FLAG > + | UNSIGNED_FLAG | UNIQUE_KEY_FLAG; > + } > + opt_attribute > + ; > + > field_def: > opt_attribute > | opt_generated_always AS virtual_column_func > @@ -6441,12 +6453,6 @@ field_type: > { $$.set(MYSQL_TYPE_SET); } > | LONG_SYM opt_binary > { $$.set(MYSQL_TYPE_MEDIUM_BLOB); } > - | SERIAL_SYM > - { > - $$.set(MYSQL_TYPE_LONGLONG); > - Lex->last_field->flags|= (AUTO_INCREMENT_FLAG | NOT_NULL_FLAG | UNSIGNED_FLAG | > - UNIQUE_KEY_FLAG); > - } > ; > > spatial_type: > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits >

1 0

[Maria-developers] Archive Storage Engine
by Varun Gupta 08 Feb '17

08 Feb '17

Hi, Here is the issue https://jira.mariadb.org/browse/MDEV-11645 on which I need suggestions The observations made are: - in 10.1, varchar endspace was bzero-ed archive depended on it to not pack garbage - in 10.2, varchar endspace is not bzero-ed which breaks the archive SE. Was this change intended? If yes, then we should update ha_archive to fill the garbage parts of the buffer with zeroes that will cause it to be (slightly?) slower but there's no other way. BR, Varun

3 4

Re: [Maria-developers] [Commits] e84a5064674: After review fixes for MDEV-11759.
by Marko Mäkelä 08 Feb '17

08 Feb '17

On Tue, Feb 7, 2017 at 8:12 PM, jan <jan.lindstrom(a)mariadb.com> wrote: > # Write file to make mysql-test-run.pl expect the "crash", but don't > # start it until it's told to > -# We give 30 seconds to do a clean shutdown because we do not want > -# to redo apply the pages of t1.ibd at the time of recovery. > -# We want SQL to initiate the first access to t1.ibd. > # Wait until disconnected. Remove all these messages. > diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/fil0crypt.cc > index fed1cf94d84..4299600709a 100644 > --- a/storage/innobase/fil/fil0crypt.cc > +++ b/storage/innobase/fil/fil0crypt.cc > @@ -982,7 +982,16 @@ fil_space_verify_crypt_checksum( > /* Declare empty pages non-corrupted */ > if (checksum == 0 > && *reinterpret_cast<const ib_uint64_t*>(page + FIL_PAGE_LSN) == 0) { > - return (true); > + /* make sure that the page is really empty */ > + for (ulint i = 0; i < UNIV_PAGE_SIZE; i++) { > + if (page[i] != 0) { > + ib_logf(IB_LOG_LEVEL_INFO, > + "Checksum fields zero but page is not empty."); > + > + return(false); > + } > + } > + return(true); > } Please use buf_page_is_zeroes() instead of duplicating the code. Do not spam the error log if the checksum happens to be 0. I guess we could also have FIL_PAGE_LSN at 0 when importing a file. > @@ -1016,16 +1025,28 @@ fil_space_verify_crypt_checksum( > bool encrypted = (checksum == cchecksum1 || checksum == cchecksum2 > || checksum == BUF_NO_CHECKSUM_MAGIC); > > - /* Old InnoDB versions did not initialize > - FIL_PAGE_FILE_FLUSH_LSN field so there could be garbage > - and above checksum check could produce false positive. > - Thus we also check does the traditional stored > - checksum fields match the calculated one. Both of these > - could naturally produce false positive but then > - we just decrypt the page and after that corrupted > - pages very probable stay corrupted and valid > - pages very probable stay valid. > + /* MySQL 5.6 and MariaDB 10.0 and 10.1 will write an LSN to the > + first page of each system tablespace file at > + FIL_PAGE_FILE_FLUSH_LSN offset. On other pages and in other files, > + the field might have been uninitialized until MySQL 5.5. In MySQL 5.7 > + (and MariaDB Server 10.2.2) WL#7990 stopped writing the field for other > + than page 0 of the system tablespace. I think that we should conduct some research to make the comments more useful and reliable. Looking at the MySQL 3.23.49 source code, I see that we indeed did not initialize FIL_PAGE_FILE_FLUSH_LSN (or FIL_PAGE_ARCH_LOG_NO for that matter) in the system tablespace. The latter field was repurposed as SPACE_ID later. This means that the system tablespace (which was the only option before MySQL 4.1) can contain garbage in the FIL_PAGE_FILE_FLUSH_LSN field, except if we can infer from some other attributes of the system tablespace that it was created by MySQL 5.5 or later. The next interesting MySQL version to check is 4.1.2 where innodb_file_per_table was introduced. The oldest version in the 4.1 series that I was able to find is 4.1.21. It indeed does not make any effort to initialize FIL_PAGE_FILE_FLUSH_LSN on other pages than the first one. Because the buffer pool can be used for general-purpose memory allocations (BUF_BLOCK_MEMORY), it is possible that the FLUSH_LSN field will be written as nonzero garbage, even if we were able to assume that the initially allocated memory was zero-initialized by the operating system. So, the above comment is correct. > + Starting from MariaDB 10.1 the fiedl has been repurposed for > + encryption key_version. Correct the typo "fiedl". > + > + Starting with MySQL 5.7 (and MariaDB Server 10.2), the > + field has been repurposed for SPATIAL INDEX pages. Refer to the field name FIL_RTREE_SPLIT_SEQ_NUM in the comment, so that it can be found easily by grep. > + > + Note that FIL_PAGE_FILE_FLUSH_LSN is not included in the InnoDB page > + checksum. > + > + Thus,FIL_PAGE_FILE_FLUSH_LSN could contain uninitialized value, 0, > + correct key_version or spatial index information, thus we also > + verify page using traditional checksum check to detect if page > + would be valid but not encrypted. > */ Well, 10.1 cannot handle spatial index information. I would simply say: "Thus, FIL_PAGE_FILE_FLUSH_LSN could contain any value. While the field would usually be 0 for pages that are not encrypted, we cannot assume that a nonzero value means that the page is encrypted. Therefore we must validate the page both as encrypted and unencrypted when FIL_PAGE_FILE_FLUSH_LSN does not contain 0." OK to push with these changes. Note: I should still go through the whole logic, to be able to say if we really fixed the issues. I currently believe that more work is needed, but that should be tracked as a separate issue. Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

Re: [Maria-developers] [Commits] 3a8f101: MDEV-11379 - AliSQL: [Feature] Issue#8: SELECT FOR UPDATE WAIT
by Michael Widenius 08 Feb '17

08 Feb '17

Hi! >>>>> "Sergey" == Sergey Vojtovich <svoj(a)mariadb.org> writes: Sergey> revision-id: 3a8f101a173b987b314ba5fcc5c74d80ab56802b (mariadb-10.2.2-160-g3a8f101) Sergey> parent(s): c846ebe9df9adc90a5c25be0dce816c5d4302794 Sergey> committer: Sergey Vojtovich Sergey> timestamp: 2017-02-07 13:27:42 +0400 Sergey> message: Sergey> MDEV-11379 - AliSQL: [Feature] Issue#8: SELECT FOR UPDATE WAIT Sergey> Extended syntax so that it is now possible to set lock_wait_timeout for the Sergey> following statements: Sergey> SELECT ... FOR UPDATE [WAIT n|NOWAIT] Sergey> SELECT ... LOCK IN SHARED MODE [WAIT n|NOWAIT] Sergey> LOCK TABLE ... [WAIT n|NOWAIT] Sergey> This is amended AliSQL patch. We prefer Oracle syntax for [WAIT n|NOWAIT] Sergey> instead of original [WAIT [n]|NO_WAIT]. Agree it's better to use a syntax that someone else is using. Sergey> diff --git a/include/mysql/plugin.h b/include/mysql/plugin.h Sergey> index b3c71c65..b0a5849 100644 Sergey> --- a/include/mysql/plugin.h Sergey> +++ b/include/mysql/plugin.h Sergey> @@ -633,6 +633,7 @@ void thd_storage_lock_wait(MYSQL_THD thd, long long value); Sergey> int thd_tx_isolation(const MYSQL_THD thd); Sergey> int thd_tx_is_read_only(const MYSQL_THD thd); Sergey> int thd_rpl_is_parallel(const MYSQL_THD thd); Sergey> +unsigned long thd_lex_lock_wait_timeout(const MYSQL_THD thd); Sergey> /** Sergey> Create a temporary file. <cut> Sergey> +++ b/sql/sql_base.cc Sergey> @@ -2723,13 +2723,18 @@ Open_table_context::Open_table_context(THD *thd, uint flags) Sergey> :m_thd(thd), Sergey> m_failed_table(NULL), Sergey> m_start_of_statement_svp(thd->mdl_context.mdl_savepoint()), Sergey> - m_timeout(flags & MYSQL_LOCK_IGNORE_TIMEOUT ? Sergey> - LONG_TIMEOUT : thd->variables.lock_wait_timeout), Sergey> m_flags(flags), Sergey> m_action(OT_NO_ACTION), Sergey> m_has_locks(thd->mdl_context.has_locks()), Sergey> m_has_protection_against_grl(FALSE) Sergey> -{} Sergey> +{ Sergey> + if (flags & MYSQL_LOCK_IGNORE_TIMEOUT) Sergey> + m_timeout= LONG_TIMEOUT; Sergey> + else if (thd->lex->lock_wait_timeout == ULONG_MAX) Sergey> + m_timeout= thd->variables.lock_wait_timeout; Sergey> + else Sergey> + m_timeout= thd->lex->lock_wait_timeout; Sergey> +} Instead of adding thd->lex->lock_wait_timeout I suggest that we instead use the existing facititliy of SET STATEMENT innodb_lock_wait_timeout=xxx SELECT This has the following benefits: - We only need one innodb timeout variable, not two - Probably simpler code. Very few changes needed in InnoDB - One will get exactly same behaviour independent of the syntax that is used This would change the task to be implemented the following wat: - If WAIT|NOWAIT is used, change in sql_yacc.yy to use the SET STATEMENT facility to set innodb_lock_wait_timeout and lock_wait_timeout - Add support for nowait (timeout == 0 would mean nowait) I agree that having the new syntax is good as it's used by other databases and simpler than having to set two variables. Regards, Monty

1 0

Re: [Maria-developers] Please review MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler
by Alexander Barkov 07 Feb '17

07 Feb '17

Hello Alexey, Thanks for your suggestions. An improved version is attached. Please see comments inline: On 02/07/2017 03:51 PM, Alexey Botchkov wrote: > Hi, Alexander. > > Basically i approve this patch. It seems in logic with the previous work > in that area. > Two comments though >> -void Item_func_round::fix_length_and_dec() >> +void Item_func_round::fix_length_and_dec_decimal(uint decimals_to_set) >> { >> - int decimals_to_set; >> - longlong val1; > .... >> + decimals= decimals_to_set; >> + int decimals_delta= args[0]->decimals - decimals_to_set; >> + int precision= args[0]->decimal_precision(); >> + int length_increase= ((decimals_delta <= 0) || truncate) ? 0 : 1; >> + precision-= decimals_delta - length_increase; >> + max_length= my_decimal_precision_to_length_no_truncation(precision, > ..... > > Please move declarations of the 'int decimals_delta' 'int precision' and > 'int length_increase' > to the beginning of the function. It does make reading easier, and our > rules require it. > I'd also replace two lines with one: >> + int precision= args[0]->decimal_precision(); >> + precision-= decimals_delta - length_increase; > to > precision= args[0]->decimal_precision() + length_increase - > decimals_delta; Done. Thanks. It now looks better. > > +void Item_func_round::fix_arg_decimal() > +{ > if (!args[1]->const_item()) > { > > Since we started changing this code we can add some fixes to it. > Firstly i think 'const_item()' should be replaced with the > 'basic_const_item()' there. > The later is faster and safer in case when the 'const item' is a > complicated (yet constant) subquery. As discussed on IRC, it can introduce a behavior change, which is not desirable under terms of this task. > Secondly it'd be nicer if we do the 'if (args[1]->const_item)' the first > choice. (i mean not !args[1]->...) > but if you don't like that you can skip this suggestion :) Done. > > Best regards. > HF. > > > On Mon, Feb 6, 2017 at 11:26 AM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Alexey, > > Can you please review a patch for MDEV-12001? > > Thanks! > >

1 0

Re: [Maria-developers] [Commits] 90010cb4cbc: MDEV-11974: MariaDB 10.2 encryption does not support spatial indexes
by Marko Mäkelä 07 Feb '17

07 Feb '17

On Tue, Feb 7, 2017 at 3:56 PM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: 90010cb4cbcc538181f043dbeccd776776bdd5a4 (mariadb-10.2.3-171-g90010cb4cbc) > parent(s): 92bbf4ad0477e09bcc86907696cd114ef42e6914 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-02-07 15:55:01 +0200 > message: > > MDEV-11974: MariaDB 10.2 encryption does not support spatial indexes Add a short explanation why we cannot support encrypted spatial indexes. > fil0crypt.cc: Do not encrypt page if page type is > FIL_PAGE_RTREE (this is required for background > encryption innodb-encrypt-tables=ON) > > ha_innodb.cc: check_table_options() Do not allow creating > table with ENCRYPTED=YES if table contains spatial index. Instead of file names, please use the function names in the commit comment. > +# > +# Index creation > +# > +CREATE TABLE t1 (pk INT PRIMARY KEY AUTO_INCREMENT, > +c VARCHAR(256), coordinate POINT NOT NULL) ENCRYPTED=YES ENGINE=INNODB; > +--replace_regex /#sql-[0-9a-f_]*`/#sql-temporary`/ > +--error ER_CANT_CREATE_TABLE > +CREATE SPATIAL INDEX b on t1(coordinate); > +--replace_regex /#sql-[0-9a-f_]*`/#sql-temporary`/ > +--error ER_CANT_CREATE_TABLE > +ALTER TABLE t1 ADD SPATIAL INDEX b(coordinate); > +DROP TABLE t1; Test this also with default encryption. > +--echo # Wait max 10 min for key encryption threads to encrypt all space > +let $cnt=600; > +while ($cnt) > +{ > + let $success=`SELECT COUNT(*) = 0 FROM INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION WHERE MIN_KEY_VERSION = 0`; Use --let $wait_timeout=600 --let $wait_condition= SELECT COUNT(*) = 0 FROM INFORMATION_SCHEMA.INNODB_TABLESPACES_ENCRYPTION WHERE MIN_KEY_VERSION = 0 --source include/wait_condition.inc > +DROP TABLE t1, t2; > \ No newline at end of file Add the missing newline to the end of the .test file. Please demonstrate in some way that page compression actually works with SPATIAL INDEX. Can you for example run --suite=innodb_gis with some --mysqld=--innodb… option that will cause all tables to be compressed by default? Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

[Maria-developers] Please review MDEV-12001 Split Item_func_round::fix_length_and_dec to virtual methods in Type_handler
by Alexander Barkov 06 Feb '17

06 Feb '17

Hello Alexey, Can you please review a patch for MDEV-12001? Thanks!

1 0

Re: [Maria-developers] [Commits] 787610a: MDEV-10766: Queries which start with WITH clause do not get inserted into query cache
by Sergey Petrunia 04 Feb '17

04 Feb '17

Hi Varun, On Fri, Feb 03, 2017 at 08:20:51PM +0530, Varun wrote: > revision-id: 787610a1f5cec04311fd16f6c1de87171e5cfdd4 (mariadb-10.2.3-159-g787610a) > parent(s): bc12d993d7bc94a9533028a258afc7e4ceb21e92 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-02-03 20:17:16 +0530 > message: > > MDEV-10766: Queries which start with WITH clause do not get inserted into query cache > > Added conditions so that the WITH queries are also added to the query cache > > --- > mysql-test/r/query_cache.result | 30 ++++++++++++++++++++++++++++++ > mysql-test/t/query_cache.test | 14 ++++++++++++++ > sql/sql_cache.cc | 5 ++++- > 3 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/query_cache.result b/mysql-test/r/query_cache.result > index d34c96c..fa169d4 100644 > --- a/mysql-test/r/query_cache.result > +++ b/mysql-test/r/query_cache.result > @@ -2136,6 +2136,36 @@ Qcache_hits 1 > use test; > drop database `foo.bar`; > End of 10.0 tests > +FLUSH USER_STATISTICS; > +FLUSH CLIENT_STATISTICS; I'm wondering what is the reason for the above? Qcache_XXX status variables are flushed with FLUSH STATUS. > +flush status; > +show status like "Qcache_inserts"; > +Variable_name Value > +Qcache_inserts 0 > +create table t1 (i int); > +with cte as (select * from t1) select * from cte; > +i > +show status like "Qcache_queries_in_cache"; > +Variable_name Value > +Qcache_queries_in_cache 1 > +show status like "Qcache_inserts"; > +Variable_name Value > +Qcache_inserts 1 > +show status like "Qcache_hits"; > +Variable_name Value > +Qcache_hits 0 > +with cte as (select * from t1) select * from cte; > +i > +show status like "Qcache_queries_in_cache"; > +Variable_name Value > +Qcache_queries_in_cache 1 > +show status like "Qcache_inserts"; > +Variable_name Value > +Qcache_inserts 1 > +show status like "Qcache_hits"; > +Variable_name Value > +Qcache_hits 1 > +drop table t1; > restore defaults > SET GLOBAL query_cache_type= default; > SET GLOBAL query_cache_size= default; BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

2 1

[Maria-developers] the patch for mdev-9976 broke some tests
by Alexander Barkov 01 Feb '17

01 Feb '17

Hello Igor, These tests fails in 10.2 due to ROWS now being a reserved word: binlog_encryption.binlog_write_error rpl.rpl_killed_ddl binlog.binlog_write_error Can you please fix this? Thanks!

1 0

[Maria-developers] MDEV-10142 - bb-10.2-compatibility
by jerome brauge 01 Feb '17

01 Feb '17

Hello, To build this branch on windows (Visual Studio Express 2015), I have to add "#pragma warning( disable : 4099 )" in sql_lex.h. But I don't know if it's the right solution. I have some questions: - I read the comment of Michael Widenius on subtask MDEV-10574. I think that it's important to change behavior of string functions whose returns a string (like rtrim, ltrim, substring, concat, ...) to return null instead of ''. Without this, we have to use UDF instead native functions and it's never good from a performance point of view. Have you made a decision ? - On Oracle, we can easily disable/enable triggers. I found an old request for this on jira : MDEV-7579. As you already manage order_action now, perhaps you can simply use negative orders to indicate disabled triggers. - do you have planning to implement operator || to concat strings ? Best regards, J. Brauge

3 16

[Maria-developers] valgrind + SSL
by Alexander Barkov 01 Feb '17

01 Feb '17

Hello Elena, You wrote on IRC: On 01/31/2017 08:10 PM, Slack wrote: > bar: hi! We have this in 10.0 valgrind, what do you think about it? > http://buildbot.askmonty.org/buildbot/builders/work-amd64-valgrind/builds/9… > > i'm asking you because you fixed a similar-looking problem once: > https://jira.mariadb.org/browse/MDEV-4890 I'm not very familiar with this part. But IIUC, two things could happen: - Somebody changed the code in mysqld.cc, so the SSL cleanup routines do not get called correctly any more. - Somebody is wrong inside the library itself. When did this problem first appear? After some commit? Or after the SSQL library update/change on the build box? Is it specific to 10.0? When I was fixing MDEV-4890, I fixed both: - added some cleanup function calls - added the second SSL suppression The problem was reproducible on my box. Now I cannot reproduce the problem on my box. I tried: cmake . -DCMAKE_BUILD_TYPE=Debug -DWITH_VALGRIND=1 ./mtr --valgrind binlog.binlog_max_extension ./mtr --valgrind federated.error_on_close-8313 ./mtr --valgrind rpl.rpl_old_master Note, we have these valgrind suppressions: > { > OpenSSL still reachable. > Memcheck:Leak > fun:*alloc > fun:CRYPTO_malloc > fun:sk_new > obj:*libssl* > fun:SSL_COMP_get_compression_methods > fun:SSL_library_init > } > > > { > OpenSSL still reachable. > Memcheck:Leak > fun:*alloc > fun:CRYPTO_malloc > obj:*libssl* > fun:SSL_COMP_get_compression_methods > fun:SSL_library_init > } Do you know why the problem does not get suppressed? Perhaps we should try to remove "obj:*libssl*" ? But before doing this it would be nice to know when the problem first appeared.

1 0

Re: [Maria-developers] Please review MDEV-9979 Keywords UNBOUNDED, PRECEDING, FOLLOWING, TIES, OTHERS should be non-reserved
by Vicențiu Ciorbaru 31 Jan '17

31 Jan '17

Hi Alexander, OK to push. Please add the version of the standard you used to check these keywords as non-reserved both to the commit message, as well as the keywords themselves. I used 2011 standard draft to check. Vicențiu On Mon, 23 Jan 2017 at 09:28 Alexander Barkov <bar(a)mariadb.com> wrote: > Hello Vicențiu, > > Can you please review a patch for MDEV-9979. > > Thanks! >

2 1

[Maria-developers] Please review MDEV-11692 Comparison data type aggregation for pluggable data types
by Alexander Barkov 31 Jan '17

31 Jan '17

Hello Vicențiu, can you please review a patch for MDEV-11692. The patch is for bb-10.2-ext. Thanks!

2 1

Re: [Maria-developers] [Commits] ef590fa: MDEV-7409 On RBR, extend the PROCESSLIST info to include at least the name of the recently used table
by Daniel Black 31 Jan '17

31 Jan '17

On 24/01/17 04:30, SachinSetiya wrote: > revision-id: ef590faa0e21d25a2dc5153f938135612b17ecbc (mariadb-10.1.20-31-gef590fa) > thd_proc_info(thd, message); > + DBUG_EXECUTE_IF("should_sleep_for_mdev7409",{ > + my_sleep(500000); > + };); Shouldn't these be DEBUG_SYNC_C("do_exec_row"); and then use debug_sync to generate test synchronisation. > int error= write_row(rgi, slave_exec_mode == SLAVE_EXEC_MODE_IDEMPOTENT); > thd_proc_info(thd, tmp); > + thd->db= tmp_db; > > if (error && !thd->is_error()) > { > @@ -12371,32 +12380,45 @@ int Delete_rows_log_event::do_exec_row(rpl_group_info *rgi) > { > int error; > const char *tmp= thd->get_proc_info(); > - const char *message= "Delete_rows_log_event::find_row()"; > + char *tmp_db= thd->db; > + char *message, msg[128]; > + my_snprintf(msg, sizeof(msg),"Delete_rows_log_event::find_row() on table %s", > + m_table->s->table_name.str); > + thd->db= m_table->s->db.str; > + message= msg; Why assign a pointer? Isn't defining message as a char[128] sufficient? Thanks for writing this feature.

3 3

Re: [Maria-developers] [Commits] d44c14e: MDEV-10812 WSREP causes responses being sent to protocol commands that must not send a response
by Nirbhay Choubey 30 Jan '17

30 Jan '17

Hi Sachin! On Fri, Jan 27, 2017 at 12:53 AM, SachinSetiya <sachin.setiya(a)mariadb.com> wrote: > revision-id: d44c14e1e0dea312779ba0a8633583ee94284295 > (mariadb-10.1.21-2-gd44c14e) > parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-27 11:23:17 +0530 > message: > > MDEV-10812 WSREP causes responses being sent to protocol commands that > must not send a response > > Problem:- When using wsrep (w/ galera) and issuing commands that can cause > deadlocks, deadlock exception errors are sent in responses to commands > such as close prepared statement and close connection which, by spec, must > not send a > In commit message, we normally limit line width to 60 chars. > response. > > Solution:- In dispatch_command, we will handle COM_QUIT and COM_STMT_CLOSE > commands even in case of error. I think you forgot to add the patch credit you had in previous commits. > --- > mysql-test/suite/galera/r/galera_mdev_10812.result | 11 +++++++++ > mysql-test/suite/galera/t/galera_mdev_10812.test | 27 > ++++++++++++++++++++++ > sql/sql_parse.cc | 10 +++++++- > 3 files changed, 47 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/galera/r/galera_mdev_10812.result > b/mysql-test/suite/galera/r/galera_mdev_10812.result > new file mode 100644 > index 0000000..fba1000 > --- /dev/null > +++ b/mysql-test/suite/galera/r/galera_mdev_10812.result > @@ -0,0 +1,11 @@ > +# > +# MDEV-10812: On COM_STMT_CLOSE/COM_QUIT, when wsrep_conflict_state > +# is ABORTED, it causes wrong response to be sent to the client > +# > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t1(a INT PRIMARY KEY); > +INSERT INTO t1 VALUES(1),(2),(3); > +START TRANSACTION ; > +UPDATE t1 SET a=a+100; > +UPDATE t1 SET a=a+100; > +DROP TABLE t1; > diff --git a/mysql-test/suite/galera/t/galera_mdev_10812.test > b/mysql-test/suite/galera/t/galera_mdev_10812.test > new file mode 100644 > index 0000000..4539ab6 > --- /dev/null > +++ b/mysql-test/suite/galera/t/galera_mdev_10812.test > @@ -0,0 +1,27 @@ > +--source include/galera_cluster.inc > +--source include/have_innodb.inc > + > +--echo # > +--echo # MDEV-10812: On COM_STMT_CLOSE/COM_QUIT, when wsrep_conflict_state > +--echo # is ABORTED, it causes wrong response to be sent to the client > +--echo # > + > +# First create a deadlock > +--connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1 > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t1(a INT PRIMARY KEY); > +INSERT INTO t1 VALUES(1),(2),(3); > +START TRANSACTION ; > +UPDATE t1 SET a=a+100; > + > +--sleep 2 > +--connection node_2 > +UPDATE t1 SET a=a+100; > + > +--sleep 2 > +--connection node_1a > +# here we get deadlock error > +--disconnect node_1a > + > +--connection node_2 > +DROP TABLE t1; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 033e88a..cb0bd41 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > { > wsrep_client_rollback(thd); > } > - if (thd->wsrep_conflict_state== ABORTED) > + /* We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep > aborted. */ > + if (thd->wsrep_conflict_state == ABORTED && > + command != COM_STMT_CLOSE && command != COM_QUIT) > { > my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction"); > WSREP_DEBUG("Deadlock error for: %s", thd->query()); > @@ -1918,6 +1920,12 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > > if (WSREP(thd)) > { > + /* > + MDEV-10812 > + In the case of COM_QUIT/COM_CLOSE thread status should be disabled. > s/COM_CLOSE/COM_STMT_CLOSE/ > + */ > + DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE) > + || thd->get_stmt_da()->is_disabled()); > Could you check the indentation of the above line? Considering above changes, I have no more suggestions. Ok to push. Best, Nirbhay > /* wsrep BF abort in query exec phase */ > mysql_mutex_lock(&thd->LOCK_wsrep_thd); > do_end_of_statement= thd->wsrep_conflict_state != REPLAYING && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 2

[Maria-developers] 10.2 branch is locked down
by Sergei Golubchik 29 Jan '17

29 Jan '17

Hi, 10.2 branch has recently become quite red in buildbot. Eventually we'll be implementing something to prevent it in the future, but for now we've simply locked it down to prevent further pushes. If you want to push make sure you push into a stage branch first, then this stage branch should be green or at least it should fix at least some of test failures from the main 10.2. Then either send an email or ping on irc or create a pull request and we'll add you to the list of people who can push. At the moment this list is empty. When 10.2 is green again, we'll remove this protection. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 5190ebd: MDEV-11817: Altering a table with more rows than ..
by Sachin Setiya 29 Jan '17

29 Jan '17

Hi Nirbhay! On Tue, Jan 24, 2017 at 8:02 AM, Nirbhay Choubey <nirbhay(a)mariadb.com> wrote: > > revision-id: 5190ebde3aa142219e1c703df6ea1a78f171182c (mariadb-10.1.21-7-g5190ebd) > parent(s): 15f46d517435f3570e2c788349637a06d818a619 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2017-01-23 21:32:50 -0500 > message: > > MDEV-11817: Altering a table with more rows than .. > > .. wsrep_max_ws_rows causes cluster to break when running > Galera cluster in TOI mode > > During ALTER TABLE, while copying records to temporary > table, since the records are not placed into the binary > log, wsrep_affected_rows must also not be incremented. > I think, you should write a more detailed commit message, So that user/developer can link this commit to jira task body. May be add something like this Problem:- 1. When doing altering table in galera cluster if table has more records than wsrep_max_ws_rows it gives error. 2. Doing this also breaks replication. Solution:- 1. Your commit message. 2. Replication is broke because setting up wsrep_max_ws_rows is not transferred to another cluster. And as ALTER command is passed in statement format and it is passed much earlier. here in Sql_cmd_alter_table::execute(THD *thd) function. #ifdef WITH_WSREP TABLE *find_temporary_table(THD *thd, const TABLE_LIST *tl); if ((!thd->is_current_stmt_binlog_format_row() || !find_temporary_table(thd, first_table))) { WSREP_TO_ISOLATION_BEGIN(((lex->name.str) ? select_lex->db : NULL), ((lex->name.str) ? lex->name.str : NULL), first_table); } this send alter command to another node , before it is executed on node. Galera assumes that if alter command is failed on one node it should also be failing on other nodes. Or may be this can be added in comment. > > --- > .../suite/galera/r/galera_var_max_ws_rows.result | 23 ++++++++ > .../suite/galera/t/galera_var_max_ws_rows.test | 28 +++++++++- > sql/handler.cc | 61 ++++++++++++---------- > 3 files changed, 83 insertions(+), 29 deletions(-) > > diff --git a/mysql-test/suite/galera/r/galera_var_max_ws_rows.result b/mysql-test/suite/galera/r/galera_var_max_ws_rows.result > index 6e239c7..555bd1a 100644 > --- a/mysql-test/suite/galera/r/galera_var_max_ws_rows.result > +++ b/mysql-test/suite/galera/r/galera_var_max_ws_rows.result > @@ -113,3 +113,26 @@ INSERT INTO t1 (f2) VALUES (2); > ERROR HY000: wsrep_max_ws_rows exceeded > DROP TABLE t1; > DROP TABLE ten; > +# > +# MDEV-11817: Altering a table with more rows than > +# wsrep_max_ws_rows causes cluster to break when running > +# Galera cluster in TOI mode > +# > +CREATE TABLE t1(c1 INT)ENGINE = INNODB; > +SET GLOBAL wsrep_max_ws_rows= DEFAULT; > +INSERT INTO t1 VALUES(1); > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +SET GLOBAL wsrep_max_ws_rows= 10; > +ALTER TABLE t1 CHANGE COLUMN c1 c1 BIGINT; > +SHOW CREATE TABLE t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `c1` bigint(20) DEFAULT NULL > +) ENGINE=InnoDB DEFAULT CHARSET=latin1 > +SELECT COUNT(*) FROM t1; > +COUNT(*) > +16 > +DROP TABLE t1; > diff --git a/mysql-test/suite/galera/t/galera_var_max_ws_rows.test b/mysql-test/suite/galera/t/galera_var_max_ws_rows.test > index 944238b..4b2ba60 100644 > --- a/mysql-test/suite/galera/t/galera_var_max_ws_rows.test > +++ b/mysql-test/suite/galera/t/galera_var_max_ws_rows.test > @@ -146,10 +146,34 @@ INSERT INTO t1 (f2) VALUES (1); > --error ER_ERROR_DURING_COMMIT > INSERT INTO t1 (f2) VALUES (2); > > +DROP TABLE t1; > +DROP TABLE ten; > + > +--echo # > +--echo # MDEV-11817: Altering a table with more rows than > +--echo # wsrep_max_ws_rows causes cluster to break when running > +--echo # Galera cluster in TOI mode > +--echo # > +--connection node_1 > +CREATE TABLE t1(c1 INT)ENGINE = INNODB; > +SET GLOBAL wsrep_max_ws_rows= DEFAULT; > +INSERT INTO t1 VALUES(1); > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +INSERT INTO t1 SELECT * FROM t1; > +SET GLOBAL wsrep_max_ws_rows= 10; > +ALTER TABLE t1 CHANGE COLUMN c1 c1 BIGINT; > + > +--connection node_2 > +SHOW CREATE TABLE t1; > +SELECT COUNT(*) FROM t1; > +DROP TABLE t1; > + > +--connection node_1 > > --disable_query_log > --eval SET GLOBAL wsrep_max_ws_rows = $wsrep_max_ws_rows_orig > --enable_query_log > > -DROP TABLE t1; > -DROP TABLE ten; > +--source include/galera_end.inc > diff --git a/sql/handler.cc b/sql/handler.cc > index af06427..48946de 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -5724,6 +5724,30 @@ static int write_locked_table_maps(THD *thd) > } > > > +static int check_wsrep_max_ws_rows() > +{ > +#ifdef WITH_WSREP > + if (wsrep_max_ws_rows) > + { > + THD *thd= current_thd; > + > + if (!WSREP(thd)) > + return 0; > + > + thd->wsrep_affected_rows++; > + if (thd->wsrep_exec_mode != REPL_RECV && > + thd->wsrep_affected_rows > wsrep_max_ws_rows) > + { > + trans_rollback_stmt(thd) || trans_rollback(thd); > + my_message(ER_ERROR_DURING_COMMIT, "wsrep_max_ws_rows exceeded", MYF(0)); > + return ER_ERROR_DURING_COMMIT; > + } > + } > +#endif /* WITH_WSREP */ > + return 0; > +} > + > + I think declaring check_wsrep_max_ws_rows() on top would be a much better option. > typedef bool Log_func(THD*, TABLE*, bool, const uchar*, const uchar*); > > static int binlog_log_row(TABLE* table, > @@ -5765,6 +5789,13 @@ static int binlog_log_row(TABLE* table, > bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE || > table->file->has_transactions(); > error= (*log_func)(thd, table, has_trans, before_record, after_record); > + > + /* > + Now that the record has been logged, increment wsrep_affected_rows and > + also check whether its within the allowable limits (wsrep_max_ws_rows). > + */ > + if (error == 0) > + error= check_wsrep_max_ws_rows(); > } > } > return error ? HA_ERR_RBR_LOGGING_FAILED : 0; > @@ -5875,30 +5906,6 @@ int handler::ha_reset() > } > > > -static int check_wsrep_max_ws_rows() > -{ > -#ifdef WITH_WSREP > - if (wsrep_max_ws_rows) > - { > - THD *thd= current_thd; > - > - if (!WSREP(thd)) > - return 0; > - > - thd->wsrep_affected_rows++; > - if (thd->wsrep_exec_mode != REPL_RECV && > - thd->wsrep_affected_rows > wsrep_max_ws_rows) > - { > - trans_rollback_stmt(thd) || trans_rollback(thd); > - my_message(ER_ERROR_DURING_COMMIT, "wsrep_max_ws_rows exceeded", MYF(0)); > - return ER_ERROR_DURING_COMMIT; > - } > - } > -#endif /* WITH_WSREP */ > - return 0; > -} > - > - > int handler::ha_write_row(uchar *buf) > { > int error; > @@ -5923,7 +5930,7 @@ int handler::ha_write_row(uchar *buf) > DBUG_RETURN(error); /* purecov: inspected */ > > DEBUG_SYNC_C("ha_write_row_end"); > - DBUG_RETURN(check_wsrep_max_ws_rows()); > + DBUG_RETURN(0); > } > > > @@ -5954,7 +5961,7 @@ int handler::ha_update_row(const uchar *old_data, uchar *new_data) > rows_changed++; > if (unlikely(error= binlog_log_row(table, old_data, new_data, log_func))) > return error; > - return check_wsrep_max_ws_rows(); > + return 0; > } > > int handler::ha_delete_row(const uchar *buf) > @@ -5981,7 +5988,7 @@ int handler::ha_delete_row(const uchar *buf) > rows_changed++; > if (unlikely(error= binlog_log_row(table, buf, 0, log_func))) > return error; > - return check_wsrep_max_ws_rows(); > + return 0; > } > > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- Regards Sachin Setiya Software Engineer at MariaDB

2 2

[Maria-developers] Fwd: [Commits] d1124c9: MDEV-10812 WSREP causes responses being sent to protocol commands that must not send a response
by Sachin Setiya 27 Jan '17

27 Jan '17

Hi Nirbhay! On Wed, Jan 25, 2017 at 7:53 PM, Nirbhay Choubey <nirbhay(a)mariadb.com> wrote: > Hi Sachin, > > My comments below. > > On Sun, Jan 22, 2017 at 7:18 AM, SachinSetiya <sachin.setiya(a)mariadb.com> > wrote: >> >> revision-id: d1124c98cbede6366e4b4d27a23dcf5b9d207cf0 >> (mariadb-10.1.21-2-gd1124c9) >> parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e >> author: Sachin Setiya >> committer: Sachin Setiya >> timestamp: 2017-01-22 17:47:29 +0530 >> message: >> >> MDEV-10812 WSREP causes responses being sent to protocol commands that >> must not send a response >> >> Problem:- When using wsrep (w/ galera) and issuing commands that can cause >> deadlocks, deadlock exception errors are sent in responses to commands >> such as close prepared statement () which, by spec, must not send a > > > s/close prepared statement ()/close prepared statement and close connection/ > >> response. >> >> Solution:- We will not jump to dispatch_end: when there is deadlock and >> query is either COM_QUIT or COM_STMT_CLOSE. > > > Or, I would say, in dispatch_command, we handle COM_QUIT and COM_STMT_CLOSE > commands even in case of error. Changed. > >> >> >> Patch Credit:- Jaka Močnik >> >> --- >> .../galera/r/galera_prepared_statement.result | 18 ++++++++++++++++ >> .../suite/galera/t/galera_prepared_statement.test | 25 >> ++++++++++++++++++++++ >> sql/sql_parse.cc | 6 +++++- >> 3 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/mysql-test/suite/galera/r/galera_prepared_statement.result >> b/mysql-test/suite/galera/r/galera_prepared_statement.result >> index de5ac9c..3425648 100644 >> --- a/mysql-test/suite/galera/r/galera_prepared_statement.result >> +++ b/mysql-test/suite/galera/r/galera_prepared_statement.result >> @@ -27,7 +27,25 @@ ALTER TABLE t1 ADD COLUMN f2 INTEGER; >> ALTER TABLE t1 DROP COLUMN f1; >> EXECUTE st1; >> ERROR 22007: Incorrect integer value: 'abc' for column 'f2' at row 1 >> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; >> +CREATE TABLE t5(a int primary key); >> +INSERT INTO t5 VALUES(1),(2),(3); >> +START TRANSACTION ; >> +PREPARE st5 FROM 'SELECT * FROM t5'; >> +EXECUTE st5; >> +a >> +1 >> +2 >> +3 >> +update t5 set a=a+100; >> +EXECUTE st5; >> +a >> +101 >> +102 >> +103 >> +update t5 set a=a+100; >> DROP TABLE t1; >> DROP TABLE t2; >> DROP TABLE t3; >> DROP TABLE t4; >> +DROP TABLE t5; >> >> diff --git a/mysql-test/suite/galera/t/galera_prepared_statement.test >> b/mysql-test/suite/galera/t/galera_prepared_statement.test >> index 3bee097..debc00e 100644 >> --- a/mysql-test/suite/galera/t/galera_prepared_statement.test >> +++ b/mysql-test/suite/galera/t/galera_prepared_statement.test >> @@ -38,8 +38,33 @@ ALTER TABLE t1 DROP COLUMN f1; >> --error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD >> EXECUTE st1; >> >> +# MDEV-10812 >> +# On Closing Prepare Stamemnt, When wsrep_conflict_state is ABORTED >> +# It causes wrong response to be sent on Clients. >> + >> +# First Create a Deadlock >> --connection node_1 >> +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; >> +CREATE TABLE t5(a int primary key); >> +INSERT INTO t5 VALUES(1),(2),(3); >> +START TRANSACTION ; >> +PREPARE st5 FROM 'SELECT * FROM t5'; >> +EXECUTE st5; >> +update t5 set a=a+100; >> +EXECUTE st5; >> + >> +--sleep 2 >> +--connection node_2 >> +update t5 set a=a+100; >> + >> +--sleep 2 >> +--connection node_1 >> +# here we have deadlock >> +--disconnect node_1 >> + >> +--connection node_2 >> DROP TABLE t1; >> DROP TABLE t2; >> DROP TABLE t3; >> DROP TABLE t4; >> +DROP TABLE t5; > > > > The test always hits command != COM_QUIT assert if I revert : > > + if (thd->wsrep_conflict_state == ABORTED && > + command != COM_STMT_CLOSE && command != COM_QUIT) > > So, I modified your test a bit (simplified) to not use PS. > > --echo # > --echo # MDEV-10812: On closing prepare stamemnt, when wsrep_conflict_state > --echo # is ABORTED, it causes wrong response to be sent to the client > --echo # > > # First create a deadlock > --connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1 > SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > CREATE TABLE t1(a INT PRIMARY KEY); > INSERT INTO t1 VALUES(1),(2),(3); > START TRANSACTION ; > UPDATE t1 SET a=a+100; > > --sleep 2 > --connection node_2 > UPDATE t1 SET a=a+100; > > --sleep 2 > --connection node_1a > # here we get deadlock error > --disconnect node_1a > > --connection node_2 > DROP TABLE t1; > > I think it should be ok. > > Yes, also Because Initial prepared statement test was written because, I thought we will hit COM_STMT_CLOSE, but is simple not possible using mtr. Test changed. >> >> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc >> index 033e88a..d45b196 100644 >> --- a/sql/sql_parse.cc >> +++ b/sql/sql_parse.cc >> @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command >> command, THD *thd, >> { >> wsrep_client_rollback(thd); >> } >> - if (thd->wsrep_conflict_state== ABORTED) >> + /* we let COM_QUIT and STMT_CLOSE execute even if wsrep aborted */ > Changed. > > We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep aborted. > >> + if (thd->wsrep_conflict_state == ABORTED && >> + command != COM_STMT_CLOSE && command != COM_QUIT) >> { >> my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction"); >> WSREP_DEBUG("Deadlock error for: %s", thd->query()); >> @@ -1918,6 +1920,8 @@ bool dispatch_command(enum enum_server_command >> command, THD *thd, >> >> if (WSREP(thd)) >> { >> + DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE) >> + || thd->get_stmt_da()->is_disabled()); > > > Please add a comment explaining this assert. > Added. > Thanks, > Nirbhay > >> >> /* wsrep BF abort in query exec phase */ >> mysql_mutex_lock(&thd->LOCK_wsrep_thd); >> do_end_of_statement= thd->wsrep_conflict_state != REPLAYING && >> _______________________________________________ >> commits mailing list >> commits(a)mariadb.org >> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits > > http://lists.askmonty.org/pipermail/commits/2017-January/010534.html -- Regards Sachin Setiya Software Engineer at MariaDB -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

[Maria-developers] MDEV-10142 - / MDEV-10343 - bb-10.2-compatibility
by jerome brauge 27 Jan '17

27 Jan '17

Hi, I'm testing datatype compatibility provide by MDEV-10343 and I have some comments: 1) Oracle character datatype (char, varchar, varchar2) doesn't accept a length of 0 char Example : select cast('tt' as char(0)) from dual; ORA-01723: zero-length columns are not allowed MariaDB return an empty string (to relate with MDEV-10574) This point is a study case, no matter. 2) NUMBER Type You have choosing TYPE_DOUBLE if scale/prec is not specified and I think that it's a mistake for 2 reasons: - Oracle NUMBER() is near an "exact" datatype and cannot be replace by a standard double. - Convert number to char never use scientific notation The use of a decimal(64,24) makes it possible to obtain results very close to those of oracle. I attach two files to show this. In fact FLOAT and INTEGER are subtype of NUMBER for Oracle. MariaDB float and double seems to be like BINARY_FLOAT and BINARY_DOUBLE. See https://docs.oracle.com/cd/B28359_01/server.111/b28285/sqlqr06.htm#CHDBBHHE for more details. Best regard, Jérôme.

2 1

Re: [Maria-developers] [Commits] fb49b98: MDEV-10436 non-deterministic vcol does not force rbr
by Daniel Black 27 Jan '17

27 Jan '17

On 25/01/17 00:33, sachin wrote: > @@ -5824,6 +5826,16 @@ int THD::decide_logging_format(TABLE_LIST *tables) > table->table->file->ht) > multi_access_engine= TRUE; > > + /* > + Loop through each table field if it is virtual field check if it is deterministic > + or not. > + */ > + for (field_ptr= table->table->field; *field_ptr; field_ptr++) > + { > + field= *field_ptr; > + if (field->vcol_info && field->vcol_info->flags & VCOL_NOT_STRICTLY_DETERMINISTIC) > + lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_NON_DETER_VCOL); you could break the loop at this point. > + } > prev_access_table= table->table; > } >

1 0

[Maria-developers] MDEV-11829 - Questions on timezones
by Seth Shelnutt 26 Jan '17

26 Jan '17

I am working on a patch for adding datetime parsing support for RFC3339 as reported in MDEV-11829. In examining methods to use the provided offset to convert the parsed datetime to the timezone of mariadb. I have some questions on the my_system_gmt_sec function in sql-common/my_time.c . >From testing on linux (rhel 7 and gentoo), it seems the timezone computed by my_system_gmt_sec is off by 7200 seconds and has a inverse sign. I am trying to determine why this is and if it is designed to be this way. From the comments in the file, it seems my_system_gmt_sec was designed specifically due to cross-platform issues. However there is no mention that it the timezone computed is suppose to be off or have an inverse sign. I could not find any usages of the my_time_zone global variable or where another function used the timezone computed by my_system_gmt_sec, so this might not effect anything, but I wanted some clarification. Here is some testing results: | ---------------------------- | ------------------------------------------ | -------------- | | Server TimeZone | my_system_gmt_sec timezone | Difference | | ---------------------------- | ------------------------------------------ | -------------- | | -18000 (-05:00 EST) | 25200 | 43200 | | 3600 (+01:00 CET) | -10800 | 14400 | | 0 (+00:00 GMT) | 7200 | 7200 | | ---------------------------- | ------------------------------------------ | -------------- | In testing I've been able to determine that the 7200 second deviation is caused by compensating for the "-3600" seconds twice, first in setting an initial timezone in my_init_time: https://github.com/MariaDB/server/blob/10.1/sql-common/my_time.c#L709 Inside my_system_gmt_sec the "current_timezone" is set to the "my_time_zone" which is at 3600 initially. Then below another 3600 is added to the timezone computation: https://github.com/MariaDB/server/blob/10.1/sql-common/my_time.c#L946 The diff variable that is computed has the correct offset, with an inverse sign. If current_timezone were set to "diff" then it would have the correct timezone but again with an inverse sign. The inverse sign comes from the doing t - l_time instead of l_time - t. Should the timezone computed have an inverse sign? Is there a reason it is offset by 7200 seconds? Perhaps the behavior is different on other platform, that I have not tested. Thank you, Seth Shelnutt

1 0

[Maria-developers] MDEV-11913 Split sp_get_item_value() into methods in Type_handler
by Alexander Barkov 26 Jan '17

26 Jan '17

Hello Sanja, Can you please review a patch for MDEV-11913? Thanks!

1 0

Re: [Maria-developers] [Commits] d1124c9: MDEV-10812 WSREP causes responses being sent to protocol commands that must not send a response
by Nirbhay Choubey 25 Jan '17

25 Jan '17

Hi Sachin, My comments below. On Sun, Jan 22, 2017 at 7:18 AM, SachinSetiya <sachin.setiya(a)mariadb.com> wrote: > revision-id: d1124c98cbede6366e4b4d27a23dcf5b9d207cf0 > (mariadb-10.1.21-2-gd1124c9) > parent(s): a14638581b4c8ef175e68dccff07967d819b3b7e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-22 17:47:29 +0530 > message: > > MDEV-10812 WSREP causes responses being sent to protocol commands that > must not send a response > > Problem:- When using wsrep (w/ galera) and issuing commands that can cause > deadlocks, deadlock exception errors are sent in responses to commands > such as close prepared statement () which, by spec, must not send a > s/close prepared statement ()/close prepared statement and close connection/ response. > > Solution:- We will not jump to dispatch_end: when there is deadlock and > query is either COM_QUIT or COM_STMT_CLOSE. > Or, I would say, in dispatch_command, we handle COM_QUIT and COM_STMT_CLOSE commands even in case of error. > > Patch Credit:- Jaka Močnik > > --- > .../galera/r/galera_prepared_statement.result | 18 ++++++++++++++++ > .../suite/galera/t/galera_prepared_statement.test | 25 > ++++++++++++++++++++++ > sql/sql_parse.cc | 6 +++++- > 3 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/galera/r/galera_prepared_statement.result > b/mysql-test/suite/galera/r/galera_prepared_statement.result > index de5ac9c..3425648 100644 > --- a/mysql-test/suite/galera/r/galera_prepared_statement.result > +++ b/mysql-test/suite/galera/r/galera_prepared_statement.result > @@ -27,7 +27,25 @@ ALTER TABLE t1 ADD COLUMN f2 INTEGER; > ALTER TABLE t1 DROP COLUMN f1; > EXECUTE st1; > ERROR 22007: Incorrect integer value: 'abc' for column 'f2' at row 1 > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t5(a int primary key); > +INSERT INTO t5 VALUES(1),(2),(3); > +START TRANSACTION ; > +PREPARE st5 FROM 'SELECT * FROM t5'; > +EXECUTE st5; > +a > +1 > +2 > +3 > +update t5 set a=a+100; > +EXECUTE st5; > +a > +101 > +102 > +103 > +update t5 set a=a+100; > DROP TABLE t1; > DROP TABLE t2; > DROP TABLE t3; > DROP TABLE t4; > +DROP TABLE t5; diff --git a/mysql-test/suite/galera/t/galera_prepared_statement.test > b/mysql-test/suite/galera/t/galera_prepared_statement.test > index 3bee097..debc00e 100644 > --- a/mysql-test/suite/galera/t/galera_prepared_statement.test > +++ b/mysql-test/suite/galera/t/galera_prepared_statement.test > @@ -38,8 +38,33 @@ ALTER TABLE t1 DROP COLUMN f1; > --error ER_TRUNCATED_WRONG_VALUE_FOR_FIELD > EXECUTE st1; > > +# MDEV-10812 > +# On Closing Prepare Stamemnt, When wsrep_conflict_state is ABORTED > +# It causes wrong response to be sent on Clients. > + > +# First Create a Deadlock > --connection node_1 > +SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; > +CREATE TABLE t5(a int primary key); > +INSERT INTO t5 VALUES(1),(2),(3); > +START TRANSACTION ; > +PREPARE st5 FROM 'SELECT * FROM t5'; > +EXECUTE st5; > +update t5 set a=a+100; > +EXECUTE st5; > + > +--sleep 2 > +--connection node_2 > +update t5 set a=a+100; > + > +--sleep 2 > +--connection node_1 > +# here we have deadlock > +--disconnect node_1 > + > +--connection node_2 > DROP TABLE t1; > DROP TABLE t2; > DROP TABLE t3; > DROP TABLE t4; > +DROP TABLE t5; > The test always hits command != COM_QUIT assert if I revert : + if (thd->wsrep_conflict_state == ABORTED && + command != COM_STMT_CLOSE && command != COM_QUIT) So, I modified your test a bit (simplified) to not use PS. --echo # --echo # MDEV-10812: On closing prepare stamemnt, when wsrep_conflict_state --echo # is ABORTED, it causes wrong response to be sent to the client --echo # # First create a deadlock --connect node_1a, 127.0.0.1, root, , test, $NODE_MYPORT_1 SET TRANSACTION ISOLATION LEVEL SERIALIZABLE; CREATE TABLE t1(a INT PRIMARY KEY); INSERT INTO t1 VALUES(1),(2),(3); START TRANSACTION ; UPDATE t1 SET a=a+100; --sleep 2 --connection node_2 UPDATE t1 SET a=a+100; --sleep 2 --connection node_1a # here we get deadlock error --disconnect node_1a --connection node_2 DROP TABLE t1; I think it should be ok. > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 033e88a..d45b196 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1265,7 +1265,9 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > { > wsrep_client_rollback(thd); > } > - if (thd->wsrep_conflict_state== ABORTED) > + /* we let COM_QUIT and STMT_CLOSE execute even if wsrep aborted */ > We let COM_QUIT and COM_STMT_CLOSE to execute even if wsrep aborted. + if (thd->wsrep_conflict_state == ABORTED && > + command != COM_STMT_CLOSE && command != COM_QUIT) > { > my_error(ER_LOCK_DEADLOCK, MYF(0), "wsrep aborted transaction"); > WSREP_DEBUG("Deadlock error for: %s", thd->query()); > @@ -1918,6 +1920,8 @@ bool dispatch_command(enum enum_server_command > command, THD *thd, > > if (WSREP(thd)) > { > + DBUG_ASSERT((command != COM_QUIT && command != COM_STMT_CLOSE) > + || thd->get_stmt_da()->is_disabled()); > Please add a comment explaining this assert. Thanks, Nirbhay > /* wsrep BF abort in query exec phase */ > mysql_mutex_lock(&thd->LOCK_wsrep_thd); > do_end_of_statement= thd->wsrep_conflict_state != REPLAYING && > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

[Maria-developers] MDEV-11780 Crash with PREPARE + SP out parameter + literal
by Alexander Barkov 24 Jan '17

24 Jan '17

Hello Sergei, Please review a patch for MDEV-11780. Thanks!

2 3

[Maria-developers] Fwd: Mdev-10436 - Non-deterministic vcol does not force rbr
by Sachin Setiya 24 Jan '17

24 Jan '17

Hi Igor, Need a little help in Mdev-10436, I an sanja discussed that we need some method is Item class to see whether it can be determined deterministicly , Is it okay ? Can I change Item class to incorporate this change ? -- Regards Sachin Setiya Software Engineer at MariaDB -- Regards Sachin Setiya Software Engineer at MariaDB

2 1

[Maria-developers] Please review MDEV-11134 Assertion `fixed' failed in Item::const_charset_converter(THD*, CHARSET_INFO*, bool, const char*)
by Alexander Barkov 23 Jan '17

23 Jan '17

Hello Sergei, can you please review a fix for MDEV-11134. I made it the easiest way, just fixed the assert to cover this special case when Item_param::safe_charset_converter() is called from mysql_prepare_create_table(). But perhaps it can be done in different ways: 1. Do call fix_fields() - Fix mysql_prepare_create_table() to call fix_fields. - Fix Item_param::cleanup() not to set fixed to false. or 2. Sync Item_param::fixed with Item_param::basic_const_item() - Fix Item_param::set_xxx() to set both state=XXX_VALUE and fixed=true. - Fix Item_param::cleanup() not to set fixed to false (like in #1) Thanks!

2 6

Re: [Maria-developers] [Commits] 8140454d2b7: MDEV-11254: innodb-use-trim has no effect in 10.2
by Marko Mäkelä 23 Jan '17

23 Jan '17

This is somewhat better, but I think we should still clean up the code a little more. On Mon, Jan 23, 2017 at 1:32 PM, jan <jan.lindstrom(a)mariadb.com> wrote: > fil0fil.cc: Remove unneeded status call and add test is > spare files and punch hole supported by file system where > tablespace is created. Add call to get file system > block size. Used file node is added to IORequest. Added > functions to check is punch hole supported and setting > punch hole. s/is spare files/if sparse files/ s/where/when/ > dberr.h: Add error code if punch hole operation fails. I think that it would be better to explicitly mention the error code here. The name of the code matters more than the file name where it is declared. > os0file.h: Remove unneeded m_block size. Add m_bpage > to know actual size of the block and m_fil_node > to know tablespace file system block size and does it > support punch hole. Mention the class or struct name too. There could be multiple classes declared in the file os0file.h. > os0file.cc: Add function to do punch_hole operation, > get the file system block size and determine > does file system support sparse files (for punch hole). What is the name of the added function? > +--disable_warnings > +let $innodb_compression_algorithm_orig=`SELECT @@innodb_compression_algorithm`; > +let $innodb_file_format_orig = `SELECT @@innodb_file_format`; > +let $innodb_file_per_table_orig = `SELECT @@innodb_file_per_table`; > + > +SET GLOBAL innodb_file_format = `Barracuda`; > +SET GLOBAL innodb_file_per_table = ON; > +--enable_warnings > +--enable_query_log In 10.2, do not add unnecessary references to the deprecated variable innodb_file_format, and do not bother to mess with innodb_file_per_table. The defaults are fine as is. > +let $wait_condition= SELECT variable_value > 0 FROM information_schema.global_status WHERE variable_name = 'innodb_num_pages_page_compressed'; > +--source include/wait_condition.inc > + > +let $wait_condition= SELECT variable_value > 0 FROM information_schema.global_status WHERE variable_name = 'innodb_num_page_compressed_trim_op'; > +--source include/wait_condition.inc > + > +SELECT VARIABLE_VALUE > 0 FROM information_schema.global_status where variable_name = 'INNODB_NUM_PAGES_PAGE_COMPRESSED'; > +SELECT VARIABLE_VALUE > 0 FROM information_schema.global_status where variable_name = 'INNODB_NUM_PAGE_COMPRESSED_TRIM_OP'; Can we wait for a single condition here, using AND in the WHERE condition? I do not think that we need the subsequent SELECT statements. > +/** > +Should we punch hole to deallocate unused portion of the page. > +@param[in] bpage Page control block > +@return true if punch hole should be used, false if not */ > +bool > +buf_page_should_punch_hole( > + const buf_page_t* bpage) > +{ > + return (bpage && bpage->real_size != bpage->size.physical()); > +} > + > +/** > +Calculate the length of trim (punch_hole) operation. > +@param[in] bpage Page control block > +@param[in] write_length Write length > +@return length of the trim or zero. */ > +ulint > +buf_page_get_trim_length( > + const buf_page_t* bpage, > + ulint write_length) > +{ > + return (bpage ? bpage->size.physical() - write_length : 0); > +} Please declare these as methods of buf_page_t, and move the NULL check to the caller. > @@ -912,7 +912,7 @@ buf_dblwr_write_block_to_datafile( > type |= IORequest::DO_NOT_WAKE; > } > > - IORequest request(type); > + IORequest request(type, bpage); > > /* We request frame here to get correct buffer in case of > encryption and/or page compression */ > @@ -924,7 +924,7 @@ buf_dblwr_write_block_to_datafile( > fil_io(request, sync, bpage->id, bpage->size, 0, > bpage->size.physical(), > (void*) frame, > - (void*) bpage, NULL); > + (void*) bpage); > } else { > ut_ad(!bpage->size.is_compressed()); > It feels redundant that we are passing bpage both in the IORequest request and directly to the fil_io() call. One parameter should be enough. And there should be no need for the parameters that are derived directly from bpage. Can we have a separate fil_io() call for the redo log, and a separate one for buffer pool blocks? > +/** > +Get should we punch hole to tablespace. > +@param[in] node File node > +@return true, if punch hole should be tried, false if not. */ > +bool > +fil_node_should_punch_hole( > + const fil_node_t* node) > +{ > + return (node && node->space && node->space->punch_hole); > +} Who would call this with node=NULL? And when would node->space ever be NULL for a valid fil_node_t? > +/** > +Set punch hole to tablespace to given value. > +@param[in] node File node > +@param[in] val value to be set. */ > +void > +fil_space_set_punch_hole( > + fil_node_t* node, > + bool val) > +{ > + if (node && node->space) { > + node->space->punch_hole = val; > + } > +} Also here, I would remove the unnecessary if condition. > diff --git a/storage/innobase/include/os0api.h b/storage/innobase/include/os0api.h > new file mode 100644 > index 00000000000..ea2a113bdec > --- /dev/null > +++ b/storage/innobase/include/os0api.h Instead of creating a new file, can we please move these declarations into an existing file, such as fsp0types.h? The file name prefix is misleading, because there is nothing operating system dependent about the contents. > diff --git a/storage/innobase/include/os0file.h b/storage/innobase/include/os0file.h > index 57ee015dfdd..cd016a46972 100644 > --- a/storage/innobase/include/os0file.h > +++ b/storage/innobase/include/os0file.h > @@ -36,7 +36,8 @@ Created 10/21/1995 Heikki Tuuri > #ifndef os0file_h > #define os0file_h > > -#include "univ.i" > +#include "page0size.h" > +#include "os0api.h" > > #ifndef _WIN32 > #include <dirent.h> > @@ -46,8 +47,10 @@ Created 10/21/1995 Heikki Tuuri > > /** File node of a tablespace or the log data space */ > struct fil_node_t; > +struct fil_space_t; > > extern bool os_has_said_disk_full; > +extern my_bool srv_use_trim; > > /** Number of pending read operations */ > extern ulint os_n_pending_reads; Instead of duplicating declarations, please add #include directives. > @@ -177,6 +180,8 @@ static const ulint OS_FILE_ERROR_MAX = 200; > #define IORequestLogRead IORequest(IORequest::LOG | IORequest::READ) > #define IORequestLogWrite IORequest(IORequest::LOG | IORequest::WRITE) > > + > + > /** > The IO Context that is passed down to the low level IO code */ > class IORequest { Do not add empty lines. > + /** @return true if punch hole should be used */ > + bool punch_hole() const > + MY_ATTRIBUTE((warn_unused_result)) > + { > + return((m_type & PUNCH_HOLE) == PUNCH_HOLE); > + } > + Comparing against 0 should be more efficient. > - /** @return the block size to use for IO */ > - ulint block_size() const > - MY_ATTRIBUTE((warn_unused_result)) > + /** Set the pointer to file node for IO > + @param[in] node File node */ > + void set_fil_node(fil_node_t* node) Consider removing this method, and setting this in the constructor. Why fil_node_t and not fil_space_t? > + /** @return true if punch hole is supported */ > + static bool is_punch_hole_supported() > + { > + > + /* In this debugging mode, we act as if punch hole is supported, > + and then skip any calls to actually punch a hole here. > + In this way, Transparent Page Compression is still being tested. */ > + DBUG_EXECUTE_IF("ignore_punch_hole", > + return(true); > + ); Please add a test that uses SET DEBUG_DBUG='+d,ignore_punch_hole'. > +#if defined(HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE) || defined(_WIN32) > + return(true); > +#else > + return(false); > +#endif /* HAVE_FALLOC_PUNCH_HOLE_AND_KEEP_SIZE || _WIN32 */ > + } This looks wrong design to me. IORequest::is_punch_hole_supported() should depend on the fil_node_t and the actual file system capabilities. > + void space_no_punch_hole(void) { > + fil_space_set_punch_hole(m_fil_node, false); > + } I would prefer more straight code, such as m_fil_node->punch_hole = false; or m_fil_node->clear_punch_hole(); > +/** Check if the file system supports sparse files. > + > +Warning: On POSIX systems we try and punch a hole from offset 0 to > +the system configured page size. This should only be called on an empty > +file. > + > +Note: On Windows we use the name and on Unices we use the file handle. > + > +@param[in] name File name > +@param[in] fh File handle for the file - if opened > +@return true if the file system supports sparse files */ > +bool > +os_is_sparse_file_supported( > + const char* path, > + os_file_t fh) > + MY_ATTRIBUTE((warn_unused_result)); Is it possible to use the file handle on Windows? Or can we define two different functions, one for Windows and another for POSIX? > +/** Free storage space associated with a section of the file. > +@param[in] fh Open file handle > +@param[in] off Starting offset (SEEK_SET) > +@param[in] len Size of the hole > +@return DB_SUCCESS or error code */ > +dberr_t > +os_file_punch_hole( > + IORequest& type, > + os_file_t fh, > + os_offset_t off, > + os_offset_t len) > + MY_ATTRIBUTE((warn_unused_result)); > + > +/** Free storage space associated with a section of the file. > +@param[in] fh Open file handle > +@param[in] off Starting offset (SEEK_SET) > +@param[in] len Size of the hole > +@return DB_SUCCESS or error code */ > +dberr_t > +os_file_punch_hole( > + os_file_t fh, > + os_offset_t off, > + os_offset_t len) > + MY_ATTRIBUTE((warn_unused_result)); The parameter 'type' of the first function is not defined. Why do we need two very similar-looking functions? Could one of them be defined inline as a wrapper of the other? > @@ -156,9 +157,6 @@ class page_size_t { > > private: > > - /* Disable implicit copying. */ > - void operator=(const page_size_t&); > - > /* For non compressed tablespaces, physical page size is equal to > the logical page size and the data is stored in buf_page_t::frame > (and is also always equal to univ_page_size (--innodb-page-size=)). Also reimplement page_size_t::copy_from() as { *this = src; } > @@ -737,6 +740,19 @@ os_file_handle_error_no_exit( > const char* operation, > bool silent); > > +/** Punch a hole in the file if it was a write > +@param[in] type IO context > +@param[in] fh Open file handle > +@param[in] len Compressed buffer length for write > +@return DB_SUCCESS or error code */ > +static > +dberr_t > +os_file_io_complete( > + IORequest& type, > + os_file_t fh, > + ulint offset, > + ulint len); Can this be made a method of IORequest? > + if (err != 0) { > + ib::warn() << "fstat() failed on file " << name > + << " error "<< errno << " : " << strerror(errno); > + os_file_handle_error_no_exit(name, "fstat()", FALSE); Remove the ib::warn(). One error report should be enough, just like we would do it on Windows. > + ib::warn() > + << "fallocate(" << fh > + <<", FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE, " > + << off << ", " << len << ") returned errno: " > + << errno; Do we need this detailed reporting? What use is the numeric value of the file handle to the user? A file name would be much more useful. And ideally, this error should be pushed to the client. > + /* Deallocate unused blocks from file system. > + This is newer done to page 0 or to log files.*/ s/newer/never/. However, during .ibd file creation, we do punch a hole to page 0. > + if (offset > 0 > + && !type.is_log() > + && type.is_write() > + && type.punch_hole()) { > + *err = os_file_io_complete( > + const_cast<IORequest&>(type), > + file, > + static_cast<ulint>(offset), > + n); Remove the const_cast, and properly change all IORequest parameters to non-const. > @@ -4680,7 +4913,7 @@ os_file_pwrite( > (void) my_atomic_addlint(&os_n_pending_writes, 1); > MONITOR_ATOMIC_INC(MONITOR_OS_PENDING_WRITES); > > - ssize_t n_bytes = os_file_io(type, file, const_cast<void*>(buf), > + ssize_t n_bytes = os_file_io(type, file, (void*)buf, > n, offset, err); Avoid C-style casts at any cost. Can we split the os_file_io() into os_file_read() and os_file_write() somehow? How much common code is there actually between the two? Also, could that function be made a method of IORequest? > @@ -5195,6 +5429,31 @@ os_file_read_no_error_handling_func( > +/** NOTE! Use the corresponding macro os_file_write(), not directly > +Requests a synchronous write operation. > +@param[in] type IO flags > +@param[in] file handle to an open file > +@param[out] buf buffer from which to write > +@param[in] offset file offset from the start where to read > +@param[in] n number of bytes to read, starting from offset > +@return DB_SUCCESS if request was successful, false if fail */ > +dberr_t > +os_file_write_func( > + IORequest& type, > + const char* name, > + os_file_t file, > + const void* buf, > + os_offset_t offset, > + ulint n) Can this be made a method of IORequest? > diff --git a/storage/innobase/srv/srv0srv.cc b/storage/innobase/srv/srv0srv.cc > index 5d478e4529f..4b862cc6267 100644 > --- a/storage/innobase/srv/srv0srv.cc > +++ b/storage/innobase/srv/srv0srv.cc > @@ -184,7 +184,7 @@ my_bool srv_use_native_aio = TRUE; > my_bool srv_numa_interleave = FALSE; > /* If this flag is TRUE, then we will use fallocate(PUCH_HOLE) > to the pages */ > -UNIV_INTERN my_bool srv_use_trim = FALSE; > +UNIV_INTERN my_bool srv_use_trim = TRUE; > /* If this flag is TRUE, then we disable doublewrite buffer */ > UNIV_INTERN my_bool srv_use_atomic_writes = FALSE; > /* If this flag IS TRUE, then we use this algorithm for page compressing the pages */ Omit the redundant initialization and let the variable live in the BSS segment. Global parameters will be initialized via the table in ha_innodb.cc. -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

Re: [Maria-developers] [Commits] 98b2a9c: MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings
by Nirbhay Choubey 20 Jan '17

20 Jan '17

Hi Sachin, The overall patch looks ok. I, however, have a few minor comments inline. On Thu, Jan 19, 2017 at 1:21 AM, SachinSetiya <sachin.setiya(a)mariadb.com> wrote: > revision-id: 98b2a9c967a5eaa1f99bb3ef229ff2af62018ffe > (mariadb-10.0.28-34-g98b2a9c) > parent(s): 9bf92706d19761722b46d66a671734466cb6e98e > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2017-01-19 11:50:59 +0530 > message: > > MDEV-4774 Strangeness with max_binlog_stmt_cache_size Settings > > Problem:- When setting max_binlog_stmt_cache_size=18446744073709547520 > from either command line or .cnf file, server fails to start. > > Solution:- Added one more function eval_num_suffix_ull , which uses > strtoull to get unsigned ulonglong from string. And getopt_ull calles this > Typo s/calles/calls/ > function instead of eval_num_suffix. Also changed previous eval_num_suffix > to > eval_num_suffix_ll to remain consistent. > > --- > .../r/binlog_max_binlog_stmt_cache_size.result | 14 +++++ > .../binlog/t/binlog_max_binlog_stmt_cache_size.opt | 1 + > .../t/binlog_max_binlog_stmt_cache_size.test | 11 ++++ > mysys/my_getopt.c | 66 > ++++++++++++++++++---- > 4 files changed, 81 insertions(+), 11 deletions(-) > > diff --git a/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > new file mode 100644 > index 0000000..6fbec90 > --- /dev/null > +++ b/mysql-test/suite/binlog/r/binlog_max_binlog_stmt_cache_size.result > @@ -0,0 +1,14 @@ > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709547520 > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +Warnings: > +Warning 1292 Truncated incorrect max_binlog_stmt_cache_size > value: '18446744073709547519' > +select @@max_binlog_stmt_cache_size; > +@@max_binlog_stmt_cache_size > +18446744073709543424 > +set global max_binlog_stmt_cache_size= 18446744073709547520; > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > new file mode 100644 > index 0000000..c52ef14 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.opt > @@ -0,0 +1 @@ > +--max_binlog_stmt_cache_size=18446744073709547520 > diff --git a/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > new file mode 100644 > index 0000000..bc30b48 > --- /dev/null > +++ b/mysql-test/suite/binlog/t/binlog_max_binlog_stmt_cache_size.test > @@ -0,0 +1,11 @@ > +source include/have_log_bin.inc; > +select @@max_binlog_stmt_cache_size; > + > +--let $cache_size=`select @@max_binlog_stmt_cache_size;` > + > +set global max_binlog_stmt_cache_size= 18446744073709547520; > +select @@max_binlog_stmt_cache_size; > + > +set global max_binlog_stmt_cache_size= 18446744073709547519; > +select @@max_binlog_stmt_cache_size; > I would also add tests for ULLONG_MAX and ULLONG_MAX +/- 1. +--eval set global max_binlog_stmt_cache_size= $cache_size > diff --git a/mysys/my_getopt.c b/mysys/my_getopt.c > index 2a45571..0934a50 100644 > --- a/mysys/my_getopt.c > +++ b/mysys/my_getopt.c > @@ -895,11 +895,32 @@ my_bool getopt_compare_strings(register const char > *s, register const char *t, > /* > function: eval_num_suffix > > + Transforms suffix like k/m/g to there real value. > Typo : s/there/their/ > +*/ > +static inline long eval_num_suffix(char *suffix, int *error) > +{ > + long num= 1; > + if (*suffix == 'k' || *suffix == 'K') > + num*= 1024L; > + else if (*suffix == 'm' || *suffix == 'M') > + num*= 1024L * 1024L; > + else if (*suffix == 'g' || *suffix == 'G') > + num*= 1024L * 1024L * 1024L; > + else if (*suffix) > + { > + *error= 1; > + return 0; > + } > + return num; > +} > Please add a new line here (as a separator). > +/* > + function: eval_num_suffix_ll > + > Transforms a number with a suffix to real number. Suffix can > be k|K for kilo, m|M for mega or g|G for giga. > */ > > -static longlong eval_num_suffix(char *argument, int *error, char > *option_name) > +static longlong eval_num_suffix_ll(char *argument, int *error, char > *option_name) > { > char *endchar; > longlong num; > DBUG_ENTER("eval_num_suffix"); You should also update the function name. > @@ -916,23 +937,46 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > *error= 1; > DBUG_RETURN(0); > } > - if (*endchar == 'k' || *endchar == 'K') > - num*= 1024L; > - else if (*endchar == 'm' || *endchar == 'M') > - num*= 1024L * 1024L; > - else if (*endchar == 'g' || *endchar == 'G') > - num*= 1024L * 1024L * 1024L; > - else if (*endchar) > - { > + num*= eval_num_suffix(endchar, error); > + if (*error) > fprintf(stderr, > "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > *endchar, option_name, argument); > + DBUG_RETURN(num); > +} > + > +/* > + function: eval_num_suffix_ull > + > + Transforms a number with a suffix to positive Integer. Suffix can > + be k|K for kilo, m|M for mega or g|G for giga. > +*/ > + > +static ulonglong eval_num_suffix_ull(char *argument, int *error, char > *option_name) > The above line >80 chars, please break it into 2 lines. > +{ > + char *endchar; > + ulonglong num; > + DBUG_ENTER("eval_num_suffix"); > Update the function name. > + > + *error= 0; > + errno= 0; > + num= strtoull(argument, &endchar, 10); > + if (errno == ERANGE) > + { > + my_getopt_error_reporter(ERROR_LEVEL, > + "Incorrect integer value: '%s'", argument); > *error= 1; > DBUG_RETURN(0); > } > + num*= eval_num_suffix(endchar, error); > + if (*error) > + fprintf(stderr, > + "Unknown suffix '%c' used for variable '%s' (value '%s')\n", > + *endchar, option_name, argument); > DBUG_RETURN(num); > } > > + > /* > function: getopt_ll > > @@ -946,7 +990,7 @@ static longlong eval_num_suffix(char *argument, int > *error, char *option_name) > > static longlong getopt_ll(char *arg, const struct my_option *optp, int > *err) > { > - longlong num=eval_num_suffix(arg, err, (char*) optp->name); > + longlong num=eval_num_suffix_ll(arg, err, (char*) optp->name); > return getopt_ll_limit_value(num, optp, NULL); > } > > @@ -1023,7 +1067,7 @@ longlong getopt_ll_limit_value(longlong num, const > struct my_option *optp, > > static ulonglong getopt_ull(char *arg, const struct my_option *optp, int > *err) > { > - ulonglong num= eval_num_suffix(arg, err, (char*) optp->name); > + ulonglong num= eval_num_suffix_ull(arg, err, (char*) optp->name); > return getopt_ull_limit_value(num, optp, NULL); > } > > Best, Nirbhay

2 3

[Maria-developers] Contribution in mariadb
by Dulton Ghosh 19 Jan '17

19 Jan '17

Respected Sir/Ma'am, I am Dulton Ghosh, a final year student of IIT Guwahati. I want to contribute in MariaDB in upcoming Google Summer of Code. Please give me some instuctions to do so. Regards, Dulton Ghosh IIT Guwahati

2 1

Re: [Maria-developers] Creating test cases for Spider
by Michael Widenius 18 Jan '17

18 Jan '17

Hi! > I will today update the 10.2 spider tree with the latest 10.2 tree > that includes all gcov patches and then try this myself. I have now merged 10.2-spider tree with the latest 10.2 tree, which includes the new HASH and better support for gcov. I am now reviewing the last part of multi_range_read (all files except ha_partition.cc are reviewed). Hope to have this last part done soon. Regards, Monty

2 1

Re: [Maria-developers] [Commits] 14747e4: MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242)
by Sergey Petrunia 17 Jan '17

17 Jan '17

Hi Varun, On Tue, Jan 17, 2017 at 03:24:39PM +0530, Varun wrote: > revision-id: 14747e4b84f68ee7e10a6e5a24f4b7ec6f3240f9 (mariadb-10.1.20-38-g14747e4) > parent(s): b56f726e42e8bc0427a0c33cfffb7e1c5399ea16 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-17 15:22:22 +0530 > message: > > MDEV-10731: Wrong NULL match results in "Subquery returns more than 1 row" (error code 1242) > > NOT NULL predicate was not added to tables in case of an update query having a subquery. > > --- > mysql-test/r/update_innodb.result | 30 +++++++++++++++++ > mysql-test/t/update_innodb.test | 31 +++++++++++++++++ > sql/sql_select.cc | 70 +++++++++++++++++++++++++-------------- > 3 files changed, 106 insertions(+), 25 deletions(-) > > diff --git a/mysql-test/r/update_innodb.result b/mysql-test/r/update_innodb.result > index 88c86c5..6a02dfd 100644 > --- a/mysql-test/r/update_innodb.result > +++ b/mysql-test/r/update_innodb.result > @@ -29,3 +29,33 @@ CREATE ALGORITHM=UNDEFINED VIEW `v1` AS select `t4`.`c1` AS `c1`,`t4`.`c2` AS `c > UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON vw.c2 = del.c2 SET a.c2 = ( SELECT max(t.c1) FROM t3 t, v1 i WHERE del.c2 = t.c2 AND vw.c3 = i.c3 AND t.c3 = 4 ) WHERE a.c2 IS NULL OR a.c2 < '2011-05-01'; > drop view v1; > drop table t1,t2,t3,t4; > +# > +# MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +# The MDEV number and title are wrong! Please fix. > +CREATE TABLE a ( Please make tables names to follow the t1,t2,t3,... naming convention used in the testcase. > +a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +b_id INT(20) UNSIGNED NULL DEFAULT NULL, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (a_id) > +) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > +CREATE TABLE b ( > +b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > +c_id VARCHAR(255) NULL DEFAULT NULL, > +PRIMARY KEY (b_id), > +INDEX idx_c_id (c_id) > +) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > +INSERT INTO a (b_id, c_id) VALUES (1, NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +select b_id from a; > +b_id > +1 > +UPDATE a SET b_id = (SELECT b.b_id FROM b b WHERE b.c_id = a.c_id); > +select b_id from a; > +b_id > +NULL > +drop table a,b; > diff --git a/mysql-test/t/update_innodb.test b/mysql-test/t/update_innodb.test > index 67c356c..f2b7ade 100644 > --- a/mysql-test/t/update_innodb.test > +++ b/mysql-test/t/update_innodb.test > @@ -37,3 +37,34 @@ UPDATE t1 a JOIN t2 b ON a.c1 = b.c1 JOIN v1 vw ON b.c2 = vw.c1 JOIN t3 del ON v > > drop view v1; > drop table t1,t2,t3,t4; > + > +--echo # > +--echo # MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +--echo # > + > +CREATE TABLE a ( > + a_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + b_id INT(20) UNSIGNED NULL DEFAULT NULL, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (a_id) > + ) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > + > +CREATE TABLE b ( > + b_id INT(20) UNSIGNED NOT NULL AUTO_INCREMENT, > + c_id VARCHAR(255) NULL DEFAULT NULL, > + PRIMARY KEY (b_id), > + INDEX idx_c_id (c_id) > + ) > +COLLATE = 'utf8_general_ci' > +ENGINE = InnoDB; > + > +INSERT INTO a (b_id, c_id) VALUES (1, NULL); > +INSERT INTO b (c_id) VALUES (NULL); > +INSERT INTO b (c_id) VALUES (NULL); > + > +select b_id from a; > +UPDATE a SET b_id = (SELECT b.b_id FROM b b WHERE b.c_id = a.c_id); > +select b_id from a; > +drop table a,b; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 85bfb6b..3543cc2 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -9363,34 +9363,54 @@ static void add_not_null_conds(JOIN *join) > not_null_item is the t1.f1, but it's referred_tab is 0. > */ > if (!referred_tab) > - continue; > - if (!(notnull= new (join->thd->mem_root) > + { > + if (!(notnull= new (join->thd->mem_root) > Item_func_isnotnull(join->thd, item))) > - DBUG_VOID_RETURN; > - /* > - We need to do full fix_fields() call here in order to have correct > - notnull->const_item(). This is needed e.g. by test_quick_select > - when it is called from make_join_select after this function is > - called. > - */ Note this as (LOC1) > - if (notnull->fix_fields(join->thd, &notnull)) > - DBUG_VOID_RETURN; > - DBUG_EXECUTE("where",print_where(notnull, > - referred_tab->table->alias.c_ptr(), > - QT_ORDINARY);); > - if (!tab->first_inner) > - { > - COND *new_cond= referred_tab->join == join ? > - referred_tab->select_cond : > - join->outer_ref_cond; > - add_cond_and_fix(join->thd, &new_cond, notnull); > - if (referred_tab->join == join) > - referred_tab->set_select_cond(new_cond, __LINE__); > - else > - join->outer_ref_cond= new_cond; > + DBUG_VOID_RETURN; > + /* > + We need to do full fix_fields() call here in order to have correct > + notnull->const_item(). This is needed e.g. by test_quick_select > + when it is called from make_join_select after this function is > + called. > + */ > + if (notnull->fix_fields(join->thd, &notnull)) > + DBUG_VOID_RETURN; > + > + if (!tab->first_inner) > + add_cond_and_fix(join->thd, &join->outer_ref_cond, notnull); > + else > + add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > } > else > - add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > + { > + if (!(notnull= new (join->thd->mem_root) > + Item_func_isnotnull(join->thd, item))) > + DBUG_VOID_RETURN; > + /* > + We need to do full fix_fields() call here in order to have correct > + notnull->const_item(). This is needed e.g. by test_quick_select > + when it is called from make_join_select after this function is > + called. > + */ This is LOC2. After the patch, there is a lot of code duplication between LOC1 and LOC2: - creation of Item_func_isnotnull - fix_fields call and its comment - etc Please factor out the common code. The "print_where" call can print "outer_ref_table_cond" when referred_tab=NULL. > + if (notnull->fix_fields(join->thd, &notnull)) > + DBUG_VOID_RETURN; > + DBUG_EXECUTE("where",print_where(notnull, > + referred_tab->table->alias.c_ptr(), > + QT_ORDINARY);); > + if (!tab->first_inner) > + { > + COND *new_cond= referred_tab->join == join ? > + referred_tab->select_cond : > + join->outer_ref_cond; > + add_cond_and_fix(join->thd, &new_cond, notnull); > + if (referred_tab->join == join) > + referred_tab->set_select_cond(new_cond, __LINE__); > + else > + join->outer_ref_cond= new_cond; > + } > + else > + add_cond_and_fix(join->thd, tab->first_inner->on_expr_ref, notnull); > + } > } > } > } BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 43e2a78f2a0: Remove MYSQL_COMPRESSION.
by Jan Lindström 16 Jan '17

16 Jan '17

Hi, On Mon, Jan 16, 2017 at 2:10 PM, <marko.makela(a)mariadb.com> wrote: > > diff --git a/storage/innobase/btr/btr0btr.cc b/storage/innobase/btr/ > btr0btr.cc > index d1d9dfe64fe..1fb5cb06949 100644 > --- a/storage/innobase/btr/btr0btr.cc > +++ b/storage/innobase/btr/btr0btr.cc > @@ -215,10 +215,6 @@ btr_root_get( > buf_block_t* root = btr_root_block_get(index, RW_SX_LATCH, > mtr); > > - if (root && root->page.encrypted == true) { > - root = NULL; > - } > - > return(root ? buf_block_get_frame(root) : NULL); > This is unrelated to MySQL encryption, it is related to MariaDB encryption, not sure if btr_root_block_get would always return NULL when page is encrypted, upper code is peppered with assertions, thus this might not be safe. > > - if (ptype == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED) { > - header_len += FIL_PAGE_COMPRESSION_METHOD_SIZE; > - } > - > - /* Do not try to uncompressed pages that are not compressed */ > - if (ptype != FIL_PAGE_PAGE_COMPRESSED && > - ptype != FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED && > - ptype != FIL_PAGE_COMPRESSED) { > + switch (ptype) { > + case FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED: > + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE > + + FIL_PAGE_COMPRESSION_METHOD_SIZE; > + break; > + case FIL_PAGE_PAGE_COMPRESSED: > + header_len = FIL_PAGE_DATA + FIL_PAGE_COMPRESSED_SIZE; > + break; > + default: > + /* The page is not in our format. */ > return; > This is also unrelated change but more like cosmetic, so ok. > } > > > diff --git a/storage/innobase/include/buf0buf.ic > b/storage/innobase/include/buf0buf.ic > index f2b1b151598..5e75c446bbd 100644 > --- a/storage/innobase/include/buf0buf.ic > +++ b/storage/innobase/include/buf0buf.ic > @@ -734,9 +734,6 @@ buf_block_get_frame( > case BUF_BLOCK_ZIP_PAGE: > case BUF_BLOCK_ZIP_DIRTY: > case BUF_BLOCK_NOT_USED: > - if (block->page.encrypted) { > - goto ok; > - } > Hmm, this is unrelated but could be actually correct. > ut_error; > break; > case BUF_BLOCK_FILE_PAGE: > > > @@ -145,52 +137,22 @@ fil_page_type_validate( > page_type == FIL_PAGE_TYPE_BLOB || > page_type == FIL_PAGE_TYPE_ZBLOB || > page_type == FIL_PAGE_TYPE_ZBLOB2 || > - page_type == FIL_PAGE_COMPRESSED || > - page_type == FIL_PAGE_TYPE_UNKNOWN || > - page_type == FIL_PAGE_ENCRYPTED || > - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || > - page_type == FIL_PAGE_ENCRYPTED_RTREE))) { > - > - uint key_version = mach_read_from_4(page + > FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION); > - bool page_compressed = (page_type == > FIL_PAGE_PAGE_COMPRESSED); > - bool page_compressed_encrypted = (page_type == > FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED); > + page_type == FIL_PAGE_TYPE_UNKNOWN))) { > Why you change this code, it is not MySQL compression code ? > + > ulint space = mach_read_from_4(page + > FIL_PAGE_ARCH_LOG_NO_OR_SPACE_ID); > ulint offset = mach_read_from_4(page + FIL_PAGE_OFFSET); > - ib_uint64_t lsn = mach_read_from_8(page + FIL_PAGE_LSN); > - ulint compressed_len = mach_read_from_2(page + > FIL_PAGE_DATA); > fil_system_enter(); > fil_space_t* rspace = fil_space_get_by_id(space); > fil_system_exit(); > > /* Dump out the page info */ > - fprintf(stderr, "InnoDB: Space %lu offset %lu name %s > page_type %lu page_type_name %s\n" > - "InnoDB: key_version %u page_compressed %d > page_compressed_encrypted %d lsn %llu compressed_len %lu\n", > - space, offset, rspace->name, page_type, > fil_get_page_type_name(page_type), > - key_version, page_compressed, > page_compressed_encrypted, (ulonglong)lsn, compressed_len); > - fflush(stderr); > - > - ut_ad(page_type == FIL_PAGE_PAGE_COMPRESSED || > - page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED || > - page_type == FIL_PAGE_INDEX || > - page_type == FIL_PAGE_RTREE || > - page_type == FIL_PAGE_UNDO_LOG || > - page_type == FIL_PAGE_INODE || > - page_type == FIL_PAGE_IBUF_FREE_LIST || > - page_type == FIL_PAGE_TYPE_ALLOCATED || > - page_type == FIL_PAGE_IBUF_BITMAP || > - page_type == FIL_PAGE_TYPE_SYS || > - page_type == FIL_PAGE_TYPE_TRX_SYS || > - page_type == FIL_PAGE_TYPE_FSP_HDR || > - page_type == FIL_PAGE_TYPE_XDES || > - page_type == FIL_PAGE_TYPE_BLOB || > - page_type == FIL_PAGE_TYPE_ZBLOB || > - page_type == FIL_PAGE_TYPE_ZBLOB2 || > - page_type == FIL_PAGE_COMPRESSED || > - page_type == FIL_PAGE_TYPE_UNKNOWN || > - page_type == FIL_PAGE_ENCRYPTED || > - page_type == FIL_PAGE_COMPRESSED_AND_ENCRYPTED || > - page_type == FIL_PAGE_ENCRYPTED_RTREE); > - > + ib::fatal() << "Page " << space << ":" << offset > + << " name " << (rspace ? rspace->name : "???") > + << " page_type " << page_type > + << " key_version " > + << mach_read_from_4(page + > FIL_PAGE_FILE_FLUSH_LSN_OR_KEY_VERSION) > + << " lsn " << mach_read_from_8(page + FIL_PAGE_LSN) > + << " compressed_len " << mach_read_from_2(page + > FIL_PAGE_DATA); > return false; > } > > > ok to push, rest of the changes. R: Jan

2 1

Re: [Maria-developers] de9ef13: MDEV-10956 Strict Password Validation Breaks Replication.
by Sergei Golubchik 16 Jan '17

16 Jan '17

Hi, Alexey! On Jan 16, Alexey Botchkov wrote: > revision-id: de9ef1390655abfb2b46a158148a9e27410298a2 (mariadb-10.1.20-44-gde9ef13) > parent(s): ab1e6fefd869242d962cb91a006f37bb9ad534a7 > committer: Alexey Botchkov > timestamp: 2017-01-16 02:20:15 +0400 > message: > > MDEV-10956 Strict Password Validation Breaks Replication. > > Password validation switched off completely for slave threads. I'd rather disable only strict_password_validation check in replication threads. Because it, indeed, makes no sense there. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] eb8d31d: MDEV-10232: Scalar result of subquery changes after adding an outer select stmt
by Sergey Petrunia 16 Jan '17

16 Jan '17

Hi Varun, Please make the fix for MDEV-10232 to be one changeset. Ok to push that changeset. On Sun, Jan 15, 2017 at 07:44:35PM +0530, Varun wrote: > revision-id: eb8d31d96f32da53d34d4c992fba061fc8732078 (mariadb-10.1.20-38-geb8d31d) > parent(s): 905aac9c78068224839db551a84ed080167d1657 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-15 19:40:46 +0530 > message: > > MDEV-10232: Scalar result of subquery changes after adding an outer select stmt > > postreview > > --- > sql/sql_select.cc | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 13eda46..85bfb6b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1614,7 +1614,8 @@ JOIN::optimize_inner() > <fields> to ORDER BY <fields>. There are three exceptions: > - if skip_sort_order is set (see above), then we can simply skip > GROUP BY; > - - if we are in a subquery, we don't have to maintain order > + - if we are in a subquery, we don't have to maintain order unless there > + is a limit clause in the subquery. > - we can only rewrite ORDER BY if the ORDER BY fields are 'compatible' > with the GROUP BY ones, i.e. either one is a prefix of another. > We only check if the ORDER BY is a prefix of GROUP BY. In this case > @@ -1626,7 +1627,7 @@ JOIN::optimize_inner() > if (!order || test_if_subpart(group_list, order)) > { > if (skip_sort_order || > - select_lex->master_unit()->item && !select_limit) // This is a subquery > + (select_lex->master_unit()->item && select_limit == HA_POS_ERROR)) // This is a subquery > order= NULL; > else > order= group_list; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 905aac9: MDEV-10232: Scalar result of subquery changes after adding an outer select stmt
by Sergey Petrunia 14 Jan '17

14 Jan '17

Hi Varun, On Fri, Jan 13, 2017 at 08:22:49PM +0530, Varun wrote: > revision-id: 905aac9c78068224839db551a84ed080167d1657 (mariadb-10.1.20-37-g905aac9) > parent(s): ebb8c9fb26f86cff8c0d81bd2415f415cef952bb > author: Varun Gupta > committer: Varun Gupta > timestamp: 2017-01-13 20:17:33 +0530 > message: > > MDEV-10232: Scalar result of subquery changes after adding an outer select stmt > > In a subquery, we don't have to maintain order > Added a fix such that order is considered when there is a limit clause. > > --- > mysql-test/r/subselect4.result | 12 ++++++++++++ > mysql-test/t/subselect4.test | 10 ++++++++++ > sql/sql_select.cc | 2 +- > 3 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/subselect4.result b/mysql-test/r/subselect4.result > index 87645d1..7764783 100644 > --- a/mysql-test/r/subselect4.result > +++ b/mysql-test/r/subselect4.result > @@ -2401,5 +2401,17 @@ SELECT x FROM t1 WHERE id > (SELECT MAX(id) - 1000 FROM t1) ORDER BY x LIMIT 1; > x > 0 > drop table t1; > +# > +# MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +# > +create table t1(c1 int, c2 int, primary key(c2)); > +insert into t1 values(2,1),(1,2); > +select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; > +x > +1 > +(select c1 from t1 group by c1,c2 order by c1 limit 1); > +c1 > +1 > +drop table t1; > SET optimizer_switch= @@global.optimizer_switch; > set @@tmp_table_size= @@global.tmp_table_size; > diff --git a/mysql-test/t/subselect4.test b/mysql-test/t/subselect4.test > index 4eb9701..787c579 100644 > --- a/mysql-test/t/subselect4.test > +++ b/mysql-test/t/subselect4.test > @@ -1956,5 +1956,15 @@ SELECT x FROM t1 WHERE id > (SELECT MAX(id) - 1000 FROM t1) ORDER BY x LIMIT 1; > > drop table t1; > > +--echo # > +--echo # MDEV-10232 Scalar result of subquery changes after adding an outer select stmt > +--echo # > + > +create table t1(c1 int, c2 int, primary key(c2)); > +insert into t1 values(2,1),(1,2); > +select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; > +(select c1 from t1 group by c1,c2 order by c1 limit 1); > +drop table t1; > + > SET optimizer_switch= @@global.optimizer_switch; > set @@tmp_table_size= @@global.tmp_table_size; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 3f06ec8..13eda46 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -1626,7 +1626,7 @@ JOIN::optimize_inner() Above this change we still have a comment with this statement: >> - if we are in a subquery, we don't have to maintain order Please adjust it accordingly. > if (!order || test_if_subpart(group_list, order)) > { > if (skip_sort_order || > - select_lex->master_unit()->item) // This is a subquery > + select_lex->master_unit()->item && !select_limit) // This is a subquery > order= NULL; > else > order= group_list; So I'm trying with the patch: MariaDB [test]> explain select (select c1 from t1 group by c1,c2 order by c1 limit 1) as x; +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | 1 | PRIMARY | NULL | NULL | NULL | NULL | NULL | NULL | NULL | No tables used | | 2 | SUBQUERY | t1 | ALL | NULL | NULL | NULL | NULL | 2 | Using filesort | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ 2 rows in set (20.08 sec) Good. What if there is no LIMIT clause? MariaDB [test]> explain select (select c1 from t1 group by c1,c2 order by c1 ) as x; +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ | 1 | PRIMARY | NULL | NULL | NULL | NULL | NULL | NULL | NULL | No tables used | | 2 | SUBQUERY | t1 | ALL | NULL | NULL | NULL | NULL | 2 | Using filesort | +------+-------------+-------+------+---------------+------+---------+------+------+----------------+ 2 rows in set (6.06 sec) Bad. "Using filesort" should have been removed. The reason it fails is that "!select_limit" is not the right way to check the presense of LIMIT clause. Without limit, I have: (gdb) p/x select_limit $15 = 0xffffffffffffffff (the name of the constant to use is "HA_POS_ERROR") Another issue: I get this warning with the new code: || /home/psergey/dev-git/10.1/sql/sql_select.cc: In member function 'int JOIN::optimize_inner()': sql_select.cc|1629 col 45| warning: suggest parentheses around '&&' within '||' [-Wparentheses] || select_lex->master_unit()->item && !select_limit) // This is a subquery || ^ BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] a2a42a9: MDEV-11623: MariaDB 10.1 fails to start datadir created with MariaDB 10.0/MySQL 5.6 using innodb-page-size!=16K
by Marko Mäkelä 13 Jan '17

13 Jan '17

Sorry for the delay. This patch looks great to me, except for one thing: I think that we should write the corrected flags back to the data file. Given that 10.1 is already in GA status, we should consider our options carefully. My preference would be (1) below: (1) If we adjust 10.1.21 so that it writes back corrected flags for both old and new data files, then older 10.1.x would be unable to open the files (if compression, DATA_DIRECTORY or non-default innodb_page_size was used). (2) If we adjust 10.1.21 so that it writes correct flags for new data files (like the current patch does), then older 10.1.x would not be able to read those created-with-newer-version data files. (3) If we adjust 10.2 only, so that it writes the correct flags for new files and also adjusts the flags in old files, then we would seem to have a clear upgrade path. Whatever we choose, I would like to remove the flag-adjustment code from 10.3 or 10.4. This would mean that an upgrade (or import) directly from 10.1 is only possible if the problematic features (DATA_DIRECTORY, compression, non-standard innodb_page_size) were not used. Otherwise the upgrade would have to be done through 10.2. On Thu, Dec 29, 2016 at 10:41 PM, Jan Lindström <jan.lindstrom(a)mariadb.com> wrote: > revision-id: a2a42a9181729b661ba308f2d0a75d8b547ed09d (mariadb-10.1.20-15-ga2a42a9) > parent(s): 23cc1be270c7304963643947d8e5ab88f4e312ee > author: Jan Lindström > committer: Jan Lindström > timestamp: 2016-12-29 22:40:30 +0200 > message: > > MDEV-11623: MariaDB 10.1 fails to start datadir created with MariaDB 10.0/MySQL 5.6 using innodb-page-size!=16K > > Problem was that in MariaDB 10.1 page size flag is on different > position (13) on tablespace flags compared to MySQL 5.6 and > MariaDB 10.0 or older position (6). DATA_DIR flag position difference > was already handled. > > /** Tablespace flags position and name in MySQL 5.6/MariaDB 10.0 or older > and MariaDB 10.1.20 or older MariaDB 10.1 and in MariaDB 10.1.21 > or newer. > MySQL 5.6 MariaDB 10.1.x MariaDB 10.1.21 > ==================================================================== > Below flags in same offset > ==================================================================== > 0: POST_ANTELOPE 0:POST_ANTELOPE 0: POST_ANTELOPE > 1: ZIP_SSIZE 1:ZIP_SSIZE 1: ZIP_SSIZE > 5: ATOMIC_BLOBS 5:ATOMIC_BLOBS 5: ATOMIC_BLOBS It could be clearer to explicitly identify multi-bit fields, e.g., 1..4 for ZIP_SSIZE. > ===================================================================== > Below note the difference in order > ===================================================================== > 6: PAGE_SSIZE 6:PAGE_COMPRESSION 6: PAGE_SSIZE > 10: DATA_DIR 7:PAGE_COMPRESSION_LEVEL 10: DATA_DIR > 11: UNUSED 11:ATOMIC_WRITES > ===================================================================== > Note that below flags have been moved > ===================================================================== > 13:PAGE_SSIZE 11: RESERVED (5.7 SHARED) > 17:DATA_DIR 12: RESERVED (5.7 TEMPORARY) > 18:UNUSED 13: RESERVED (5.7 ENCRYPTION) > 14: PAGE_COMPRESSION > 15: PAGE_COMPRESSION_LEVEL > 19: ATOMIC_WRITES > 21: UNUSED s/have been moved/were in incorrect position in MariaDB 10.1.x/ It could be good to explain the impact of this. That is, what would happen if we upgrade from MariaDB 10.0 to the buggy MariaDB 10.1, or if we upgrade from the buggy MariaDB 10.1 to 10.1.21 where this has been fixed. As far as I can tell, with respect to the DATA_DIR flag we should be mostly OK, because that flag should only play a role in the table flags when determining where the data file is located, not so much in the tablespace flags inside the data file. (Moving files from MariaDB 10.1 to MySQL 5.7 could be hurt, because the DATA_DIR flag would be interpreted as the 5.7 ENCRYPTION flag.) Next, let us consider PAGE_SSIZE (bits 13..16 in the buggy MariaDB 10.1.x, 6..9 elsewhere). Upgrade from 10.0 or 5.6 to the buggy 10.1.x: We read the bits 13..16. These would always be 0 in the old data files, so we will interpret the data file as if it had innodb_page_size=16k. That is, upgrade or import from 5.6 or 10.0 to the buggy 10.1.x will be refused if innodb_page_size differs from 16k. Is this correct? (This is something that we cannot fix, because we cannot retroactively change old MariaDB 10.1.x versions.) Upgrade from the buggy 10.1.x to the fixed 10.1.x or 10.2: We read the bits 6..9 which were incorrectly used as PAGE_COMPRESSION, PAGE_COMPRESSION_LEVEL. That is, upgrade should work if innodb_page_size=16k (the flags are expected to be 0) and compression is not used (the bits 6..9 were written as 0 by the buggy 10.1.x). To help users who use the buggy 10.1.x with innodb_page_size!=16k or with compression, we can adjust the flags: > fsp0fsp.ic: fsp_flags_get_page_size() : By default read > used page size from original position i.e. after ATOMIC_BLOBS. > But if we detect that buggy MariaDB 10.1 flags are used, then > we read it from FSP_FLAGS_PAGE_SSIZE_MARIADB101. [snip] > Tested also with MySQL 5.7 datadir (after deleting redo logs > and copying mysql/* files from 10.1) but that failed because > SYS_INDEXES got an extra column in 5.7 SYS_INDEXES.MERGE_THRESHOLD. You could theoretically have initialized the database in MySQL 5.6 and then upgraded it to 5.7. All tables that were created in 5.6 should be accessible. But would not have added much value. > After shutdown, started MariaDB 10.1.21 with fix and > verified that all tables created are accessible. Were the flags corrected in the data file afterwards? I think that we should correct the flags. Please add a mysql-test case that demonstrates that the flags are corrected. Something like this: --source include/have_innodb.inc --source include/not_embedded.inc # restart will restore the old values SET GLOBAL innodb_file_per_table=1; SET GLOBAL innodb_file_format=Barracuda; let INNODB_PAGE_SIZE=`select @@innodb_page_size`; let MYSQLD_DATADIR=`select @@datadir`; CREATE TABLE tr(a INT)ENGINE=InnoDB ROW_FORMAT=REDUNDANT; CREATE TABLE tc(a INT)ENGINE=InnoDB ROW_FORMAT=COMPACT; CREATE TABLE td(a INT)ENGINE=InnoDB ROW_FORMAT=DYNAMIC; CREATE TABLE tz(a INT)ENGINE=InnoDB ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=1; # TODO: repeat the same with DATA_DIRECTORY and COMPRESSION --source include/shutdown_mysqld.inc perl; # corrupt the FSP_FLAGS in each file; write the page checksum as 0xdeadbeef # (see log_data_file_size.test or some recent test from MySQL) EOF; --let $restart_parameters = --innodb-read-only; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc perl; # check that the FSP_FLAGS are still corrupted # (they must not be adjusted in read-only mode) EOF; --let $restart_parameters=; --source include/start_mysqld.inc CHECK TABLE tr; CHECK TABLE tc; CHECK TABLE td; CHECK TABLE tz; --source include/shutdown_mysqld.inc perl; # check that the FSP_FLAGS now correspond to $ENV{INNODB_PAGE_SIZE} EOF; --source include/start_mysqld.inc DROP TABLE tr,tc,td,tz; > @@ -3859,21 +3837,11 @@ fil_open_single_table_tablespace( > + /* Validate this single-table-tablespace with SYS_TABLES. */ > + bool flags_correct = dict_compare_flags(def.flags, flags); I think that the function name should be more descriptive, making clear that the first argument contains the table flags and the second the FIL_SPACE_FLAGS: dict_tf_match_space(def.flags, flags) The comment does not really add any value. I would omit it. > diff --git a/storage/innobase/fsp/fsp0fsp.cc b/storage/innobase/fsp/fsp0fsp.cc > index 87aa5f7..7ff5244 100644 > --- a/storage/innobase/fsp/fsp0fsp.cc > +++ b/storage/innobase/fsp/fsp0fsp.cc > @@ -4182,3 +4182,141 @@ fsp_page_is_free_func( > return xdes_mtr_get_bit( > descr, XDES_FREE_BIT, page_no % FSP_EXTENT_SIZE, mtr); > } > + > +/********************************************************************//** > +Verify that dictionary flags modified to tablespace flags > +and actual tablespace flags stored to FSP header match. > +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) > +@param[in] fsp_flags Actual flags stored to FSP header in page 0 > +@return true if flags match, false if not */ > +bool > +fsp_verify_flags( > + ulint dict_flags, > + ulint fsp_flags) > +{ > + ulint dict_unused = FSP_FLAGS_GET_UNUSED(dict_flags); > + ulint dict_antelope = FSP_FLAGS_GET_POST_ANTELOPE(dict_flags); > + ulint dict_zssize = FSP_FLAGS_GET_ZIP_SSIZE(dict_flags); > + ulint dict_ablobs = FSP_FLAGS_HAS_ATOMIC_BLOBS(dict_flags); Add an early return before all these computations if dict_flags==fsp_flags. I think that the name dict_flags is misleading. Both flags are supposed to be tablespace flags, not data dictionary (or table) flags. How about this: s/dict_flags/expected/ s/fsp_flags/actual/ And then define the local variables with expected_ and actual_ prefixes, instead of dict_ and fsp_. That would make the code easier to follow. > + DBUG_EXECUTE_IF("fsp_verify_flags_failure", > + return(false);); Where is the test to exercise this? Do we even need this? I think it is better to inject faults into the data files using Perl code in mysql-test (see my previous suggestion). Other than this, the function looks OK to me. > diff --git a/storage/innobase/include/dict0dict.h b/storage/innobase/include/dict0dict.h > index 42f93b5..479954d 100644 > --- a/storage/innobase/include/dict0dict.h > +++ b/storage/innobase/include/dict0dict.h > @@ -1904,6 +1904,17 @@ dict_table_get_index_on_first_col( > #endif /* !UNIV_HOTBACKUP */ > > > +/** Compare tablespace flags. > +@param[in] flags SYS_TABLES.flags > +@param[in] mod_flags Tablespace flags. > +@return true if flags match, false if not */ > +UNIV_INLINE > +bool > +dict_compare_flags( > + ulint flags, > + ulint mod_flags) > + MY_ATTRIBUTE((warn_unused_result)); > + Can we declare the inline function directly in the .h file, instead of splitting it into the .ic file? I would suggest different names: bool dict_tf_match_space(ulint table_flags, ulint fsp_flags); > @@ -980,12 +982,11 @@ dict_sys_tables_type_to_tf( > PAGE_COMPRESSION_LEVEL, ATOMIC_WRITES are the same. */ > flags |= type & (DICT_TF_MASK_ZIP_SSIZE > | DICT_TF_MASK_ATOMIC_BLOBS > - | DICT_TF_MASK_DATA_DIR > - | DICT_TF_MASK_PAGE_COMPRESSION > - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > - | DICT_TF_MASK_ATOMIC_WRITES > + | DICT_TF_MASK_DATA_DIR > + | DICT_TF_MASK_PAGE_COMPRESSION > + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > + | DICT_TF_MASK_ATOMIC_WRITES); > > - ); > > return(flags); > } This appears to be a white-space-only change. We could avoid it, unless this is actually fixing the indentation. The spaces and TABs got lost in transit, so I cannot tell.. > @@ -1018,8 +1019,8 @@ dict_tf_to_sys_tables_type( > type |= flags & (DICT_TF_MASK_ZIP_SSIZE > | DICT_TF_MASK_ATOMIC_BLOBS > | DICT_TF_MASK_DATA_DIR > - | DICT_TF_MASK_PAGE_COMPRESSION > - | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > + | DICT_TF_MASK_PAGE_COMPRESSION > + | DICT_TF_MASK_PAGE_COMPRESSION_LEVEL > | DICT_TF_MASK_ATOMIC_WRITES); > > return(type); Same here. > DBUG_EXECUTE_IF("dict_tf_verify_flags_failure", > - return(ULINT_UNDEFINED);); > + return(false);); Nice catch. But is there any test to exercise this? If not, I would remove the DBUG_EXECUTE_IF altogether. > ut_a(page_ssize == 0 || page_ssize != 0); /* silence compiler */ > ut_a(compact == 0 || compact == 1); /* silence compiler */ > ut_a(data_dir == 0 || data_dir == 1); /* silence compiler */ > - ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler */ > + ut_a(post_antelope == 0 || post_antelope == 1); /* silence compiler > + */ I think it would be better to declare these variables as __attribute__((unused)) or to remove the variables altogether if they are unused. > + > + if (table_unused || fsp_unused) { > + ib_logf(IB_LOG_LEVEL_ERROR, > + "Table flags has unused %ld" > + " in the data dictionary" > + " but the flags in file has unused %ld\n", > + table_unused, fsp_unused); > + > + return (false); > + } 0x%lx would be more user-friendly here. > + ib_logf(IB_LOG_LEVEL_ERROR, > + "Table flags has page_compression %ld" > + " in the data dictionary" > + " but the flags in file ahas page_compression %ld\n", > page_compression, fsp_page_compression); s/ahas/has/ > +/********************************************************************//** > +Verify that dictionary flags modified to tablespace flags > +and actual tablespace flags stored to FSP header match. > +@param[in] dict_flags dict_tf_to_fsp_flags(dict_table_t::flags) > +@param[in] fsp_flags Actual flags stored to FSP header in page 0 > +@return true if flags match, false if not */ > +bool > +fsp_verify_flags( > + ulint dict_flags, > + ulint fsp_flags) > + MY_ATTRIBUTE((warn_unused_result)); > + This seems to be missing UNIV_INTERN? I think we need it in 10.1 at least for innodb_plugin. > + /** In MariaDB 10.1.20 or older MariaDB 10.1 PAGE_SSIZE > + was stored after ATOMIC_WRITES not after ATOMIC_BLOBS > + as in MySQL 5.6, MariaDB 10.0 or older. New tables > + in MariDB 10.1.21 or newer store PAGE_SSIZE after ATOMIC_BLOBS. > + */ > + ssize = FSP_FLAGS_GET_PAGE_SSIZE(flags); Do not use /** comments before local variables. I think that one of the ‘after’ in the comment should be ‘before’. Also fix the typo MariDB. -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 1

[Maria-developers] Please review MDEV-9522 Split sql_select.cc:can_change_cond_ref_to_const into virtual methods in Type_handler
by Alexander Barkov 13 Jan '17

13 Jan '17

Hello Vicențiu, can you please review a patch for MDEV-9522 ? Thanks!

2 3

[Maria-developers] MDEV-11030 Assertion `precision > 0' failed in decimal_bin_size
by Alexander Barkov 12 Jan '17

12 Jan '17

Hello Sanja, Can you please review a patch for MDEV-11030? Thanks!

1 1

Re: [Maria-developers] [Commits] c32bc1afe82: MDEV-11254: innodb-use-trim has no effect in 10.2
by Marko Mäkelä 11 Jan '17

11 Jan '17

On Tue, Jan 10, 2017 at 11:49 AM, jan <jan.lindstrom(a)mariadb.com> wrote: > revision-id: c32bc1afe820aa90c2e2666a77ce2bb43019df3d (mariadb-10.2.3-43-gc32bc1afe82) > parent(s): 3d46768da2a784ddc9c341d1fb03468525bd38f1 > author: Jan Lindström > committer: Jan Lindström > timestamp: 2017-01-10 11:36:42 +0200 > message: > > MDEV-11254: innodb-use-trim has no effect in 10.2 > > In MySQL 5.7 InnoDB os0file.[h|cc] was refactored making > trim or punch hole operation not work with MariaDB. > > buf0buf.cc: buf_page_is_corrupted() remember that compressed > pages do not contain checksum and FIL_TAIL header. What is a FIL_TAIL header? I suppose it is referring to the 8 bytes FIL_PAGE_DATA_END at the end of the page frame? And which compressed pages? ROW_FORMAT=COMPRESSED pages do contain a page footer. > buf0dblwr.cc: Set up the page size and write size to IORequest > and set up punch_hole() if actual write is not physical size. Actual write to where? Apparently (looking at the code), it is to the user data file, not to the doublewrite buffer. The commit message should mention this important detail. (I think we should not punch holes to the doublewrite buffer area (pages 64..191 in the system tablespace).) > buf0rea.cc, fil0fil.cc, fil0fil.h: Remove unnecessary parameter > write_size from os_file_write, fil_io, os_aio functions. I see that this parameter does not exist in MySQL 5.7. OK to remove. > os0file.h, os0file.cc: Implement new version of os_file_io_complete > to do punch_hole operation and punch hole operation should > be based on file system block size determined using fstat or > DeviceIOControl. Please also mention the InnoDB wrapper function os_file_get_block_size(), which was added. But, where is that function being called? Did you forget to adjust fil_node_open_file()? Please submit a revised patch for review. Some detailed comments follow. > +set autocommit=0; > +call innodb_insert_proc(16000); > +commit; > +set autocommit=1; Do we really have to insert this many records? What would be the minimum test case? Did you test with innodb_page_size=4k or 8k? Consider removing the two SET statements and adding a BEGIN before the CALL instead. That would be more standard SQL. > +--disable_query_log > +--disable_warnings > +EVAL SET GLOBAL innodb_compression_algorithm = $innodb_compression_algorithm_orig; > +EVAL SET GLOBAL innodb_file_per_table = $innodb_file_per_table_orig; > +EVAL SET GLOBAL innodb_file_format = $innodb_file_format_orig; > +--enable_warnings > +--enable_query_log There is no need to assign innodb_file_per_table or innodb_file_format in 10.2. The defaults are just fine. If you keep adding these statements in 10.2, merging to 10.3 will require extra work after 10.3 removes these deprecated parameters (copying WL#7704 from MySQL 8.0.0). > + bool page_encrypted = false; > + bool page_compressed = false; > + ulint page_type = mach_read_from_2(read_buf+FIL_PAGE_TYPE); > + > + page_compressed = (page_type == FIL_PAGE_PAGE_COMPRESSED_ENCRYPTED > + || page_type == FIL_PAGE_PAGE_COMPRESSED); Please remove the initialization of page_compressed to false. Could we fuse page_compressed into page_encrypted to simplify those conditions that currently check for page_compressed&&page_encrypted? > @@ -978,9 +978,13 @@ buf_dblwr_write_block_to_datafile( > ut_a(buf_block_get_state(block) == BUF_BLOCK_FILE_PAGE); > buf_dblwr_check_page_lsn(block->frame); > > + if (bpage->real_size != bpage->size.physical()) { > + request.set_punch_hole(); > + } > + > fil_io(request, > - sync, bpage->id, bpage->size, 0, bpage->size.physical(), > - frame, block, (ulint *)&bpage->write_size); > + sync, bpage->id, bpage->size, 0, bpage->real_size, > + frame, block); > } > } I understand that this is the write to the final location. Sure, we should punch the hole there. > diff --git a/storage/innobase/buf/buf0flu.cc b/storage/innobase/buf/buf0flu.cc > index b82c4db18ad..de534cc6e89 100644 > --- a/storage/innobase/buf/buf0flu.cc > +++ b/storage/innobase/buf/buf0flu.cc > @@ -1093,11 +1093,15 @@ buf_flush_write_block_low( > > ulint type = IORequest::WRITE | IORequest::DO_NOT_WAKE; > > - IORequest request(type); > + IORequest request(type, bpage->size, (ulint *)&bpage->write_size); > + > + if (bpage->real_size != bpage->size.physical()) { > + request.set_punch_hole(); > + } Could we pass bpage as a parameter to the IORequest::IORequest(), and also include the set_punch_hole() call there? I think that buf_page_t is already too big, and we should not bloat it with a 64-bit field for write_size. The reason that I separated buf_page_t from buf_block_t was that for ROW_FORMAT=COMPRESSED, the block descriptors would use less space. Do we even need write_size in buf_page_t? I think it would better belong to buf_block_t. That is because ROW_FORMAT=COMPRESSED is never used together with hole-punching. > @@ -37,6 +37,7 @@ Created 10/21/1995 Heikki Tuuri > #define os0file_h > > #include "univ.i" > +#include "page0size.h" #include "page0size.h" also does #include "univ.i" > > #ifndef _WIN32 > #include <dirent.h> > @@ -554,6 +555,8 @@ class IORequest { > /** Default constructor */ > IORequest() > : > + m_page_size(univ_page_size), > + m_write_size(NULL), > m_block_size(UNIV_SECTOR_SIZE), > m_type(READ), > m_compression() We only have to add a single buf_page_t* const parameter. Maybe we could also remove m_block_size? > + /** > + @param[in] type Request type, can be a value that is > + ORed from the above enum > + @param[in] page_size Page size > + @param[in] write_size Actual write size */ > + IORequest(ulint type, > + const page_size_t& page_size, > + ulint* write_size) > + : > + m_page_size(page_size), > + m_write_size(write_size), > + m_block_size(UNIV_SECTOR_SIZE), > + m_type(static_cast<uint16_t>(type)), > + m_compression() > + { > + if (is_log()) { > + disable_compression(); > + } > + > + if (!is_punch_hole_supported()) { > + clear_punch_hole(); > + } > + } I do not think we need this constructor. > + /** operator = > + @param[in] from Source to copy */ > + IORequest& operator=(const IORequest& from) > + { > + m_page_size.copy_from(from.m_page_size); > + m_write_size = from.m_write_size; > + m_block_size = from.m_block_size; > + m_type = from.m_type; > + m_compression = from.m_compression; > + return (*this); > + } What is wrong with the compiler-generated assignment operator? If it complains that there is no page_size_t::operator=(), we can easily let it be generated by the compiler. What is this assignment needed for? > + /** Copy constructor. > + @param[in] from source. */ > + IORequest(const IORequest& from) > + : m_page_size(from.m_page_size.physical(), > + from.m_page_size.logical(), > + from.m_page_size.is_compressed()), > + m_write_size(from.m_write_size), > + m_block_size(from.m_block_size), > + m_type(from.m_type), > + m_compression(from.m_compression) > + { > + } > + What is this needed for? Why cannot the compiler generate this? As far as I can tell, this is just a bitwise copy. > @@ -688,6 +742,22 @@ class IORequest { > m_block_size = static_cast<uint32_t>(block_size); > } > > + /** Set the write size variable for later use. > + @param[in] write_size Write size variable */ > + void write_size(ulint* write_size) > + { > + m_write_size = write_size; > + } > + > + /** Set the actual write size done in IO > + @param[in] write_size Write size to set */ > + void write_size(ulint write_size) > + { > + if (m_write_size) { > + *m_write_size = write_size; > + } > + } > + The two methods do two completely different things, and at the very least they should have distinctive names. Do we need the first method at all? > +/***********************************************************************//** > +Try to get number of bytes per sector from file system. > +@return file block size */ > +UNIV_INTERN > +ulint > +os_file_get_block_size( > +/*===================*/ > + os_file_t file, /*!< in: handle to a file */ > + const char* name); /*!< in: file name */ Do not use /*****/ or /*====*/ or /*!< comments in new code. This function appears to be unused. > -/** Decompress after a read and punch a hole in the file if it was a write > +/** Punch a hole in the file if it was a write > @param[in] type IO context > @param[in] fh Open file handle > -@param[in,out] buf Buffer to transform > -@param[in,out] scratch Scratch area for read decompression > -@param[in] src_len Length of the buffer before compression > -@param[in] len Compressed buffer length for write and size > - of buf len for read > +@param[in] len Compressed buffer length for write > @return DB_SUCCESS or error code */ > static > dberr_t > os_file_io_complete( > - const IORequest&type, > + IORequest& type, > os_file_t fh, > - byte* buf, > - byte* scratch, > - ulint src_len, > ulint offset, > ulint len); Why is the const qualifier removed? > @@ -832,6 +825,69 @@ os_aio_simulated_handler( > void** m2, > IORequest* type); > > +/***********************************************************************//** > +Try to get number of bytes per sector from file system. > +@return file block size */ > +UNIV_INTERN > +ulint > +os_file_get_block_size( > +/*===================*/ > + os_file_t file, /*!< in: handle to a file */ > + const char* name) /*!< in: file name */ > +{ > + ulint fblock_size = 512; > + > +#if defined(UNIV_LINUX) > + struct stat local_stat; > + int err; > + > + err = fstat((int)file, &local_stat); > + > + if (err != 0) { > + ib::warn() << "fstat() failed on file " << name > + << " error "<< errno << " : " << strerror(errno); > + os_file_handle_error_no_exit(name, "fstat()", FALSE); > + } else { > + fblock_size = local_stat.st_blksize; > + } > +#endif /* UNIV_LINUX */ Why only Linux? fstat() is already in POSIX.1-2001. > @@ -953,8 +1009,8 @@ class AIOHandler { > ut_a(slot->type.is_read() || !slot->skip_punch_hole); > > return(os_file_io_complete( > - slot->type, slot->file, slot->buf, > - slot->compressed_page, slot->original_len, > + const_cast<IORequest&>(slot->type), > + slot->file, > static_cast<ulint>(slot->offset), > slot->len)); > } Please never use const_cast in a context where the object can be modified. It is unclear to me why we would need to lose the constness of IORequest here. (The members of IORequest do not need changes. Writes through pointers in IORequest are allowed even if IORequest itself is const.) > @@ -1659,85 +1715,78 @@ os_file_read_string( > + ulint amount = trim_len / type.block_size(); > + switch(type.block_size()) { > + case 512: > + srv_stats.page_compression_trim_sect512.add(amount); > + break; Can we store the log2 of the block_size somewhere and use an array of compression attributes, to avoid so much repeated code? > @@ -4252,6 +4300,7 @@ os_file_punch_hole_win32( > fh, FSCTL_SET_ZERO_DATA, &punch, sizeof(punch), > NULL, 0, &temp, NULL); > > + > return(!result ? DB_IO_NO_PUNCH_HOLE : DB_SUCCESS); > } > One empty line is enough. Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

1 0

Re: [Maria-developers] [JIRA] (MDEV-11685) sql_mode can't be set with non-ascii connection charset
by Alexander Barkov 11 Jan '17

11 Jan '17

Hello Nirbhay, On 01/09/2017 09:48 PM, Nirbhay Choubey wrote: > Hi Bar, > > On Mon, Jan 9, 2017 at 1:00 AM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Nirbhay, > > IIRC, you was working on this issue. > What is the current status? > > > Yes, I have just committed a patch. > Could you please take a look at it? The patch http://lists.askmonty.org/pipermail/commits/2017-January/010405.html looks Ok. I have a proposal for tests. The problem is that utf32, utf16 and ucs2 are not always compiled-in. So in case when some of the affected charsets is missing, sql_mode.test will fail. Can you please move the tests into ctype_utf32.test, ctype_utf16.test, ctype_ucs2.test ? I also suggest to add "SELECT @@sql_mode", to make sure that the assignment actually does what it's supposed to do. Remembering sql_mode is not necessary. It's OK to restore it to DEFAULT. Remembering character_set_connection is not necessary. It's Ok to do "SET NAMES utf8" at the end. Collecting all together, I'd suggest this chuck for ctype_utf32.test: --echo # --echo # MDEV-11685: sql_mode can't be set with non-ascii connection charset --echo # SET character_set_connection=utf32; SET sql_mode='NO_ENGINE_SUBSTITUTION'; SELECT @@sql_mode; SET sql_mode=DEFAULT; SET NAMES utf8; and similar chunks for ctype_utf16.test and ctype_ucs2.test. Thanks! > > Best, > Nirbhay > > > > Thanks! > > On 01/03/2017 09:31 PM, Elena Stepanova (JIRA) wrote: > > > > [ > https://jira.mariadb.org/browse/MDEV-11685?page=com.atlassian.jira.plugin.s… > <https://jira.mariadb.org/browse/MDEV-11685?page=com.atlassian.jira.plugin.s…> > ] > > > > Elena Stepanova reassigned MDEV-11685: > > -------------------------------------- > > > > Labels: upstream (was: ) > > Affects Version/s: 5.5 > > 10.0 > > 10.1 > > 10.2 > > Assignee: Alexander Barkov > > > >> sql_mode can't be set with non-ascii connection charset > >> ------------------------------------------------------- > >> > >> Key: MDEV-11685 > >> URL: https://jira.mariadb.org/browse/MDEV-11685 > <https://jira.mariadb.org/browse/MDEV-11685> > >> Project: MariaDB Server > >> Issue Type: Bug > >> Components: Character Sets > >> Affects Versions: 10.2.3, 5.5, 10.0, 10.1, 10.2 > >> Reporter: Nirbhay Choubey > >> Assignee: Alexander Barkov > >> Labels: upstream > >> > >> {code} > >> MariaDB [test]> set sql_mode = 'NO_ENGINE_SUBSTITUTION'; > >> Query OK, 0 rows affected (0.00 sec) > >> MariaDB [test]> set character_set_connection=utf32; > >> Query OK, 0 rows affected (0.00 sec) > >> MariaDB [test]> set sql_mode = 'NO_ENGINE_SUBSTITUTION'; > >> ERROR 1231 (42000): Variable 'sql_mode' can't be set to the value > of 'NO_ENGINE_SUBSTITUTION' > >> {code} > > > > > > > > -- > > This message was sent by Atlassian JIRA > > (v7.2.1#72003) > > > >

2 2

[Maria-developers] InnoDB: Instant ADD COLUMN that works on old data files
by Marko Mäkelä 11 Jan '17

11 Jan '17

I would like to seek technical review and feedback for an idea. In the long term, I would like InnoDB to support instant ALTER TABLE for a larger set of operations: https://jira.mariadb.org/browse/MDEV-11424 Instant ALTER TABLE of failure-free record format changes Probably one of the most common operations is ADD COLUMN. For that, a ticket had already been filed and some patches exist: https://jira.mariadb.org/browse/MDEV-11369 Instant add column for InnoDB Tencent and Alibaba have ADD COLUMN patches that basically introduce new InnoDB record formats so that each record will carry the number of columns, similar to how ROW_FORMAT=REDUNDANT (the original InnoDB row format) works. A problem with that approach is that we would only be able to support instant ADD COLUMN on data files that already are in a suitable format. The whole purpose of instant ALTER TABLE is to avoid conversions of huge data files. Requiring a conversion before the feature can be used would in my opinion defeat the purpose. I came up with a solution that only requires some changes to the InnoDB page format, not to the record formats: Require each record in a clustered index leaf page to contain the same number of fields (on write, convert the whole page). We can repurpose the previously unused (always 0) field PAGE_MAX_TRX_ID on clustered index leaf pages so that the number of fields in each leaf page can be determined. This is the idea that I posted in MDEV-11369 last week and edited today. As far as I can tell, it should work on any existing data file, and it would be a step towards the full-blown MDEV-11424 in a distant future version of MariaDB. One challenge with the page-at-a-time conversion is that inserting or updating records into a leaf page may require the page to be split into several pages, especially in the case where multiple CHAR(xxx) NOT NULL columns would need to be materialized as a result of the insert or update. Does this seem to make sense to you? I am looking forward to your feedback. Best regards, Marko -- DON’T MISS M|17 April 11 - 12, 2017 The Conrad Hotel New York City https://m17.mariadb.com/ Marko Mäkelä, Lead Developer InnoDB MariaDB Corporation

2 2

[Maria-developers] Fwd: Total No of events in event group.
by Sachin Setiya 10 Jan '17

10 Jan '17

Hi Kristian! On Tue, Jan 10, 2017 at 4:41 PM, Kristian Nielsen <knielsen(a)knielsen-hq.org> wrote: > Sachin Setiya <sachin.setiya(a)mariadb.com> writes: > >> Can we do something like send total no of Rows_log_events in a event >> group to slave. We can write this info in Gtid_log_event. Reason for >> doing this is that on slave we can show how much progress we have >> made. > > My immediate impression is that this sounds like a bad idea. > > It increases the binlog size without adding any real information - after all > the event count can be determined easily by just reading the events. > You mean counting the all events in event group without applying them. > It also requires careful design and testing for impact on forwards- and > backwards compatibility. > > And event count is not a very meaningful progress indicator. One event group > could have 10 Rows_log_event each with a single row operation. While another > could have a single Rows_log_event that contains a hundred row operations. > Okay, So it there a better way to show progress in slave ? > And why only rows event? What about Query_log_events in a transaction in > statement-based replication? We can generalize this idea to Query_log_events also. > > - Kristian. Regards sachin

2 3

[Maria-developers] Total No of events in event group.
by Sachin Setiya 10 Jan '17

10 Jan '17

Hi Kristian! Can we do something like send total no of Rows_log_events in a event group to slave. We can write this info in Gtid_log_event. Reason for doing this is that on slave we can show how much progress we have made. -- Regards sachin

2 1

[Maria-developers] Status of MDEV-4557 / 7409
by Sachin Setiya 08 Jan '17

08 Jan '17

Hi Daniel! I was thinking of taking 4557 , but in comment I see you have ported the patch for 10.0 but it was not merged. Are you going to take 4557 , or can I take it ? -- Regards Sachin Setiya Software Engineer at MariaDB

2 1

[Maria-developers] Mdev-10664 Add statuses about optimistic parallel replication stalls.
by Sachin Setiya 08 Jan '17

08 Jan '17

Hi Kristian, I am stuck at one problem in Mdev-10664. Suppose there is multisource replication('master1' and 'master2' ) and we want to update status var, How to know which master_info to update ?. Does slave threads have current replication channel name? (I was not able to find any , I have looked in rpl_group_info) I am attaching patch file, only above problem need to solved in patch. -- Regards Sachin Setiya Software Engineer at MariaDB

3 5

Re: [Maria-developers] Review for MDEV-10332
by Sergei Golubchik 29 Dec '16

29 Dec '16

Hi, Georg! First, I've looked a bit at EVP_CIPHER_CTX_new/etc. With the CRYPTO_set_mem_functions() one can overwrite malloc/realloc/free functions that OpenSSL is using. We absolutely should do it. At the very least, OpenSSL should use my_malloc/my_realloc/my_free. But a more interesting approach would be to allocate a buffer on the stack and let our openssl m/r/f helpers take the memory from there. And print a warning with a fallback to malloc if that buffer isn't enough. This way openssl won't need mallocs is most cases. Still, I'd suggest to do this in a separate commit. In your main "OpenSSL 1.1 support" commit, just use CRYPTO_set_mem_functions to put OpenSSL through my_malloc/my_realloc/my_free. On Nov 15, Georg Richter wrote: > From 6103b311c4c5740998d342afbba9ac70f8ec5731 Mon Sep 17 00:00:00 2001 > From: Georg Richter <georg(a)mariadb.com> > Date: Tue, 15 Nov 2016 18:39:54 +0100 > Subject: [PATCH] MDEV-10332: Added support for OpenSSL 1.1 > > diff --git a/mysql-test/r/openssl_6975,tlsv10.result b/mysql-test/r/openssl_6975,tlsv10.result > index a65167f..e216ec2 100644 > --- a/mysql-test/r/openssl_6975,tlsv10.result > +++ b/mysql-test/r/openssl_6975,tlsv10.result > @@ -1,11 +1,11 @@ > create user ssl_sslv3@localhost; > -grant select on test.* to ssl_sslv3@localhost require cipher "RC4-SHA"; > +grant select on test.* to ssl_sslv3@localhost require cipher "DES-CBC3-SHA"; did you verify that after your changes all tests also work with older openssl versions and with yassl? > create user ssl_tls12@localhost; > grant select on test.* to ssl_tls12@localhost require cipher "AES128-SHA256"; > TLS1.2 ciphers: user is ok with any cipher > ERROR 2026 (HY000): SSL connection error: sslv3 alert handshake failure > diff --git a/mysys_ssl/my_crypt.cc b/mysys_ssl/my_crypt.cc > index 49bd9af..d72bc72 100644 > --- a/mysys_ssl/my_crypt.cc > +++ b/mysys_ssl/my_crypt.cc > @@ -36,9 +36,20 @@ > class MyCTX > { > public: > - EVP_CIPHER_CTX ctx; > - MyCTX() { EVP_CIPHER_CTX_init(&ctx); } > - virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(&ctx); ERR_remove_state(0); } > + EVP_CIPHER_CTX *ctx; > +#if defined(HAVE_YASSL) > + MyCTX() { ctx= new EVP_CIPHER_CTX; EVP_CIPHER_CTX_init(ctx); } > + virtual ~MyCTX() { EVP_CIPHER_CTX_cleanup(ctx); delete ctx; ERR_remove_state(0); } > +#else > + MyCTX() {ctx= EVP_CIPHER_CTX_new(); EVP_CIPHER_CTX_init(ctx); } > + virtual ~MyCTX() { EVP_CIPHER_CTX_free(ctx); > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > + ERR_remove_state(0); > +#else > + ERR_clear_error(); > +#endif > + } > +#endif No #ifdef's here, please. YaSSL-specific code from mysys_ssl/yassl.cc makes yassl to look sufficiently like openssl, so that mysys_ssl/my_crypt.cc wouldn't need any ifdefs. Please, keep it that way. And, certainly, don't put any #if's that check openssl version in the code, keep them in the headers or in the beginning of the file (there is, already #define ERR_remove_state(X) there) > virtual int init(const EVP_CIPHER *cipher, int encrypt, const uchar *key, > uint klen, const uchar *iv, uint ivlen) > @@ -72,7 +83,10 @@ class MyCTX_nopad : public MyCTX > { > public: > const uchar *key; > + const uchar *src; > int klen; > + uchar oiv[90]; 90 bytes for an IV? > + uint extra_bytes; > > MyCTX_nopad() : MyCTX() { } > ~MyCTX_nopad() { } > @@ -285,7 +310,11 @@ int my_random_bytes(uchar *buf, int num) > instead of whatever random engine is currently set in OpenSSL. That way > we are guaranteed to have a non-blocking random. > */ > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > RAND_METHOD *rand = RAND_SSLeay(); > +#else > + RAND_METHOD *rand = RAND_OpenSSL(); > +#endif no #if's in the code, please > if (rand == NULL || rand->bytes(buf, num) != 1) > return MY_AES_OPENSSL_ERROR; > return MY_AES_OK; > diff --git a/mysys_ssl/my_md5.cc b/mysys_ssl/my_md5.cc > index 7139ea9..4885033 100644 > --- a/mysys_ssl/my_md5.cc > +++ b/mysys_ssl/my_md5.cc > @@ -58,10 +57,11 @@ static void md5_result(MD5_CONTEXT *context, uchar digest[MD5_HASH_SIZE]) > > #elif defined(HAVE_OPENSSL) > #include <openssl/evp.h> > -typedef EVP_MD_CTX MD5_CONTEXT; > +typedef void MD5_CONTEXT; > > -static void md5_init(MD5_CONTEXT *context) > +static void md5_init(MD5_CONTEXT *ctx) > { > + EVP_MD_CTX *context= (EVP_MD_CTX *)ctx; > EVP_MD_CTX_init(context); > #ifdef EVP_MD_CTX_FLAG_NON_FIPS_ALLOW > /* Ok to ignore FIPS: MD5 is not used for crypto here */ > @@ -123,21 +143,43 @@ void my_md5_multi(uchar *digest, ...) > { > va_list args; > va_start(args, digest); > - > - MD5_CONTEXT md5_context; > +#ifdef HAVE_YASSL > + MD5_CONTEXT *md5_context= new MD5_CONTEXT; > +#else > +#if OPENSSL_VERSION_NUMBER < 0x10100000L > + EVP_MD_CTX *md5_context= (EVP_MD_CTX *)alloca(sizeof(EVP_MD_CTX)); > +#else > + EVP_MD_CTX *md5_context= EVP_MD_CTX_new(); > +#endif > +#endif > const uchar *str; > > - md5_init_fast(&md5_context); > + md5_init_fast(md5_context); > for (str= va_arg(args, const uchar*); str; str= va_arg(args, const uchar*)) > - md5_input(&md5_context, str, va_arg(args, size_t)); > + md5_input(md5_context, str, va_arg(args, size_t)); > > - md5_result(&md5_context, digest); > + md5_result(md5_context, digest); > va_end(args); > +#if defined(HAVE_YASSL) > + delete md5_context; > +#else > +#if OPENSSL_VERSION_NUMBER >= 0x10100000L > + EVP_MD_CTX_free(md5_context); > +#endif > +#endif > } > > size_t my_md5_context_size() > { > +#if defined(HAVE_YASSL) > return sizeof(MD5_CONTEXT); > +#elif OPENSSL_VERSION_NUMBER < 0x10100000L > + return sizeof(EVP_MD_CTX); > +#else > + /* Since OpenSSL 1.1 we can't determine the length of EVP_MD_CTX anymore, > + so we need to return a number (48 bytes + 16 bytes reserved) */ > + return 64; How can you be sure that it's enough? I'd just return sizeof(void*) and in md5_init stored a return value of EVP_MD_CTX_new() there. Until we do that optimization that I mentioned in the beginning of the email. And get rid of #if's please. > +#endif > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] TRUE LOCK=NONE for slave
by Sachin Setiya 29 Dec '16

29 Dec '16

Hello everyone! I am thinking of implementing true lock=none for slaves. Currently Alter command is only executed on slave when master commits it. I(thanks marko for suggesting this idea) am thinking of writing BEGIN_DDL_EVENT in binlog after master hits ha_prepare_inplace_alter_table. Then master will write QUERY_EVENT on binlog with actual alter query . On commit/rollback master will write COMMIT_DDL_EVENT/ROLLBACK_DDL_EVENT. On slave there will be pool of threads(configurable global variable), which will apply these DDLs. On receiving BEGIN_DDL_EVENT slave thread will pass the QUERY_EVENT to one of the worker thread. Worker thread will execute until ha_inplace_alter_table. Actual commit_inplace_alter will be called by sql thread. If sql thread recieve some kind of rollback event , then it will somehow signal worker thread to stop executing alter. If none of the worker threads are available then event will be en-queued, then If we received rollback event the we will simply discard event from queue, If we received commit event then SQL thread will synchronously process DDL event. Please let me know what you think. Credits for idea Marko and Lixun. Regards sachin

3 3

Re: [Maria-developers] [Commits] d158f55659c: MDEV-11679 Remove redundant function fsp_header_get_crypt_offset()
by Jan Lindström 29 Dec '16

29 Dec '16

On Wed, Dec 28, 2016 at 5:16 PM, <marko.makela(a)mariadb.com> wrote: > > diff --git a/storage/innobase/fil/fil0crypt.cc b/storage/innobase/fil/ > fil0crypt.cc > index 75efcdfdab0..095b9f706c0 100644 > --- a/storage/innobase/fil/fil0crypt.cc > +++ b/storage/innobase/fil/fil0crypt.cc > @@ -1179,10 +1179,11 @@ fil_crypt_start_encrypting_space( > > /* 3 - compute location to store crypt data */ > byte* frame = buf_block_get_frame(block); > - ulint maxsize = 0; > ut_ad(crypt_data); > - crypt_data->page0_offset = > - fsp_header_get_crypt_offset(page_size, &maxsize); > + crypt_data->page0_offset = FSP_HEADER_OFFSET > + + fsp_header_get_encryption_offset(page_size); > + const ulint maxsize = page_size.logical() > + - crypt_data->page0_offset - FIL_PAGE_DATA_END; > > This I do not like, why fsp_header_get_encryption_offset does not calculate the actual offset so that there is no need for adjustments? Now every place where you call this you need to add FSP_HEADER_OFFSET to returned offset. Similarly for maxsize actually meaning free space on the page. This is naturally technically correct, but is it maintainable? R: Jan

1 0

Re: [Maria-developers] e687577: MDEV-10340: support COM_RESET_CONNECTION
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 12, Oleksandr Byelkin wrote: > revision-id: e6875772b0d65a6341456a6f4aca3353bbdae3e4 (mariadb-10.2.2-131-ge687577) > parent(s): 7ca1e2abad42a7436e6b668b4568d6fadc2ca165 > committer: Oleksandr Byelkin > timestamp: 2016-12-12 14:41:45 +0100 > message: > > MDEV-10340: support COM_RESET_CONNECTION > > draft to check with client > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index a2a4ed2..b760aeb 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -1679,6 +1680,13 @@ bool dispatch_command(enum enum_server_command command, THD *thd, > break; > } > #endif > + case COM_RESET_CONNECTION: > + { > + thd->status_var.com_other++; > + thd->change_user(); > + my_ok(thd, 0, 0, 0); > + break; > + } Looks ok, but I suspect that, just like for COM_CHANGE_USER, you might need here: thd->clear_error(); // if errors from rollback ok to push with the change as above. But please make sure that C/C part is pushed too Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 33b01257190: MDEV-11584: GRANT inside an SP does not work well on 2nd execution
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 21, Oleksandr Byelkin wrote: > revision-id: 33b01257190cef414a4b0f00bf36ee6a3ceda0a7 (mariadb-10.1.20-6-g33b01257190) > parent(s): 9e032d6150b39cf36a22e7c1503e06fae9c4016d > committer: Oleksandr Byelkin > timestamp: 2016-12-21 20:11:14 +0100 > message: > > MDEV-11584: GRANT inside an SP does not work well on 2nd execution > > Allocate password hash in statment memory > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 3f26bc2e08a..b44b120cf04 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1067,7 +1067,14 @@ static bool fix_lex_user(THD *thd, LEX_USER *user) > make_scramble= my_make_scrambled_password; > } > > - char *buff= (char *) thd->alloc(scramble_length + 1); > + char *buff; > + { > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + buff= (char *) thd->alloc(scramble_length + 1); > + if (arena) > + thd->restore_active_arena(arena, &backup); > + } Ok to push, just remove the unnecessary pair of curly braces and the indentation. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] Fwd: mixing of user-defined data types with other data types
by Vicențiu Ciorbaru 28 Dec '16

28 Dec '16

Hi Alexander! Comments inline. Most are stylistic comments. I think the patch is great. The Item_type_holder is really bugging me though. It feels poorly designed in the current place. > diff --git a/sql/field.cc b/sql/field.cc > index 6f273e6..7876a6f 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -84,9 +84,13 @@ const char field_separator=','; > */ > #define FIELDTYPE_TEAR_FROM (MYSQL_TYPE_BIT + 1) > #define FIELDTYPE_TEAR_TO (MYSQL_TYPE_NEWDECIMAL - 1) > -#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (255 - FIELDTYPE_TEAR_TO)) > +#define FIELDTYPE_LAST 254 > +#define FIELDTYPE_NUM (FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO)) > These defines here look sketchy. I know it's not from your patch but can we not turn them to const ints instead? Also, the naming is horibly inconsistent, one uses 2 and others use to. I vote for _to_. We can have the full function definition as: const int FIELDTYPE_TEAR_FROM= MYSQL_TYPE_BIT + 1; const int FIELDTYPE_TEAR_TO= MYSQL_TYPE_NEWDECIMAL - 1; const int FIELDTYPE_LAST= 254; const int FIELDTYPE_NUM= FIELDTYPE_TEAR_FROM + (FIELDTYPE_LAST - FIELDTYPE_TEAR_TO); static inline int field_type_to_index (enum_field_types field_type) { DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); DBUG_ASSERT(field_type <= FIELDTYPE_LAST); field_type= real_type_to_type(field_type); if (field_type < FIELDTYPE_TEAR_FROM) return field_type; return FIELDTYPE_TEAR_FROM + (field_type - FIELDTYPE_TEAR_TO) - 1; } The ternary operator provides no benefit in this case, let's replace it with regular if statement. Same number of lines but the code is cleaner I'd say. > > static inline int field_type2index (enum_field_types field_type) > { > + DBUG_ASSERT(real_type_to_type(field_type) < FIELDTYPE_TEAR_FROM || > + real_type_to_type(field_type) > FIELDTYPE_TEAR_TO); > + DBUG_ASSERT(field_type <= FIELDTYPE_LAST); > field_type= real_type_to_type(field_type); > return (field_type < FIELDTYPE_TEAR_FROM ? > field_type : > @@ -94,6 +98,12 @@ static inline int field_type2index (enum_field_types field_type) > } > > > +/** > + Implements data type merge rules for the built-in traditional data types. > + - UNION > + - CASE and its abbreviations COALESCE, IF, IFNULL > + - LEAST/GREATEST > +*/ > I don't like this comment here as it's not complete. I'd much rather have it also explain how to interpret this table. I had to look up a calling spot to figure out exactly how it's done. I'd also have it say: Ex: Given Fields A and B of real_types a and b, we find the result type of COALESCE(A, B) by querying: field_types_merge_rules[field_type_to_index(a)][field_type_to_index(b)]. > > > static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > { > /* MYSQL_TYPE_DECIMAL -> */ > @@ -948,46 +927,18 @@ static enum_field_types field_types_merge_rules [FIELDTYPE_NUM][FIELDTYPE_NUM]= > enum_field_types Field::field_type_merge(enum_field_types a, > enum_field_types b) > { > - DBUG_ASSERT(real_type_to_type(a) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(a) > FIELDTYPE_TEAR_TO); > - DBUG_ASSERT(real_type_to_type(b) < FIELDTYPE_TEAR_FROM || > - real_type_to_type(b) > FIELDTYPE_TEAR_TO); > return field_types_merge_rules[field_type2index(a)] > [field_type2index(b)]; > } > > - > -static Item_result field_types_result_type [FIELDTYPE_NUM]= > +const Type_handler * > +Type_handler::aggregate_for_result_traditional(const Type_handler *a, > + const Type_handler *b) > { > - //MYSQL_TYPE_DECIMAL MYSQL_TYPE_TINY > - DECIMAL_RESULT, INT_RESULT, > - //MYSQL_TYPE_SHORT MYSQL_TYPE_LONG > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_FLOAT MYSQL_TYPE_DOUBLE > - REAL_RESULT, REAL_RESULT, > - //MYSQL_TYPE_NULL MYSQL_TYPE_TIMESTAMP > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_LONGLONG MYSQL_TYPE_INT24 > - INT_RESULT, INT_RESULT, > - //MYSQL_TYPE_DATE MYSQL_TYPE_TIME > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_DATETIME MYSQL_TYPE_YEAR > - STRING_RESULT, INT_RESULT, > - //MYSQL_TYPE_NEWDATE MYSQL_TYPE_VARCHAR > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BIT <16>-<245> > - STRING_RESULT, > - //MYSQL_TYPE_NEWDECIMAL MYSQL_TYPE_ENUM > - DECIMAL_RESULT, STRING_RESULT, > - //MYSQL_TYPE_SET MYSQL_TYPE_TINY_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_MEDIUM_BLOB MYSQL_TYPE_LONG_BLOB > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_BLOB MYSQL_TYPE_VAR_STRING > - STRING_RESULT, STRING_RESULT, > - //MYSQL_TYPE_STRING MYSQL_TYPE_GEOMETRY > - STRING_RESULT, STRING_RESULT > -}; > + enum_field_types ta= a->real_field_type(); > + enum_field_types tb= b->real_field_type(); > + return Type_handler::get_handler_by_real_type(Field::field_type_merge(ta, tb)); Nit: this line is over 80 characters. Do we care? Most code tends to comply to it. It adds one extra line to split so it might not be worth it to adhere to a full 100% 80 character limit. > +} > > > /* > diff --git a/sql/field.h b/sql/field.h > index 541da5a..fa84e7d 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -835,7 +835,6 @@ class Field: public Value_source > virtual Item_result cmp_type () const { return result_type(); } > static bool type_can_have_key_part(enum_field_types); > The field_type_merge function breaks all the other naming patterns. We have result_type, cmp_type real_type and now field_type_merge. Wouldn't it be better to name it field_merge_type, to be consistent? Or since this is a lookup kind of operation, perhaps name it (get|lookup)_merge_field_type? I know it's not _exactly_ like result_type and compare_type, but it generally is used in a simillar context. > > static enum_field_types field_type_merge(enum_field_types, enum_field_types); > - static Item_result result_merge_type(enum_field_types); > virtual bool eq(Field *field) > { > return (ptr == field->ptr && null_ptr == field->null_ptr && > diff --git a/sql/item.cc b/sql/item.cc > index c97f41f..c9b6155 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -9657,20 +9657,8 @@ Item_type_holder::Item_type_holder(THD *thd, Item *item) > maybe_null= item->maybe_null; > collation.set(item->collation); > get_full_info(item); > - /** > - Field::result_merge_type(real_field_type()) should be equal to > - result_type(), with one exception when "this" is a Item_field for > - a BIT field: > - - Field_bit::result_type() returns INT_RESULT, so does its Item_field. > - - Field::result_merge_type(MYSQL_TYPE_BIT) returns STRING_RESULT. > - Perhaps we need a new method in Type_handler to cover these type > - merging rules for UNION. > - */ > - DBUG_ASSERT(real_field_type() == MYSQL_TYPE_BIT || > - Item_type_holder::result_type() == > - Field::result_merge_type(Item_type_holder::real_field_type())); > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > Alright so this seems to be fixed here, looking at Type_handler_bit, inheriting from Type_handler_int_result. Do we test this somewhere though? I couldn't find it in the test case, perhaps you can point it out to me. > > + if (Item_type_holder::result_type() == INT_RESULT) > decimals= 0; > prev_decimal_int_part= item->decimal_int_part(); > #ifdef HAVE_SPATIAL > @@ -9785,12 +9773,21 @@ bool Item_type_holder::join_types(THD *thd, Item *item) > DBUG_PRINT("info:", ("in type %d len %d, dec %d", > get_real_type(item), > item->max_length, item->decimals)); > - set_handler_by_real_type(Field::field_type_merge(real_field_type(), > - get_real_type(item))); > + const Type_handler *item_type_handler= > + Type_handler::get_handler_by_real_type(get_real_type(item)); > + if (aggregate_for_result(item_type_handler)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + Item_type_holder::type_handler()->name().ptr(), > + item_type_handler->name().ptr(), > + "UNION"); > + DBUG_RETURN(true); > + } > + > { > uint item_decimals= item->decimals; > /* fix variable decimals which always is NOT_FIXED_DEC */ > - if (Field::result_merge_type(real_field_type()) == INT_RESULT) > + if (Item_type_holder::result_type() == INT_RESULT) > Ok, we're avoiding a virtual function call here. What I don't like is that this Item_type_holder class basically "abuses" the Item interface to such a degree that it even has to directly declare functions such as: double Item_type_holder::val_real() { DBUG_ASSERT(0); // should never be called return 0.0; } Let's discuss about cleaning this up later. To me it feels like this Item does not really belong in the Item class and should be factored out. Probably a whole project on its own :) > > > item_decimals= 0; > decimals= MY_MAX(decimals, item_decimals); > } > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index 98b179b..e5e366e 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -180,32 +179,40 @@ static int agg_cmp_type(Item_result *type, Item **items, uint nitems) > @return aggregated field type. > */ > > -enum_field_types agg_field_type(Item **items, uint nitems, > - bool treat_bit_as_number) > Why is this function in item_cmpfunc.cc and not in sql_type.cc? > +bool > +Type_handler_hybrid_field_type::aggregate_for_result(const char *funcname, > + Item **items, uint nitems, > + bool treat_bit_as_number) > { I would move uint i to be a local for variable. This is a C-style loop. Is there a compiler that doesn't support this in one of our builders? Also, how about size_t instead of uint? (Probably not necessary but wlad made a point of prefering to use that for iterators and such). > uint i; > if (!nitems || items[0]->result_type() == ROW_RESULT) > { > DBUG_ASSERT(0); > - return MYSQL_TYPE_NULL; > + set_handler(&type_handler_null); > + return true; > } > - enum_field_types res= items[0]->field_type(); > + set_handler(items[0]->type_handler()); > uint unsigned_count= items[0]->unsigned_flag; > for (i= 1 ; i < nitems ; i++) > { > - enum_field_types cur= items[i]->field_type(); > + const Type_handler *cur= items[i]->type_handler(); > if (treat_bit_as_number && > - ((res == MYSQL_TYPE_BIT) ^ (cur == MYSQL_TYPE_BIT))) > + ((type_handler() == &type_handler_bit) ^ (cur == &type_handler_bit))) > { > - if (res == MYSQL_TYPE_BIT) > - res= MYSQL_TYPE_LONGLONG; // BIT + non-BIT > + if (type_handler() == &type_handler_bit) > + set_handler(&type_handler_longlong); // BIT + non-BIT > else > - cur= MYSQL_TYPE_LONGLONG; // non-BIT + BIT > + cur= &type_handler_longlong; // non-BIT + BIT > + } > + if (aggregate_for_result(cur)) > + { > + my_error(ER_CANT_AGGREGATE_2TYPES, MYF(0), > + type_handler()->name().ptr(), cur->name().ptr(), funcname); > + return true; > } > - res= Field::field_type_merge(res, cur); > unsigned_count+= items[i]->unsigned_flag; > } > - switch (res) { > + switch (field_type()) { > case MYSQL_TYPE_TINY: > case MYSQL_TYPE_SHORT: > case MYSQL_TYPE_LONG: > diff --git a/sql/sql_type.cc b/sql/sql_type.cc > index 8746595..397b5cf 100644 > --- a/sql/sql_type.cc > +++ b/sql/sql_type.cc > @@ -54,6 +51,41 @@ static Type_handler_set type_handler_set; > Type_handler_null type_handler_null; > Type_handler_row type_handler_row; > Type_handler_varchar type_handler_varchar; > +Type_handler_newdecimal type_handler_newdecimal; > +Type_handler_longlong type_handler_longlong; > +Type_handler_bit type_handler_bit; > + > + I'm sure there's a better way to write this so that it gets initialized at compile time instead of at runtime (before main). Perhaps we can define the Type_aggregator differently. I need to look this Up. For now it will work. Standard offers no guarantees regarding the order, but it shouldn't matter for us as the address shouldn't change for global objects during initialization. "It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. If the initialization is deferred to some point in time after the first statement of main, it shall occur before the first use of any function or object defined in the same translation unit as the object to be initialized." > +Type_aggregator type_aggregator_for_result; > + > + > +class Static_data_initializer > +{ > +public: > + static Static_data_initializer m_singleton; > + Static_data_initializer() > + { > +#ifdef HAVE_SPATIAL > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_null, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_geometry, > + &type_handler_geometry); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_blob, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_varchar, > + &type_handler_long_blob); > + type_aggregator_for_result.add(&type_handler_geometry, > + &type_handler_string, > + &type_handler_long_blob); > +#endif > + } > +}; > + > +Static_data_initializer Static_data_initializer::m_singleton; > > > /** > diff --git a/sql/sql_type.h b/sql/sql_type.h > index 92dee61..4bc5542 100644 > --- a/sql/sql_type.h > +++ b/sql/sql_type.h > @@ -214,6 +214,31 @@ class Type_std_attributes > }; > > I don't like this class too much. One can easily break it by either changing LEX_CSTRING::str or LEX_CSTRING::length without changing the other one. I suggest we make the inheritance private so that the only way to access the members is through the methods available. A suggestion I have is to create a generic "String" class that has this same behaviour, without calling it Name. Afterwards typedefing it to Name. > > +class Name: public LEX_CSTRING > +{ > +public: > + Name(const char *str_arg, uint length_arg) > + { > + LEX_CSTRING::str= str_arg; > + LEX_CSTRING::length= length_arg; > + } Also, these ought to be removed if they are commented out. > + /* > + Name() > + { > + LEX_CSTRING::str= NULL; > + LEX_CSTRING::length= 0; > + } > + Name(const LEX_STRING &name) > + { > + LEX_CSTRING::str= name.str; > + LEX_CSTRING::length= name.length; > + } > + */ > + const char *ptr() const { return LEX_CSTRING::str; } > + uint length() const { return LEX_CSTRING::length; } > +}; > + > + > class Type_handler > { > protected: > @@ -235,6 +260,11 @@ class Type_handler > DBUG_ASSERT(type != TIME_RESULT); > return get_handler_by_cmp_type(type); > } > + static const > + Type_handler *aggregate_for_result_traditional(const Type_handler *h1, > + const Type_handler *h2); > + This function can return a const reference from a const static object within the class's namespace. Why create an object every time this is called? Compiler might optimize it, but why risk it? This goes for all the implementations. > + virtual const Name name() const= 0; > virtual enum_field_types field_type() const= 0; > virtual enum_field_types real_field_type() const { return field_type(); } > virtual Item_result result_type() const= 0; > @@ -884,5 +958,63 @@ class Type_handler_hybrid_real_field_type: > extern Type_handler_row type_handler_row; > extern Type_handler_null type_handler_null; > extern Type_handler_varchar type_handler_varchar; > +extern Type_handler_newdecimal type_handler_newdecimal; > +extern Type_handler_longlong type_handler_longlong; > +extern Type_handler_bit type_handler_bit; > + > +class Type_aggregator > +{ I'm not sure if Element is a good name for this. Aggregate_result_pair maybe? > + class Element > + { > + public: > + const Type_handler *m_handler1; > + const Type_handler *m_handler2; > + const Type_handler *m_result; > + Element() { } > + Element(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + :m_handler1(handler1), m_handler2(handler2), m_result(result) > + { } > + bool eq(const Type_handler *handler1, const Type_handler *handler2) const > + { > + return m_handler1 == handler1 && m_handler2 == handler2; > + } > + }; So, we've discussed this before to change to a dynamic array. I prefer the template Dynamic_array over DYNAMIC_ARRAY. > + Element m_element_array[256]; > + uint m_element_count; > + const Element *element(uint i) const { return &m_element_array[i]; } > +public: > + Type_aggregator() > + :m_element_count(0) > + { } > + bool add(const Type_handler *handler1, > + const Type_handler *handler2, > + const Type_handler *result) > + { > + m_element_array[m_element_count]= Element(handler1, handler2, result); > + m_element_count++; > + return false; > + } > + const Element *find_element(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + for (uint i= 0; i < m_element_count; i++) > + { > + const Element *el= element(i); > + if (el->eq(handler1, handler2) || el->eq(handler2, handler1)) > + return el; > + } > + return NULL; > + } > + const Type_handler *find_handler(const Type_handler *handler1, > + const Type_handler *handler2) const > + { > + const Element *el= find_element(handler1, handler2); > + return el ? el->m_result : NULL; > + } > +}; > + > +extern Type_aggregator type_aggregator_for_result; > > #endif /* SQL_TYPE_H_INCLUDED */

2 2

Re: [Maria-developers] 33b01257190: MDEV-11584: GRANT inside an SP does not work well on 2nd execution
by Sergei Golubchik 28 Dec '16

28 Dec '16

Hi, Oleksandr! On Dec 21, Oleksandr Byelkin wrote: > revision-id: 33b01257190cef414a4b0f00bf36ee6a3ceda0a7 (mariadb-10.1.20-6-g33b01257190) > parent(s): 9e032d6150b39cf36a22e7c1503e06fae9c4016d > committer: Oleksandr Byelkin > timestamp: 2016-12-21 20:11:14 +0100 > message: > > MDEV-11584: GRANT inside an SP does not work well on 2nd execution > > Allocate password hash in statment memory > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 3f26bc2e08a..b44b120cf04 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -1067,7 +1067,14 @@ static bool fix_lex_user(THD *thd, LEX_USER *user) > make_scramble= my_make_scrambled_password; > } > > - char *buff= (char *) thd->alloc(scramble_length + 1); > + char *buff; > + { > + Query_arena *arena, backup; > + arena= thd->activate_stmt_arena_if_needed(&backup); > + buff= (char *) thd->alloc(scramble_length + 1); > + if (arena) > + thd->restore_active_arena(arena, &backup); > + } where is it called from, why does it happen during the parsing? > if (buff == NULL) > return true; > make_scramble(buff, user->pwtext.str, user->pwtext.length); Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 28 Dec '16

28 Dec '16

Sanja, On 12/27/2016 09:39 PM, Oleksandr Byelkin wrote: > Hi, Alexander! > > It is OK to push after fixing in the comment of the patch > mysql_list_fields with mysqld_list_fields (IMHO it is server function > do not set type etc.) Thanks for review! I fixed the comment. But then I realized that it's a good idea to add tests for other field types (e.g. DECIMAL, DATETIME, ENUM), and found that my fix was slightly incomplete: Item_ident_for_show::Item_ident_for_show() should copy not only collation from the field, but the entire Type_std_attributes (i.e. unsigned_flag, decimals and max_length). So I also fixed this, with some additional minor changes to avoid code duplication. This is an updated version. Please have a look. Thanks! > > On Tue, Dec 27, 2016 at 4:46 PM, Alexander Barkov <bar(a)mariadb.org > <mailto:bar@mariadb.org>> wrote: > > Hello Sanja, > > Can you please review a patch for MDEV-11672? > > Thanks! > >

1 0

Re: [Maria-developers] [Commits] 9416779: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 28 Dec '16

28 Dec '16

abc <sachin.setiya(a)mariadb.com> writes: > I have updates the patch, please have a look, > revision-id: 941677928aa40aa1f5abf981b74d2c3c80441459 (mariadb-galera-10.0.28-8-g9416779) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-27 15:40:28 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR The patch looks ok now. > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. > > Solution:- While applying write-set from server, we will generate values > for extra persistent columns. "write-set" is a Galera term. In replication, they are refered to as binlog events, or row events in the specific case of row-based replication events. > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--echo #SELECT quert > +--connection master > +select * from t1 where a > 2 and a < 4; > + > +--sync_slave_with_master > +select * from t1 where a > 2 and a < 4; You do not need to sync_slave_with_master for SELECT. > diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc > index df8036c..21b8a80 100644 > --- a/sql/rpl_record.cc > +++ b/sql/rpl_record.cc > @@ -408,6 +408,12 @@ unpack_row(rpl_group_info *rgi, > } > > /* > + Add Extra slave persistent columns > + */ > + if ((error= fill_extra_persistent_columns(table, cols->n_bits))) > + DBUG_RETURN(error); You moved this from write_row() to unpack_row(), so I assume this was to fix the similar bug for updates? - Kristian.

2 1

[Maria-developers] check_sql_mode
by jerome brauge 27 Dec '16

27 Dec '16

Hello, I'm trying to add an option to sql_mode. For this, I'm adding in sql_class.h : #define MODE_CONCAT_NULL_YIELDS_NULL (1ULL << 32) But now, my serveur doesn't start and show "Sysvar 'sql_mode' failed 'def_val <= my_set_bits(typelib.count)'" The problem seems to be in sys_vars.ic (Sys_var_set) : { option.var_type|= GET_SET; global_var(ulonglong)= def_val; SYSVAR_ASSERT(typelib.count > 0); SYSVAR_ASSERT(typelib.count <= 64); SYSVAR_ASSERT(def_val <= my_set_bits(typelib.count)); SYSVAR_ASSERT(size == sizeof(ulonglong)); } Function my_set_bits works on 32bit and typelib.count may be up to 64. In my case typelib.count is 33 and my_set_bits(33) returns 1, and the assert is false. What is my mistake ? Best regards, Jérôme.

3 6

[Maria-developers] Please review MDEV-11672 mysql_list_field() returns wrong default values for VIEW
by Alexander Barkov 27 Dec '16

27 Dec '16

Hello Sanja, Can you please review a patch for MDEV-11672? Thanks!

1 0

[Maria-developers] MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Sachin Setiya 27 Dec '16

27 Dec '16

Hi Kristian, I have updates the patch, please have a look, http://lists.askmonty.org/pipermail/commits/2016-December/010338.html. I also created a branch bb-10-0-gal-sachin , where I pushed this commit . Build bot status:- https://buildbot.askmonty.org/buildbot/grid?category=main&branch=bb-10.0-ga… -- Regards Sachin Setiya Software Engineer at MariaDB

1 0

Re: [Maria-developers] [Commits] b60fb6d: MDEV-11636 Extra persistent columns on slave always gets NULL in RBR
by Kristian Nielsen 22 Dec '16

22 Dec '16

sachin.setiya(a)mariadb.com writes: > revision-id: b60fb6daee2fcc2f433a50b5a5639065f5a46fe8 (mariadb-galera-10.0.28-8-gb60fb6d) > parent(s): be430b80df0cdd4eba32df1570195721dbfd1b39 > author: Sachin Setiya > committer: Sachin Setiya > timestamp: 2016-12-22 19:22:36 +0530 > message: > > MDEV-11636 Extra persistent columns on slave always gets NULL in RBR > > Problem:- In replication if slave has extra persistent column then these > column are not computed while applying write-set from master. Generally, when slave and master differs there will be situations where things will not fully work. However, in this case, the result is really corrupt data (a persistent column with wrong values), and I agree this should be fixed (and the patch looks simple). I am not familiar with the code for virtual columns. I assume you looked at the normal code path for virtual columns and made similar functionality for row-based replication applier. Your code looks reasonable to me. You add code in Rows_log_event::write_row(). What about update_row? It is not tested in the testcase, you should add test of the UPDATE case (and fix the code, if needed). The testcase needs more work, see comments inline. Getting replication tests to work reliably (not generate occasional spurious errors on buildbot etc.) is quite tricky (spoken from painful experience :-). Please push this to a feature tree and make sure the test passes on all builders before pushing to main. (A useful trick also is to run on your own machine something like: ./mysql-test-run.pl rpl.rpl_alter_extra_persistent --repeat=1000 to catch at least some timing-dependent errors). > diff --git a/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test b/mysql-test/suite/rpl/t/rpl_alter_extra_persistent.test > +--sync_slave_with_master > + > +connection slave; > +select * from t1; Use ORDER BY a (here and below), to avoid any risk of getting different output order. > +--connection master > +insert into t1 values(5); > +insert into t1 values(6); > + > +--connection slave > +sleep 2; Never use sleep in test cases. Use an appropriate mechanism to sync the slave to the master. > +select * from t1; > + > +--echo #Break Unique Constraint > +alter table t1 add column z4 int as (a % 6) persistent unique; > + > +--connection master > + > +--echo #entering duplicate value for slave persistent column > +insert into t1 values(7); > +select * from t1; > + > +--connection slave > +select * from t1; > +--query_vertical show slave status; This will not work reliably. You cannot put SHOW SLAVE STATUS output in .result, it will not be stable between test runs. And there is a race on whether the slave has time to hit the duplicate error. For this, use the existing include/wait_for_slave_sql_error.inc file, which can reliably detect a slave failure. See comments in the file and example usage in other test files. > + > +--connection master > +drop table t1; > + > +--connection slave > +alter table t1 drop column z4 ; > +start slave; > +drop table t1; This drop table should not be there (the drop table from the master will be replicated). > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 1cc58cd..a8f035b 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -11541,6 +11541,9 @@ Rows_log_event::write_row(rpl_group_info *rgi, > DBUG_RETURN(error); > } > > + if ((error= fill_extra_persistent_columns(table, &m_cols)) > + DBUG_RETURN(error); > + Is similar code needed for update_row? At least, this should be tested in the test case. - Kristian.

1 0

[Maria-developers] aria engine crash at recovery
by Roberto Spadim 22 Dec '16

22 Dec '16

hi guys i got a crash and aria engine don't startup, any help? 161222 03:41:13 mysqld_safe Starting mysqld daemon with databases from /home/mysql/data/local/ 2016-12-22 3:41:13 139878188566400 [Warning] 'THREAD_CONCURRENCY' is deprecated and will be removed in a future release. 2016-12-22 3:41:13 139878188566400 [Note] /usr/mariadb/mariadb/bin//mysqld (mysqld 10.1.5-MariaDB-log) starting as process 19217 ... 2016-12-22 3:41:13 139878188566400 [Note] mysqld: Aria engine: starting recovery recovered pages: 0% 3082852397% 0%161222 3:41:13 [ERROR] mysqld got signal 11 ; This could be because you hit a bug. It is also possible that this binary or one of the libraries it was linked against is corrupt, improperly built, or misconfigured. This error can also be caused by malfunctioning hardware. To report this bug, see http://kb.askmonty.org/en/reporting-bugs We will try our best to scrape up some info that will hopefully help diagnose the problem, but since we have already crashed, something is definitely wrong and this may fail. Server version: 10.1.5-MariaDB-log key_buffer_size=629145600 read_buffer_size=4194304 max_used_connections=0 max_threads=52 thread_count=0 It is possible that mysqld could use up to key_buffer_size + (read_buffer_size + sort_buffer_size)*max_threads = 1041422 K bytes of memory Hope that's ok; if not, decrease some variables in the equation. Thread pointer: 0x0x0 Attempting backtrace. You can use the following information to find out where mysqld died. If you see no messages after this, something went terribly wrong... stack_bottom = 0x0 thread_stack 0x48000 mysys/stacktrace.c:247(my_print_stacktrace)[0x7f37ee645b0b] sql/signal_handler.cc:153(handle_fatal_signal)[0x7f37ee1dfdb5] /lib64/libpthread.so.0(+0x10d00)[0x7f37ecd72d00] maria/ma_recovery.c:2142(exec_REDO_LOGREC_CLR_END)[0x7f37ee5683b7] maria/ma_recovery.c:588(display_and_apply_record)[0x7f37ee56985d] maria/ma_recovery.c:350(maria_apply_log)[0x7f37ee56d45c] maria/ma_recovery.c:242(maria_recovery_from_log)[0x7f37ee56df1e] maria/ha_maria.cc:3594(ha_maria_init)[0x7f37ee546c2d] sql/handler.cc:512(ha_initialize_handlerton(st_plugin_int*))[0x7f37ee1e2518] sql/sql_plugin.cc:1403(plugin_initialize)[0x7f37ee089e65] sql/sql_plugin.cc:1675(plugin_init(int*, char**, int))[0x7f37ee08add2] sql/mysqld.cc:5064(init_server_components)[0x7f37edfc37e8] sql/mysqld.cc:5659(mysqld_main(int, char**))[0x7f37edfc61ca] /lib64/libc.so.6(__libc_start_main+0xf0)[0x7f37ec3a79f0] /usr/mariadb/mariadb/bin//mysqld(+0x39dbad)[0x7f37edfbabad]

1 4

[Maria-developers] How correctly allocate memory to avoid problems with 2nd execution (instruction)
by Oleksandr Byelkin 22 Dec '16

22 Dec '16

Hi! It is for all who decide to write or already is writing something for server. There is 2 ways to execute query/statement: 1) conventional (one time) 2) with storing the statement and executing it many times (Prepared statements (PS)/ Stored Procedures(SP)) Also there is so called Query_arena, which is a MEM_ROOT and list of allocated Item-s in it. (we need list of items because they could require not trivial destruction/cleanup procedure before freeing memory, see cleanup() methods of Item-s) For conventional execution there is only one (run-time) query arena (part of THD in this case) where all memory allocated, and all Item-s linked. After execution Item-s cleaned up and memory freed. For case with stored statements (PS/SP) there is one Query_arena where Parsing result allocated (statement arena), and the other one so called run-time arena (which freed after execution as for conventional execution). During parsing statement arena is active (default), during all execution steps the run-time arena is active (if nothing changed it specially). So if some one decided allocate some memory during execution one should think if it is: I) only for execution II) should live with the statement All references on the memory allocated as case I) should be returned in the state as it was before execution or taken other measures to prevent using them during the next execution. All object which allocated as persistent (for statement of course) should be a) allocated only once (can be first execution or prepare phase, it does not matter, what does matter: do not make memory leak by allocating on each execution) b) reference on them should not be lost (because it supposed to be reused during next execution) c) procedure of allocation during run-time should looks like this: Query_arena *arena, backup; // backup run-time arena and set as default statement arena if needed // (if it is not conventional execution) arena= thd->activate_stmt_arena_if_needed(&backup);\ <MAKE ALLOOCATION HERE USING thd->mem_root> // return old default arena if it was changed before if (arena) thd->restore_active_arena(arena, &backup);

1 0

[Maria-developers] Fwd: Update of RPM layout
by Michal Schorm 21 Dec '16

21 Dec '16

Hello all, I went through an idea [1] to lighten the server package. I tested it, and I found out, I can save about 40MB of space, if I'd move all Perl utilites to a standalone subpackage. It would make sense to move them, so the users don't need to install Perl with the server package. Especially in technologies like containers (or cloud images) size matters. It all depends on how often are those utilities [2] used together with the server, sice we don't want to displace importatnt scripts. In case, it would look like a good idea, there is also option to move to this subpackage other - infrequently used - utilities. -> Q: Hey, Users and Admins, which utilities do you think are essential for the server? Note: It can be solved by soft dependencies, so those utils would be installed with the server by default. [1] https://bugzilla.redhat.com/show_bug.cgi?id=1205082 [2] mysql_find_rows mysqlaccess mysql_convert_table_format mysql_fix_extensions mysql_setpermission mysql_zap mysqldumpslow mysqld_multi mysqlhotcopy -- Michal Schorm Core Services - Databases Team mail: mschorm(a)redhat.com Brno-IRC: mschorm

1 0

[Maria-developers] Please review MDEV-10386 Assertion `fixed == 1' failed in virtual String* Item_func_conv_charset::val_str(String*)
by Alexander Barkov 21 Dec '16

21 Dec '16

Hello Sergei, Please review a patch for MDEV-10386. Thanks!

2 3

[Maria-developers] mysql_secure_installation documentation
by Michal Schorm 20 Dec '16

20 Dec '16

Hello all, I took care about bug against "mysql_secure_installation" script. [1] The problem is pretty simple. When you use "mysql_secure_installation" script, it will ingore all of yours current configuration. However, there are some options, that can be used. One of them is used to add your custom config file. It is documented, for example, here: [2] Unfortunatelly, the "man mysql_secure_installation" is quite short of infromation. (same as [3]) I believe, the information about options really should be added to both manpage and [3]. That should also solve the bug, without any [1] https://bugzilla.redhat.com/show_bug.cgi?id=1260821 [2] https://dev.mysql.com/doc/refman/5.7/en/mysql-secure-installation.html [3] https://mariadb.com/kb/en/mariadb/mysql_secure_installation/ -- I would like your general opinion about this. Based on it, I can file issue on JIRA. -- Michal Schorm Core Services - Databases Team mail: mschorm(a)redhat.com Brno-IRC: mschorm

1 0

[Maria-developers] Please review MDEV-11615 Split Item_hybrid_func::fix_attributes into virtual methods in Type_handler
by Alexander Barkov 20 Dec '16

20 Dec '16

Hello Sanja, Please review a patch for MDEV-11615. Thanks!

1 0

[Maria-developers] 10.3 branches
by Alexander Barkov 20 Dec '16

20 Dec '16

Hello Serg, all, A few weeks ago I created a new branch "10.3" and pushed essential patches needed for the compatibility project. It worked as follows: - In 10.3: git rebase origin/10.2 git push --force - In bb-10.2-compatibility: git rebase origin/10.3 git push --force Now as other developers start to push into 10.3, this won't work anymore. We briefly discussed with Monty that, as the compatibility project should be as stable as possible and merging all 10.3 patches into bb-10.2-compatibility is not desirable, we could try something like this: Add a new branch (Monty proposed "10.3-base" as the name) and propagate changes as follows: - In 10.3-base: git rebase origin/10.2 git push --force - In bb-10.2-compatibility git rebase origin/10.3-base git push --force - In 10.3: git merge origin/10.3-base git push (no --force) 10.2 ---(rebase)--->10.3-base--(rebase)-->bb-10.2-compatibility | | (merge) (merge) | | V | 10.3<-----------------/ Any objections about this scheme? I'd only propose to use "10.2-ext" instead of "10.3-base", as this intermediate branch is going to be more 10.2 than 10.3. Thanks.

3 6

Re: [Maria-developers] [Commits] 9f863a15b03: MDEV-11602 InnoDB leaks foreign key metadata on DDL operations
by Jan Lindström 19 Dec '16

19 Dec '16

ok to push. Most likely the double free was result of some other bug fixed later. R: Jan On Mon, Dec 19, 2016 at 4:00 PM, <marko.makela(a)mariadb.com> wrote: > revision-id: 9f863a15b037c0ddffc2d12c1f7d841466f9f345 (mariadb-10.0.28-27- > g9f863a15b03) > parent(s): eb4f2e063c341d9f3644339c68cb01679e782001 > author: Marko Mäkelä > committer: Marko Mäkelä > timestamp: 2016-12-19 15:57:41 +0200 > message: > > MDEV-11602 InnoDB leaks foreign key metadata on DDL operations > > Essentially revert MDEV-6759, which addressed a double free of memory > by removing the freeing altogether, introducing the memory leaks. > No double free was observed when running the test suite -DWITH_ASAN. > > Replace some mem_heap_free(foreign->heap) with dict_foreign_free(foreign) > so that the calls can be located and instrumented more easily when needed. > > --- > storage/innobase/dict/dict0dict.cc | 8 ++++---- > storage/innobase/dict/dict0load.cc | 2 +- > storage/xtradb/dict/dict0dict.cc | 8 ++++---- > storage/xtradb/dict/dict0load.cc | 2 +- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/storage/innobase/dict/dict0dict.cc b/storage/innobase/dict/ > dict0dict.cc > index f75d584e70d..2e40946224e 100644 > --- a/storage/innobase/dict/dict0dict.cc > +++ b/storage/innobase/dict/dict0dict.cc > @@ -1614,6 +1614,7 @@ struct dict_foreign_remove_partial > if (table != NULL) { > table->referenced_set.erase(foreign); > } > + dict_foreign_free(foreign); > } > }; > > @@ -3535,8 +3536,7 @@ dict_foreign_add_to_cache( > } > > if (for_in_cache) { > - /* Free the foreign object */ > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } else { > for_in_cache = foreign; > } > @@ -3564,7 +3564,7 @@ dict_foreign_add_to_cache( > " the ones in table."); > > if (for_in_cache == foreign) { > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > @@ -3620,7 +3620,7 @@ dict_foreign_add_to_cache( > be one */ > } > > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > diff --git a/storage/innobase/dict/dict0load.cc b/storage/innobase/dict/ > dict0load.cc > index e68bbe7d10a..77a31e5de63 100644 > --- a/storage/innobase/dict/dict0load.cc > +++ b/storage/innobase/dict/dict0load.cc > @@ -489,7 +489,7 @@ dict_process_sys_foreign_rec( > } > > /* This recieves a dict_foreign_t* that points to a stack variable. > - So mem_heap_free(foreign->heap) is not used as elsewhere. > + So dict_foreign_free(foreign) is not used as elsewhere. > Since the heap used here is freed elsewhere, foreign->heap > is not assigned. */ > foreign->id = mem_heap_strdupl(heap, (const char*) field, len); > diff --git a/storage/xtradb/dict/dict0dict.cc b/storage/xtradb/dict/ > dict0dict.cc > index 5339ecfaed6..3845138c8bf 100644 > --- a/storage/xtradb/dict/dict0dict.cc > +++ b/storage/xtradb/dict/dict0dict.cc > @@ -1614,6 +1614,7 @@ struct dict_foreign_remove_partial > if (table != NULL) { > table->referenced_set.erase(foreign); > } > + dict_foreign_free(foreign); > } > }; > > @@ -3539,8 +3540,7 @@ dict_foreign_add_to_cache( > } > > if (for_in_cache) { > - /* Free the foreign object */ > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } else { > for_in_cache = foreign; > } > @@ -3564,7 +3564,7 @@ dict_foreign_add_to_cache( > " the ones in table."); > > if (for_in_cache == foreign) { > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > @@ -3620,7 +3620,7 @@ dict_foreign_add_to_cache( > be one */ > } > > - mem_heap_free(foreign->heap); > + dict_foreign_free(foreign); > } > > return(DB_CANNOT_ADD_CONSTRAINT); > diff --git a/storage/xtradb/dict/dict0load.cc b/storage/xtradb/dict/ > dict0load.cc > index ca7de72c9b9..9ea4f4d873a 100644 > --- a/storage/xtradb/dict/dict0load.cc > +++ b/storage/xtradb/dict/dict0load.cc > @@ -491,7 +491,7 @@ dict_process_sys_foreign_rec( > } > > /* This recieves a dict_foreign_t* that points to a stack variable. > - So mem_heap_free(foreign->heap) is not used as elsewhere. > + So dict_foreign_free(foreign) is not used as elsewhere. > Since the heap used here is freed elsewhere, foreign->heap > is not assigned. */ > foreign->id = mem_heap_strdupl(heap, (const char*) field, len); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 8bfc350: MDEV-10148: Database crashes in the query to the View
by Oleksandr Byelkin 19 Dec '16

19 Dec '16

Hi, Sergei! On 12/16/2016 03:16 PM, Sergei Petrunia wrote: > revision-id: 8bfc35041195b46c8f4666dd02f600e878f45643 > parent(s): 96d2e0480ffbc36c803db5a7d5bcefd3470c5f72 > committer: Sergei Petrunia > branch nick: 5.5 > timestamp: 2016-12-16 17:16:02 +0300 > message: > > MDEV-10148: Database crashes in the query to the View > > Fix st_select_lex::is_merged_child_of to work across merged views or > derived tables. > OK to push! [skip]

1 0

[Maria-developers] Please review MDEV-10524 Assertion `arg1_int >= 0' failed in Item_func_additive_op::result_precision()
by Alexander Barkov 19 Dec '16

19 Dec '16

Hello Sergei, Please review a patch for MDEV-10524. Thanks!

2 2

Re: [Maria-developers] [Commits] 12034ae: MDEV-10927: Crash When Using sort_union Optimization
by Sergey Petrunia 16 Dec '16

16 Dec '16

The patch is ok, but there is a problem with the testcase: when I run the testcase on the unmodified 5.5 binary, it passes. When I run the tescase from the MDEV, I get the crash. Please fix the testcase. This will also provide test coverage (which was lacking, and this is why have this bug in the first place). On Fri, Dec 16, 2016 at 11:19:43AM +0530, Varun wrote: > revision-id: 12034aec831d776725a43c7537ab4256efb1c974 (mariadb-5.5.53-25-g12034ae) > parent(s): 14e1f32894cdbe63a614738cfd93e9d0818dedee > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-16 11:03:24 +0530 > message: > > MDEV-10927: Crash When Using sort_union Optimization > > In file sql/filesort.cc,when merge_buffers() is called then > - queue_remove(&queue,0) is called > - For the function queue_remove there is assertion states that the element to be removed should have index >=1 > - this is causing the assertion to fail. > > Fixed by removing the top element. > > --- > mysql-test/r/index_merge_innodb.result | 52 ++++++++++++++++++++++++++++++++++ > mysql-test/t/index_merge_innodb.test | 42 ++++++++++++++++++++++++++- > sql/filesort.cc | 2 +- > 3 files changed, 94 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/r/index_merge_innodb.result b/mysql-test/r/index_merge_innodb.result > index b93d15f..49e0a57 100644 > --- a/mysql-test/r/index_merge_innodb.result > +++ b/mysql-test/r/index_merge_innodb.result > @@ -792,4 +792,56 @@ a b c > 8 m m > 9 d d > DROP TABLE t1; > +# > +# MDEV-10927: Crash When Using sort_union Optimization > +# > +SET optimizer_switch='index_merge_sort_intersection=on'; > +CREATE TABLE IF NOT EXISTS t1 ( > +id int(11) NOT NULL AUTO_INCREMENT, > +contact_id int(11) NOT NULL, > +role_id int(11) NOT NULL, > +company_type_id int(11) NOT NULL, > +company_id int(11) NOT NULL, > +team_id int(11) NOT NULL, > +user_id int(11) NOT NULL, > +PRIMARY KEY (id), > +KEY contact_id (contact_id), > +KEY user_id (user_id), > +KEY company_id (company_id), > +KEY team_id (team_id) USING BTREE > +) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > +(5343408, 23913609, 0, 6, 2321, 0, 0), > +(7841449, 39582328, 1194, 0, 0, 0, 57291), > +(7841507, 28707292, 1194, 0, 0, 0, 57291), > +(7841543, 26376778, 1194, 0, 0, 0, 137571), > +(7841563, 42475686, 1194, 0, 0, 0, 137571), > +(7841573, 35956831, 1194, 0, 0, 0, 447223), > +(7841575, 37975231, 1194, 0, 0, 0, 447223), > +(7843049, 42693923, 1194, 0, 0, 0, 376261), > +(7843600, 42696877, 1194, 0, 0, 0, 376261), > +(12858344, 82851915, 1194, 0, 0, 0, 993011), > +(12858554, 80183799, 1194, 0, 0, 0, 993011), > +(12858803, 82649193, 1194, 0, 0, 0, 1288749), > +(12858878, 82700925, 1194, 0, 0, 0, 1288749), > +(12860248, 28642729, 1194, 0, 0, 0, 1234288), > +(12860258, 28642729, 0, 0, 0, 0, 1234288), > +(12860259, 28642729, 1194, 0, 0, 0, 0); > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +contact_id > +23913609 > +26376778 > +28642729 > +28707292 > +35956831 > +37975231 > +39582328 > +42475686 > +42693923 > +42696877 > +80183799 > +82649193 > +82700925 > +82851915 > +DROP TABLE t1; > set optimizer_switch= @optimizer_switch_save; > diff --git a/mysql-test/t/index_merge_innodb.test b/mysql-test/t/index_merge_innodb.test > index 6a1cb53..f1fdbbd 100644 > --- a/mysql-test/t/index_merge_innodb.test > +++ b/mysql-test/t/index_merge_innodb.test > @@ -172,5 +172,45 @@ WHERE ( tb.b != ta.b OR tb.a = ta.a ) > > DROP TABLE t1; > > -set optimizer_switch= @optimizer_switch_save; > +--echo # > +--echo # MDEV-10927: Crash When Using sort_union Optimization > +--echo # > > +SET optimizer_switch='index_merge_sort_intersection=on'; > + > +CREATE TABLE IF NOT EXISTS t1 ( > + id int(11) NOT NULL AUTO_INCREMENT, > + contact_id int(11) NOT NULL, > + role_id int(11) NOT NULL, > + company_type_id int(11) NOT NULL, > + company_id int(11) NOT NULL, > + team_id int(11) NOT NULL, > + user_id int(11) NOT NULL, > + PRIMARY KEY (id), > + KEY contact_id (contact_id), > + KEY user_id (user_id), > + KEY company_id (company_id), > + KEY team_id (team_id) USING BTREE > + ) ENGINE=InnoDB AUTO_INCREMENT=12860259 DEFAULT CHARSET=utf8 ROW_FORMAT=COMPACT; > + > +INSERT INTO t1 (id, contact_id, role_id, company_type_id, company_id, team_id, user_id) VALUES > + (5343408, 23913609, 0, 6, 2321, 0, 0), > + (7841449, 39582328, 1194, 0, 0, 0, 57291), > + (7841507, 28707292, 1194, 0, 0, 0, 57291), > + (7841543, 26376778, 1194, 0, 0, 0, 137571), > + (7841563, 42475686, 1194, 0, 0, 0, 137571), > + (7841573, 35956831, 1194, 0, 0, 0, 447223), > + (7841575, 37975231, 1194, 0, 0, 0, 447223), > + (7843049, 42693923, 1194, 0, 0, 0, 376261), > + (7843600, 42696877, 1194, 0, 0, 0, 376261), > + (12858344, 82851915, 1194, 0, 0, 0, 993011), > + (12858554, 80183799, 1194, 0, 0, 0, 993011), > + (12858803, 82649193, 1194, 0, 0, 0, 1288749), > + (12858878, 82700925, 1194, 0, 0, 0, 1288749), > + (12860248, 28642729, 1194, 0, 0, 0, 1234288), > + (12860258, 28642729, 0, 0, 0, 0, 1234288), > + (12860259, 28642729, 1194, 0, 0, 0, 0); > +--sorted_result > +SELECT SQL_NO_CACHE contact_id FROM t1 FORCE INDEX (user_id,company_id) WHERE user_id = '57291' OR (user_id IN(55838,55839,56998,57260,57291,60227,121997,137571,173600,219233,306480,354991,358594,376261,398572,447223,472009,646282,932548,993011,1004609,1021262,1177117,1234288,1283121,1288749,1337771,57291) AND role_id = '1194') OR company_id = '2321'; > +DROP TABLE t1; > +set optimizer_switch= @optimizer_switch_save; > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 5bb5c64..38404b0 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -1411,7 +1411,7 @@ int merge_buffers(SORTPARAM *param, IO_CACHE *from_file, > if (!(error= (int) read_to_buffer(from_file, buffpek, > rec_length))) > { > - queue_remove(&queue,0); > + (void) queue_remove_top(&queue); > reuse_freed_buff(&queue, buffpek, rec_length); > } > else if (error == -1) BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MDEV-11558 Split Item_type_holder::display_length into virtual methods in Type_handler
by Alexander Barkov 16 Dec '16

16 Dec '16

Hello Nirbhay, Can you please review a patch for MDEV-11558? Thanks!

2 1

Re: [Maria-developers] [Commits] 5f02c66: MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES
by Sergei Golubchik 15 Dec '16

15 Dec '16

Hi, Sergey! On Dec 13, Sergey Vojtovich wrote: > revision-id: 5f02c661f3d30baf08071acad29cef40fad0739c (mariadb-10.2.2-225-g5f02c66) > parent(s): 65b4d7457e40ee25302d2df28b0d200ff59d9e6d > committer: Sergey Vojtovich > timestamp: 2016-12-13 16:17:49 +0400 > message: > > MDEV-11227 - mysqlimport -l doesn't issue UNLOCK TABLES > > Implementation of MDEV-7660 introduced unwanted incompatible change: > modifications under LOCK TABLES with autocommit enabled are rolled back on > disconnect. Previously everything was committed, because LOCK TABLES didn't > adjust autocommit setting. Thanks, very helpful comment (that I didn't quote in my reply for brevity)! Few questions below: > diff --git a/mysql-test/t/delayed.test b/mysql-test/t/delayed.test > index dea16c8..30fb694 100644 > --- a/mysql-test/t/delayed.test > +++ b/mysql-test/t/delayed.test > @@ -319,7 +319,7 @@ drop table if exists t1; > --enable_warnings > create table t1 (a int, b int); > insert into t1 values (1,1); > -lock table t1 read; > +lock table t1 read local; why? > connect (update,localhost,root,,); > connection update; > --send insert delayed into t1 values (2,2); > diff --git a/sql/mdl.h b/sql/mdl.h > index 7d659af..d77c07c 100644 > --- a/sql/mdl.h > +++ b/sql/mdl.h > @@ -196,6 +196,12 @@ enum enum_mdl_type { > */ > MDL_SHARED_UPGRADABLE, > /* > + A shared metadata lock for cases when we need to read data from table > + and block all concurrent modifications to it (for both data and metadata). > + Used by LOCK TABLES READ statement. > + */ > + MDL_SHARED_READ_ONLY, Hmm, stronger than MDL_SHARED_WRITE? All read locks were before write locks until now. Why is that changed? > + /* > An upgradable shared metadata lock which blocks all attempts to update > table data, allowing reads. > A connection holding this kind of lock can read table metadata and read > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index 1ca7e9c..d3f4517 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) > to general/slow log would be disabled in read only transactions. > */ > if (table_list->mdl_request.type >= MDL_SHARED_WRITE && > + table_list->mdl_request.type != MDL_SHARED_READ_ONLY && if you're going to keep read and write locks mixed, then I'd suggest to move this condition into a small inline helper, like table_list->mdl_request.is_write_lock(). it's used few times already in this very file and there will be only one place to change if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO > thd->tx_read_only && > !(flags & (MYSQL_LOCK_LOG_TABLE | MYSQL_OPEN_HAS_MDL_LOCK))) > { > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index 6dc9192..0d740bb 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2728,27 +2728,76 @@ bool sp_process_definer(THD *thd) > static bool lock_tables_open_and_lock_tables(THD *thd, TABLE_LIST *tables) > { > Lock_tables_prelocking_strategy lock_tables_prelocking_strategy; > + MDL_deadlock_and_lock_abort_error_handler deadlock_handler; > + MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); > uint counter; > TABLE_LIST *table; > > thd->in_lock_tables= 1; > > +retry: > + > if (open_tables(thd, &tables, &counter, 0, &lock_tables_prelocking_strategy)) > goto err; > > - /* > - We allow to change temporary tables even if they were locked for read > - by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > - TABLES time and by the statement which is later executed under LOCK TABLES > - we ensure that for temporary tables we always request a write lock (such > - discrepancy can cause problems for the storage engine). > - We don't set TABLE_LIST::lock_type in this case as this might result in > - extra warnings from THD::decide_logging_format() even though binary logging > - is totally irrelevant for LOCK TABLES. > - */ > for (table= tables; table; table= table->next_global) > - if (!table->placeholder() && table->table->s->tmp_table) > - table->table->reginfo.lock_type= TL_WRITE; > + { > + if (!table->placeholder()) > + { > + if (table->table->s->tmp_table) > + { > + /* > + We allow to change temporary tables even if they were locked for read > + by LOCK TABLES. To avoid a discrepancy between lock acquired at LOCK > + TABLES time and by the statement which is later executed under LOCK > + TABLES we ensure that for temporary tables we always request a write > + lock (such discrepancy can cause problems for the storage engine). > + We don't set TABLE_LIST::lock_type in this case as this might result > + in extra warnings from THD::decide_logging_format() even though > + binary logging is totally irrelevant for LOCK TABLES. > + */ > + table->table->reginfo.lock_type= TL_WRITE; > + } > + else if (table->mdl_request.type == MDL_SHARED_READ && > + ! table->prelocking_placeholder && > + table->table->file->lock_count() == 0) > + { > + /* > + In case when LOCK TABLE ... READ LOCAL was issued for table with > + storage engine which doesn't support READ LOCAL option and doesn't > + use THR_LOCK locks we need to upgrade weak SR metadata lock acquired > + in open_tables() to stronger SRO metadata lock. > + This is not needed for tables used through stored routines or > + triggers as we always acquire SRO (or even stronger SNRW) metadata > + lock for them. > + */ > + deadlock_handler.init(); > + thd->push_internal_handler(&deadlock_handler); > + > + bool result= thd->mdl_context.upgrade_shared_lock( > + table->table->mdl_ticket, > + MDL_SHARED_READ_ONLY, > + thd->variables.lock_wait_timeout); > + > + thd->pop_internal_handler(); > + > + if (deadlock_handler.need_reopen()) > + { > + /* > + Deadlock occurred during upgrade of metadata lock. > + Let us restart acquring and opening tables for LOCK TABLES. will you use MDL_SHARED_READ_ONLY right away when retrying? or it'll do the upgrade thing again? > + */ > + close_tables_for_reopen(thd, &tables, mdl_savepoint); > + if (thd->open_temporary_tables(tables)) > + goto err; > + goto retry; > + } > + > + if (result) > + goto err; > + } > + } > + } > > if (lock_tables(thd, tables, counter, 0) || > thd->locked_tables_list.init_locked_tables(thd)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

[Maria-developers] MDEV-11514, MDEV-11497, MDEV-11554, MDEV-11555 - IN and CASE type aggregation problems
by Alexander Barkov 15 Dec '16

15 Dec '16

Hello Sanja, Please review a patch fixing a number of problems in IN and CASE. This is a per-requisite for further Type_handler related work, to clean-up the code before moving it to more IN and CASE related methods in Type_handler. Thanks!

1 0

Re: [Maria-developers] [Commits] bbc52df: MDEV-11490 Galera_3nodes test suite does not suppress Warnings.
by Nirbhay Choubey 14 Dec '16

14 Dec '16

Hi Sachin, I see that you have added a lot of warnings to the suite-level suppression list. Does *all* the new warnings show up for all the tests? I'd only add ones that make sense for the entire suite, and add the individuals to the specific test by calling mtr.add_suppression("<pattern>"). Ping me on slack if you need to discuss this further. Best, Nirbhay On Tue, Dec 6, 2016 at 1:53 AM, <sachin.setiya(a)mariadb.com> wrote: > revision-id: bbc52df39b7dedca50943a7cd99ee80b9e2f5b12 > (mariadb-10.1.19-24-gbbc52df) > parent(s): 611f91605adce17df87acf96b5aede0b73d0fc12 > author: SachinSetiya > committer: SachinSetiya > timestamp: 2016-12-06 12:21:16 +0530 > message: > > MDEV-11490 Galera_3nodes test suite does not suppress Warnings. > > Problem:- While running individual tests of Galera_3nodes , > We get warnings like '[Warning] WSREP: Could not open state file > for reading: '. And because of this individual tests fails. > > Solution:- We change suite.pm of Galera_3nodes to supress these warnings. > > --- > mysql-test/suite/galera_3nodes/suite.pm | 34 > ++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/mysql-test/suite/galera_3nodes/suite.pm > b/mysql-test/suite/galera_3nodes/suite.pm > index 39d5acb..74f7dfa 100644 > --- a/mysql-test/suite/galera_3nodes/suite.pm > +++ b/mysql-test/suite/galera_3nodes/suite.pm > @@ -25,7 +25,8 @@ return "No my_print_defaults" unless $epath; > push @::global_suppressions, > ( > qr(WSREP: wsrep_sst_receive_address is set to '127.0.0.1), > - qr(WSREP: Could not open saved state file for reading: ), > + qr(WSREP: Could not open saved state file for reading: .*), > + qr(WSREP: Could not open state file for reading: .*), > qr(WSREP: Gap in state sequence. Need state transfer.), > qr(WSREP: Failed to prepare for incremental state transfer:), > qr(WSREP:.*down context.*), > @@ -33,16 +34,39 @@ push @::global_suppressions, > qr(WSREP: last inactive check more than .* skipping check), > qr(WSREP: SQL statement was ineffective), > qr(WSREP: Releasing seqno [0-9]* before [0-9]* was assigned.), > - qr|WSREP: access file$gvwstate.dat$ failed$No such file or > directory$|, > + qr|WSREP: access file$.*gvwstate.dat$ failed$No such file or > directory$|, > qr(WSREP: Quorum: No node with complete state), > qr(WSREP: Initial position was provided by configuration or SST, > avoiding override), > qr|WSREP: discarding established $time wait$ .*|, > qr(WSREP: There are no nodes in the same segment that will ever be > able to become donors, yet there is a suitable donor outside. Will use that > one.), > qr(WSREP: evs::proto.*), > - qr|WSREP: Ignoring possible split-brain (allowed by configuration) > from view:.*|, > + qr|WSREP: Ignoring possible split-brain $allowed by configuration$ > from view:.*|, > + qr(WSREP: no nodes coming from prim view, prim not possible), > + qr(WSREP: Member .* requested state transfer from .* but it is > impossible to select State Transfer donor: Resource temporarily > unavailable), > + qr(WSREP: user message in state LEAVING), > + qr(WSREP: .* sending install message failed: Transport endpoint is > not connected), > + qr(WSREP: .* sending install message failed: Resource temporarily > unavailable), > + qr(WSREP: Maximum writeset size exceeded by .*), > + qr(WSREP: transaction size exceeded.*), > + qr(WSREP: RBR event .*), > + qr(WSREP: Ignoring error for TO isolated action: .*), > + qr(WSREP: transaction size limit .*), > + qr(WSREP: rbr write fail, .*), > + qr(WSREP: .*Backend not supported: foo.*), > + qr(WSREP: .*Failed to initialize backend using .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Socket type not supported), > + qr(WSREP: failed to open gcomm backend connection: 110: failed to > reach primary view: 110 .*), > + qr(WSREP: .*Failed to open backend connection: -110 .*), > + qr(WSREP: .*Failed to open channel 'my_wsrep_cluster' at .*), > + qr(WSREP: gcs connect failed: Connection timed out), > + qr|WSREP: wsrep::connect$.*$ failed: 7|, > + qr(WSREP: SYNC message from member .* in non-primary configuration. > Ignored.), > qr(WSREP: Could not find peer:), > - qr(WSREP: Protocol violation. JOIN message sender .*), > - qr(WSREP: JOIN message from member [0-9]* in non-primary > configuration. Ignored.), > + qr(WSREP: TO isolation failed for: .*), > + qr|WSREP: gcs_caused returned .*|, > + qr|WSREP: Protocol violation. JOIN message sender .* is not in state > transfer $SYNCED$. Message ignored.|, > + qr(WSREP: Action message in non-primary configuration from member > [0-9]*), > ); > > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

Re: [Maria-developers] [Commits] 9bd354e: MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed
by Sergey Petrunia 13 Dec '16

13 Dec '16

Hi Varun, Please find a small note below. Ok to push after it has been addressed. On Tue, Dec 13, 2016 at 05:09:58AM +0530, Varun wrote: > revision-id: 9bd354ef91ae81e0f997f29acdae4f8a2997b3c8 (mariadb-10.1.19-40-g9bd354e) > parent(s): 67b570af506a42f14a8ae30603d52ca9de68cce5 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2016-12-13 05:07:02 +0530 > message: > > MDEV-11060 sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > > In file sql/opt_range.cc,when calculate_cond_selectivity_for_table() is called with optimizer_use_condition_selectivity=4 then > - thd->no_errors is set to 1 > - the original value of thd->no_error is not restored to its original value > - this is causing the assertion to fail in the subsequent queries > > Fixed by restoring the original value of thd->no_errors > > --- > mysql-test/r/selectivity_innodb.result | 23 +++++++++++++++++++++++ > mysql-test/t/selectivity_innodb.test | 28 ++++++++++++++++++++++++++++ > sql/opt_range.cc | 1 + > 3 files changed, 52 insertions(+) > > diff --git a/mysql-test/r/selectivity_innodb.result b/mysql-test/r/selectivity_innodb.result > index 4355812..b57873d 100644 > --- a/mysql-test/r/selectivity_innodb.result > +++ b/mysql-test/r/selectivity_innodb.result > @@ -1670,6 +1670,29 @@ DROP TABLE t1,t2; > # > # End of 10.0 tests > # > +# > +# MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +# > +set optimizer_use_condition_selectivity=4; > +drop table if exists t1, t2; > +Warnings: > +Note 1051 Unknown table 'test.t1' > +Note 1051 Unknown table 'test.t2' > +drop view if exists v1; > +Warnings: > +Note 1051 Unknown table 'test.v1' Please remove 'DROP TABLE IF EXISTS' statements as they are not necessary. The previous test ends with DROP TABLE commands for tables t1, t2. There is no DROP VIEW command, so it is useful that the VIEW is dropped before it is used (just in case the test is run against a server that already has the view). Check out other test files, most of them start with lines like: --disable_warnings drop table if exists t1,t2; --enable_warnings Please also suppress warnings for the DROP VIEW statement. > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > +create view v1 as select * from t1, t2 where d=2; > +insert v1 (a,c) values (NULL, 20); > +ERROR 23000: Column 'a' cannot be null > +drop table t1,t2; > +drop view v1; > +# > +# End of 10.1 tests > +# > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/mysql-test/t/selectivity_innodb.test b/mysql-test/t/selectivity_innodb.test > index 25aa0ab..bb87106 100644 > --- a/mysql-test/t/selectivity_innodb.test > +++ b/mysql-test/t/selectivity_innodb.test > @@ -138,6 +138,34 @@ DROP TABLE t1,t2; > --echo # End of 10.0 tests > --echo # > > + > +--echo # > +--echo # MDEV-11060: sql/protocol.cc:532: void Protocol::end_statement(): Assertion `0' failed > +--echo # > + > + > +set optimizer_use_condition_selectivity=4; > + > +drop table if exists t1, t2; > +drop view if exists v1; > + > +create table t1 (a int not null, b int, c int) engine=InnoDB; > +create trigger trgi before insert on t1 for each row set new.a=if(new.a is null,new.b,new.c); > + > +create table t2 (d int, e int) engine=InnoDB; > +update t1, t2 set a=NULL, b=2, c=NULL where b=d and e=200; > + > +create view v1 as select * from t1, t2 where d=2; > +--error ER_BAD_NULL_ERROR > +insert v1 (a,c) values (NULL, 20); > + > +drop table t1,t2; > +drop view v1; > + > +--echo # > +--echo # End of 10.1 tests > +--echo # > + > set use_stat_tables= @tmp_ust; > set optimizer_use_condition_selectivity= @tmp_oucs; > SET SESSION STORAGE_ENGINE=DEFAULT; > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > index bed57f3..0577ef0 100644 > --- a/sql/opt_range.cc > +++ b/sql/opt_range.cc > @@ -3112,6 +3112,7 @@ bool calculate_cond_selectivity_for_table(THD *thd, TABLE *table, Item **cond) > } > > free_alloc: > + thd->no_errors= 0; > thd->mem_root= param.old_root; > free_root(&alloc, MYF(0)); > > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

Re: [Maria-developers] [Commits] 27af988: MDEV-11479 Improved wsrep_dirty_reads
by Vicențiu Ciorbaru 13 Dec '16

13 Dec '16

Hi Sachin! Comments inline: On Sun, 4 Dec 2016 at 08:36 <sachin.setiya(a)mariadb.com> wrote: > revision-id: 27af98805c27c8e0c41dc8c19bf80aefff4c6d3d > (mariadb-galera-5.5.53-3-g27af988) > parent(s): 72fd15f7c31aa3e3705ae1b005a3247a985c5bb3 > author: SachinSetiya > committer: SachinSetiya > NIT: To get these to display correctly you should have a space between Sachin and Setiya. > timestamp: 2016-12-04 12:05:05 +0530 > message: > > MDEV-11479 Improved wsrep_dirty_reads > > Tasks:- > Changes in wsrep_dirty_reads variable > 1.) Global + Session scope (Current: session-only) > 2.) Allow all commands that do not change data (besides SELECT) > 3.) Allow prepared Statements that do not change data > 4.) Works with wsrep_sync_wait enabled > You also seemed to have made the variable available as a command line option. Please specify that as well. > --- > .../suite/galera/r/galera_var_dirty_reads.result | 29 > +++++++++++++++++++--- > .../suite/galera/t/galera_var_dirty_reads.test | 25 > ++++++++++++++++++- > .../sys_vars/r/wsrep_dirty_reads_basic.result | 19 ++++++++++++-- > .../suite/sys_vars/t/wsrep_dirty_reads_basic.test | 13 ++++++++-- > sql/sql_parse.cc | 18 ++++++++++---- > sql/sys_vars.cc | 4 +-- > sql/wsrep_mysqld.cc | 3 ++- > sql/wsrep_mysqld.h | 1 + > 8 files changed, 96 insertions(+), 16 deletions(-) > > diff --git a/mysql-test/suite/galera/r/galera_var_dirty_reads.result > b/mysql-test/suite/galera/r/galera_var_dirty_reads.result > index 6a2aa1e..d009c1b 100644 > --- a/mysql-test/suite/galera/r/galera_var_dirty_reads.result > +++ b/mysql-test/suite/galera/r/galera_var_dirty_reads.result > @@ -28,9 +28,32 @@ SELECT @@max_allowed_packet; > SELECT 2+2 from DUAL; > 2+2 > 4 > -SELECT sysdate() from DUAL; > -sysdate() > -2016-10-28 23:13:06 > +SET @@session.wsrep_dirty_reads=ON; > +SELECT * FROM t1; > +i > +1 > +prepare stmt_show from 'select 1'; > +prepare stmt_select from 'select * from t1'; > +prepare stmt_insert from 'insert into t1 values(1)'; > +execute stmt_show; > +1 > +1 > +execute stmt_select; > +i > +1 > +execute stmt_insert; > +ERROR 08S01: WSREP has not yet prepared node for application use > +select * from t1; > +i > +1 > +execute stmt_show; > +1 > +1 > +execute stmt_select; > +i > +1 > +execute stmt_insert; > +ERROR 08S01: WSREP has not yet prepared node for application use > SELECT * FROM t1; > i > 1 > You're not really testing the global aspect of the variable here. You're only testing the sesssion behaviour of it. Also, test with it both ON and OFF (when setting it globally). Here's a sample that tests both session and global behaviour: --enable_connect_log SELECT @@expensive_subquery_limit; SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; create user user1; create user user2; --connect (con1, localhost, user1,,) SELECT @@expensive_subquery_limit; SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; connection default; SELECT @@expensive_subquery_limit; SET @@global.expensive_subquery_limit=400; SELECT @@expensive_subquery_limit; --connect (con2, localhost, user1,,) SELECT @@expensive_subquery_limit; connection default; drop user user1; drop user user2; ************************************ SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 100 SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 create user user1; create user user2; connect con1, localhost, user1,,; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 100 SET @@session.expensive_subquery_limit=300; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 connection default; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 SET @@global.expensive_subquery_limit=400; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 300 connect con2, localhost, user1,,; SELECT @@expensive_subquery_limit; @@expensive_subquery_limit 400 connection default; drop user user1; drop user user2; Notice how the last connection has the default value 400 as that's the global value. For your use case, you'd also have to execute the prepared statements to see if it works (execute select_stmt). > diff --git a/mysql-test/suite/galera/t/galera_var_dirty_reads.test > b/mysql-test/suite/galera/t/galera_var_dirty_reads.test > index dd7bc88..44931aa 100644 > --- a/mysql-test/suite/galera/t/galera_var_dirty_reads.test > +++ b/mysql-test/suite/galera/t/galera_var_dirty_reads.test > @@ -44,7 +44,30 @@ SET @@session.wsrep_dirty_reads=OFF; > SELECT 2; > SELECT @@max_allowed_packet; > SELECT 2+2 from DUAL; > -SELECT sysdate() from DUAL; > + > +#Query which does not change data should be allowed MDEV-11479 > +SET @@session.wsrep_dirty_reads=ON; > +SELECT * FROM t1; > + > +#Prepared statement creation should be allowed MDEV-11479 > +prepare stmt_show from 'select 1'; > +prepare stmt_select from 'select * from t1'; > +prepare stmt_insert from 'insert into t1 values(1)'; > + > +#Only prepare statement which does not change data should be allowed > +execute stmt_show; > +execute stmt_select; > +--error ER_UNKNOWN_COM_ERROR > +execute stmt_insert; + > +#wsrep_dirty_read should work when wsrep_sync_wait is 1 or non zero > +#because we already are disconnected , So It does not make any sense > +#to wait for other nodes > I don't think you're testing this correctly. See my comment on the if statement which is supposed to govern this. > +select * from t1; > +execute stmt_show; > +execute stmt_select; > +--error ER_UNKNOWN_COM_ERROR > +execute stmt_insert; > > --disable_query_log > --eval SET @@global.wsrep_cluster_address = '$wsrep_cluster_address_saved' > diff --git a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > index d2a62d6..649ac83 100644 > --- a/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > +++ b/mysql-test/suite/sys_vars/r/wsrep_dirty_reads_basic.result > @@ -5,12 +5,13 @@ > SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads; > # default > SELECT @@global.wsrep_dirty_reads; > -ERROR HY000: Variable 'wsrep_dirty_reads' is a SESSION variable > +@@global.wsrep_dirty_reads > +0 > SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > 0 > > -# scope and valid values > +# valid values for session > SET @@session.wsrep_dirty_reads=OFF; > SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > @@ -24,6 +25,20 @@ SELECT @@session.wsrep_dirty_reads; > @@session.wsrep_dirty_reads > 0 > > +# valid values for global > +SET @@global.wsrep_dirty_reads=OFF; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +0 > +SET @@global.wsrep_dirty_reads=ON; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +1 > +SET @@global.wsrep_dirty_reads=default; > +SELECT @@global.wsrep_dirty_reads; > +@@global.wsrep_dirty_reads > +0 > + > # invalid values > SET @@session.wsrep_dirty_reads=NULL; > ERROR 42000: Variable 'wsrep_dirty_reads' can't be set to the value of > 'NULL' > Test invalid values for global scope as well. It should not change obviously but since we're writing tests, might as well do that too. > diff --git a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > index a47524f..000c4c4 100644 > --- a/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > +++ b/mysql-test/suite/sys_vars/t/wsrep_dirty_reads_basic.test > @@ -8,12 +8,12 @@ > SET @wsrep_dirty_reads_session_saved = @@session.wsrep_dirty_reads; > > --echo # default > ---error ER_INCORRECT_GLOBAL_LOCAL_VAR > + > SELECT @@global.wsrep_dirty_reads; > SELECT @@session.wsrep_dirty_reads; > > --echo > ---echo # scope and valid values > +--echo # valid values for session > SET @@session.wsrep_dirty_reads=OFF; > SELECT @@session.wsrep_dirty_reads; > SET @@session.wsrep_dirty_reads=ON; > @@ -22,6 +22,15 @@ SET @@session.wsrep_dirty_reads=default; > SELECT @@session.wsrep_dirty_reads; > > --echo > +--echo # valid values for global > +SET @@global.wsrep_dirty_reads=OFF; > +SELECT @@global.wsrep_dirty_reads; > +SET @@global.wsrep_dirty_reads=ON; > +SELECT @@global.wsrep_dirty_reads; > +SET @@global.wsrep_dirty_reads=default; > +SELECT @@global.wsrep_dirty_reads; > + > +--echo > --echo # invalid values > --error ER_WRONG_VALUE_FOR_VAR > SET @@session.wsrep_dirty_reads=NULL; > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc > index eb4f714..a56d694 100644 > --- a/sql/sql_parse.cc > +++ b/sql/sql_parse.cc > @@ -2393,10 +2393,11 @@ mysql_execute_command(THD *thd) > */ > if (thd->variables.wsrep_on && !thd->wsrep_applier && !wsrep_ready && > lex->sql_command != SQLCOM_SET_OPTION && > - !(thd->variables.wsrep_dirty_reads && > - lex->sql_command == SQLCOM_SELECT) && > - !(lex->sql_command == SQLCOM_SELECT && > - !all_tables) && > + !(thd->variables.wsrep_dirty_reads && > + (sql_command_flags[lex->sql_command] & > + CF_CHANGES_DATA) == 0) && > Put this line on one line only, the single & is confusing, or better, turn it into a small inline function, such as command_changes_data() > + !(lex->sql_command == SQLCOM_SELECT && > + !all_tables) && > Can't tell for certain but this condition might not be necessary if you have the previous one. What happens when you have a stored procedure that inserts into a table? Does the CF_CHANGES_DATA flag filter out the query or not? ex: select function_with_side_efect(1); This seems like a good test case that I'm not sure is tested for this variable anyway. Please try it. > !wsrep_is_show_query(lex->sql_command)) > { > #if DIRTY_HACK > @@ -2512,7 +2513,14 @@ mysql_execute_command(THD *thd) > case SQLCOM_SHOW_INDEX_STATS: > case SQLCOM_SELECT: > #ifdef WITH_WSREP > - if (WSREP_CLIENT(thd) && wsrep_sync_wait(thd)) goto error; > + /* > + wsrep_dirty_read should work when wsrep_sync_wait is 1 or non-zero > + because we already are disconnected. So, it does not make any sense > + to wait for other nodes > + */ > > This here is not correct. This will not execute the select if wsrep_dirty_reads is true and wsrep_ready is true and wsrep_sync_wait(thd) is non-zero. if (WSREP_CLIENT(thd) && thd->variables.wsrep_dirty_reads(wsrep_ready > + if (WSREP_CLIENT(thd) && (wsrep_ready || > !thd->variables.wsrep_dirty_reads) > + && wsrep_sync_wait(thd)) > + goto error case SQLCOM_SHOW_VARIABLES: > case SQLCOM_SHOW_CHARSETS: > case SQLCOM_SHOW_COLLATIONS: > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index bdcedff..0d864f4 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3967,8 +3967,8 @@ static Sys_var_mybool Sys_wsrep_restart_slave( > > static Sys_var_mybool Sys_wsrep_dirty_reads( > "wsrep_dirty_reads", "Do not reject SELECT queries even when the > node " > - "is not ready.", SESSION_ONLY(wsrep_dirty_reads), NO_CMD_LINE, > - DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > + "is not ready.", SESSION_VAR(wsrep_dirty_reads), + CMD_LINE(OPT_ARG), DEFAULT(FALSE), NO_MUTEX_GUARD, NOT_IN_BINLOG); > We might need a guard here since global variables can be changed from multiple places. Not 100% sure about this but that's my intuition. Need to look this up. > #endif /* WITH_WSREP */ > > diff --git a/sql/wsrep_mysqld.cc b/sql/wsrep_mysqld.cc > index 48114fc..f21ff92 100644 > --- a/sql/wsrep_mysqld.cc > +++ b/sql/wsrep_mysqld.cc > @@ -65,7 +65,8 @@ my_bool wsrep_restart_slave_activated = 0; // node has > dropped, and slave > // restart will be needed > my_bool wsrep_slave_UK_checks = 0; // slave thread does UK checks > my_bool wsrep_slave_FK_checks = 0; // slave thread does FK checks > - > +bool wsrep_dirty_reads = 0; // is dirty reads allowed for > + // disconnectd/non major node > Typo: disconnectd -> disconnected. I really hate how the whole file is set up with TRUE/FALSE/0 for either bool or my_bool. It's so inconsistent. I'd prefer 'true' here to complicate stuff further. :) To be "consistent" I'd make it a my_bool, but we should clean up all of this mess at one point. > /* > Set during the creation of first wsrep applier and rollback threads. > Since these threads are critical, abort if the thread creation fails. > diff --git a/sql/wsrep_mysqld.h b/sql/wsrep_mysqld.h > index 3fb5189..9b9179a 100644 > --- a/sql/wsrep_mysqld.h > +++ b/sql/wsrep_mysqld.h > @@ -85,6 +85,7 @@ extern ulong wsrep_max_ws_size; > extern ulong wsrep_max_ws_rows; > extern const char* wsrep_notify_cmd; > extern my_bool wsrep_certify_nonPK; > +extern bool wsrep_dirty_reads; > Remember to update the type here too. > extern long wsrep_max_protocol_version; > extern long wsrep_protocol_version; > extern ulong wsrep_forced_binlog_format;

2 1

Re: [Maria-developers] [Commits] 514b557: MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Sergei Golubchik 12 Dec '16

12 Dec '16

Hi, Vicentiu! I think mariadb should support 5.7 user table (without the password column) without any errors. That shouldn't be too difficult, just detect at run-time whether the column "Password" exists and adjust all other column numbers accordingly. On Dec 12, Vicentiu wrote: > revision-id: 514b557d066fcd1a90ccbbcdbaf5fa5fe65311eb (mariadb-10.2.2-138-g514b557) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 04:26:27 +0200 > message: > > MDEV-11170: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 02e74a5: MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir:
by Vicențiu Ciorbaru 12 Dec '16

12 Dec '16

Hi Sergei! Can you please review this patch for MDEV-11700? Details presented in the commit message. I'm not sure which solution we want. Possible solutions: A) have the user fix it by first starting the server with --skip-grant-tables to avoid "mysql.user table is damaged", then run mysql_upgrade, to fix the problem. Without this patch, this solution fails at mysql_upgrade. B) Have the server understand MySQL 5.7.6 + table format where the Password column is missing. For 10.2 I've chosen solution A. Although this should probably also be targeted for 10.1? For 10.3, since we're planning on doing some authentication changes, we can implement a variant of B. Thoughts? On Mon, 12 Dec 2016 at 03:50 vicentiu <vicentiu(a)mariadb.org> wrote: > revision-id: 02e74a5614b08cfce505002abc26d4634648308e > (mariadb-10.2.2-138-g02e74a5) > parent(s): 9320d8ae30c18420bef659618175836221d363ea > author: Vicențiu Ciorbaru > committer: Vicențiu Ciorbaru > timestamp: 2016-12-12 03:42:11 +0200 > message: > > MDEV-11700: MariaDB 10.2 cannot start on MySQL 5.7 datadir: > > Fatal error: mysql.user table is damaged or in unsupported 3.20 format > > The problem stems from MySQL 5.7.6. According to MySQL documentation: > In MySQL 5.7.6, the Password column was removed and all credentials are > stored in the authentication_string column. > > If opening a MySQL 5.7.6 (and up) datadir with MariaDB 10.2, the user table > appears corrupted. In order to fix this, the server must be started with > --skip-grant-tables and then a subsequent mysql_upgrade command must be > issued. > > This patch updates the mysql_upgrade command to also add the removed > Password column. The password column is necessary, otherwise > the mysql_upgrade script fails due to the Event_scheduler not being able > to start, as it can't find Event_priv in the table where it ought to be. > MySQL's version has column position 28 (0 index) vs our datadir version > expects position 29. > > --- > scripts/mysql_system_tables_fix.sql | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/scripts/mysql_system_tables_fix.sql > b/scripts/mysql_system_tables_fix.sql > index ea1059c..80d0ef7 100644 > --- a/scripts/mysql_system_tables_fix.sql > +++ b/scripts/mysql_system_tables_fix.sql > @@ -164,6 +164,12 @@ ALTER TABLE user > MODIFY Host char(60) NOT NULL default '', > MODIFY User char(80) NOT NULL default '', > ENGINE=MyISAM, CONVERT TO CHARACTER SET utf8 COLLATE utf8_bin; > + > +# In MySQL 5.7.6 the Password column is removed. Recreate it to preserve > the number > +# of columns MariaDB expects in the user table. > +ALTER TABLE user > + ADD Password char(41) character set latin1 collate latin1_bin NOT NULL > default '' AFTER User; > + > ALTER TABLE user > MODIFY Password char(41) character set latin1 collate latin1_bin NOT > NULL default '', > MODIFY Select_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' > NOT NULL, >

2 2

[Maria-developers] Review for version#2 of MDEV-11297: Add support for LIMIT clause in GROUP_CONCAT
by Sergey Petrunia 11 Dec '16

11 Dec '16

Hi Varun, Nice to see that nearly all comments from the previous review were addressed. Please find my further input below. General questions: How is DISTINCT handled? Syntax like GROUP_CONCAT(DISTINCT col ORDER BY ... LIMIT ...) is allowed by the parser, however I don't see any handling for DISTINCT in your patch. Did you forget to update group_concat_limit.result file? Please check. > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.cc 10.2-varun-mdev11297-noc/sql/item_sum.cc > --- 10.2-varun-mdev11297-orig/sql/item_sum.cc 2016-12-11 12:17:04.023961449 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.cc 2016-12-11 12:18:40.411957365 +0300 > @@ -3114,6 +3114,11 @@ int dump_leaf_key(void* key_arg, element > String *result= &item->result; > Item **arg= item->args, **arg_end= item->args + item->arg_count_field; > uint old_length= result->length(); > + ulonglong *offset_limit= &item->copy_offset_limit; > + ulonglong *row_limit = &item->copy_row_limit; > + > + if (item->limit_clause && !(*row_limit)) > + return 1; > > if (item->no_appended) > item->no_appended= FALSE; > @@ -3122,20 +3127,23 @@ int dump_leaf_key(void* key_arg, element > > tmp.length(0); > > - for (; arg < arg_end; arg++) > + if (!item->limit_clause || !(*offset_limit)) > { > - String *res; > - /* > - We have to use get_tmp_table_field() instead of > - real_item()->get_tmp_table_field() because we want the field in > - the temporary table, not the original field > - We also can't use table->field array to access the fields > - because it contains both order and arg list fields. > - */ > - if ((*arg)->const_item()) > - res= (*arg)->val_str(&tmp); > - else > + > + for (; arg < arg_end; arg++) > { > + String *res; > + /* > + We have to use get_tmp_table_field() instead of > + real_item()->get_tmp_table_field() because we want the field in > + the temporary table, not the original field > + We also can't use table->field array to access the fields > + because it contains both order and arg list fields. > + */ > + if ((*arg)->const_item()) > + res= (*arg)->val_str(&tmp); > + else > + { > Field *field= (*arg)->get_tmp_table_field(); Please make the code to use the right indentation. > if (field) > { > @@ -3146,9 +3154,19 @@ int dump_leaf_key(void* key_arg, element > } > else > res= (*arg)->val_str(&tmp); > + } > + if (res) > + { > + result->append(*res); > + } > } > - if (res) > - result->append(*res); > + if ((*row_limit)) > + (*row_limit)--; > + } > + else > + { > + item->no_appended= TRUE; If I comment out the above line, the test still passes. Please add test coverage that shows it is needed, or remove it. > + (*offset_limit)--; > } > > item->row_count++; > @@ -3199,7 +3217,8 @@ Item_func_group_concat:: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool distinct_arg, List<Item> *select_list, > const SQL_I_List<ORDER> &order_list, > - String *separator_arg) > + String *separator_arg, bool limit_clause, > + Item *rowLimit, Item *offsetLimit) camelCase or javaStyleVariableNames are against the coding style. Please use something like row_limit_arg if there is no better option. (In general, please try to make your new code fit in with the rest of the code. Did you see camelCase anywhere? If not, it's a sign not to use it). > :Item_sum(thd), tmp_table_param(0), separator(separator_arg), tree(0), > unique_filter(NULL), table(0), > order(0), context(context_arg), > @@ -3208,7 +3227,9 @@ Item_func_group_concat(THD *thd, Name_re > row_count(0), > distinct(distinct_arg), > warning_for_row(FALSE), > - force_copy_fields(0), original(0) > + force_copy_fields(0),row_limit(0), > + offset_limit(0),limit_clause(limit_clause), > + copy_offset_limit(0), copy_row_limit(0),original(0) Please use 'NULL' for pointers, not '0'. > { > Item *item_select; > Item **arg_ptr; > @@ -3250,6 +3271,16 @@ Item_func_group_concat(THD *thd, Name_re > /* orig_args is only used for print() */ > orig_args= (Item**) (order + arg_count_order); > memcpy(orig_args, args, sizeof(Item*) * arg_count); > + > + if (limit_clause) > + { > + if (!(row_limit=(Item*) thd->alloc(sizeof(Item)))) > + return; I'm not sure what the above is supposed to mean? Why allocate sizeof(Item) bytes? > + row_limit= rowLimit; ... especially when the next thing you do is the above assignment, so you lose the value that you've got from the thd->alloc() call? > + if (!(offset_limit= (Item*)thd->alloc(sizeof(Item)))) > + return; > + offset_limit= offsetLimit; The same question as above. > + } > } > > > @@ -3269,7 +3300,9 @@ Item_func_group_concat::Item_func_group_ > warning_for_row(item->warning_for_row), > always_null(item->always_null), > force_copy_fields(item->force_copy_fields), > - original(item) > + row_limit(item->row_limit), offset_limit(item->offset_limit), > + limit_clause(item->limit_clause),copy_offset_limit(item->copy_offset_limit), > + copy_row_limit(item->copy_row_limit), original(item->original) > { > quick_group= item->quick_group; > result.set_charset(collation.collation); > @@ -3382,6 +3415,10 @@ void Item_func_group_concat::clear() > null_value= TRUE; > warning_for_row= FALSE; > no_appended= TRUE; > + if (offset_limit) > + copy_offset_limit= offset_limit->val_int(); > + if (row_limit) > + copy_row_limit= row_limit->val_int(); > if (tree) > reset_tree(tree); > if (unique_filter) > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/item_sum.h 10.2-varun-mdev11297-noc/sql/item_sum.h > --- 10.2-varun-mdev11297-orig/sql/item_sum.h 2016-12-11 12:17:04.043961448 +0300 > +++ 10.2-varun-mdev11297-noc/sql/item_sum.h 2016-12-11 12:18:40.415957365 +0300 > @@ -1583,6 +1583,12 @@ class Item_func_group_concat : public It > bool always_null; > bool force_copy_fields; > bool no_appended; > + Item *row_limit; > + Item *offset_limit; > + bool limit_clause; > + ulonglong copy_offset_limit; > + ulonglong copy_row_limit; > + In previous review I have requested: >> Please add (short) comments describing each new member variable. And you still haven't done that. > /* > Following is 0 normal object and pointer to original one for copy > (to correctly free resources) > @@ -1602,7 +1608,8 @@ protected: > public: > Item_func_group_concat(THD *thd, Name_resolution_context *context_arg, > bool is_distinct, List<Item> *is_select, > - const SQL_I_List<ORDER> &is_order, String *is_separator); > + const SQL_I_List<ORDER> &is_order, String *is_separator, > + bool limit_clause, Item *row_limit, Item *offset_limit); > > Item_func_group_concat(THD *thd, Item_func_group_concat *item); > ~Item_func_group_concat(); > diff -urpN '--exclude=.*' 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 10.2-varun-mdev11297-noc/sql/sql_yacc.yy > --- 10.2-varun-mdev11297-orig/sql/sql_yacc.yy 2016-12-11 12:17:04.107961446 +0300 > +++ 10.2-varun-mdev11297-noc/sql/sql_yacc.yy 2016-12-11 12:18:40.887957345 +0300 > @@ -1786,7 +1786,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > %type <num> > order_dir lock_option > udf_type opt_local opt_no_write_to_binlog > - opt_temporary all_or_any opt_distinct > + opt_temporary all_or_any opt_distinct opt_glimit_clause > opt_ignore_leaves fulltext_options union_option > opt_not > select_derived_init transaction_access_mode_types > @@ -1844,7 +1844,7 @@ bool my_yyoverflow(short **a, YYSTYPE ** > opt_escape > sp_opt_default > simple_ident_nospvar simple_ident_q > - field_or_var limit_option > + field_or_var limit_option glimit_option > part_func_expr > window_func_expr > window_func > @@ -10449,16 +10449,34 @@ sum_expr: > | GROUP_CONCAT_SYM '(' opt_distinct > { Select->in_sum_expr++; } > expr_list opt_gorder_clause > - opt_gconcat_separator > + opt_gconcat_separator opt_glimit_clause > ')' > { > SELECT_LEX *sel= Select; > sel->in_sum_expr--; > - $$= new (thd->mem_root) > - Item_func_group_concat(thd, Lex->current_context(), $3, $5, > - sel->gorder_list, $7); > - if ($$ == NULL) > - MYSQL_YYABORT; > + if (!$8) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, FALSE, 0, 0); > + else > + { > + if (sel->select_limit && sel->offset_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, > + sel->offset_limit); > + else if (sel->select_limit) > + $$= new (thd->mem_root) > + Item_func_group_concat(thd, Lex->current_context(), $3, $5, > + sel->gorder_list, $7, TRUE, sel->select_limit, 0); > + } > + if ($$ == NULL) > + MYSQL_YYABORT; > + > + sel->select_limit= NULL; > + sel->offset_limit= NULL; > + sel->explicit_limit= 0; > + > $5->empty(); > sel->gorder_list.empty(); > } > @@ -10680,6 +10698,95 @@ gorder_list: > { if (add_gorder_to_list(thd, $1,(bool) $2)) MYSQL_YYABORT; } > ; What is the difference between glimit_option and limit_option? Looking at the grammar, they look the same. Please either remove glimit_option or explain why do you need it. I can see that limit_clause and glimit_clause have to be different because limit_clause also supports LIMIT ROWS EXAMINED. Actually, there is one thing in limit_clause that I think should be in glimit_clause also. It's this call: Lex->set_stmt_unsafe(LEX::BINLOG_STMT_UNSAFE_LIMIT); Did you omit it intentionally? Let's meet and discuss it. > +opt_glimit_clause: > + /* empty */ { $$ = 0; } > + | glimit_clause { $$ = 1; } > + ; > + > +glimit_clause_init: > + LIMIT{} > + ; > + > +glimit_clause: > + glimit_clause_init glimit_options{} > + ; > + > +glimit_options: > + glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= 0; > + sel->explicit_limit= 1; > + } > + | glimit_option ',' glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $3; > + sel->offset_limit= $1; > + sel->explicit_limit= 1; > + } > + | glimit_option OFFSET_SYM glimit_option > + { > + SELECT_LEX *sel= Select; > + sel->select_limit= $1; > + sel->offset_limit= $3; > + sel->explicit_limit= 1; > + } > + ; > + > +glimit_option: > + ident > + { > + Item_splocal *splocal; > + LEX *lex= thd->lex; > + Lex_input_stream *lip= & thd->m_parser_state->m_lip; > + sp_variable *spv; > + sp_pcontext *spc = lex->spcont; > + if (spc && (spv = spc->find_variable($1, false))) > + { > + splocal= new (thd->mem_root) > + Item_splocal(thd, $1, spv->offset, spv->sql_type(), > + lip->get_tok_start() - lex->sphead->m_tmp_query, > + lip->get_ptr() - lip->get_tok_start()); > + if (splocal == NULL) > + MYSQL_YYABORT; > +#ifndef DBUG_OFF > + splocal->m_sp= lex->sphead; > +#endif > + lex->safe_to_cache_query=0; > + } > + else > + my_yyabort_error((ER_SP_UNDECLARED_VAR, MYF(0), $1.str)); > + if (splocal->type() != Item::INT_ITEM) > + my_yyabort_error((ER_WRONG_SPVAR_TYPE_IN_LIMIT, MYF(0))); > + splocal->limit_clause_param= TRUE; > + $$= splocal; > + } > + | param_marker > + { > + $1->limit_clause_param= TRUE; > + } > + | ULONGLONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | LONG_NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + | NUM > + { > + $$= new (thd->mem_root) Item_uint(thd, $1.str, $1.length); > + if ($$ == NULL) > + MYSQL_YYABORT; > + } > + ; > + > in_sum_expr: > opt_all > { BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Please review MDEV-11528 Split Item_func_min_max::val_xxx() and Item_func_min_max::get_date() into methods in Type_handler
by Alexander Barkov 10 Dec '16

10 Dec '16

Hello Alexey, Can you please review a patch for MDEV-11528. Thanks!

1 0

[Maria-developers] MDEV-11503 Introduce Type_handler::make_in_vector() and Item_func_in_fix_comparator_compatible_types()
by Alexander Barkov 09 Dec '16

09 Dec '16

Hello Sanja, Please review a patch for MDEV-11503. Thanks!

1 1

[Maria-developers] Fix for TokuDB and parallel replication
by Kristian Nielsen 09 Dec '16

09 Dec '16

Parallel replication so far did not work well with TokuDB, as some people who tried it found out. I have now pushed to 10.1 some patches to solve the problems. There are two main fixes: 1. Fix some races where a waiting transaction would miss its wakeup and get a lock timeout on a waiting row lock, even though the lock was released by the holding transaction. This fix is due to great work by Rich Prohaska. This problem is actually not specific to replication, normal transactions on a master will experience it too. But it hurts replication a lot, since replication must commit transactions in order, so one stalled transaction stalls all following transactions as well. 2. Implement the conflict detection and handling necessary for optimistic parallel replication to work. This basically implements thd_rpl_deadlock_check() and hton->kill_query methods. This should solve the problems where optimistic parallel replication with TokuDB breaks with lock wait timeouts. If someone wants to test it, I have made a tree available with just these fixes on top of MariaDB 10.1.19: https://github.com/knielsen/server/tree/tokudb_optimistic_parallel_replicat… The fix should appear in 10.1.20 eventually. The first part of the patch has also been submitted by Rich to upstream. When this is (hopefully) merged upstream, and upstream merged into MariaDB, the MariaDB version of the fix should be replaced with the Percona one. I tried making the MariaDB version of the fix identical to Rich's pull request and keeping it in a separate commit, so this should hopefully be simple to do when the time comes. - Kristian.

3 9

Re: [Maria-developers] [Commits] 00f5065: MDEV-11422 rpl.rpl_parallel_optimistic_nobinlog failed
by Jan Lindström 08 Dec '16

08 Dec '16

Hi, All other looks correct, I'm concerned only following change that is not needed to fix this bug: On Thu, Dec 8, 2016 at 2:35 PM, <marko.makela(a)mariadb.com> wrote: > > > - err = lock_trx_handle_wait(trx, > - (lock_mutex_taken || already_have_lock_mutex), > - (trx_mutex_taken || already_have_trx_mutex)); > +#ifdef UNIV_DEBUG > + dberr_t err = > +#endif > + lock_trx_handle_wait(trx, true, true); > + > + ut_ad(err == DB_SUCCESS || err == DB_LOCK_WAIT > + || err == DB_DEADLOCK); > > > In above is true, true always correct? we conditionally take lock and trx mutexes on some cases if we already do not have them, and below function will take them if they are not already taken. There is complex rules as Galera can call this from different situations and we already have had cases where some mutex was taken twice or no mutex was taken. R: Jan

1 0

[Maria-developers] MDEV-10787 Assertion `ltime->neg == 0' failed in void date_to_datetime(MYSQL_TIME*)
by Alexander Barkov 07 Dec '16

07 Dec '16

Hello Sergei, please review a patch for MDEV-10787. Thanks!

2 2

[Maria-developers] Temporary push of multi_range_read support to 10.2
by Michael Widenius 07 Dec '16

07 Dec '16

Hi! Kentoku, I have now pushed into 10.2 the code for spider patches: 014,015,023,033,035,037,040,042,044,045,049,050,051,053,059 The push is done to make it easier for Jacob and you to make test cases for the new code. I will work on reviewing and cleaning up this patch during the next few days and then do a push a final version when I have got some more test cases that covers most of the new code. Regards, Monty

1 0

[Maria-developers] OSS-Fuzz
by Daniel Black 07 Dec '16

07 Dec '16

In case anyone is interested. Code needs to be written to take a random bit of data and push this into the server somehow. Apart from that it appears reasonably automated. https://github.com/google/oss-fuzz

1 0

Re: [Maria-developers] [Commits] 895309f: MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error
by Sergei Golubchik 06 Dec '16

06 Dec '16

Hi, Oleksandr! On Dec 06, Oleksandr Byelkin wrote: > revision-id: 895309f441e0262229671dcec5d49c4ff8f8ca2c (mariadb-5.5.53-16-g895309f) > parent(s): 18cdff6765b954431934f6e6d0b1e281c8f0e0b8 > committer: Oleksandr Byelkin > timestamp: 2016-12-06 14:18:24 +0100 > message: > > MDEV-10663: Use of Inline table columns in HAVING clause throws 1463 Error > > check for VIEW/DERIVED fields ok to push! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] f79d9d4: [MDEV]-11162 Assertion failing because cleanup does not get called
by Nirbhay Choubey 06 Dec '16

06 Dec '16

Hi Varun, On Tue, Dec 6, 2016 at 9:34 AM, Varun <varunraiko1803(a)gmail.com> wrote: > revision-id: f79d9d454bc3fa6fecb34fec850752a688c0ccc7 > (mariadb-10.0.28-17-gf79d9d4) > parent(s): e99990c631905cd923257331ef124a2bc9276e0d > author: Varun > committer: Varun > timestamp: 2016-12-06 19:48:58 +0530 > message: > > [MDEV]-11162 Assertion failing because cleanup does not get called > Looks like you forgot to add the result file. Besides, since the test is simple, you could instead add it to an existing _debug_ select/optimizer test. Best, Nirbhay > --- > mysql-test/t/bug-11162.test | 6 ++++++ > sql/sql_select.cc | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/mysql-test/t/bug-11162.test b/mysql-test/t/bug-11162.test > new file mode 100644 > index 0000000..5924851 > --- /dev/null > +++ b/mysql-test/t/bug-11162.test > @@ -0,0 +1,6 @@ > +--source include/have_innodb.inc > + > +CREATE TABLE t1 (i INT) ENGINE=InnoDB; > +SELECT ( SELECT DISTINCT GROUP_CONCAT(SLEEP(0)) FROM t1 GROUP BY i ); > +SELECT i FROM t1 order by i LIMIT 1; > +drop table t1; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 2db9a2b..f345d3c 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -3144,6 +3144,7 @@ JOIN::destroy() > */ > tmp_table_param.cleanup(); > tmp_join->tmp_table_param.copy_field= 0; > + cleanup(1); > DBUG_RETURN(tmp_join->destroy()); > } > cond_equal= 0; > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

1 0

Re: [Maria-developers] [Commits] 78c093a: MDEV-10435 crash with bad stat tables.
by Sergei Golubchik 06 Dec '16

06 Dec '16

Hi, Alexey! On Dec 06, Alexey Botchkov wrote: > revision-id: 78c093a042ccee6c7b8d11ab1b7e6069efb4af24 (mariadb-10.1.19-24-g78c093a) > parent(s): 611f91605adce17df87acf96b5aede0b73d0fc12 > committer: Alexey Botchkov > timestamp: 2016-12-06 18:32:48 +0400 > message: > > MDEV-10435 crash with bad stat tables. > > Functions from sql/statistics.cc don't seem to expect > stat tables to fail or to have inadequate structure. > Table open errors suppressed and some validity > checks added. > > diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc > index 27bc0fb..70d7d26 100644 > --- a/sql/sql_statistics.cc > +++ b/sql/sql_statistics.cc > @@ -129,6 +129,137 @@ inline void init_table_list_for_single_stat_table(TABLE_LIST *tbl, > } > > +class Stat_table_intact : public Table_check_intact grrr, I never understood why every user of Table_check_intact needs to create a new class. That's a lot of copy-pasting :( > +{ > +public: > + Stat_table_intact() {} > +protected: > + void report_error(uint code, const char *fmt, ...) {} You have an empty report_error() method here. How do report errors about incorrect table definition? May be you should copy (ugh!) report_error from, say, Event_db_intact? > +}; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-11485 Split Item_func_between::val_int() into virtual methods in Type_handler
by Alexander Barkov 05 Dec '16

05 Dec '16

Hello Sanja, Please review a patch for MDEV-11485. Thanks!

1 0

[Maria-developers] Please review MDEV-10717 Assertion `!null_value' failed in virtual bool Item::send(Protocol*, String*)
by Alexander Barkov 05 Dec '16

05 Dec '16

Hello Sergei, Please review a patch for MDEV-10717. Thanks!

2 1

Re: [Maria-developers] Working on spider patches, MDEV-7698
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff +++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc 2015-10-14 01:48:53.392665313 +0900 @@ -327,7 +327,9 @@ void ha_partition::init_handler_variable m_file_buffer= NULL; m_name_buffer_ptr= NULL; m_engine_array= NULL; +/* m_connect_string= NULL; +*/ m_file= NULL; m_file_tot_parts= 0; m_reorged_file= NULL; @@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition( if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem))) goto error_create; +/* tbl->s->connect_string = p_elem->connect_string; +*/ I don't think this it will work removing the usage of p_elem->connect_string This is because each partition may have a different connect string. Here is an example from fedarated_partion.test: eval create table t1 (s1 int primary key) engine=federated partition by list (s1) (partition p1 values in (1,3) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1', partition p2 values in (2,4) connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2'); The above works in mariadb 10.2 but not in your spider tree. >From this patch I will, for now, only take the code related to HA_EXTRA_WRITE_CAN_REPLACE HA_EXTRA_WRITE_CANNOT_REPLACE --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc 2015-10-13 23:49:10.188129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc 2015-10-14 01:48:54.296665317 +0900 @@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st *(char**) tgt= my_strdup(value, MYF(0)); else *(char**) tgt= 0; - my_free(old); + if (old) + my_free(old); As my_free is safe to call with NULL, the above is not needed diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h ./002_mariadb-10.1.8-spider/sql/sql_priv.h --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h 2015-10-13 23:49:10.189129839 +0900 +++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h 2015-10-14 01:48:54.642665315 +0900 @@ -27,6 +27,8 @@ #ifndef SQL_PRIV_INCLUDED #define SQL_PRIV_INCLUDED +#define PLUGIN_VAR_CAN_MEMALLOC + #ifndef MYSQL_CLIENT The above is not needed, as all code that is testing this is doing: #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 100000 Which is always true in MariaDB 10.x Regards, Monty

2 7

[Maria-developers] Working on spider patches, MDEV-7701
by Michael Widenius 05 Dec '16

05 Dec '16

Hi! Next patch, MDEV 7701 +++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc 2016-05-05 21:25:04.289731712 +0900 @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu } /* Category 9) Operations only used by MERGE */ case HA_EXTRA_ADD_CHILDREN_LIST: + DBUG_RETURN(loop_extra(operation)); case HA_EXTRA_ATTACH_CHILDREN: + int result; + handler **file; + if ((result = loop_extra(operation))) + DBUG_RETURN(result); + m_num_locks = 0; + additional_table_flags = + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE); + file = m_file; + do + { + m_num_locks += (*file)->lock_count(); + additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^ + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE))); The above test could be simple: additional_table_flags &= (ulonglong) ((*file)->ha_table_flags(); Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below. I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much. @@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons handler **file= m_file; COND *res_cond = NULL; DBUG_ENTER("ha_partition::cond_push"); +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS + if (set_top_table_fields) + { + do + { + if ((*file)->set_top_table_and_fields(top_table, + top_table_field, + top_table_fields)) + DBUG_RETURN(cond); + } while (*(++file)); + file= m_file; + } +#endif I added this without the define, but I merged it to the loop for cond_push. Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version. However, I would like to have a clear comment the purpose of the function set_top_table_and_fields(). struct st_mysql_storage_engine partition_storage_engine= { MYSQL_HANDLERTON_INTERFACE_VERSION }; +++ ./003_mariadb-10.2.0-vp/sql/handler.h 2016-05-05 21:25:04.314731713 +0900 @@ -255,6 +257,11 @@ enum enum_alter_inplace_result { */ #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE) +#define HA_CAN_BG_SEARCH (1LL << 45) +#define HA_CAN_BG_INSERT (1LL << 46) +#define HA_CAN_BG_UPDATE (1LL << 47) +#define HA_CAN_MULTISTEP_MERGE (1LL << 48) Do you really need HA_CAN_BG_XXX flags? I checked your spider tree and this is not used anywhere, so I think we should leave them out for now. If we don't need these, we can remove the loop for setting them in HA_EXTRA_ATTACH_CHILDREN too. If we need them, I would like to have a comment for why they are needed and when we will need then add these in a separate patch. @@ -3525,6 +3540,29 @@ public: Pops the top if condition stack, if stack is not empty. */ virtual void cond_pop() { return; }; + virtual int set_top_table_and_fields(TABLE *top_table, + Field **top_table_field, + uint top_table_fields) + { + if (!set_top_table_fields) + { + set_top_table_fields = TRUE; + this->top_table = top_table; + this->top_table_field = top_table_field; + this->top_table_fields = top_table_fields; + } + return 0; + } Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void. I will make it void for now and change the relevant code. We can make it int again if needed in the future. diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc ./003_mariadb-10.2.0-vp/sql/sql_base.cc --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc 2016-04-17 02:47:27.000000000 +0900 +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc 2016-05-05 21:25:04.319731713 +0900 @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table table= table->find_table_for_update(); - if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM) - { - TABLE_LIST *child; + if (table->table && + ( + table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM || + (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE) + ) + ) + continue I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler. This is now pushed into 10.2-spider Regards, Monty

2 2

Re: [Maria-developers] MDEV-11330 Split Item_func_hybrid_field_type::val_xxx() into methods in Type_handler
by Vicențiu Ciorbaru 05 Dec '16

05 Dec '16

Hi Alexander! I find this version much cleaner! Looks good to me. What I'd really like to see later is if we can write some unit tests for these conversions. Not mtr linked tests but actual unit tests specifically using these Items. I am up for chipping in and writing some myself. One thing I'd like to discuss later in the context of reentrant functions are these side effects such as "return (null_value= true);" when fetching data from an Item. I think they should go from base items and only perhaps happen in "cache" items. We can potentially then turn all these methods to be const methods. Vicențiu On Thu, 1 Dec 2016 at 15:18 Alexander Barkov <abarkov73(a)gmail.com> wrote: > Hello Vicențiu, > > Please review a modified version. > > As agreed, I'm not using the cast approach for the built-in types > and define a set of data type conversion methods > directly Type_handler_hybrid_field_type methods instead. > > Thanks! >

1 0

[Maria-developers] MariaRocks port: why handler::init_with_fields() call is needed + call for comments
by Sergey Petrunia 02 Dec '16

02 Dec '16

Hello, I think me and Serg have tried having this discussion before on several occasions but were not successful due to me not having the details back then. Now I'm trying with the details. == Description of the issue == Look at MariaDB, the code in TABLE_SHARE::init_from_binary_frm_image (or open_binary_frm() in mysql-5.6 codebase) The code goes like this: ... /* Allocate handler */ if (!(handler_file= get_new_handler(share, thd->mem_root, plugin_hton(se_plugin)))) // ^(LOCATION-1) ... /* Fix key->name and key_part->field */ if (key_parts) { longlong ha_option= handler_file->ha_table_flags(); // ^(LOCATION-2) ... for (uint key=0 ; key < keys ; key++,keyinfo++) { ... for (i=0; i < key_parts; key_part++, i++) { ... field= key_part->field= share->field[key_part->fieldnr-1]; // (LOCATION-3) ... } } Note that - a handler object is created at (LOCATION-1). A TABLE_SHARE object exists at that point, but it has table_share->key_info==NULL. This means the storage object has no information about the indexes in the table it is representing. - At (LOCATION-2), the code retrieves ha_table_flags(). A bit below that point it checks for "ha_option & HA_PRIMARY_KEY_IN_READ_INDEX" (there are also other checks, both harmful and harmless, but let's focus on this one for now). In MyRocks, whether HA_PRIMARY_KEY_IN_READ_INDEX flag is set depends on the datatypes of the primary key columns. However, ha_rocksdb object is requested to provide ha_table_flags() before it has a chance of looking at the table DDL and actually knowing the value. == Solution in MyRocks == It's here: https://github.com/facebook/mysql-5.6/commit/c1f1f0b64eaecd132fc6ec21663faf… Basically, there is a new handler::init_with_fields() call which SQL layer calls after information about table fields is filled. == Questions == Any objections to this approach? Alternative solutions that do not require a lot of re-writing? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0