October 2019 - developers - lists.mariadb.org

Re: [Maria-developers] ef2519fee4e: MDEV-16546 System versioning setting to allow history modification
by Aleksey Midenkov 08 Jun '21

08 Jun '21

Hello, Sergei! On Fri, May 3, 2019 at 8:43 PM Sergei Golubchik <serg(a)mariadb.com> wrote: > > Hi, Aleksey! > > On May 03, Aleksey Midenkov wrote: > > revision-id: ef2519fee4e (versioning-1.0.5-17-gef2519fee4e) > > parent(s): 56145be2951 > > author: Aleksey Midenkov <midenok(a)gmail.com> > > committer: Aleksey Midenkov <midenok(a)gmail.com> > > timestamp: 2018-06-28 13:42:09 +0300 > > message: > > > > MDEV-16546 System versioning setting to allow history modification > > > > 1. Add server variable system_versioning_modify_history which will > > allow to set values for row_start, row_end in DML operations. > > > > 2. If secure_timestamp is YES or REPLICATION, > > system_versioning_modify_history does not have effect. If > > secure_timestamp is SUPER, system_versioning_modify_history requires > > special privilege (same as for setting current timestamp). > > I thought more about this idea. We don't really want to have the history > editable, do we? Well, I'm thinking about rollback table data to specific point in time. That could be a useful feature. > But it's needed for replication, to keep the master and > slave identical. That's what secure_timestamp is for. > > The idea was that this new variable, system_versioning_modify_history, > will be just a convenience feature, it will not allow history editing > any more than one can do without it. > > But now I suspect that even with secure_timestamp=NO one cannot truly > edit history. One can only insert new rows with arbitrary timestamps. > For example, to insert a row with row_start=1000 and row_end=2000, one > needs to do (if secure_timestamp=NO): > > set timestamp=1000; > insert row; > set timestamp=2000; > delete row; > > But I don't see how one can update or delete a history row with > secure_timestamp=NO. > > Now, with a SUPER privilege and secure_timestamp=NO or SUPER, one can > use the BINLOG command and truly edit the history arbitrarily, by faking > row events. I don't really get it why this is so important: since there is some limitation by configuration and privilege, we are just fine. Everything can be changed at filesystem level after all. > > The conclusion, I believe, is that system_versioning_modify_history > should allow INSERTs when secure_timestamp=NO, and it should allow > UPDATE/DELETE only for a SUPER user when secure_timestamp=NO or SUPER. I don't see a reason to argue on that. The only thing that is not clear, why we don't allow INSERTs when secure_timestamp=SUPER? > > The second thing I don't like at all, is when a table is created like > > CREATE TABLE t1 (a int) WITH SYSTEM VERSIONING > > with row_start/row_end implicit. You don't have it in the test, but > anyway one should be able to load history into such a table, while the > table does not have row_start and row_end columns. From the user point > of view these columns don't exist, they're pseudo-columns, like ROWID. > They just cannot be insertable-into, conceptually. But a user will want > to restore the history, right? I don't have a solution for this yet :( > Any ideas? We don't have to follow the conception if it doesn't help us. Since we have physical row_start/row_end, we don't have to pretend they don't exist. Who will win from that? > > See below a couple of minor comments about the patch itself. > > ... These are going to be fixed. > > Regards, > Sergei > Chief Architect MariaDB > and security(a)mariadb.org -- All the best, Aleksey Midenkov @midenok

2 15

[Maria-developers] MariaDB for CentOS 8
by Thomas Stephen Lee 12 Dec '19

12 Dec '19

Hi, Please release MariaDB 10.4 for CentOS 8. https://downloads.mariadb.org/mariadb/repositories/ does not show CentOS 8. thanks -- Thomas Stephen Lee

3 5

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

Re: [Maria-developers] 7a331ec820b: MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val.
by Sergei Golubchik 25 Oct '19

25 Oct '19

