- developers - lists.mariadb.org

Re: [Maria-developers] systemd changes in Debian Sid
by Faustin Lammler 12 Nov '19

12 Nov '19

Hi Otto! I did not had time to look at this for now. I believe I will have a bit of time before the week-end and surely next week. Faustin Otto Kekäläinen <otto(a)debian.org>, 11/11/2019 – 15:06:24 (+0100): > Hello Faustin! > > I have been updating the MariaDB 10.3 packaging in Debian to compat level > 12, and that includes some changes to how the systemd services are > installed. Do you have any comments? > > https://salsa.debian.org/mariadb-team/mariadb-10.3/commits/bugfix/systemd-r… > > Are there some systemd issues regarding 10.3 that you think we should put > in while at this? > > - Otto

1 0

Re: [Maria-developers] c790763e1f0: MDEV-19903 Setup default partitions for system versioning
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Aleksey! Looks good! Ok to push On Nov 06, Aleksey Midenkov wrote: > revision-id: c790763e1f0 (mariadb-10.4.7-51-gc790763e1f0) > parent(s): f10c9ac941e > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:00:33 +0300 > message: > > MDEV-19903 Setup default partitions for system versioning > > Implement syntax like: > > create table t1 (x int) with system versioning partition by system_time; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 4de59fa0f92: MENT-360: ASAN heap-use-after-free in strmake_root / Query_arena::strmake
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Oleksandr! On Nov 06, Oleksandr Byelkin wrote: > revision-id: 4de59fa0f92 (mariadb-10.2.27-129-g4de59fa0f92) > parent(s): 4b21495343d > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2019-10-18 15:14:20 +0200 > message: > > MENT-360: ASAN heap-use-after-free in strmake_root / Query_arena::strmake > > Do not allow thd->query point to freed memory, use safe method to set query. > > --- > mysql-test/r/processlist.result | 32 +++++++++++++++++++++++ > mysql-test/t/processlist.test | 58 +++++++++++++++++++++++++++++++++++++++++ > sql/sql_prepare.cc | 7 ++++- > 3 files changed, 96 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/r/processlist.result b/mysql-test/r/processlist.result > index ab518d961ef..066154feaf1 100644 > --- a/mysql-test/r/processlist.result > +++ b/mysql-test/r/processlist.result > @@ -43,3 +43,35 @@ Message Incorrect string value: '\xF0\x9F\x98\x8Eyy...' for column `information_ > # > # End of 10.1 tests > # > +# > +# MENT-360: ASAN heap-use-after-free in strmake_root / > +# Query_arena::strmake > +# > +CREATE PROCEDURE pr1() > +BEGIN > +DECLARE CONTINUE HANDLER FOR 1146 SET @a = 1; > +DECLARE CONTINUE HANDLER FOR 1243 SET @a = 1; > +LOOP > +PREPARE stmt FROM "CREATE TEMPORARY TABLE ps AS SELECT * FROM non_existing_table"; > +EXECUTE stmt; > +END LOOP; > +END $ > +CREATE PROCEDURE pr2() > +BEGIN > +DECLARE CONTINUE HANDLER FOR 1094 SET @a = 1; > +LOOP > +SHOW EXPLAIN FOR 1; > +END LOOP; > +END $ > +connect con1,localhost,root,,; > +CALL pr1(); > +connect con2,localhost,root,,; > +CALL pr2(); > +connection default; > +KILL 6; > +KILL 7; ^^^ you forgot replace_result also, mysqltest was designed for deterministic test cases, it'd be better to rewrite this test to be deterministic, if possible. > +DROP PROCEDURE pr1; > +DROP PROCEDURE pr2; > +# > +# end of Enterprise tests > +# > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 525f09d611b..d0043979135 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -2885,6 +2885,11 @@ void mysql_sql_stmt_prepare(THD *thd) > > if (stmt->prepare(query.str, (uint) query.length)) > { > + /* > + stmt->prepare() sets thd->query_string, so we have to reset it beck, > + before it will point to uninitialised memory > + */ > + thd->set_query(orig_query); > /* Statement map deletes the statement on erase */ > thd->stmt_map.erase(stmt); > } > @@ -2902,7 +2907,7 @@ void mysql_sql_stmt_prepare(THD *thd) > But here we should restore the original query so it's mentioned in > logs properly. > */ > - thd->set_query_inner(orig_query); > + thd->set_query(orig_query); > DBUG_VOID_RETURN; > } may be better to have set_query() only once, like in bool res=stmt->prepare(query.str, (uint) query.length); thd->set_query(orig_query); if (res) ... Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] aeb3432849e: MDEV-20076: SHOW GRANTS does not quote role names properly
by Sergei Golubchik 06 Nov '19

06 Nov '19

Hi, Oleksandr! On Nov 06, Oleksandr Byelkin wrote: > revision-id: aeb3432849e (mariadb-10.3.19-2-gaeb3432849e) > parent(s): 5d3bd2b75b5 > author: Oleksandr Byelkin <sanja(a)mariadb.com> > committer: Oleksandr Byelkin <sanja(a)mariadb.com> > timestamp: 2019-11-06 12:35:19 +0100 > message: > > MDEV-20076: SHOW GRANTS does not quote role names properly > > Quotes added to output. > > +# > +# MDEV-20076: SHOW GRANTS does not quote role names properly > +# > +create role 'role-1'; > +create user 'user-1'@'localhost'; > +grant select on mysql.user to 'role-1'; > +GRANT 'role-1' TO 'user-1'@'localhost'; > +show grants for 'role-1'; > +Grants for role-1 > +GRANT USAGE ON *.* TO 'role-1' > +GRANT SELECT ON `mysql`.`user` TO 'role-1' > +show grants for 'user-1'@'localhost'; > +Grants for user-1@localhost > +GRANT 'role-1' TO 'user-1'@'localhost' > +GRANT USAGE ON *.* TO 'user-1'@'localhost' > +drop role 'role-1'; > +drop user 'user-1'@'localhost'; Please, add a test for names that contains a single quote. Like create role `role'1`; create user `user'1`@localhost; and the rest as in your test above. Or, if you want it to look a bit less artificial create role `rock'n'roll`; create user `O'Brien`@localhost; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] d5352b8154d: MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field
by Sergei Golubchik 31 Oct '19

