- developers - lists.mariadb.org

Re: [Maria-developers] [Commits] 321a43d9ac5: MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured
by andrei.elkin＠pp.inet.fi 12 Jun '19

12 Jun '19

Sachin, salute! The patch looks good, but I think it misses some verbosity in few places, please read on. > revision-id: 321a43d9ac586034790407dc519c72e90b3bff81 (mariadb-10.1.39-54-g321a43d9ac5) > parent(s): b003b0c934cf6b59358e31144c4f69cf34622fb8 > author: Sachin > committer: Sachin > timestamp: 2019-06-06 12:03:45 +0530 > message: > > MDEV-8874 Replication filters configured in my.cnf are ignored if slave reset and reconfigured > > Don't delete the rpl_filter on RESET SLAVE. > > --- > mysql-test/lib/My/Config.pm | 8 +- > mysql-test/suite/multi_source/mdev-8874.cnf | 25 +++++ > mysql-test/suite/multi_source/mdev-8874.result | 93 +++++++++++++++++++ > mysql-test/suite/multi_source/mdev-8874.test | 123 +++++++++++++++++++++++++ > sql/rpl_mi.cc | 2 - > 5 files changed, 248 insertions(+), 3 deletions(-) > > diff --git a/mysql-test/lib/My/Config.pm b/mysql-test/lib/My/Config.pm > index 12647edf0a4..349a397ed6d 100644 > --- a/mysql-test/lib/My/Config.pm > +++ b/mysql-test/lib/My/Config.pm > @@ -327,7 +327,13 @@ sub new { > # Skip comment > next; > } > - > + > + # Replication Filter > + elsif ( $line =~ /^([\w]+.[\w]+)\s*=\s*(.*)\s*/){ > + my $option= $1; > + my $value= $2; > + $self->insert($group_name, $option, $value); > + } What was a problem at the above point? [Anything that you think may not be straightforward for a patch reader is always worth to highlight in the description.] > else { > croak "Unexpected line '$line' found in '$path'"; > } > diff --git a/mysql-test/suite/multi_source/mdev-8874.cnf b/mysql-test/suite/multi_source/mdev-8874.cnf > new file mode 100644 > index 00000000000..01da70cbaa0 > --- /dev/null > +++ b/mysql-test/suite/multi_source/mdev-8874.cnf > @@ -0,0 +1,25 @@ > +!include my.cnf > + > +[mysqld.1] > +log-bin > +log-slave-updates > + > +[mysqld.2] > +log-bin > +log-slave-updates > + > +[mysqld.3] > +log-bin > +log-slave-updates > + > +[mysqld.4] > +server-id=4 > +log-bin=server4-bin > +log-slave-updates > +m1.replicate_ignore_table='a.t1' > +m2.replicate_ignore_table='b.t1' > +replicate_ignore_table='c.t1' > + > +[ENV] > +SERVER_MYPORT_4= @mysqld.4.port > +SERVER_MYSOCK_4= @mysqld.4.socket > diff --git a/mysql-test/suite/multi_source/mdev-8874.result b/mysql-test/suite/multi_source/mdev-8874.result > new file mode 100644 > index 00000000000..6fb364a3d2d > --- /dev/null > +++ b/mysql-test/suite/multi_source/mdev-8874.result > @@ -0,0 +1,93 @@ > +create database a; > +use a; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +create database b; > +use b; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +create database c; > +use c; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; > +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; > +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; > +start all slaves; > +set default_master_connection = 'm1'; > +include/wait_for_slave_to_start.inc > +set default_master_connection = 'm2'; > +include/wait_for_slave_to_start.inc > +set default_master_connection = ''; > +include/wait_for_slave_to_start.inc > +use a; > +show tables; > +Tables_in_a > +t2 > +use b; > +show tables; > +Tables_in_b > +t2 > +use c; > +show tables; > +Tables_in_c > +t2 > +#TEST > +STOP ALL SLAVES; > +Warnings: > +Note 1938 SLAVE 'm2' stopped > +Note 1938 SLAVE '' stopped > +Note 1938 SLAVE 'm1' stopped > +RESET SLAVE 'm1' ALL ; > +RESET SLAVE 'm2' ALL ; > +RESET SLAVE ALL ; > +drop database a; > +drop database b; > +drop database c; > +change master 'm1' to master_port=MYPORT_1 , master_host='127.0.0.1', master_user='root'; > +change master 'm2' to master_port=MYPORT_2 , master_host='127.0.0.1', master_user='root'; > +change master to master_port=MYPORT_3 , master_host='127.0.0.1', master_user='root'; > +start all slaves; > +Warnings: > +Note 1937 SLAVE 'm2' started > +Note 1937 SLAVE '' started > +Note 1937 SLAVE 'm1' started > +set default_master_connection = 'm1'; > +include/wait_for_slave_to_start.inc > +set default_master_connection = 'm2'; > +include/wait_for_slave_to_start.inc > +set default_master_connection = ''; > +include/wait_for_slave_to_start.inc > +use a; > +show tables; > +Tables_in_a > +t2 > +use b; > +show tables; > +Tables_in_b > +t2 > +use c; > +show tables; > +Tables_in_c > +t2 > +#CleanUp > +drop database a; > +drop database b; > +drop database c; > +stop all slaves; > +Warnings: > +Note 1938 SLAVE 'm2' stopped > +Note 1938 SLAVE '' stopped > +Note 1938 SLAVE 'm1' stopped > +SET default_master_connection = "m1"; > +include/wait_for_slave_to_stop.inc > +SET default_master_connection = "m2"; > +include/wait_for_slave_to_stop.inc > +SET default_master_connection = ""; > +include/wait_for_slave_to_stop.inc To the test part, I suggest we always follow a well-know description pattern (which is a great time saver when it comes to future test maintenance, apart from the current reviewing), to include the task id (present in the name though) and title, what failed and *how* the test proves the fixes. I can bet the failure was also exposed by reset @@global.'connection_name'.'rule_type' variables, which the test is really missing. Could you please add up that? > diff --git a/mysql-test/suite/multi_source/mdev-8874.test b/mysql-test/suite/multi_source/mdev-8874.test > new file mode 100644 > index 00000000000..b6d1aafe139 > --- /dev/null > +++ b/mysql-test/suite/multi_source/mdev-8874.test > @@ -0,0 +1,123 @@ > +--source include/not_embedded.inc > +--source include/have_innodb.inc > +--source include/have_debug.inc > + > +--connect (server_1,127.0.0.1,root,,,$SERVER_MYPORT_1) > +--connect (server_2,127.0.0.1,root,,,$SERVER_MYPORT_2) > +--connect (server_3,127.0.0.1,root,,,$SERVER_MYPORT_3) > +--connect (server_4,127.0.0.1,root,,,$SERVER_MYPORT_4) > + > +--connection server_1 > +create database a; > +use a; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +--save_master_pos > + > +--connection server_2 > +create database b; > +use b; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +--save_master_pos > + > +--connection server_3 > +create database c; > +use c; > +create table t1(a int); > +insert into t1 values(1); > +create table t2(a int); > +insert into t2 values(1); > +--save_master_pos > + > +--connection server_4 > +--disable_warnings > +--replace_result $SERVER_MYPORT_1 MYPORT_1 > +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; > +--replace_result $SERVER_MYPORT_2 MYPORT_2 > +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; > +--replace_result $SERVER_MYPORT_3 MYPORT_3 > +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; > +start all slaves; > +set default_master_connection = 'm1'; > +--source include/wait_for_slave_to_start.inc > +set default_master_connection = 'm2'; > +--source include/wait_for_slave_to_start.inc > +set default_master_connection = ''; > +--source include/wait_for_slave_to_start.inc > + > +--enable_warnings > +--sync_with_master 0,'m1' > +--sync_with_master 0,'m2' > +--sync_with_master 0,'' > +use a; When the test proves something via printing out, it's always good to prepare the output with an --echo what-is-expected so a thankful reader will bless you .. and the others may mercy :-) > +show tables; > +use b; > +show tables; > +use c; > +show tables; > +--echo #TEST > +STOP ALL SLAVES; > +RESET SLAVE 'm1' ALL ; > +RESET SLAVE 'm2' ALL ; > +RESET SLAVE ALL ; > +drop database a; > +drop database b; > +drop database c; > +--replace_result $SERVER_MYPORT_1 MYPORT_1 > +eval change master 'm1' to master_port=$SERVER_MYPORT_1 , master_host='127.0.0.1', master_user='root'; > +--replace_result $SERVER_MYPORT_2 MYPORT_2 > +eval change master 'm2' to master_port=$SERVER_MYPORT_2 , master_host='127.0.0.1', master_user='root'; > +--replace_result $SERVER_MYPORT_3 MYPORT_3 > +eval change master to master_port=$SERVER_MYPORT_3 , master_host='127.0.0.1', master_user='root'; > +start all slaves; > +set default_master_connection = 'm1'; > +--source include/wait_for_slave_to_start.inc > +set default_master_connection = 'm2'; > +--source include/wait_for_slave_to_start.inc > +set default_master_connection = ''; > +--source include/wait_for_slave_to_start.inc > +--sync_with_master 0,'m1' > +--sync_with_master 0,'m2' > +--sync_with_master 0,'' > + Naturally, the same applies to the following block. > +use a; > +show tables; > +use b; > +show tables; > +use c; > +show tables; > + > + > +#--echo #restart the server > +#--source include/restart_mysqld.inc > + > + > +--echo #CleanUp > +--connection server_1 > +drop database a; > +--save_master_pos > + > +--connection server_2 > +drop database b; > +--save_master_pos > + > +--connection server_3 > +drop database c; > +--save_master_pos > + > +--connection server_4 > +--sync_with_master 0,'m1' > +--sync_with_master 0,'m2' > +--sync_with_master 0,'' > +stop all slaves; > +SET default_master_connection = "m1"; > +--source include/wait_for_slave_to_stop.inc > +SET default_master_connection = "m2"; > +--source include/wait_for_slave_to_stop.inc > +SET default_master_connection = ""; > +--source include/wait_for_slave_to_stop.inc > diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc > index 70e60b1d4ad..7aea89337a7 100644 > --- a/sql/rpl_mi.cc > +++ b/sql/rpl_mi.cc > @@ -122,8 +122,6 @@ Master_info::~Master_info() > */ > if (strncmp(connection_name.str, STRING_WITH_LEN("wsrep"))) > #endif > - rpl_filters.delete_element(connection_name.str, connection_name.length, > - (void (*)(const char*, uchar*)) free_rpl_filter); > my_free(connection_name.str); > delete_dynamic(&ignore_server_ids); > mysql_mutex_destroy(&run_lock); > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 1