Hi, Alexey! On Oct 25, Alexey Botchkov wrote: > revision-id: 7a331ec820b (mariadb-10.3.18-65-g7a331ec820b) > parent(s): 716d396bb3b > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2019-10-22 01:40:48 +0400 > message: > > MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val. > > Autoincrement calculation should check if tables were opened. > > --- > mysql-test/main/partition_innodb.result | 13 +++++++++++++ > mysql-test/main/partition_innodb.test | 15 +++++++++++++++ > sql/ha_partition.cc | 2 ++ > 3 files changed, 30 insertions(+) > > diff --git a/mysql-test/main/partition_innodb.result b/mysql-test/main/partition_innodb.result > index f3d24347ff9..93a73a785fe 100644 > --- a/mysql-test/main/partition_innodb.result > +++ b/mysql-test/main/partition_innodb.result > @@ -1028,5 +1028,18 @@ COUNT(*) > 2 > DROP TABLE t1; > # > +# MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val > +# > +CREATE TABLE t1 (a INT) > +ENGINE=InnoDB > +PARTITION BY RANGE (a) ( > +PARTITION p0 VALUES LESS THAN (6), > +PARTITION pn VALUES LESS THAN MAXVALUE > +); > +INSERT INTO t1 VALUES (4),(5),(6); > +ALTER TABLE t1 MODIFY a INT AUTO_INCREMENT PRIMARY KEY; > +UPDATE t1 PARTITION (p0) SET a = 3 WHERE a = 5; > +DROP TABLE t1; > +# > # End of 10.3 tests > # > diff --git a/mysql-test/main/partition_innodb.test b/mysql-test/main/partition_innodb.test > index 629bc29e758..1e0af06b4da 100644 > --- a/mysql-test/main/partition_innodb.test > +++ b/mysql-test/main/partition_innodb.test > @@ -1105,6 +1105,21 @@ INSERT INTO t1 VALUES (1, 7, 8, 9), (2, NULL, NULL, NULL), (3, NULL, NULL, NULL) > SELECT COUNT(*) FROM t1 WHERE x IS NULL AND y IS NULL AND z IS NULL; > DROP TABLE t1; > > +--echo # > +--echo # MDEV-18244 Server crashes in ha_innobase::update_thd / ... / ha_partition::update_next_auto_inc_val > +--echo # > + > +CREATE TABLE t1 (a INT) > + ENGINE=InnoDB > + PARTITION BY RANGE (a) ( > + PARTITION p0 VALUES LESS THAN (6), > + PARTITION pn VALUES LESS THAN MAXVALUE > + ); > +INSERT INTO t1 VALUES (4),(5),(6); > +ALTER TABLE t1 MODIFY a INT AUTO_INCREMENT PRIMARY KEY; > +UPDATE t1 PARTITION (p0) SET a = 3 WHERE a = 5; > +DROP TABLE t1; > + > --echo # > --echo # End of 10.3 tests > --echo # > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 5a78249644d..e461532db1a 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -8174,6 +8174,8 @@ int ha_partition::info(uint flag) > ("checking all partitions for auto_increment_value")); > do > { > + if (!bitmap_is_set(&m_opened_partitions, (uint)(file_array - m_file))) > + continue; > file= *file_array; > file->info(HA_STATUS_AUTO | no_lock_flag); > set_if_bigger(auto_increment_value, It seems that the intention was to find the max auto_increment_value over all partitions. If you skip some, it won't be max anymore. On the other hand, I don't see why UPDATE should get the max auto-inc of all partitions. May be the correct fix would be not to do it in UPDATE? Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 52f3829b95c: MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes
by Sergei Golubchik 25 Oct '19

25 Oct '19

Hi, Sachin! This is ok to push. But, still, about Aria. When you push the fix for MDEV-18791, it will be very easy to forget this one. May be you add the same test case for Aria now? It'll fail with something like "not supported" error, which is fine, just use --error. But when you fix MDEV-18791 this test will be enabled and it'll ensure you won't forget to fix Aria for bulk inserts. On Oct 25, Sachin Setiya wrote: > revision-id: 52f3829b95c (mariadb-10.4.7-110-g52f3829b95c) > parent(s): 93d2211e02f > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-10-09 21:17:32 +0530 > message: > > MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes > > Problem:- > > So the issue is when we do bulk insert with rows > > MI_MIN_ROWS_TO_DISABLE_INDEXES(100) , We try to disable the indexes to > speedup insert. But current logic also disables the long unique indexes. > > Solution:- In ha_myisam::start_bulk_insert if we find long hash index > (HA_KEY_ALG_LONG_HASH) we will not disable the index. > > This commit also refactors the mi_disable_indexes_for_rebuild function, > Since this is function is called at only one place, it is inlined into > start_bulk_insert > > mi_clear_key_active is added into myisamdef.h because now it is also used > in ha_myisam.cc file. > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0