31 Oct '19

Hi, Aleksey! On Oct 25, Aleksey Midenkov wrote: > revision-id: d5352b8154d (mariadb-10.2.25-54-gd5352b8154d) > parent(s): 1153950ad0a > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-07-22 15:40:06 +0300 > message: > > MDEV-20015 Assertion `!in_use->is_error()' failed in TABLE::update_virtual_field > > Preserve and restore statement DA. This is strange. Diagnostics areas aren't supposed to be temporarily created on the frame, they aren't arenas. Why TABLE::update_virtual_field() is called at all if there's already an error? > diff --git a/sql/table.cc b/sql/table.cc > index f5b5bad99cc..65611d78bde 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -7682,15 +7682,25 @@ int TABLE::update_virtual_fields(handler *h, enum_vcol_update_mode update_mode) > > int TABLE::update_virtual_field(Field *vf) > { > - DBUG_ASSERT(!in_use->is_error()); > + Diagnostics_area *stmt_da= NULL; > + Diagnostics_area tmp_stmt_da(in_use->query_id, false, true); > + bool error; > Query_arena backup_arena; > DBUG_ENTER("TABLE::update_virtual_field"); > + if (unlikely(in_use->is_error())) > + { > + stmt_da= in_use->get_stmt_da(); > + in_use->set_stmt_da(&tmp_stmt_da); > + } > in_use->set_n_backup_active_arena(expr_arena, &backup_arena); > bitmap_clear_all(&tmp_set); > vf->vcol_info->expr->walk(&Item::update_vcol_processor, 0, &tmp_set); > vf->vcol_info->expr->save_in_field(vf, 0); > in_use->restore_active_arena(expr_arena, &backup_arena); > - DBUG_RETURN(in_use->is_error()); > + error= in_use->is_error(); > + if (stmt_da) > + in_use->set_stmt_da(stmt_da); > + DBUG_RETURN(error); > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

Re: [Maria-developers] f6269a85ad7: MDEV-17553 Enable setting start datetime for interval partitioned history of system versioned tables
by Sergei Golubchik 31 Oct '19

31 Oct '19

Hi, Aleksey! On Sep 26, Aleksey Midenkov wrote: > revision-id: f6269a85ad7 (mariadb-10.4.7-38-gf6269a85ad7) > parent(s): a9ca752f1a9 > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 11:58:56 +0300 > message: > > MDEV-17553 Enable setting start datetime for interval partitioned history of system versioned tables > > * Interactive STARTS syntax I wouldn't use the word "interactive" here, better say "Explicit STARTS syntax" > * SHOW CREATE > * Default STARTS rounding depending on INTERVAL type This is questionable. I don't see why it's much better than the old behavior. It kind of makes sense, but that's all. If it would've worked that way from the start, it would be fine. But now, I'm not sure it offers such important benefits to break the compatibility for it. > * Warn when STARTS timestamp is further than INTERVAL > > [Closes tempesta-tech/mariadb#574] > > === Dependency hints (auto-detected by git-deps) === > 336c0139a89 MDEV-16370 row-based binlog events (updates by primary key) can not be applied multiple times to system versioned tables ^^^ remove these dependency hints before pushing, please > diff --git a/mysql-test/suite/versioning/r/partition.result b/mysql-test/suite/versioning/r/partition.result > index 9e532824414..3606c4e407f 100644 > --- a/mysql-test/suite/versioning/r/partition.result > +++ b/mysql-test/suite/versioning/r/partition.result > @@ -1,3 +1,4 @@ > +call mtr.add_suppression("need more HISTORY partitions"); why these warnings suddenly started to show up in the error log? > set system_versioning_alter_history=keep; > # Check conventional partitioning on temporal tables > create or replace table t1 ( > @@ -266,11 +267,11 @@ x > 6 > insert into t1 values (7), (8); > Warnings: > -Warning 4114 Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions Hmm, "is out of LIMIT" sounds strange, I liked "is full" more. Is it that important to say LIMIT or INTERVAL here? > ### warn about full partition > delete from t1; > Warnings: > -Warning 4114 Versioned table `test`.`t1`: partition `p1` is full, add more HISTORY partitions > +Warning 4114 Versioned table `test`.`t1`: last HISTORY partition (`p1`) is out of LIMIT, need more HISTORY partitions > select * from t1 partition (p1) order by x; > x > 4 > diff --git a/mysql-test/suite/versioning/r/partition_rotation.result b/mysql-test/suite/versioning/r/partition_rotation.result > index 69b30a56bd6..f6db36b117b 100644 > --- a/mysql-test/suite/versioning/r/partition_rotation.result > +++ b/mysql-test/suite/versioning/r/partition_rotation.result > @@ -55,4 +57,265 @@ i > explain partitions select * from t1 for system_time all where row_end = @ts; > id select_type table partitions type possible_keys key key_len ref rows Extra > 1 SIMPLE t1 p1_p1sp0,p1_p1sp1 # NULL NULL NULL NULL # # > -drop table t1; > +## INTERVAL ... STARTS > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts 'a' > +(partition p0 history, partition pn current); > +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS' > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts '00:00:00' > +(partition p0 history, partition pn current); > +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS' There are clear and well defined rules how to cast TIME to TIMESTAMP, so we can allow that. But it might be confusing, so if you'd like, let's keep it your way and lift this restriction later if needed. > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts '2000-00-01 00:00:00' > +(partition p0 history, partition pn current); > +ERROR HY000: Wrong parameters for partitioned `t1`: wrong value for 'STARTS' > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts '2000-01-01 00:00:00' > +(partition p0 history, partition pn current); > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `i` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' > +(PARTITION `p0` HISTORY ENGINE = MyISAM, > + PARTITION `pn` CURRENT ENGINE = MyISAM) > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts 946684800 Now, this is very questionable. There is a well defined rule how to cast numbers to temporal values, and this isn't it. If you want to allow numbers, it has to be partition by system_time interval 1 day starts 20000101000000 Whatever the server is doing internally when parsing frms isn't really relevant for the end user, is it? > +(partition p0 history, partition pn current); > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `i` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' > +(PARTITION `p0` HISTORY ENGINE = MyISAM, > + PARTITION `pn` CURRENT ENGINE = MyISAM) > +# Test STARTS warning > +set timestamp= unix_timestamp('2000-01-01 00:00:00'); > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day > +(partition p0 history, partition pn current); > +show create table t1; > +Table Create Table > +t1 CREATE TABLE `t1` ( > + `i` int(11) DEFAULT NULL > +) ENGINE=MyISAM DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > + PARTITION BY SYSTEM_TIME INTERVAL 1 DAY STARTS TIMESTAMP'2000-01-01 00:00:00' > +(PARTITION `p0` HISTORY ENGINE = MyISAM, > + PARTITION `pn` CURRENT ENGINE = MyISAM) > +# no warning > +create or replace table t1 (i int) with system versioning > +partition by system_time interval 1 day starts '2000-01-02 00:00:00' Why no warning here? If I insert and delete a row right now, it'll happen at 2000-01-01 00:00:00. So, it'll be before STARTS, what partition it should go into? As far as I understand, STARTS normally must be before NOW() not before NOW()+INTERVAL > diff --git a/sql/partition_info.cc b/sql/partition_info.cc > index ba6fc8a49ec..119c00dfce6 100644 > --- a/sql/partition_info.cc > +++ b/sql/partition_info.cc > @@ -2381,6 +2383,111 @@ static bool strcmp_null(const char *a, const char *b) > return true; > } > > +/** > + Assign INTERVAL and STARTS for SYSTEM_TIME partitions. > + > + @return true on error > +*/ > + > +bool partition_info::vers_set_interval(THD* thd, Item* interval, > + interval_type int_type, Item* starts, > + const char *table_name) why do you pass the table name as an argument now? > +{ > + DBUG_ASSERT(part_type == VERSIONING_PARTITION); > + > + const bool interactive= !table; what does that mean? * first, SQL is not interactive, using this adjective is confusing, * second you didn't write if(!table) because you wanted to use a self-documenting variable name, I presume. This name isn't helping, it confuses even more :( please, either use a really self-documenting name, or just write if(!table) with a comment. ... okay, now I see what you mean. Better use a comment: if (!table) { /* called from mysql_unpack_partition() not from mysql_parse() */ > + MYSQL_TIME ltime; > + uint err; > + vers_info->interval.type= int_type; > + > + /* 1. assign INTERVAL to interval.step */ > + if (interval->fix_fields_if_needed_for_scalar(thd, &interval)) > + return true; > + bool error= get_interval_value(thd, interval, int_type, &vers_info->interval.step) || > + vers_info->interval.step.neg || vers_info->interval.step.second_part || > + !(vers_info->interval.step.year || vers_info->interval.step.month || > + vers_info->interval.step.day || vers_info->interval.step.hour || > + vers_info->interval.step.minute || vers_info->interval.step.second); > + if (error) { indentation { > + my_error(ER_PART_WRONG_VALUE, MYF(0), table_name, "INTERVAL"); > + return true; > + } > + > + /* 2. assign STARTS to interval.start */ > + if (starts) > + { I've already commented on the following, but here it is again: > + if (starts->fix_fields_if_needed_for_scalar(thd, &starts)) > + return true; > + switch (starts->result_type()) > + { > + case INT_RESULT: > + case DECIMAL_RESULT: > + case REAL_RESULT: > + if (starts->val_int() > TIMESTAMP_MAX_VALUE) > + goto interval_starts_error; > + vers_info->interval.start= (time_t) starts->val_int(); > + break; wrong conversion to temporal > + case STRING_RESULT: > + case TIME_RESULT: > + { > + Datetime::Options opt(TIME_NO_ZERO_DATE | TIME_NO_ZERO_IN_DATE, thd); > + starts->get_date(thd, &ltime, opt); > + vers_info->interval.start= TIME_to_timestamp(thd, &ltime, &err); > + if (err) > + goto interval_starts_error; > + break; > + } > + case ROW_RESULT: > + default: > + goto interval_starts_error; > + } > + if (interactive) > + { > + my_tz_OFFSET0->gmt_sec_to_TIME(&ltime, thd->query_start()); > + if (date_add_interval(thd, &ltime, int_type, vers_info->interval.step)) > + return true; I think this isn't needed > + my_time_t boundary= my_tz_OFFSET0->TIME_to_gmt_sec(&ltime, &err); > + if (vers_info->interval.start > boundary) { > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN, > + ER_PART_STARTS_BEYOND_INTERVAL, > + ER_THD(thd, ER_PART_STARTS_BEYOND_INTERVAL), > + table_name); > + } > + } > + } > + else // calculate default STARTS depending on INTERVAL > + { > + thd->variables.time_zone->gmt_sec_to_TIME(&ltime, thd->query_start()); > + if (vers_info->interval.step.second) > + goto interval_set_starts; > + ltime.second= 0; I think this isn't needed > + if (vers_info->interval.step.minute) > + goto interval_set_starts; > + ltime.minute= 0; > + if (vers_info->interval.step.hour) > + goto interval_set_starts; > + ltime.hour= 0; > + if (vers_info->interval.step.day) > + goto interval_set_starts; > + ltime.day= 1; > + if (vers_info->interval.step.month) > + goto interval_set_starts; > + ltime.month= 1; > + DBUG_ASSERT(vers_info->interval.step.year); > + > +interval_set_starts: > + vers_info->interval.start= TIME_to_timestamp(thd, &ltime, &err); > + if (err) > + goto interval_starts_error; > + } > + > + return false; > + > +interval_starts_error: > + my_error(ER_PART_WRONG_VALUE, MYF(0), table_name, "STARTS"); > + return true; > +} > + Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] 02362a77d0d: MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key
by Sergei Golubchik 29 Oct '19

29 Oct '19

Hi, Sachin! Looks good now, thanks. See comments below On Oct 29, Sachin Setiya wrote: > revision-id: 02362a77d0d (mariadb-10.4.7-111-g02362a77d0d) > parent(s): 52f3829b95c > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-10-14 16:10:54 +0530 > message: > > MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > > Problem:- Insert delayed is not working with Long Unique Index. > It is failing with > 1. INSERT DELAYED INTO t1 VALUES(); > 2. INSERT DELAYED INTO t1 VALUES(1); > 3. Potential Race condition When Insert DELAYED gets dup key error(After fix), > And it will change original table key_info by calling > re_setup_keyinfo_hash, And second thread is in check_duplicate_long_entries > 4. Insert delayed into INVISIBLE COLUMN will also not work. > > There are 4 main issue > > 1. while calling make_new_field we forgot to & LONG_UNIQUE_HASH_FIELD > flag into new field flags. > > 2. New field created created into get_local_table by make_new_field does > not respect old field visibility, Assigning old field visibility will > solve Problem 4 and part of problem 2. > > 3. As we know Problem 3 race condition is caused because table and > delayed table share same key_info, So we will make a copy of original table > key_info in get_local_table. > > 4. In parse_vcol_defs we have this code block > keypart->field->vcol_info= > table->field[keypart->field->field_index]->vcol_info; > Which is wrong because we should not change original > table->field->vcol_info with vcol_info which is create on delayed > thread. > > --- > mysql-test/main/long_unique_bugs.result | 29 +++++++++++++++++++++++++++++ > mysql-test/main/long_unique_bugs.test | 32 ++++++++++++++++++++++++++++++++ > sql/field.cc | 7 ++++++- > sql/sql_insert.cc | 31 +++++++++++++++++++++++++++++++ > sql/table.cc | 31 ++++++++++++++++++++++++++++++- > sql/table.h | 2 ++ > 6 files changed, 130 insertions(+), 2 deletions(-) > > diff --git a/mysql-test/main/long_unique_bugs.test b/mysql-test/main/long_unique_bugs.test > index 13a4e1367a0..365393aa2ea 100644 > --- a/mysql-test/main/long_unique_bugs.test > +++ b/mysql-test/main/long_unique_bugs.test > @@ -340,3 +340,35 @@ while ($count) > --eval $insert_stmt > --enable_query_log > drop table t1; > +# > +# MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > +# > +CREATE TABLE t1 (a blob, UNIQUE(a)) ENGINE=MyISAM; > +INSERT DELAYED t1 () VALUES (1); > +INSERT t1 () VALUES (2); > +# Cleanup > +DROP TABLE t1; > +CREATE TABLE t1 (a char(50), UNIQUE(a(10)) USING HASH); > +INSERT DELAYED t1 () VALUES (1); > +INSERT t1 () VALUES (2); > +# Cleanup > +DROP TABLE t1; > +CREATE TABLE t1 ( > + a CHAR(128), > + b CHAR(128) AS (a), > + c varchar(5000), > + UNIQUE(c,b(64)) > +) ENGINE=myisam; > +INSERT DELAYED t1 (a,c) VALUES (1,1); > +--sleep 1 > +INSERT t1 (a,c) VALUES (2,2); > +INSERT t1 (a,c) VALUES (3,3); > +drop table t1; > +create table t1(a int , b int invisible); > +insert into t1 values(1); > +insert delayed into t1(a,b) values(2,2); > +--echo #Should not fails "Should not fail" > +insert delayed into t1 values(2); I suspect there's a race condition here. You cannot know that INSERT DELAYED will insert rows before you do your SELECT. Other tests usually do FLUSH TABLE t1 before SELECT. Alternatively you can use include/wait_condition.inc file to wait for, say, SELECT COUNT(*) = 3 > +select a,b from t1 order by a; > +# Cleanup > +DROP TABLE t1; > diff --git a/sql/field.cc b/sql/field.cc > index 0eb53f40a54..d1187237f23 100644 > --- a/sql/field.cc > +++ b/sql/field.cc > @@ -2373,8 +2373,13 @@ Field *Field::make_new_field(MEM_ROOT *root, TABLE *new_table, > tmp->flags&= (NOT_NULL_FLAG | BLOB_FLAG | UNSIGNED_FLAG | > ZEROFILL_FLAG | BINARY_FLAG | ENUM_FLAG | SET_FLAG | > VERS_SYS_START_FLAG | VERS_SYS_END_FLAG | > - VERS_UPDATE_UNVERSIONED_FLAG); > + VERS_UPDATE_UNVERSIONED_FLAG | LONG_UNIQUE_HASH_FIELD); > tmp->reset_fields(); > + /* > + Calling make_new_field will return a VISIBLE field, If caller function > + wants original visibility he should change it later. "visibility it should change" > + This is done because view created on invisible fields are visible. "because invisible fields explicitly named in a view become visible" > + */ > tmp->invisible= VISIBLE; > return tmp; > } > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index fe6c5fa8ec4..d80044ec37d 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2607,6 +2607,11 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd) > { > if (!(*field= (*org_field)->make_new_field(client_thd->mem_root, copy, 1))) > goto error; > + /* > + We want same visibility as of original table because we are just creating > + a clone for delayed insert. > + */ > + (*field)->invisible= (*org_field)->invisible; I'm surprised it matters. I'd thought that all sanity checks, and in particular whether number of columns matches the number of values, are done in the connection user thread before submitting the job to the delayed insert thread. > (*field)->unireg_check= (*org_field)->unireg_check; > (*field)->orig_table= copy; // Remove connection > (*field)->move_field_offset(adjust_ptrs); // Point at copy->record[0] > @@ -2621,7 +2626,33 @@ TABLE *Delayed_insert::get_local_table(THD* client_thd) > if (share->virtual_fields || share->default_expressions || > share->default_fields) > { > + /* > + If we have long unique table then delayed insert can modify key structure > + (re/setup_keyinfo_hash_all) of original table when it gets insert error, > + parse_vcol_defs will also modify key_info structure. So it is better to > + clone the table->key_info for copy table. > + We will not be cloning key_part_info or even changing any field ptr. > + Because re/setup_keyinfo_hash_all only modify key_info array. So it will > + be like having new key_info array for copy table with old key_part_info > + ptr. > + */ > + if (share->long_unique_table) > + { > + KEY *key_info; > + if (!(key_info= (KEY*) client_thd->alloc(share->keys*sizeof(KEY)))) > + goto error; > + copy->key_info= key_info; > + memcpy(key_info, table->key_info, sizeof(KEY)*share->keys); > + } > + /* > + parse_vcol_defs expects key_infos to be in user defined format. > + */ > + copy->setup_keyinfo_hash_all(); > bool error_reported= FALSE; > + /* > + We won't be calling re_setup_keyinfo_hash because parse_vcol_defs changes > + key_infos to storage engine format > + */ > if (unlikely(parse_vcol_defs(client_thd, client_thd->mem_root, copy, > &error_reported, > VCOL_INIT_DEPENDENCY_FAILURE_IS_WARNING))) > diff --git a/sql/table.cc b/sql/table.cc > index 718d0dce072..d0fcd449098 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1223,7 +1223,16 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, > new (mem_root) Item_field(thd, keypart->field), > new (mem_root) Item_int(thd, length)); > list_item->fix_fields(thd, NULL); > - keypart->field->vcol_info= > + /* > + Do not change the vcol_info when vcol_info->expr is not NULL > + This will happen in the case of Delayed_insert::get_local_table() > + And if we change the vcol_info in Delayed insert , then original > + table field->vcol_info will be created on delayed insert thread > + mem_root. > + */ > + if (!keypart->field->vcol_info || > + !keypart->field->vcol_info->expr) > + keypart->field->vcol_info= > table->field[keypart->field->field_index]->vcol_info; > } > else > @@ -9084,6 +9093,26 @@ void re_setup_keyinfo_hash(KEY *key_info) > key_info->ext_key_parts= 1; > key_info->flags&= ~HA_NOSAME; > } > + > +/* > + call setup_keyinfo_hash for all keys in table > + */ > +void TABLE::setup_keyinfo_hash_all() > +{ > + for (uint i= 0; i < s->keys; i++) > + if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + setup_keyinfo_hash(&key_info[i]); also... I remember you wanted to rename these methods to something more descriptive. > +} > + > +/* > + call re_setup_keyinfo_hash for all keys in table > + */ > +void TABLE::re_setup_keyinfo_hash_all() don't add this method, you aren't using it anywhere > +{ > + for (uint i= 0; i < s->keys; i++) > + if (key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + re_setup_keyinfo_hash(&key_info[i]); > +} > /** > @brief clone of current handler. > Creates a clone of handler used in update for Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 96569b793a3: MDEV-19761 - Before Trigger not processed for Not Null Column
by Sergei Golubchik 29 Oct '19

29 Oct '19

Hi, Anel! On Oct 29, Anel Husakovic wrote: > revision-id: 96569b793a3 (mariadb-10.1.39-121-g96569b793a3) > parent(s): e32f29b7f31 > author: Anel Husakovic <anel(a)mariadb.org> > committer: Anel Husakovic <anel(a)mariadb.org> > timestamp: 2019-07-24 23:50:29 -0700 > message: > > MDEV-19761 - Before Trigger not processed for Not Null Column here you should have a commit comment explaining the bug and your solution. > --- > mysql-test/r/trigger.result | 6 +++- > mysql-test/r/trigger_no_defaults-11698.result | 29 +++++++++++++++++++ > mysql-test/r/trigger_null-8605.result | 2 +- > mysql-test/suite/funcs_1/r/innodb_trig_09.result | 4 +-- > mysql-test/suite/funcs_1/r/memory_trig_09.result | 4 +-- > mysql-test/suite/funcs_1/r/myisam_trig_09.result | 4 +-- > mysql-test/t/trigger_no_defaults-11698.test | 37 ++++++++++++++++++++---- > sql/sql_base.cc | 23 +++++++++++++++ > sql/sql_trigger.cc | 2 +- > sql/sql_trigger.h | 4 +++ > 10 files changed, 97 insertions(+), 18 deletions(-) > > diff --git a/mysql-test/r/trigger.result b/mysql-test/r/trigger.result > index 8852a622251..04bb9d9be68 100644 > --- a/mysql-test/r/trigger.result > +++ b/mysql-test/r/trigger.result > @@ -366,6 +366,8 @@ create trigger trg1 before update on t1 for each row set @a:= @a + new.j - old.j > create trigger trg2 after update on t1 for each row set @b:= "Fired"; > set @a:= 0, @b:= ""; > update t1, t2 set j = j + 10 where t1.i = t2.i; > +Warnings: > +Warning 1048 Column 'k' cannot be null this looks wrong. Why there's suddenly a warning here? > select @a, @b; > @a @b > 10 Fired > @@ -1235,11 +1237,13 @@ insert into t1 values (1,1), (2,2), (3,3); > create trigger t1_bu before update on t1 for each row > set new.j = new.j + 10; > update t1 set i= i+ 10 where j > 2; > +Warnings: > +Warning 1048 Column 'j' cannot be null and here > select * from t1; > diff --git a/sql/sql_base.cc b/sql/sql_base.cc > index e8bdff8b48f..d9fe37c83da 100644 > --- a/sql/sql_base.cc > +++ b/sql/sql_base.cc > @@ -8959,6 +8959,29 @@ void switch_to_nullable_trigger_fields(List<Item> &items, TABLE *table) > > while ((item= it++)) > item->walk(&Item::switch_to_nullable_fields_processor, 1, (uchar*)field); > + uint16 cnt=0; > + uchar *nptr; > + nptr= (uchar*)alloc_root(&table->mem_root, (table->s->fields - table->s->null_fields + 7)/8); > + // First find null_ptr for NULL field in case of mixed NULL and NOT NULL fields > + for (Field **f= field; *f; f++) > + { > + if (table->field[cnt]->null_ptr) > + { > + nptr= table->field[cnt]->null_ptr; > + break; > + } > + } > + for (Field **f= field; *f; f++) > + { > + if (!table->field[cnt]->null_ptr) > + { > + (*f)->null_bit= 1<<(cnt+1); > + (*f)->flags&= ~(NOT_NULL_FLAG); > + (*f)->null_ptr= nptr; > + } > + cnt++; > + } > + bzero(nptr, (table->s->fields - table->s->null_fields + 7)/8); what are you doing here? > table->triggers->reset_extra_null_bitmap(); > } > } > diff --git a/sql/sql_trigger.cc b/sql/sql_trigger.cc > index 4ecd8139921..7de2fde0e5c 100644 > --- a/sql/sql_trigger.cc > +++ b/sql/sql_trigger.cc > @@ -1118,7 +1118,7 @@ bool Table_triggers_list::prepare_record_accessors(TABLE *table) > > f->flags= (*fld)->flags; > f->null_ptr= null_ptr; > - f->null_bit= null_bit; > + f->null_bit= (*fld)->null_bit; > if (null_bit == 128) > null_ptr++, null_bit= 1; > else > diff --git a/sql/sql_trigger.h b/sql/sql_trigger.h > index f451dfda1ee..c1174bfd36e 100644 > --- a/sql/sql_trigger.h > +++ b/sql/sql_trigger.h > @@ -226,6 +226,10 @@ class Table_triggers_list: public Sql_alloc > trigger_table->s->null_fields + 7)/8; > bzero(extra_null_bitmap, null_bytes); > } > + uchar *get_extra_null_bitmap() const > + { > + return extra_null_bitmap; > + } This is not used anywhere > private: > bool prepare_record_accessors(TABLE *table); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] e752eed5354: MDEV-19751 Wrong partitioning by KEY() after key dropped
by Sergei Golubchik 29 Oct '19

29 Oct '19

Hi, Aleksey! On Oct 29, Aleksey Midenkov wrote: > revision-id: e752eed5354 (mariadb-10.4.7-47-ge752eed5354) > parent(s): 8403148452a > author: Aleksey Midenkov <midenok(a)gmail.com> > committer: Aleksey Midenkov <midenok(a)gmail.com> > timestamp: 2019-08-19 12:00:13 +0300 > message: > > MDEV-19751 Wrong partitioning by KEY() after key dropped > > Empty partition by key() clause implies that key might be changed by > ALTER, but inplace algorithm must not be used since repartitioning is > required for a different key. good and very clear comment. > diff --git a/sql/sql_partition.cc b/sql/sql_partition.cc > index 10b9c74d868..d76ff5e2e76 100644 > --- a/sql/sql_partition.cc > +++ b/sql/sql_partition.cc > @@ -5903,6 +5903,45 @@ the generated partition syntax in a correct manner. > *partition_changed= TRUE; > } > } > + /* > + Prohibit inplace when key takes part in partitioning expression > + and is altered (dropped). > + */ > + if (!*partition_changed && tab_part_info->part_field_array) > + { > + KEY *key_info= table->key_info; > + List_iterator_fast<Alter_drop> drop_it(alter_info->drop_list); > + for (uint key= 0; key < table->s->keys; key++, key_info++) > + { > + if (key_info->flags & HA_INVISIBLE_KEY) > + continue; > + const char *key_name= key_info->name.str; > + const Alter_drop *drop; > + drop_it.rewind(); > + while ((drop= drop_it++)) > + { > + if (drop->type == Alter_drop::KEY && > + 0 == my_strcasecmp(system_charset_info, key_name, drop->name)) > + break; > + } > + if (!drop) > + continue; > + for (uint kp= 0; kp < key_info->user_defined_key_parts; ++kp) > + { > + const KEY_PART_INFO &key_part= key_info->key_part[kp]; > + for (Field **part_field= tab_part_info->part_field_array; > + *part_field; ++part_field) > + { > + if (*part_field == key_part.field) > + { > + *partition_changed= TRUE; > + goto search_finished; > + } > + } // for (part_field) > + } // for (key_part) > + } // for (key_info) Hmm, if I read it correctly, you iterate over all key parts of every dropped key and see if this field is also part of the partitioning expression. This sounds kind of strange. What if there are two indexes that use the column `a`, say CREATE TABLE t1 (a int, b int, index(a), index(b,a)) and you partition by key(a). Dropping the second index does not change the partitioning, does it? On the other hand, if it's ALTER TABLE ... MODIFY `a` then it can change partitioning, even when the key hasn't changed, right? So, I suspect your check is too strong in some cases and too loose is others. > + search_finished:; > + } > } > if (thd->work_part_info) > { > diff --git a/storage/connect/mysql-test/connect/r/part_file.result b/storage/connect/mysql-test/connect/r/part_file.result > index 3dabd946b50..480de51cce6 100644 > --- a/storage/connect/mysql-test/connect/r/part_file.result > +++ b/storage/connect/mysql-test/connect/r/part_file.result > @@ -308,12 +308,19 @@ EXPLAIN PARTITIONS SELECT * FROM t1 WHERE id = 10; > id select_type table partitions type possible_keys key key_len ref rows Extra > 1 SIMPLE t1 1 ref XID XID 4 const 1 > DROP INDEX XID ON t1; > +Warnings: > +Warning 1105 Data repartition in 1 is unchecked > +Warning 1105 Data repartition in 2 is unchecked > +Warning 1105 Data repartition in 3 is unchecked why did the result file change? could you add a short explanation of it to the commit comment, please? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Please review MDEV-18319 BIGINT UNSIGNED Performance issue
by Alexander Barkov 29 Oct '19

29 Oct '19

Hi Sergei, Please review a patch for MDEV-18319 https://github.com/MariaDB/server/commit/8a990ad17746927c6d395ec755a262eda5… Thanks

2 1