[Maria-developers] GSoC [MDEV-6017]
by Alexey Mogilyovkin 10 Jun '19

10 Jun '19

Hello. This is the thread where I would like to publish my weekly reports every Monday. During preparatory weeks I set up development environment. I briefly examined testing framework, debugging process and now I am ready to start work. Work progress will be available on my github repository in branch "MDEV-6017" https://github.com/ZeroICQ/server/tree/MDEV-6017

3 5

[Maria-developers] Weekly Report
by Rucha Deodhar 10 Jun '19

10 Jun '19

Report for week 2: Hello! This week I ran the test case for parser written in last week and made changes in the parser according to the suggestions given, tried making insert...select..returning work. I analysed how RETURNING in delete is implemented, how insert is implemented, and how the data can be sent, which functions are responsible and what those functions do, to carry out various steps and what these steps are in order to send the data to the client. I came up with a plan where code for RETURNING should go and files where the code has to be written. There were few things which weren't very clear initially. So I referred to some documents on the internet and some source code document. My github repo is up to date with the changes in the code, result and test files. Regards, Rucha

1 0

[Maria-developers] MDEV-17951 mariadb symlinks
by Rasmus Johansson 09 Jun '19

09 Jun '19

Hi Serg, I've addressed general review requests by Wlad and review requests by Otto related to Debian packaging. The latest commit is here: https://github.com/MariaDB/server/commit/030c299f76fac044d43325abb02b5751fe… Could you give it final review to see if it can be pushed to main 10.4? Rasmus -- Rasmus Johansson, VP Engineering MariaDB | t: +358 50 499 9589 | Skype: ratzpo

