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

Re: [Maria-developers] a0e3bd09251: Part1: MDEV-20837 Add MariaDB_FUNCTION_PLUGIN
by Sergei Golubchik 16 Oct '19

16 Oct '19

Hi, Alexander! Just a couple of comments, see below: On Oct 16, Alexander Barkov wrote: > revision-id: a0e3bd09251 (mariadb-10.4.4-419-ga0e3bd09251) > parent(s): 22b645ef529 > author: Alexander Barkov <bar(a)mariadb.com> > committer: Alexander Barkov <bar(a)mariadb.com> > timestamp: 2019-10-16 16:26:29 +0400 > message: > > Part1: MDEV-20837 Add MariaDB_FUNCTION_PLUGIN > > - Defining MariaDB_FUNCTION_PLUGIN > - Changing the code in /plugins/type_inet/ and /plugins/type_test/ > to use MariaDB_FUNCTION_PLUGIN instead of MariaDB_FUNCTION_COLLECTION_PLUGIN. > diff --git a/include/mysql/plugin_function.h b/include/mysql/plugin_function.h > new file mode 100644 > index 00000000000..4ce416612e9 > --- /dev/null > +++ b/include/mysql/plugin_function.h > @@ -0,0 +1,58 @@ > +#ifndef MARIADB_PLUGIN_FUNCTION_INCLUDED > +#define MARIADB_PLUGIN_FUNCTION_INCLUDED > +/* Copyright (C) 2019, Alexander Barkov and MariaDB > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program; if not, write to the Free Software > + Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1335 USA */ > + > +/** > + @file > + > + Function Plugin API. > + > + This file defines the API for server plugins that manage functions. > +*/ > + > +#ifdef __cplusplus > + > +#include <mysql/plugin.h> > + > +/* > + API for function plugins. (MariaDB_FUNCTION_PLUGIN) > +*/ > +#define MariaDB_FUNCTION_INTERFACE_VERSION (MYSQL_VERSION_ID << 8) > + > + > +class Plugin_function > +{ > + int m_interface_version; > + Create_func *m_builder; > +public: > + Plugin_function(int interface_version, Create_func *builder) > + :m_interface_version(interface_version), > + m_builder(builder) > + { } Why do you need this ^^^ constructor? > + Plugin_function(Create_func *builder) > + :m_interface_version(MariaDB_FUNCTION_INTERFACE_VERSION), > + m_builder(builder) > + { } > + Create_func *create_func() > + { > + return m_builder; > + } > +}; > + > + > +#endif /* __cplusplus */ > + > +#endif /* MARIADB_PLUGIN_FUNCTION_INCLUDED */ > diff --git a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > index 4663ae485e2..a9422f2e4fd 100644 > --- a/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > +++ b/plugin/type_inet/mysql-test/type_inet/func_inet_plugin.result > @@ -15,14 +16,94 @@ PLUGIN_LICENSE, > PLUGIN_MATURITY, > PLUGIN_AUTH_VERSION > FROM INFORMATION_SCHEMA.PLUGINS > -WHERE PLUGIN_TYPE='FUNCTION COLLECTION' > - AND PLUGIN_NAME='func_inet'; > -PLUGIN_NAME func_inet > +WHERE PLUGIN_TYPE='FUNCTION' > + AND PLUGIN_NAME IN > +('inet_aton', > +'inet_ntoa', > +'inet6_aton', > +'inet6_ntoa', > +'is_ipv4', > +'is_ipv6', > +'is_ipv4_compat', > +'is_ipv4_mapped') > +ORDER BY PLUGIN_NAME; > +---- ---- > +PLUGIN_NAME inet6_aton > PLUGIN_VERSION 1.0 > PLUGIN_STATUS ACTIVE > -PLUGIN_TYPE FUNCTION COLLECTION > +PLUGIN_TYPE FUNCTION > PLUGIN_AUTHOR MariaDB Corporation > -PLUGIN_DESCRIPTION Function collection test > +PLUGIN_DESCRIPTION Function INET6_ATON() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental INET* functions should probably be Alpha, not Experimental. You expect them to be GA one day, don't you? > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet6_ntoa > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET6_NTOA() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet_aton > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET_ATON() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME inet_ntoa > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function INET_NTOA() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4 > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4_compat > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4_COMPAT() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv4_mapped > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV4_MAPPED() > +PLUGIN_LICENSE GPL > +PLUGIN_MATURITY Experimental > +PLUGIN_AUTH_VERSION 1.0 > +---- ---- > +PLUGIN_NAME is_ipv6 > +PLUGIN_VERSION 1.0 > +PLUGIN_STATUS ACTIVE > +PLUGIN_TYPE FUNCTION > +PLUGIN_AUTHOR MariaDB Corporation > +PLUGIN_DESCRIPTION Function IS_IPV6() > PLUGIN_LICENSE GPL > PLUGIN_MATURITY Experimental > PLUGIN_AUTH_VERSION 1.0 > diff --git a/sql/item_create.cc b/sql/item_create.cc > index e8eb76dfc12..e316723cedf 100644 > --- a/sql/item_create.cc > +++ b/sql/item_create.cc > @@ -5704,12 +5657,26 @@ void item_create_cleanup() > { > DBUG_ENTER("item_create_cleanup"); > my_hash_free(& native_functions_hash); > -#ifdef HAVE_SPATIAL > - plugin_function_collection_geometry.deinit(); > -#endif > DBUG_VOID_RETURN; > } > > + > +static Create_func * > +function_plugin_find_native_function_builder(THD *thd, const LEX_CSTRING &name) > +{ > + plugin_ref plugin; > + if ((plugin= my_plugin_lock_by_name(thd, &name, MariaDB_FUNCTION_PLUGIN))) > + { > + Create_func *builder= > + reinterpret_cast<Plugin_function*>(plugin_decl(plugin)->info)-> > + create_func(); > + plugin_unlock(thd, plugin); > + return builder; > + } > + return NULL; So, function plugins cannot be unlocked either? > +} > + > + > Create_func * > find_native_function_builder(THD *thd, const LEX_CSTRING *name) > { > @@ -5724,16 +5691,10 @@ find_native_function_builder(THD *thd, const LEX_CSTRING *name) > if (func && (builder= func->builder)) > return builder; > > - if ((builder= Plugin_find_native_func_builder_param(*name).find(thd))) > + if ((builder= function_plugin_find_native_function_builder(thd, *name))) this is what I mean by "special code path for plugins" and want to get rid of. But not in this commit, I agree. > return builder; > > -#ifdef HAVE_SPATIAL > - if (!builder) > - builder= plugin_function_collection_geometry. > - find_native_function_builder(thd, *name); > -#endif > - > - return builder; > + return NULL; > } > > Create_qfunc * Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

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