1 0

Re: [Maria-developers] a2520676ba1: MDEV-16249 CHECKSUM TABLE for a spider table is not parallel and saves all data in memory in the spider head by default
by Sergei Golubchik 06 Jun '19

06 Jun '19

Hi, Kentoku! On Jun 05, Kentoku wrote: > revision-id: a2520676ba1 (mariadb-10.4.5-10-ga2520676ba1) > parent(s): 31fe70290c5 > author: Kentoku <kentokushiba(a)gmail.com> > committer: Kentoku <kentokushiba(a)gmail.com> > timestamp: 2019-05-23 14:36:18 +0900 > message: > > MDEV-16249 CHECKSUM TABLE for a spider table is not parallel and saves all data in memory in the spider head by default please remember to add an empty line after the first line in the commit comment, see https://git-scm.com/docs/git-commit#_discussion > add checksum_null for setting null value of checksum > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index 4e7fccca3af..e47da94ab20 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -8213,7 +8214,11 @@ int ha_partition::info(uint flag) > stats.delete_length+= file->stats.delete_length; > if (file->stats.check_time > stats.check_time) > stats.check_time= file->stats.check_time; > - stats.checksum+= file->stats.checksum; > + if (!file->stats.checksum_null) > + { > + stats.checksum+= file->stats.checksum; > + stats.checksum_null= FALSE; > + } I'm not sure about it. When a checksum can be NULL? I mean, normally, if you do A+B+C and one of the values is NULL, then the whole sum is NULL. But here you simply skip NULLs, like in aggregate SUM() function. To understand what semantics works better in this case, I need to understand when a checksum can be NULL in spider. > } > if (stats.records && stats.records < 2 && > !(m_file[0]->ha_table_flags() & HA_STATS_RECORDS_IS_EXACT)) All other server changes look good, thanks. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] Fwd: [Commits] a602998: MDEV-19580 Unrelated JOINs corrupt usage of 'WHERE function() IN (subquery)'
by Sergey Petrunia 05 Jun '19

05 Jun '19

Hello Igor, On Sat, Jun 01, 2019 at 01:16:04PM -0700, Igor Babaev wrote: > Please review this patch for the bug that we discussed on slack a couple of > days ago. > As you can see the extra calls of make_cond_for_table() with > used_table=RAND_TABLE_BIT were not enough (it can be seen in debugger why). > It took me quite a lot of time to figure out that I could not trust > Item::marker==2 to check whether a conjunct was previously attached > or not. > First, I've found a testcase that stopped working after the patch. (As far as I understand, we should pass RAND_TABLE_BIT when constructing JOIN::outer_ref_cond): create table ten(a int); insert into ten values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9); create table t1 (a int, b int, c int); insert into t1 select a,a,a from ten; create table t2 as select a,b,c from t1; mysql> explain format=json select (select max(c) from t1 where t1.a=t2.a and rand()=t2.b) from t2\G *************************** 1. row *************************** EXPLAIN: { "query_block": { "select_id": 1, "table": { "table_name": "t2", "access_type": "ALL", "rows": 10, "filtered": 100 }, "subqueries": [ { "query_block": { "select_id": 2, "table": { "table_name": "t1", "access_type": "ALL", "rows": 10, "filtered": 100, "attached_condition": "((t1.a = t2.a) and (rand() = t2.b))" } } } ] } } 1 row in set (0.00 sec) If I run the select, it hits a breakpoint in Item_func_rand::val_real. Now, after this patch (I applied it to 10.1): mysql> explain format=json select (select max(c) from t1 where t1.a=t2.a and rand()=t2.b) from t2\G *************************** 1. row *************************** EXPLAIN: { "query_block": { "select_id": 1, "table": { "table_name": "t2", "access_type": "ALL", "rows": 10, "filtered": 100 }, "subqueries": [ { "query_block": { "select_id": 2, "table": { "table_name": "t1", "access_type": "ALL", "rows": 10, "filtered": 100, "attached_condition": "(t1.a = t2.a)" } } } ] } } and running the SELECT doesn't hit the breakpoint in Item_func_rand::val_real A bit more of cosmetic feedback below, marked with 'psergey:' > > @@ -8968,6 +8963,20 @@ make_join_select(JOIN *join,SQL_SELECT *select,COND > *cond) > { > tmp= make_cond_for_table(thd, cond, used_tables, current_map, i, > FALSE, FALSE); > + if (tab == join->join_tab + last_top_base_tab_idx) > + { > + /* > + This pushes conjunctive conditions of WHERE condition such > that: > + - their used_tables() contain RAND_TABLE_BIT > + - the conditions does not refer to any fields > + (such like rand() > 0.5) > + */ > + table_map rand_table_bit= (table_map) RAND_TABLE_BIT; psergey: what is the purpose of rand_table_bit? If we can't use RAND_TABLE_BIT where a table bitmap is expected, we should fix that. The way it's done now it looks redundant and is confusing. (this also applies to other declarations if rand_table_bit) > + COND *rand_cond= make_cond_for_table(thd, cond, used_tables, > + rand_table_bit, -1, > + FALSE, FALSE); > + add_cond_and_fix(thd, &tmp, rand_cond); > + } > } > /* Add conditions added by add_not_null_conds(). */ > if (tab->select_cond) ... > @@ -18852,11 +18880,27 @@ make_cond_for_table_from_pred(THD *thd, Item > *root_cond, Item *cond, > Item *item; > while ((item=li++)) > { > + /* > + Special handling of top level conjuncts with RAND_TABLE_BIT: > + if such a conjunct contains a reference to a field then it is > pushed > + to the corresponding table by the same rule as all other > conjuncts. > + Otherwise, if the conjunct is used in WHERE is is pushed to the > last > + joined table, if is it is used in ON condition of an outer join > it > + is pushed into the last inner table of the outer join. Such > conjuncts > + are pushed in a call of make_cond_for_table_from_pred() with the > + parameter 'used_table' equal to RAND_TABLE_BIT. > + */ > + if (is_top_and_level && used_table == rand_table_bit && > + item->used_tables() != rand_table_bit) > + { > + /* The conjunct with RAND_TABLE_BIT has been allready pushed */ psergey: typo: allready > + continue; > + } > Item *fix=make_cond_for_table_from_pred(thd, root_cond, item, > tables, used_table, > - join_tab_idx_arg, > + join_tab_idx_arg, > exclude_expensive_cond, > - retain_ref_cond); > + retain_ref_cond, false); > if (fix) > new_cond->argument_list()->push_back(fix); BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] Weekly Report
by Rucha Deodhar 03 Jun '19