14 Oct '19

Hi, Sachin! On Aug 20, Sachin Setiya wrote: > revision-id: c776ed4d6ec (mariadb-10.4.5-153-gc776ed4d6ec) > parent(s): 4ca016237f1 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-07-30 03:42:21 +0530 > message: > > MDEV-19848 Server crashes in check_vcol_forward_refs upon INSERT DELAYED into table with long blob key > > There are 2 issues > > 1st:- in make_new_field when we & into new field flag we forget > LONG_UNIQUE_HASH_FIELD Flag. > > 2nd:- We are calling parse_vcol_defs on keyinfo , but they are not in right > form. We should call setup_keyinfo_hash_all before calling parse_vcol_defs This looks quite suspicious. Ideally, Delayed_insert::get_local_table() should just create a copy of the table, but not modify the original table. In your case it constantly modifies KEY's of the original table. It's kind of dirty. As for the real bug, see what parse_vcol_defs() is doing for long unique fields. In particular, it sets keypart->field->vcol_info= table->field[keypart->field->field_index]->vcol_info; but the keypart here is in the original Delayed_insert::table, while field and vcol_info is in a copy (created in Delayed_insert::get_local_table). This seems to be just wrong. I'd really prefer Delayed_insert::get_local_table to not change anything in the Delayed_insert::table. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] Performance of index intersection
by David Sickmiller 13 Oct '19

13 Oct '19