03 Jun '19

Report for week 1: Hello! I made test cases and extended the parser for: INSERT...RETURNING * and INSERT...RETURNING <col_name/names> It is working fine for INSERT...ON DUPLICATE KEY UPDATE, INSERT...IGNORE, INSERT...SET, and simple insert statements. As I have never coded parser before, it was throwing errors during build and was also crashing the server sometimes. I managed to fix the errors eventually. Initially, I tried to extend <column_names> in RETURNING by referring how column names are parsed in INSERT statement, like using opt_fields production for insert_field_list, modified it a little and tried to see it if that would work. But it didn't. So I referred to DELETE...RETURNING as it was suggested earlier. I am still trying to extend it for INSERT...SELECT. It is not showing syntax error but shows error "column count doesn't match value count at row 1." I will add the test cases for this once it is working. Regards, Rucha

2 1

Re: [Maria-developers] [Commits] b3473961810: MDEV-11094: Blackhole table updates on slave fail when row annotation is enabled
by Sergey Vojtovich 29 May '19

29 May '19

Hi Sujatha, On Wed, May 29, 2019 at 04:37:27PM +0530, sujatha wrote: > revision-id: b347396181018cedc946450cb49891f1a0aa4575 (mariadb-10.1.39-48-gb3473961810) > parent(s): 8358c6f03edb941be488f009c2bd0eb9df47e8c5 > author: Sujatha > committer: Sujatha > timestamp: 2019-05-29 15:18:52 +0530 > message: > > MDEV-11094: Blackhole table updates on slave fail when row annotation is enabled ...skip... > diff --git a/storage/blackhole/ha_blackhole.cc b/storage/blackhole/ha_blackhole.cc > index 01aaa9ea15f..43bcdc541a1 100644 > --- a/storage/blackhole/ha_blackhole.cc > +++ b/storage/blackhole/ha_blackhole.cc > @@ -25,6 +25,16 @@ > #include "ha_blackhole.h" > #include "sql_class.h" // THD, SYSTEM_THREAD_SLAVE_SQL > > +static bool is_row_based_replication(THD *thd) > +{ > + /* > + A row event which has its thd->query() == NULL or a row event which has > + replicate_annotate_row_events enabled. In the later case the thd->query() > + will be pointing to the query, received through replicated annotate event > + from master. > + */ > + return ((thd->query() == NULL) || thd->variables.binlog_annotate_row_events); Any reason to have so many redundant parenthesis? Why code comment rather than function comment? May be move "thd->system_thread == SYSTEM_THREAD_SLAVE_SQL" inside as well? Regards, Sergey

1 0

[Maria-developers] Proposal for automatic code formatting (clang-format)
by Eugene Kosov 27 May '19

27 May '19

Hi. I'm proposing to add some tool that automatically formats source code. Main purpose is overall productivity. It's possible to save time on manual formatting. Also, reading a uniformly formatted code is easier. Some new languages has one correct coding style and code formatters. Examples are Go (https://golang.org/cmd/gofmt/) and (to be done) Rust (https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/guide.md) I suggest to read 'Motivation - why use a formatting tool' from second link. There is no single correct coding style in C and C++ worlds: different projects use different styles if any at all. But there is a tool which does formatting and supports different styles: clang-format. It's a binary available for every major platform. It's configurable: just put .clang-format file with formatting options in source directory and formatter will use it for current and nested directories. I actually do not propose here to use exactly clang-format but as I know it's the best of its kind. Here is a demo https://youtu.be/uZI_Qla4pNA?t=1893 Tool is configurable but it will not act exactly as you might expect and this is a pay for uniformity. So, let's add .clang-format to root directory, start using and polishing style. Here is some info on how to add it to some text editors https://clang.llvm.org/docs/ClangFormat.html I do not(!) suggesting to reformat whole project. At least for now. Here is my PR to add .clang-format files to server and InnoDB: https://github.com/MariaDB/server/pull/1270 I think that eventually .clang-format usage will lead to reformatting everything and having some project-wide precommit hook which will properly format your diffs. Do you have any objections or questions on having such a tool? Let's discuss, please. -- Eugene

1 0

Re: [Maria-developers] Regarding approach used for Early NULL filtering in the range optimizer(MDEV-15777)
by Sergey Petrunia 27 May '19

27 May '19

On Fri, May 24, 2019 at 04:39:51PM -0700, Igor Babaev wrote: > On 05/24/2019 01:41 PM, Sergey Petrunia wrote: > > On Fri, May 24, 2019 at 10:08:04AM -0700, Igor Babaev wrote: > > > On 05/24/2019 04:03 AM, Varun Gupta wrote: > > > > Hi Igor, > > > > After discussing with Sergey, we came up with these conclusions as to > > > > why we used the approach of going through all the keyuses in the KEYUSE > > > > array > > > > > > > > Cases to consider: > > > > we have an index on column a > > > > > > > > 1) a OP const > > > > where OP can be (</<=/=/between/ etc on which we do range analysis) . > > > > This case is already handled by the range optimizer. We do not create a > > > > NULL rejecting predicate for such conditions. > > > > > > > > 2) eq_join conditions (tbl1.a = tbl2.col2) > > > > This is the specific case we have tried to implement in MDEV-15777, we > > > > create NULL rejecting predicates for the keyuses for each table and then > > > > feed these predicates to range optimizer > > > There is no logical explanation for checking null rejection of fields used > > > in equalities with the help of array keyuse. > > The logic here is as follows: > > > > We already collect the set of KEYUSE::null_rejecting attributes. Its > > meaning is very close to what the patch needs. > The meaning of this argument is to be used a key value for ref access by an > index i1. > You need range access by a different index i2. There is no logical > connection > between these two indexes. The usage of i1 can be blocked by IGNORE clause > and this case > there won't be any keyuses for it. But the equality from which the > null-rejection can be > deduced is still there. You need range scan to reduce the number of records > produced > by the left operand of a join operation. And you should not care weather it > should be the > argument of any ref access. > Here is the situation: > The field of interest for range access is t1.a and you have two conditions > t1.a=t2.a with index i1 on t2.a > t1.b=t3.b with index i2 on t3.b > The use prohibited to use i1. t1.a apparently remains null-rejecting. Yet it > cannot be not detected > as such by a look through array of keyuses. I don't follow the logic. What MDEV-15777 does is to basically look at KEYUSE object, which represent tbl.key_col = other_column and feed this into the range optimizer: tbl.key_col IS NOT NULL If the query CAN use a certain index: - we will have a KEYUSE object for it - we will feed the IS NOT NULL into the range optimizer. if the query CANNOT use a certain index (because of IGNORE INDEX), then - we won't have KEYUSE object for that index (and so won't construct an IS NOT NULL condition for it) - range optimizer will not build ranges for it (so it doesn't really matter if IS NOT NULL was constructed or not) Can you provide a full example? > > > > Because of this, the part of the patch for MDEV-15777 which analyzes the KEYUSE > > array is very small: 122 lines including the comment (I counted > > make_null_rejecting_conds() and add_cond()). > > > > I think, the CPU overhead is small, too. > > > > > > 3) a < func(b,c) > > > > we do not handle this case, because: > > > > > > > > a) It is harder to process. We would need to process the whole WHERE > > > > clause to infer > > > > non-nullability of the condition. Example: > > > > > > > > (t1.f2+1 < t1.f1+t1.f3) OR ... OR t1.f1 is null > > > > > > > > here the left part of the OR condition doesn't allow f1 to be NULL, but > > > > the > > > > right condition allows it. We would also need to take into account tables > > > > inside outer joins. We would need to go through Item classes and add code > > > > which say "Item_xxx() will compute to false when its argument Y is null" > > > > (there is item->not_null_tables() currently, but not not_null_columns()). > > > Have you actually looked at the implementations of the virtual function > > > not_null_tables()? > > > Do you need me to provide the implementation of the virtual function > > > not_null_columns()? > > It will require multiple implementations. At the moment we have: > > > > nm mysqld --demangle | grep 'Item.*::not_null_tables' | wc -l > > 21 > > > > Another question: do you intend to collect not_null_columns() for all columns, > > or just columns that are a part of some index? > > > > What would a good data structure to store a set of tablename.column_name ? > It will be just a bitmap created for each table that can be used for many > other purposes. > If you want to filter out those that are not part of any index it can be > easily done > (even once for any table share) My point was that constructing such a bitmap is a lot heavier than the not_null_table_map(). table.h declares a field bitmap class: typedef Bitmap<MAX_FIELDS> Field_map; but it's huge: (gdb) p sizeof(Field_map) $52 = 544 and note that we will not be able to just put one Field_map into the TABLE object: If we think about how to compute not_null_columns() for Item_cond_and or Item_cond_or: - Item_cond_and will compute a union of all its arguments' not_null_columns(). - Item_cond_or will compute an intersection of all its arguments' not_null_columns(). - Other items have different logic. that is, we will need to keep multiple "set of fields in tables" objects around. Just one bitmap attached to the table will not suffice. This is doable, but it's going to be more code and have more CPU overhead. And I'm still not convinced why we should do this. It is not clear if this will fit the scope of MDEV-15777 and once we got it done, what's the use of it? Is this something other query optimizers do (so the users expect it?)? > > > > > > b) the conditions in form of arbitrary functions are not as frequently > > > > used as > > > > ref access conditions. > > > > > > > > to sum up a) and b) - doable but will require a lot of effort. > > > > > > > > Do you have any specific practically relevant example in mind that we > > > > should > > > > handle? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0