Hi! I've been using MySQL/MariaDB for two decades but have more recently been working with Elasticsearch. I knew to expect an inverted index to speed up querying full text fields, but I've been surprised (and a bit annoyed) at how fast ES can query structured data. (In my case, I'm largely looking for intersections of a number of varchar fields with lowish cardinality, e.g. WHERE country = 'US' AND client = 'Microsoft' AND status = 'Completed'.) Elasticsearch seems to have several things going on, but I think a core aspect, to use RDMS terminology, is that each column is indexed, and index unions/intersections are used if the WHERE clause references multiple columns. I've heard that MySQL/MariaDB has the ability to merge indexes, but I've rarely observed it in person. Googling for it yields a bunch of StackOverflow posts complaining how slow it is, with responses agreeing and explaining how to disable it. If I'm reading the MySQL/MariaDB code correctly, it looks like MariaDB will intersect indexes by looping through each index, reading the rowids of all matching keys and then, at the end (or once the buffer is full), checking whether each rowid is present in each index. I wonder if there's an opportunity to speed this up. If we first read in the rowids from one index (ideally the one with the fewest matches), we could tell the storage engine that, when reading the next index, it should skip over rowids lower than the next candidate intersection. In the best case scenario, I think this could enable InnoDB to use its page directory to skip past some of the keys, improving the performance from O(n) to O(log n). That said, this is all new to me. Maybe there's an obvious reason this wouldn't make much of an improvement, or maybe I've overlooked that it's already been done. However, if it looks promising to you folk, and it's something you'd consider merging, I'd be willing to attempt writing a PR for it. Thank you, David Sickmiller

3 6

Re: [Maria-developers] Missing memory barrier in parallel replication error handler in wait_for_prior_commit()?
by Kristian Nielsen 11 Oct '19

11 Oct '19

sujatha <sujatha.sivakumar(a)mariadb.com> writes: > I have a doubt. A simple fix as per the earlier mail discussion would > be to swap the > > order of assignments as shown below. > > wakeup_error= true > waitee= NULL > Why cannot we use the simpler approach. Please provide your inputs. This is because of the need for memory barriers, to prevent compiler and/or modern high-performance CPUs from re-ordering the memory accesses. There is nothing in C that requires the compiler to make the two assignments in the same order as in the source code. And even if it does, the CPU is free to perform the writes and/or the corresponding loads in the opposite order (the x86 architecture doesn't do that, but other architectures do). There are many resources on the net on the need for memory barriers, for example this from one of your collegues: https://mariadb.org/wp-content/uploads/2017/11/2017-11-Memory-barriers.pdf But it will be difficult to make a test case, I think. Because after correcting the order of the stores there is nowhere to put a debug-sleep, and there also is no easy way to force the CPU to take the incorrect memory order. - Kristian.

1 0

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

09 Oct '19

Hi, Sachin! On Aug 20, Sachin Setiya wrote: > revision-id: 4ca016237f1 (mariadb-10.4.5-152-g4ca016237f1) > parent(s): 4a5cd407289 > author: Sachin <sachin.setiya(a)mariadb.com> > committer: Sachin <sachin.setiya(a)mariadb.com> > timestamp: 2019-07-29 19:33:05 +0530 > message: > > MDEV-20001 Potential dangerous regression: INSERT INTO >=100 rows fail for myisam table with HASH indexes > > Dont deactivate the long unique keys on bulk insert. > > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index f478e01e441..c1169737911 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -1749,7 +1749,16 @@ void ha_myisam::start_bulk_insert(ha_rows rows, uint flags) > else > { > my_bool all_keys= MY_TEST(flags & HA_CREATE_UNIQUE_INDEX_BY_SORT); > - mi_disable_indexes_for_rebuild(file, rows, all_keys); > + if (table->s->long_unique_table) > + { > + ulonglong hash_key_map= 0ULL; > + for(uint i= 0; i < table->s->keys; i++) > + if (table->key_info[i].algorithm == HA_KEY_ALG_LONG_HASH) > + mi_set_key_active(hash_key_map, i); > + mi_disable_indexes_for_rebuild(file, rows, all_keys, hash_key_map); > + } > + else > + mi_disable_indexes_for_rebuild(file, rows, all_keys, 0ULL); I agree with the fix, but here's a comment about the implementation. mi_disable_indexes_for_rebuild() is _only_ used here, nowhere else. So, I think I'd just removed mi_disable_indexes_for_rebuild(), inlined it here and do everything in one loop. Note, that you also need to fix Aria engine, it has almost the same code. And add a test for it, please. > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: [Maria-developers] [Commits] d48294d15c4: MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on
by Sergey Petrunia 09 Oct '19

09 Oct '19

Hi Varun, This patch is a step in the right direction, but I think we're not quite there yet. The patch adds logic about semi-join materialization into the filesort code This makes things really messy. I think, some isolation would be beneficial. The first suggestion: In Sort_param (as this structure is visible in find_all_keys), add members that control the behavior inside find_all_keys: - unpack_function (default is NULL) - bool set_all_read_bits (default is FALSE) Now, Sort_param is allocated in filesort(), so we will have to add the same members in "class Filesort" and have filesort() copy them to Sort_param (necessary because find_all_keys() doesn't see the "Filesort" object). Then, these members can be set in the SQL layer e.g. in JOIN::add_sorting_to_table which creates the Filesort structure. (It would be ideal if semi-join code set them, but this is probably not possible to achieve as Filesort is set up after the semi-join code is set up). If after all these changes "JOIN_TAB::unpacking_to_base_table_fields()" is still there, please rename it to "unpack_..." so that it follows the established convention. That's all the input. On Sat, Sep 28, 2019 at 02:12:01PM +0530, Varun wrote: > revision-id: d48294d15c4895215d5facef97fc80c03cd6b4b0 (mariadb-10.4.4-341-gd48294d15c4) > parent(s): a340af922361e3958e5d6653c8b840771db282f2 > author: Varun Gupta > committer: Varun Gupta > timestamp: 2019-09-28 13:06:44 +0530 > message: > > MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > > For the case when the SJM scan table is the first table in the join order, > then if we want to do the sorting on the SJM scan table, then we need to > make sure that we unpack the values to base table fields in two cases: > 1) Reading the SJM table and writing the sort-keys inside the sort-buffer > 2) Reading the sorted data from the sort file > > --- > mysql-test/main/order_by.result | 138 +++++++++++++++++++++++++++++++++++++++- > mysql-test/main/order_by.test | 34 ++++++++++ > sql/filesort.cc | 17 +++++ > sql/opt_subselect.cc | 10 ++- > sql/records.cc | 13 ++++ > sql/records.h | 1 + > sql/sql_select.cc | 89 ++++++++++---------------- > sql/sql_select.h | 4 +- > sql/table.h | 1 + > 9 files changed, 246 insertions(+), 61 deletions(-) > > diff --git a/mysql-test/main/order_by.result b/mysql-test/main/order_by.result > index b059cc686cd..e74583670fc 100644 > --- a/mysql-test/main/order_by.result > +++ b/mysql-test/main/order_by.result > @@ -3322,7 +3322,7 @@ WHERE books.library_id = 8663 AND > books.scheduled_for_removal=0 ) > ORDER BY wings.id; > id select_type table type possible_keys key key_len ref rows filtered Extra > -1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 2 100.00 Using temporary; Using filesort > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 2 100.00 Using filesort > 1 PRIMARY wings eq_ref PRIMARY PRIMARY 4 test.books.wings_id 1 100.00 > 2 MATERIALIZED books ref library_idx library_idx 4 const 2 100.00 Using where > Warnings: > @@ -3436,3 +3436,139 @@ Note 1003 select `test`.`t4`.`a` AS `a`,`test`.`t4`.`b` AS `b`,`test`.`t4`.`c` A > set histogram_size=@tmp_h, histogram_type=@tmp_ht, use_stat_tables=@tmp_u, > optimizer_use_condition_selectivity=@tmp_o; > drop table t1,t2,t3,t4; > +# > +# MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > +# > +CREATE TABLE t1 (a INT, b int, primary key(a)); > +CREATE TABLE t2 (a INT, b INT); > +INSERT INTO t1 (a,b) VALUES (58,1),(96,2),(273,3),(23,4),(231,5),(525,6), > +(2354,7),(321421,3),(535,2),(4535,3); > +INSERT INTO t2 (a,b) VALUES (58,3),(96,3),(273,3); > +# Join order should have the SJM scan table as the first table for both > +# the queries with GROUP BY and ORDER BY clause. > +EXPLAIN SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +id select_type table type possible_keys key key_len ref rows Extra > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 3 Using filesort > +1 PRIMARY t1 eq_ref PRIMARY PRIMARY 4 test.t2.a 1 Using index > +2 MATERIALIZED t2 ALL NULL NULL NULL NULL 3 Using where > +EXPLAIN FORMAT=JSON SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "read_sorted_file": { > + "filesort": { > + "sort_key": "t1.a desc", > + "table": { > + "table_name": "<subquery2>", > + "access_type": "ALL", > + "possible_keys": ["distinct_key"], > + "rows": 3, > + "filtered": 100, > + "materialized": { > + "unique": 1, > + "query_block": { > + "select_id": 2, > + "table": { > + "table_name": "t2", > + "access_type": "ALL", > + "rows": 3, > + "filtered": 100, > + "attached_condition": "t2.b = 3 and t2.a is not null" > + } > + } > + } > + } > + } > + }, > + "table": { > + "table_name": "t1", > + "access_type": "eq_ref", > + "possible_keys": ["PRIMARY"], > + "key": "PRIMARY", > + "key_length": "4", > + "used_key_parts": ["a"], > + "ref": ["test.t2.a"], > + "rows": 1, > + "filtered": 100, > + "using_index": true > + } > + } > +} > +SELECT t1.a > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +ORDER BY t1.a DESC; > +a > +273 > +96 > +58 > +EXPLAIN SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +id select_type table type possible_keys key key_len ref rows Extra > +1 PRIMARY <subquery2> ALL distinct_key NULL NULL NULL 3 Using filesort > +1 PRIMARY t1 eq_ref PRIMARY PRIMARY 4 test.t2.a 1 > +2 MATERIALIZED t2 ALL NULL NULL NULL NULL 3 Using where > +EXPLAIN FORMAT=JSON SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +EXPLAIN > +{ > + "query_block": { > + "select_id": 1, > + "read_sorted_file": { > + "filesort": { > + "sort_key": "t1.a desc", > + "table": { > + "table_name": "<subquery2>", > + "access_type": "ALL", > + "possible_keys": ["distinct_key"], > + "rows": 3, > + "filtered": 100, > + "materialized": { > + "unique": 1, > + "query_block": { > + "select_id": 2, > + "table": { > + "table_name": "t2", > + "access_type": "ALL", > + "rows": 3, > + "filtered": 100, > + "attached_condition": "t2.b = 3 and t2.a is not null" > + } > + } > + } > + } > + } > + }, > + "table": { > + "table_name": "t1", > + "access_type": "eq_ref", > + "possible_keys": ["PRIMARY"], > + "key": "PRIMARY", > + "key_length": "4", > + "used_key_parts": ["a"], > + "ref": ["test.t2.a"], > + "rows": 1, > + "filtered": 100 > + } > + } > +} > +SELECT t1.a, group_concat(t1.b) > +FROM t1 > +WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > +GROUP BY t1.a DESC; > +a group_concat(t1.b) > +273 3 > +96 2 > +58 1 > +DROP TABLE t1, t2; > diff --git a/mysql-test/main/order_by.test b/mysql-test/main/order_by.test > index 934c503302f..b3e43d27e2f 100644 > --- a/mysql-test/main/order_by.test > +++ b/mysql-test/main/order_by.test > @@ -2276,3 +2276,37 @@ set histogram_size=@tmp_h, histogram_type=@tmp_ht, use_stat_tables=@tmp_u, > optimizer_use_condition_selectivity=@tmp_o; > > drop table t1,t2,t3,t4; > + > + > +--echo # > +--echo # MDEV-13694: Wrong result upon GROUP BY with orderby_uses_equalities=on > +--echo # > + > +CREATE TABLE t1 (a INT, b int, primary key(a)); > +CREATE TABLE t2 (a INT, b INT); > + > +INSERT INTO t1 (a,b) VALUES (58,1),(96,2),(273,3),(23,4),(231,5),(525,6), > + (2354,7),(321421,3),(535,2),(4535,3); > +INSERT INTO t2 (a,b) VALUES (58,3),(96,3),(273,3); > + > +--echo # Join order should have the SJM scan table as the first table for both > +--echo # the queries with GROUP BY and ORDER BY clause. > + > +let $query= SELECT t1.a > + FROM t1 > + WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > + ORDER BY t1.a DESC; > + > +eval EXPLAIN $query; > +eval EXPLAIN FORMAT=JSON $query; > +eval $query; > + > +let $query= SELECT t1.a, group_concat(t1.b) > + FROM t1 > + WHERE t1.a IN (SELECT a FROM t2 WHERE b=3) > + GROUP BY t1.a DESC; > + > +eval EXPLAIN $query; > +eval EXPLAIN FORMAT=JSON $query; > +eval $query; > +DROP TABLE t1, t2; > diff --git a/sql/filesort.cc b/sql/filesort.cc > index 3f4291cfb1f..e5c83293e9f 100644 > --- a/sql/filesort.cc > +++ b/sql/filesort.cc > @@ -716,11 +716,21 @@ static ha_rows find_all_keys(THD *thd, Sort_param *param, SQL_SELECT *select, > *found_rows= 0; > ref_pos= &file->ref[0]; > next_pos=ref_pos; > + JOIN_TAB *tab= sort_form->reginfo.join_tab; > + JOIN *join= tab ? tab->join : NULL; > + bool first_is_in_sjm_nest= FALSE; > > DBUG_EXECUTE_IF("show_explain_in_find_all_keys", > dbug_serve_apcs(thd, 1); > ); > > + if (join && join->table_count != join->const_tables && > + (join->join_tab + join->const_tables == tab)) > + { > + TABLE_LIST *tbl_for_first= sort_form->pos_in_table_list; > + first_is_in_sjm_nest= tbl_for_first && tbl_for_first->is_sjm_scan_table(); > + } > + > if (!quick_select) > { > next_pos=(uchar*) 0; /* Find records in sequence */ > @@ -756,13 +766,20 @@ static ha_rows find_all_keys(THD *thd, Sort_param *param, SQL_SELECT *select, > goto err; > } > > + if (first_is_in_sjm_nest) > + sort_form->column_bitmaps_set(save_read_set, save_write_set); > + > DEBUG_SYNC(thd, "after_index_merge_phase1"); > for (;;) > { > if (quick_select) > error= select->quick->get_next(); > else /* Not quick-select */ > + { > error= file->ha_rnd_next(sort_form->record[0]); > + if (first_is_in_sjm_nest) > + tab->unpacking_to_base_table_fields(); > + } > if (unlikely(error)) > break; > file->position(sort_form->record[0]); > diff --git a/sql/opt_subselect.cc b/sql/opt_subselect.cc > index 87458357865..f837a6394af 100644 > --- a/sql/opt_subselect.cc > +++ b/sql/opt_subselect.cc > @@ -4252,11 +4252,11 @@ bool setup_sj_materialization_part2(JOIN_TAB *sjm_tab) > sjm_tab->type= JT_ALL; > > /* Initialize full scan */ > - sjm_tab->read_first_record= join_read_record_no_init; > + sjm_tab->read_first_record= join_init_read_record; > sjm_tab->read_record.copy_field= sjm->copy_field; > sjm_tab->read_record.copy_field_end= sjm->copy_field + > sjm->sjm_table_cols.elements; > - sjm_tab->read_record.read_record_func= rr_sequential_and_unpack; > + sjm_tab->read_record.read_record_func= read_record_func_for_rr_and_unpack; > } > > sjm_tab->bush_children->end[-1].next_select= end_sj_materialize; > @@ -7105,3 +7105,9 @@ bool Item_in_subselect::pushdown_cond_for_in_subquery(THD *thd, Item *cond) > thd->lex->current_select= save_curr_select; > DBUG_RETURN(FALSE); > } > + > + > +bool TABLE_LIST::is_sjm_scan_table() > +{ > + return is_active_sjm() && sj_mat_info->is_sj_scan; > +} > diff --git a/sql/records.cc b/sql/records.cc > index 3d709182a4e..f6885f773d5 100644 > --- a/sql/records.cc > +++ b/sql/records.cc > @@ -709,3 +709,16 @@ static int rr_cmp(uchar *a,uchar *b) > return (int) a[7] - (int) b[7]; > #endif > } > + > + > +int read_record_func_for_rr_and_unpack(READ_RECORD *info) > +{ > + int error; > + if ((error= info->read_record_func_and_unpack_calls(info))) > + return error; > + > + for (Copy_field *cp= info->copy_field; cp != info->copy_field_end; cp++) > + (*cp->do_copy)(cp); > + > + return error; > +} > diff --git a/sql/records.h b/sql/records.h > index faf0d13c9a9..037a06b9d34 100644 > --- a/sql/records.h > +++ b/sql/records.h > @@ -55,6 +55,7 @@ struct READ_RECORD > TABLE *table; /* Head-form */ > Unlock_row_func unlock_row; > Read_func read_record_func; > + Read_func read_record_func_and_unpack_calls; > THD *thd; > SQL_SELECT *select; > uint ref_length, reclength, rec_cache_size, error_offset; > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 36d9eda3383..28bc57c692f 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -14015,37 +14015,8 @@ remove_const(JOIN *join,ORDER *first_order, COND *cond, > can be used without tmp. table. > */ > bool can_subst_to_first_table= false; > - bool first_is_in_sjm_nest= false; > - if (first_is_base_table) > - { > - TABLE_LIST *tbl_for_first= > - join->join_tab[join->const_tables].table->pos_in_table_list; > - first_is_in_sjm_nest= tbl_for_first->sj_mat_info && > - tbl_for_first->sj_mat_info->is_used; > - } > - /* > - Currently we do not employ the optimization that uses multiple > - equalities for ORDER BY to remove tmp table in the case when > - the first table happens to be the result of materialization of > - a semi-join nest ( <=> first_is_in_sjm_nest == true). > - > - When a semi-join nest is materialized and scanned to look for > - possible matches in the remaining tables for every its row > - the fields from the result of materialization are copied > - into the record buffers of tables from the semi-join nest. > - So these copies are used to access the remaining tables rather > - than the fields from the result of materialization. > - > - Unfortunately now this so-called 'copy back' technique is > - supported only if the rows are scanned with the rr_sequential > - function, but not with other rr_* functions that are employed > - when the result of materialization is required to be sorted. > - > - TODO: either to support 'copy back' technique for the above case, > - or to get rid of this technique altogether. > - */ > if (optimizer_flag(join->thd, OPTIMIZER_SWITCH_ORDERBY_EQ_PROP) && > - first_is_base_table && !first_is_in_sjm_nest && > + first_is_base_table && > order->item[0]->real_item()->type() == Item::FIELD_ITEM && > join->cond_equal) > { > @@ -19922,19 +19893,6 @@ do_select(JOIN *join, Procedure *procedure) > } > > > -int rr_sequential_and_unpack(READ_RECORD *info) > -{ > - int error; > - if (unlikely((error= rr_sequential(info)))) > - return error; > - > - for (Copy_field *cp= info->copy_field; cp != info->copy_field_end; cp++) > - (*cp->do_copy)(cp); > - > - return error; > -} > - > - > /** > @brief > Instantiates temporary table > @@ -21223,6 +21181,8 @@ bool test_if_use_dynamic_range_scan(JOIN_TAB *join_tab) > > int join_init_read_record(JOIN_TAB *tab) > { > + bool need_unpacking= FALSE; > + JOIN *join= tab->join; > /* > Note: the query plan tree for the below operations is constructed in > save_agg_explain_data. > @@ -21232,6 +21192,12 @@ int join_init_read_record(JOIN_TAB *tab) > if (tab->filesort && tab->sort_table()) // Sort table. > return 1; > > + if (join->top_join_tab_count != join->const_tables) > + { > + TABLE_LIST *tbl= tab->table->pos_in_table_list; > + need_unpacking= tbl ? tbl->is_sjm_scan_table() : FALSE; > + } > + > tab->build_range_rowid_filter_if_needed(); > > DBUG_EXECUTE_IF("kill_join_init_read_record", > @@ -21249,16 +21215,6 @@ int join_init_read_record(JOIN_TAB *tab) > if (!tab->preread_init_done && tab->preread_init()) > return 1; > > - > - if (init_read_record(&tab->read_record, tab->join->thd, tab->table, > - tab->select, tab->filesort_result, 1,1, FALSE)) > - return 1; > - return tab->read_record.read_record(); > -} > - > -int > -join_read_record_no_init(JOIN_TAB *tab) > -{ > Copy_field *save_copy, *save_copy_end; > > /* > @@ -21268,12 +21224,20 @@ join_read_record_no_init(JOIN_TAB *tab) > save_copy= tab->read_record.copy_field; > save_copy_end= tab->read_record.copy_field_end; > > - init_read_record(&tab->read_record, tab->join->thd, tab->table, > - tab->select, tab->filesort_result, 1, 1, FALSE); > + > + if (init_read_record(&tab->read_record, tab->join->thd, tab->table, > + tab->select, tab->filesort_result, 1, 1, FALSE)) > + return 1; > > tab->read_record.copy_field= save_copy; > tab->read_record.copy_field_end= save_copy_end; > - tab->read_record.read_record_func= rr_sequential_and_unpack; > + > + if (need_unpacking) > + { > + tab->read_record.read_record_func_and_unpack_calls= > + tab->read_record.read_record_func; > + tab->read_record.read_record_func = read_record_func_for_rr_and_unpack; > + } > > return tab->read_record.read_record(); > } > @@ -28981,6 +28945,19 @@ void build_notnull_conds_for_inner_nest_of_outer_join(JOIN *join, > } > > > +/* > + @brief > + Unpacking temp table fields to base table fields. > +*/ > + > +void JOIN_TAB::unpacking_to_base_table_fields() > +{ > + for (Copy_field *cp= read_record.copy_field; > + cp != read_record.copy_field_end; cp++) > + (*cp->do_copy)(cp); > +} > + > + > /** > @} (end of group Query_Optimizer) > */ > diff --git a/sql/sql_select.h b/sql/sql_select.h > index 4f7bf49f635..545d4a788cc 100644 > --- a/sql/sql_select.h > +++ b/sql/sql_select.h > @@ -223,7 +223,7 @@ typedef enum_nested_loop_state > (*Next_select_func)(JOIN *, struct st_join_table *, bool); > Next_select_func setup_end_select_func(JOIN *join, JOIN_TAB *tab); > int rr_sequential(READ_RECORD *info); > -int rr_sequential_and_unpack(READ_RECORD *info); > +int read_record_func_for_rr_and_unpack(READ_RECORD *info); > Item *remove_pushed_top_conjuncts(THD *thd, Item *cond); > Item *and_new_conditions_to_optimized_cond(THD *thd, Item *cond, > COND_EQUAL **cond_eq, > @@ -676,6 +676,7 @@ typedef struct st_join_table { > table_map remaining_tables); > bool fix_splitting(SplM_plan_info *spl_plan, table_map remaining_tables, > bool is_const_table); > + void unpacking_to_base_table_fields(); > } JOIN_TAB; > > > @@ -2352,7 +2353,6 @@ create_virtual_tmp_table(THD *thd, Field *field) > > int test_if_item_cache_changed(List<Cached_item> &list); > int join_init_read_record(JOIN_TAB *tab); > -int join_read_record_no_init(JOIN_TAB *tab); > void set_position(JOIN *join,uint idx,JOIN_TAB *table,KEYUSE *key); > inline Item * and_items(THD *thd, Item* cond, Item *item) > { > diff --git a/sql/table.h b/sql/table.h > index 1a7e5fbd4dc..35ba9bbb95d 100644 > --- a/sql/table.h > +++ b/sql/table.h > @@ -2622,6 +2622,7 @@ struct TABLE_LIST > */ > const char *get_table_name() const { return view != NULL ? view_name.str : table_name.str; } > bool is_active_sjm(); > + bool is_sjm_scan_table(); > bool is_jtbm() { return MY_TEST(jtbm_subselect != NULL); } > st_select_lex_unit *get_unit(); > st_select_lex *get_single_select(); > _______________________________________________ > 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

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

01 Oct '19

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

2 1