January 2023 - developers - lists.mariadb.org

[Maria-developers] Optimizer call followup: negative gains from rowid filters
by Sergey Petrunia 31 Jan '23

31 Jan '23

Hello, Just taking notes from the optimizer call: Check out this branch: 10.4-mdev30218-print-negative-gain (git log -3 output below) It has: - 10.4-release contents - then mdev30218_bb-10.4-release.diff patch provided by Igor - then a debug patch by me. I run: ./mtr --mem --parallel=5 --force main/rowid_filter*test grep -rn filter_gain var/ I get this: negative gains from filters: var/4/log/mysqld.1.err:52:filter_gain:-0.0569276, query=select 1 var/4/log/mysqld.1.err:58:filter_gain:-0.0569276, query=explain extended select 1 var/4/log/mysqld.1.err:64:filter_gain:-0.250816, query=SELECT * FROM t1 HAVING (7, 9) IN (SELECT t2.i1, t2.i2 FROM t2 WHERE t2.i1 = 3) var/4/log/mysqld.1.err:65:filter_gain:-0.250816, query=EXPLAIN EXTENDED var/4/log/mysqld.1.err:67:filter_gain:-10.207, query=explain var/4/log/mysqld.1.err:69:filter_gain:-10.207, query=SELECT * FROM t1 WHERE a > 0 AND b=0 var/4/log/mysqld.1.err:70:filter_gain:-1.17502, query=select count(*) from t1 where a between 21 and 30 and b=2 var/4/log/mysqld.1.err:71:filter_gain:-1.17502, query=explain extended select count(*) from t1 where a between 21 and 30 and b=2 var/4/log/mysqld.1.err:72:filter_gain:-1.17502, query=select * from t1 where a between 21 and 30 and b=2 var/4/log/mysqld.1.err:73:filter_gain:-2.18259, query=SELECT * FROM t1 var/4/log/mysqld.1.err:77:filter_gain:-2.18259, query=EXPLAIN EXTENDED SELECT * FROM t1 var/4/log/mysqld.1.err:81:filter_gain:-0.151308, query=set statement optimizer_switch='rowid_filter=on' for explain extended select t.id, fi.* var/4/log/mysqld.1.err:90:filter_gain:-0.151308, query=set statement optimizer_switch='rowid_filter=on' for select t.id, fi.* var/4/log/mysqld.1.err:99:filter_gain:-0.151308, query=set statement optimizer_switch='rowid_filter=on' for explain extended select t.id, fi.* var/4/log/mysqld.1.err:108:filter_gain:-0.151308, query=set statement optimizer_switch='rowid_filter=on' for select t.id, fi.* var/4/log/mysqld.1.err:117:filter_gain:-0.151308, query=set statement optimizer_switch='rowid_filter=on' for analyze format=json select t.id, fi.* var/3/log/mysqld.1.err:51:filter_gain:-0.0569276, query=select 1 var/3/log/mysqld.1.err:57:filter_gain:-0.0569276, query=explain extended select 1 var/3/log/mysqld.1.err:63:filter_gain:-0.250816, query=SELECT * FROM t1 HAVING (7, 9) IN (SELECT t2.i1, t2.i2 FROM t2 WHERE t2.i1 = 3) var/3/log/mysqld.1.err:64:filter_gain:-0.250816, query=EXPLAIN EXTENDED var/3/log/mysqld.1.err:66:filter_gain:-12.7213, query=explain var/3/log/mysqld.1.err:68:filter_gain:-12.7213, query=SELECT * FROM t1 WHERE a > 0 AND b=0 === git log -3 output === commit 82d5977b9fd6be5f5152d6831cd744fc739a2f24 (HEAD -> 10.4-mdev30218-print-negative-gain, origin/10.4-mdev30218-print-negative-gain, bb-10.4-release) Author: Sergei Petrunia <sergey(a)mariadb.com> Date: Tue Jan 31 20:11:17 2023 +0300 Debug patch: print filter_gain if it is less than 0. commit 47a40d90627dcf3c870cecd33c345c2e224986af Author: Sergei Petrunia <sergey(a)mariadb.com> Date: Tue Jan 31 20:07:02 2023 +0300 Apply mdev30218_bb-10.4-release.diff commit c8f2e9a5c0ac5905f28b050b7df5a9ffd914b7e7 (origin/bb-10.4-release) Author: Oleksandr Byelkin <sanja(a)mariadb.com> Date: Mon Jan 30 11:49:42 2023 +0100 Fix number of rows passing in case of EQ_REF BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0

Re: [Maria-developers] dcd69cbd19a: MDEV-29447 MDEV-26285 Refactor spider_db_mbase_util::open_item_func
by Sergei Golubchik 30 Jan '23

30 Jan '23

Hi, Yuchen, Thanks! There are two kinds of comments below, you'll see which is which. I realize that this patch mainly just moves parts of spider code around. Some of my comments apply to the new code that didn't exist before and appeared only in this patch. It's what needs to be fixed (or discussed, if you disagree) before pushing. Other comments apply to the old spider code that this patch moved. If you'd like you can say "I'm just moving spider code around" and ignore all comments of that kind. /Sergei On Jan 22, Yuchen Pei wrote: > commit dcd69cbd19a > Author: Yuchen Pei <yuchen.pei(a)mariadb.com> > Date: Tue Jan 3 16:24:04 2023 +1100 > > MDEV-29447 MDEV-26285 Refactor spider_db_mbase_util::open_item_func > > Porting commit 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) Because 3836098c29ef will not be, apparently, pushed anywhere, you'd better put a self-containing comment here. May be use the comment of 3836098c29ef? Note that you shouldn't push your 10.4-11.0 versions of the patch directly, only push the 10.3 one (when ok) and let it propagate to 11.0 through merges. Your 10.4-11.0 patches are very useful, though, they will serve as guidelines for whoever will merge this change up. > to current versions 10.3+ to fix a problem (MDEV-29447) where field > items that are arguments of a func item may be used before created / > initialised. > > Signed-off-by: Yuchen Pei <yuchen.pei(a)mariadb.com> > > diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc > index e942d1d9063..a886a4e6e99 100644 > --- a/storage/spider/spd_db_mysql.cc > +++ b/storage/spider/spd_db_mysql.cc > @@ -4035,6 +4042,170 @@ int spider_db_mbase_util::open_item_func( > uint alias_length, > bool use_fields, > spider_fields *fields > +) { > + DBUG_ENTER("spider_db_mbase_util::check_item_func"); incorrect function name in DBUG_ENTER > + > + int error = check_item_func(item_func, spider, alias, > + alias_length, use_fields, fields); > + if (error) > + DBUG_RETURN(error); > + if (!str) > + DBUG_RETURN(0); > + > + DBUG_RETURN(print_item_func(item_func, spider, str, alias, > + alias_length, use_fields, fields)); > +} > + > +namespace { > + bool item_func_is_timestampdiff( We generally use static in these cases. Why would an anonymous namespace be better? > + const char *func_name, > + int func_name_length > + ) { > + return func_name_length == 13 && > + !strncasecmp("timestampdiff", func_name, func_name_length); We build with rtti nowadays, so you can use dynamic_cast instead. and you won't need func_name and func_name_length. Ideally you won't need a check for timestampdiff at all > + } > + > + bool not_func_should_be_skipped( > + Item_func *item_func > + ){ > + DBUG_ENTER("not_func_should_be_skipped"); add DBUG_ASSERT(item_func->functype() == Item_func::NOT_FUNC); > + Item **item_list = item_func->arguments(); > + > + if (item_list[0]->type() == Item::COND_ITEM) > + { > + DBUG_PRINT("info",("spider item_list[0] is COND_ITEM")); > + Item_cond *item_cond = (Item_cond *) item_list[0]; > + if (item_cond->functype() == Item_func::COND_AND_FUNC) > + { > + DBUG_PRINT("info",("spider item_cond is COND_AND_FUNC")); > + List_iterator_fast<Item> lif(*(item_cond->argument_list())); > + bool has_expr_cache_item = FALSE; > + bool has_isnotnull_func = FALSE; > + bool has_other_item = FALSE; > + while(Item *item = lif++) > + { > + if ( > + item->type() == Item::EXPR_CACHE_ITEM > + ) { > + DBUG_PRINT("info",("spider EXPR_CACHE_ITEM")); > + has_expr_cache_item = TRUE; > + } else > + if ( > + item->type() == Item::FUNC_ITEM && > + ((Item_func *) item)->functype() == Item_func::ISNOTNULL_FUNC > + ) { > + DBUG_PRINT("info",("spider ISNOTNULL_FUNC")); > + has_isnotnull_func = TRUE; > + } else { > + DBUG_PRINT("info",("spider has other item")); > + DBUG_PRINT("info",("spider COND type=%d", item->type())); > + has_other_item = TRUE; > + } > + } > + if (has_expr_cache_item && has_isnotnull_func && !has_other_item) So, "not func should be skipped" if it's NOT (expr1 AND expr2 AND ...) and all expr's are either IS NOT NULL or a cached subquery. Why? IS NULL, IS NOT FALSE, non-cached subqueries, everything else is fine, IS NOT NULL if it is followed by `AND 2>1` is fine, OR is fine. Doesn't make much sense to me. > + { > + DBUG_PRINT("info",("spider NOT EXISTS skip")); > + DBUG_RETURN(TRUE); > + } > + } > + } > + DBUG_RETURN(FALSE); > + } > +} > + > +/** > + Check if the given item_func and its arguments can be pushed down to > + a data node. This function is recursive because we need to also check > + the arguments of the item_func. > + > + @return 0: success, otherwise: error > + */ > +int spider_db_mbase_util::check_item_func( > + Item_func *item_func, > + ha_spider *spider, > + const char *alias, > + uint alias_length, > + bool use_fields, > + spider_fields *fields > +) { > + DBUG_ENTER("spider_db_mbase_util::check_item_func"); > + > + Item_func::Functype func_type = item_func->functype(); > + DBUG_PRINT("info",("spider functype = %d", func_type)); > + > + const char *func_name = (char*) item_func->func_name(); > + int func_name_length = strlen(func_name); > + DBUG_PRINT("info",("spider func_name = %s", func_name)); > + > + /* The blacklist of the functions that cannot be pushed down */ > + if ( > + func_type == Item_func::TRIG_COND_FUNC || > + func_type == Item_func::CASE_SEARCHED_FUNC || > + func_type == Item_func::CASE_SIMPLE_FUNC Does CASE need to be skipped at all? > + ) { > + DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > + } > + > + if (func_type == Item_func::NOT_FUNC) > + { > + /* Why the following check is necessary? */ indeed > + if(not_func_should_be_skipped(item_func)) > + DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > + } > + > + if (func_type == Item_func::UDF_FUNC) old code used switch here. I think it looks more natural than a sequence of if's. A compiler might optimize them internally into a switch, but I'd better do it explicitly. > + { > + int use_pushdown_udf = spider_param_use_pushdown_udf( > + spider->trx->thd, spider->share->use_pushdown_udf); > + if (!use_pushdown_udf) { > + DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > + } > + } > + > + if (func_type == Item_func::FT_FUNC) { > + if (spider_db_check_ft_idx(item_func, spider) == MAX_KEY) > + DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > + } > + > + if (func_type == Item_func::UNKNOWN_FUNC) > + { > + if (item_func_is_timestampdiff(func_name, func_name_length)) { it doesn't seem that it needs to be skipped at all > + DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > + } > + } > + /* End of the blacklist */ > + > + /* Check the arguments recursively */ > + if (uint item_count = item_func->argument_count()) > + { > + Item **item_list = item_func->arguments(); > + for (uint roop_count = 0; roop_count < item_count; roop_count++) > + { > + Item *item = item_list[roop_count]; > + if (int error_num = spider_db_print_item_type(item, NULL, spider, NULL, > + alias, alias_length, dbton_id, use_fields, fields)) it'd be more logical to invoke check_item_func() here instead of spider_db_print_item_type with str==NULL > + DBUG_RETURN(error_num); > + } > + } > + > + DBUG_RETURN(0); > +} > + > +/** > + The function print the string corresponding to the given item_func to str. > + The function is assumed to be called only when the check by the function > + check_item_func() has passed. > + > + @return 0: success, otherwise: error > + */ > +int spider_db_mbase_util::print_item_func( > + Item_func *item_func, > + ha_spider *spider, > + spider_string *str, > + const char *alias, > + uint alias_length, > + bool use_fields, > + spider_fields *fields > ) { > int error_num; > Item *item, **item_list = item_func->arguments(); > @@ -4049,13 +4220,14 @@ int spider_db_mbase_util::open_item_func( > last_str_length = SPIDER_SQL_NULL_CHAR_LEN; > int use_pushdown_udf; > bool merge_func = FALSE; > - DBUG_ENTER("spider_db_mbase_util::open_item_func"); > - if (str) > - { > + DBUG_ENTER("spider_db_mbase_util::print_item_func"); > + DBUG_ASSERT(!check_item_func(item_func, spider, alias, alias_length, > + use_fields, fields) && str); Make it two asserts please. One for !check_item_func(...) and the other DBUG_ASSERT(str); Generally, always try to split assert into as many separate asserts as possible, don't do assert(A && B); So that when it fails it'd provide as much information as possible about what exactly went wrong. > + > if (str->reserve(SPIDER_SQL_OPEN_PAREN_LEN)) > DBUG_RETURN(HA_ERR_OUT_OF_MEM); > str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); > - } > + > DBUG_PRINT("info",("spider functype = %d", item_func->functype())); > switch (item_func->functype()) > { > @@ -4374,94 +4523,12 @@ int spider_db_mbase_util::open_item_func( > { > if (!strncasecmp("utc_timestamp", func_name, func_name_length)) pretty much all that code can use dynamic_cast nowadays > { > - if (str) > str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); > DBUG_RETURN(spider_db_open_item_string(item_func, NULL, spider, str, > alias, alias_length, dbton_id, use_fields, fields)); > } else if (!strncasecmp("timestampdiff", func_name, func_name_length)) > { > -#ifdef ITEM_FUNC_TIMESTAMPDIFF_ARE_PUBLIC but it _is_ public, so you can push down Item_func_timestamp_diff all right > - Item_func_timestamp_diff *item_func_timestamp_diff = > - (Item_func_timestamp_diff *) item_func; > - if (str) > - { > - const char *interval_str; > - uint interval_len; > - switch (item_func_timestamp_diff->int_type) > - { > - case INTERVAL_YEAR: > - interval_str = SPIDER_SQL_YEAR_STR; > - interval_len = SPIDER_SQL_YEAR_LEN; > - break; > - case INTERVAL_QUARTER: > - interval_str = SPIDER_SQL_QUARTER_STR; > - interval_len = SPIDER_SQL_QUARTER_LEN; > - break; > - case INTERVAL_MONTH: > - interval_str = SPIDER_SQL_MONTH_STR; > - interval_len = SPIDER_SQL_MONTH_LEN; > - break; > - case INTERVAL_WEEK: > - interval_str = SPIDER_SQL_WEEK_STR; > - interval_len = SPIDER_SQL_WEEK_LEN; > - break; > - case INTERVAL_DAY: > - interval_str = SPIDER_SQL_DAY_STR; > - interval_len = SPIDER_SQL_DAY_LEN; > - break; > - case INTERVAL_HOUR: > - interval_str = SPIDER_SQL_HOUR_STR; > - interval_len = SPIDER_SQL_HOUR_LEN; > - break; > - case INTERVAL_MINUTE: > - interval_str = SPIDER_SQL_MINUTE_STR; > - interval_len = SPIDER_SQL_MINUTE_LEN; > - break; > - case INTERVAL_SECOND: > - interval_str = SPIDER_SQL_SECOND_STR; > - interval_len = SPIDER_SQL_SECOND_LEN; > - break; > - case INTERVAL_MICROSECOND: > - interval_str = SPIDER_SQL_MICROSECOND_STR; > - interval_len = SPIDER_SQL_MICROSECOND_LEN; > - break; > - default: > - interval_str = ""; > - interval_len = 0; > - break; > - } > - str->length(str->length() - SPIDER_SQL_OPEN_PAREN_LEN); > - if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN + > - interval_len + SPIDER_SQL_COMMA_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(func_name, func_name_length); > - str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); > - str->q_append(interval_str, interval_len); > - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); > - } > - if ((error_num = spider_db_print_item_type(item_list[0], NULL, spider, > - str, alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - if (str) > - { > - if (str->reserve(SPIDER_SQL_COMMA_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_COMMA_STR, SPIDER_SQL_COMMA_LEN); > - } > - if ((error_num = spider_db_print_item_type(item_list[1], NULL, spider, > - str, alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - if (str) > - { > - if (str->reserve(SPIDER_SQL_CLOSE_PAREN_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, > - SPIDER_SQL_CLOSE_PAREN_LEN); > - } > - DBUG_RETURN(0); > -#else > DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > -#endif > } > } else if (func_name_length == 14) > { > @@ -5038,84 +4996,17 @@ int spider_db_mbase_util::open_item_func( > func_name = (char*)item_func->func_name(); > func_name_length = strlen(func_name); > } > - } > break; > case Item_func::CASE_SEARCHED_FUNC: > case Item_func::CASE_SIMPLE_FUNC: > -#ifdef ITEM_FUNC_CASE_PARAMS_ARE_PUBLIC Item_func_case looks somewhat different today, but I suspect it can be pushed too > - Item_func_case *item_func_case = (Item_func_case *) item_func; > - if (str) > - { > - if (str->reserve(SPIDER_SQL_CASE_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_CASE_STR, SPIDER_SQL_CASE_LEN); > - } > - if (item_func_case->first_expr_num != -1) > - { > - if ((error_num = spider_db_print_item_type( > - item_list[item_func_case->first_expr_num], NULL, spider, str, > - alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - } > - for (roop_count = 0; roop_count < item_func_case->ncases; > - roop_count += 2) > - { > - if (str) > - { > - if (str->reserve(SPIDER_SQL_WHEN_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_WHEN_STR, SPIDER_SQL_WHEN_LEN); > - } > - if ((error_num = spider_db_print_item_type( > - item_list[roop_count], NULL, spider, str, > - alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - if (str) > - { > - if (str->reserve(SPIDER_SQL_THEN_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_THEN_STR, SPIDER_SQL_THEN_LEN); > - } > - if ((error_num = spider_db_print_item_type( > - item_list[roop_count + 1], NULL, spider, str, > - alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - } > - if (item_func_case->else_expr_num != -1) > - { > - if (str) > - { > - if (str->reserve(SPIDER_SQL_ELSE_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_ELSE_STR, SPIDER_SQL_ELSE_LEN); > - } > - if ((error_num = spider_db_print_item_type( > - item_list[item_func_case->else_expr_num], NULL, spider, str, > - alias, alias_length, dbton_id, use_fields, fields))) > - DBUG_RETURN(error_num); > - } > - if (str) > - { > - if (str->reserve(SPIDER_SQL_END_LEN + SPIDER_SQL_CLOSE_PAREN_LEN)) > - DBUG_RETURN(HA_ERR_OUT_OF_MEM); > - str->q_append(SPIDER_SQL_END_STR, SPIDER_SQL_END_LEN); > - str->q_append(SPIDER_SQL_CLOSE_PAREN_STR, > - SPIDER_SQL_CLOSE_PAREN_LEN); > - } > - DBUG_RETURN(0); > -#else > DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM); > -#endif > case Item_func::JSON_EXTRACT_FUNC: > func_name = (char*) item_func->func_name(); > func_name_length = strlen(func_name); > - if (str) > - { > if (str->reserve(func_name_length + SPIDER_SQL_OPEN_PAREN_LEN)) > DBUG_RETURN(HA_ERR_OUT_OF_MEM); > str->q_append(func_name, func_name_length); > str->q_append(SPIDER_SQL_OPEN_PAREN_STR, SPIDER_SQL_OPEN_PAREN_LEN); > - } > func_name = SPIDER_SQL_COMMA_STR; > func_name_length = SPIDER_SQL_COMMA_LEN; > separator_str = SPIDER_SQL_COMMA_STR; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] To getting started for contribution at your GitHub repo
by Omkar shivaji Devgirikar 29 Jan '23

29 Jan '23

Respect sir, I Omkar Devgirikar , I am a student and as a beginner i want to start contributing in your server repo with C++ language . Can you please guide me with your project like resources for this project, how get started and some good first issues. Please look at my mail seriously and guide me please. I hope you will look at it. Thank You

2 2

Re: [Maria-developers] 06ce67c644b: MDEV-27653 long uniques don't work with unicode collations
by Sergei Golubchik 16 Jan '23

16 Jan '23

Hi, Alexander, On Jan 15, Alexander Barkov wrote: > revision-id: 06ce67c644b (mariadb-10.4.27-41-g06ce67c644b) > parent(s): 6cb84346e1b > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2023-01-10 18:27:16 +0400 > message: > > MDEV-27653 long uniques don't work with unicode collations > > diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc > index 213d77f8237..5dd19c877af 100644 > --- a/sql/sql_admin.cc > +++ b/sql/sql_admin.cc > @@ -772,11 +772,14 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables, > int check_for_upgrade= file->ha_check_for_upgrade(check_opt); > > if (check_old_types == HA_ADMIN_NEEDS_ALTER || > - check_for_upgrade == HA_ADMIN_NEEDS_ALTER) > + check_old_types == HA_ADMIN_NEEDS_UPGRADE || > + check_for_upgrade == HA_ADMIN_NEEDS_ALTER || > + check_for_upgrade == HA_ADMIN_NEEDS_UPGRADE) eh. So old code was returning HA_ADMIN_NEEDS_ALTER actually quite intentionally. REPAIR TABLE was automatically switching to ALTER if these checks were returning HA_ADMIN_NEEDS_ALTER. So it didn't matter if the message confusingly said "Please use REPAIR", because it would've been ALTER internally anyway. Meaning, I suspect, that the old code was fine and you didn't need to change `case HA_ADMIN_NEEDS_ALTER` and didn't need to replace `return HA_ADMIN_NEEDS_UPGRADE` with HA_ADMIN_NEEDS_ALTER. All you needed to do was to return HA_ADMIN_NEEDS_ALTER from the check_long_hash_compatibility() method and the rest would've likely worked automatically. Or at least with much smaller changes. > { > /* We use extra_open_options to be able to open crashed tables */ > thd->open_options|= extra_open_options; > - result_code= admin_recreate_table(thd, table); > + result_code= admin_recreate_table(thd, table) ? HA_ADMIN_FAILED : > + HA_ADMIN_OK; good catch > thd->open_options&= ~extra_open_options; > goto send_result; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

[Maria-developers] MariaDB // Temporal tables
by Robert Palm 15 Jan '23

15 Jan '23

1 0

Re: [Maria-developers] 0bef50e50b5: MDEV-20501: Assertion `maybe_null || !null_value' failed in Item_func_round::date_op
by Sergei Golubchik 13 Jan '23

13 Jan '23

Hi, Sergei, On Jan 13, Sergei Petrunia wrote: > revision-id: 0bef50e50b5 (mariadb-10.4.27-33-g0bef50e50b5) > parent(s): 5db970fc760 > author: Sergei Petrunia > committer: Sergei Petrunia > timestamp: 2023-01-04 16:50:12 +0300 > message: > > MDEV-20501: Assertion `maybe_null || !null_value' failed in Item_func_round::date_op > > When the optimizer finds a constant (or system) table $TBL which is empty > or has no matching row, it would set table->null_row=true. This is done > even for tables with table->maybe_null==0. > > Then, it would proceed to perform query optimization phases (what for?) > which will attempt to evaluate Item expressions referring to $TBL. > > Eventually some Item expression will get the value of $TBL.not_null_field, > get SQL NULL and fail an assertion. > > Fixed by not performing any query optimization steps after we've got > constant/empty tables with no matching rows. > > Test result changes contain a lot of changes like > - ... Impossible WHERE noticed after reading const tables > + ... no matching row in const table > > as well as other changes caused by slightly-different processing of > the special case of empty constant tables. > > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index 6a441c5047b..57ffd58f8b4 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -2382,6 +2382,9 @@ int JOIN::optimize_stage2() > if (subq_exit_fl) > goto setup_subq_exit; > > + if (zero_result_cause) > + goto setup_subq_exit; In many other cases it's, like zero_result_cause= "No matching min/max row"; subq_exit_fl= true; that is, subq_exit_fl is set whenever zero_result_cause is set. Meaning, perhaps you should do the same in your new code below and then you wouldn't need those lines you've added above? > if (unlikely(thd->check_killed())) > DBUG_RETURN(1); > > @@ -5392,7 +5398,32 @@ make_join_statistics(JOIN *join, List<TABLE_LIST> &tables_list, > } > } > } while (ref_changed); > - > + > + /* > + ConstRowNotFoundShortcut-1: > + Some constant/system tables have mo matching rows. This means that > + the join operation output will be empty. > + Short-cut further optimization steps. > + Note that some query plan steps will still be performed to handle > + implicit grouping, join result setup, etc. > + See also: ConstRowNotFoundShortcut-{2,3}. > + > + Do not do this if we're optimizing for some UNION's fake_select_lex. > + We might be running UglySubqueryReoptimization (grep for name) and > + in this case constant tables are not reliable. > + */ > + if ((join->const_table_map & ~found_const_table_map) && > + !(join->select_lex->master_unit() && > + join->select_lex->master_unit()->fake_select_lex == join->select_lex)) why wouldn't you check for `describe` here? "reoptimization" hack is only needed for EXPLAIN. > + { > + join->zero_result_cause= "no matching row in const table"; > + join->table_count=0; > + join->const_tables= 0; > + join->join_tab= NULL; > + > + DBUG_RETURN(0); > + } > + > join->sort_by_table= get_sort_by_table(join->order, join->group_list, > join->select_lex->leaf_tables, > join->const_table_map); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 5dcde8f6523: MDEV-27653 long uniques don't work with unicode collations
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Alexander, Looks good. A couple of comments, see below On Jan 06, Alexander Barkov wrote: > revision-id: 5dcde8f6523 (mariadb-10.4.26-64-g5dcde8f6523) > parent(s): ce443c85547 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2022-10-28 15:37:44 +0400 > message: > > MDEV-27653 long uniques don't work with unicode collations > > diff --git a/mysql-test/main/ctype_utf8.test b/mysql-test/main/ctype_utf8.test > index cc61c2ae0fe..e2d4e4ab906 100644 > --- a/mysql-test/main/ctype_utf8.test > +++ b/mysql-test/main/ctype_utf8.test > @@ -2310,3 +2310,133 @@ VALUES (_latin1 0xDF) UNION VALUES(_utf8'a' COLLATE utf8_bin); > --echo # > --echo # End of 10.3 tests > --echo # > + > + > +--echo # > +--echo # Start of 10.4 tests > +--echo # > + > +--echo # > +--echo # MDEV-27653 long uniques don't work with unicode collations > +--echo # > + > +SET NAMES utf8mb3; > + > +# CHAR > + > +CREATE TABLE t1 ( > + a CHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a CHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# VARCHAR > + > +CREATE TABLE t1 ( > + a VARCHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a VARCHAR(30) COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# TEXT > + > +CREATE TABLE t1 (a TEXT COLLATE utf8mb3_general_ci UNIQUE); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > +CREATE TABLE t1 ( > + a LONGTEXT COLLATE utf8mb3_general_ci, > + UNIQUE KEY(a(10)) USING HASH > +); > +SHOW CREATE TABLE t1; > +INSERT INTO t1 VALUES ('a'); > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +SELECT * FROM t1; > +DROP TABLE t1; > + > + > +# Testing upgrade: > +# Prior to MDEV-27653, the UNIQUE HASH function errorneously > +# took into account string octet length. > +# Old tables should still open and work, but with a wrong results. > + > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.frm $MYSQLD_DATADIR/test/t1.frm; > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.MYD $MYSQLD_DATADIR/test/t1.MYD; > +copy_file std_data/mysql_upgrade/mdev27653_100422_text.MYI $MYSQLD_DATADIR/test/t1.MYI; > +SHOW CREATE TABLE t1; > +SELECT a, OCTET_LENGTH(a) FROM t1 ORDER BY BINARY a; > +CHECK TABLE t1; > + > +# There is already a one byte value 'a' in the table > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('A'); > + > +# There is already a two-byte value 'ä' in the table > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('Ä'); > + > +# There were no three-byte values in the table so far. > +# The below value violates UNIQUE, but it gets inserted. > +# This is wrong but expected for a pre-MDEV-27653 table. > +INSERT INTO t1 VALUES ('Ấ'); > +SELECT a, OCTET_LENGTH(a) FROM t1 ORDER BY BINARY a; > +CHECK TABLE t1; > + > +# ALTER FORCE fails: it tries to rebuild the table > +# with a correct UNIQUE HASH function, but there are duplicates! > +--error ER_DUP_ENTRY > +ALTER TABLE t1 FORCE; please, try ALTER IGNORE TABLE too > + > +# Let's remove all duplicate values, so only the one-byte 'a' stays. > +# ALTER..FORCE should work after that. > +DELETE FROM t1 WHERE OCTET_LENGTH(a)>1; > +ALTER TABLE t1 FORCE; > + > +# Make sure that 'a' and 'ä' cannot co-exists any more, > +# because the table was recreated with a correct UNIQUE HASH function. > +--error ER_DUP_ENTRY > +INSERT INTO t1 VALUES ('ä'); > +DROP TABLE t1; > + > +--echo # > +--echo # End of 10.4 tests > +--echo # > diff --git a/sql/table.cc b/sql/table.cc > index b9260853381..97f71284005 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -1267,7 +1286,11 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT *mem_root, TABLE *table, > list_item= new (mem_root) Item_field(thd, keypart->field); > field_list->push_back(list_item, mem_root); > } > - Item_func_hash *hash_item= new(mem_root)Item_func_hash(thd, *field_list); > + > + Item_func_hash *hash_item= make_unique_hash_func(thd, mem_root, > + table->s->mysql_version, > + field_list); would be good to fix CHECK ... FOR UPGRADE too. > + > Virtual_column_info *v= new (mem_root) Virtual_column_info(); > field->vcol_info= v; > field->vcol_info->expr= hash_item; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 92ff948d021: MDEV-29231 View returns wrong value with SQL_MODE 'NO_BACKSLASH_ESCAPES'
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Oleksandr, On Jan 06, Oleksandr Byelkin wrote: > revision-id: 92ff948d021 (mariadb-10.3.37-53-g92ff948d021) > parent(s): 180b2bcd538 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-12-05 20:42:03 +0100 > message: > > MDEV-29231 View returns wrong value with SQL_MODE 'NO_BACKSLASH_ESCAPES' > > 1. make view printing sql_mode independent > 1.1. Print always current escape character > 1.2. Allow escape character usage independent of sql_mode > 2. Add workaround for parsing "like 3 in (0,1) escape 3" problem > (alwaysuse parances in case of printing ESCAPE) > > diff --git a/mysql-test/main/ctype_cp1250_ch.result b/mysql-test/main/ctype_cp1250_ch.result > index b0aa4cff382..88175ae5013 100644 > --- a/mysql-test/main/ctype_cp1250_ch.result > +++ b/mysql-test/main/ctype_cp1250_ch.result > @@ -129,7 +129,7 @@ EXPLAIN EXTENDED SELECT * FROM t1 WHERE CONCAT(c1)='a' AND CONCAT(c1) LIKE 'a '; > id select_type table type possible_keys key key_len ref rows filtered Extra > 1 SIMPLE t1 ALL NULL NULL NULL NULL 2 100.00 Using where > Warnings: > -Note 1003 select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like 'a ' > +Note 1003 select `test`.`t1`.`c1` AS `c1` from `test`.`t1` where concat(`test`.`t1`.`c1`) = 'a' and concat(`test`.`t1`.`c1`) like ('a ') escape '\\' I don't think there's a need to print the escape clause here, it only makes the output less readable. > diff --git a/mysql-test/main/default.result b/mysql-test/main/default.result > index c9ac96367aa..5c72b894bf6 100644 > --- a/mysql-test/main/default.result > +++ b/mysql-test/main/default.result > @@ -1936,7 +1936,7 @@ CREATE OR REPLACE TABLE t1 ( col INT DEFAULT ( 1 LIKE ( NOW() BETWEEN '2000-01-0 > SHOW CREATE TABLE t1; > Table Create Table > t1 CREATE TABLE `t1` ( > - `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12')) > + `col` int(11) DEFAULT (1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\') So, there're basically two approaches to a fix and we're inconsistently using both for different items. 1. one can always print expressions in the environment-independent way - create longer less readable text - not all items support it, new syntax might be needed (cannot emulate NO_BACKSLASH_ESCAPES with the ESCAPE clause) 2. one can set the environment (sql_mode) to the same hard-coded value before parsing views and vcols - one cannot dump the view/table definition in one sql_mode and load in another + this isn't normally a problem, as mysqldump also uses a fixed sql_mode - some item properties (what are they?) might be only set at fix_fields, fixing sql_mode during parsing won't help there Could we try to agree to one approach and use it consistently for all items from now on? > diff --git a/mysql-test/main/parser.result b/mysql-test/main/parser.result > index 0bb4e82c8b8..79ec9f539eb 100644 > --- a/mysql-test/main/parser.result > +++ b/mysql-test/main/parser.result > @@ -1325,11 +1325,11 @@ select 1 between 2 in (3,4) and 5 AS `1 between (2 in (3,4)) and 5` > create or replace view v1 as select 1 between (2 like 3) and 4; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 1 between 2 like 3 and 4 AS `1 between (2 like 3) and 4` > +select 1 between 2 like (3) escape '\\' and 4 AS `1 between (2 like 3) and 4` > create or replace view v1 as select 1 not between (2 like 3) and 4; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 1 not between 2 like 3 and 4 AS `1 not between (2 like 3) and 4` > +select 1 not between 2 like (3) escape '\\' and 4 AS `1 not between (2 like 3) and 4` > drop view v1; > # > # Start of 10.2 tests > diff --git a/mysql-test/main/precedence.result b/mysql-test/main/precedence.result > index fc6579651b4..ce26ca2acf2 100644 > --- a/mysql-test/main/precedence.result > +++ b/mysql-test/main/precedence.result > @@ -113,7 +113,7 @@ NOT 2 != 3 NOT (2 != 3) (NOT 2) != 3 > create or replace view v1 as select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3; > Select view_definition from information_schema.views where table_schema='test' and table_name='v1'; > view_definition > -select 2 not like 3 AS `NOT 2 LIKE 3`,2 not like 3 AS `NOT (2 LIKE 3)`,!2 like 3 AS `(NOT 2) LIKE 3` > +select 2 not like (3) escape '\\' AS `NOT 2 LIKE 3`,2 not like (3) escape '\\' AS `NOT (2 LIKE 3)`,!2 like (3) escape '\\' AS `(NOT 2) LIKE 3` > select NOT 2 LIKE 3, NOT (2 LIKE 3), (NOT 2) LIKE 3 union select * from v1; > NOT 2 LIKE 3 NOT (2 LIKE 3) (NOT 2) LIKE 3 > 1 1 0 > diff --git a/mysql-test/main/precedence_bugs.result b/mysql-test/main/precedence_bugs.result > index 723ab823b48..9654e3c31db 100644 > --- a/mysql-test/main/precedence_bugs.result > +++ b/mysql-test/main/precedence_bugs.result > @@ -54,7 +54,7 @@ drop table t1; > create view v1 as select 1 like (now() between '2000-01-01' and '2012-12-12' ); > show create view v1; > View v1 > -Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') AS `1 like (now() between '2000-01-01' and '2012-12-12' )` > +Create View CREATE ALGORITHM=UNDEFINED DEFINER=`root`@`localhost` SQL SECURITY DEFINER VIEW `v1` AS select 1 like (current_timestamp() between '2000-01-01' and '2012-12-12') escape '\\' AS `1 like (now() between '2000-01-01' and '2012-12-12' )` > character_set_client latin1 > collation_connection latin1_swedish_ci > drop view v1; if you'll always print ESCAPE clause, it'll make many tests in parser.test, precedence.test and precedence_bugs.test meaningless. Don't just update result files, please, remove tests that no longer make sense. Or rewrite them, when possible (but in precedence.test it's not). > diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc > index b7b0c981c2d..702689a7496 100644 > --- a/sql/item_cmpfunc.cc > +++ b/sql/item_cmpfunc.cc > @@ -5262,10 +5262,15 @@ void Item_func_like::print(String *str, enum_query_type query_type) > str->append(STRING_WITH_LEN(" not ")); > str->append(func_name()); > str->append(' '); > - if (escape_used_in_parsing) > + if (escape_used_in_parsing || (query_type | QT_VIEW_INTERNAL)) > { > - args[1]->print_parenthesised(str, query_type, precedence()); > - str->append(STRING_WITH_LEN(" escape ")); > + /* > + Explicit parencies is workaround against parser bug which üprevent > + patsing "like 3 in (0,1) escape 3" correctly > + */ > + str->append('('); > + args[1]->print(str, query_type); > + str->append(STRING_WITH_LEN(") escape ")); no, this is wrong. Don't just force parentheses there. Instead, fix the precedence, so that the parentheses would be printed automatically when needed. > escape_item->print_parenthesised(str, query_type, higher_precedence()); > } > else > @@ -5390,10 +5395,7 @@ bool fix_escape_item(THD *thd, Item *escape_item, String *tmp_str, > if (escape_str) > { > const char *escape_str_ptr= escape_str->ptr(); > - if (escape_used_in_parsing && ( > - (((thd->variables.sql_mode & MODE_NO_BACKSLASH_ESCAPES) && > - escape_str->numchars() != 1) || > - escape_str->numchars() > 1))) > + if (escape_used_in_parsing && escape_str->numchars() > 1) I still think that ESCAPE '' doesn't mean "no escape character". I tried and in my tests it didn't. > { > my_error(ER_WRONG_ARGUMENTS,MYF(0),"ESCAPE"); > return TRUE; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] 6ff49e48e4f: MDEV-26161: fix of view protocol
by Sergei Golubchik 06 Jan '23

06 Jan '23

Hi, Oleksandr, On Jan 06, Oleksandr Byelkin wrote: > revision-id: 6ff49e48e4f (mariadb-10.3.37-54-g6ff49e48e4f) > parent(s): 92ff948d021 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2022-12-06 10:22:06 +0100 > message: > > MDEV-26161: fix of view protocol could you please amend the commit comment to explain why the statement produces a different error in a view protocol? > diff --git a/mysql-test/main/gis.result b/mysql-test/main/gis.result > index 358be520b06..37613bc71a0 100644 > --- a/mysql-test/main/gis.result > +++ b/mysql-test/main/gis.result > @@ -4980,11 +4980,11 @@ ERROR HY000: Illegal parameter data type geometry for operation 'is_used_lock' > # MDEV-26161 crash in Gis_point::calculate_haversine > # > select st_distance_sphere(x'01030000000400000004000000000000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > select st_distance_sphere(x'010300000004000000040000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > select st_distance_sphere(x'010300000001000000040000', multipoint(point(124,204)), 10); > -ERROR 22003: Cannot get geometry object from data you send to the GEOMETRY field > +Got one of the listed errors > # > # End of 10.3 tests > # > diff --git a/mysql-test/main/gis.test b/mysql-test/main/gis.test > index 716fab9bfeb..07b207dcdc9 100644 > --- a/mysql-test/main/gis.test > +++ b/mysql-test/main/gis.test > @@ -3093,11 +3093,11 @@ SELECT IS_USED_LOCK(POINT(1,1)); > --echo # > --echo # MDEV-26161 crash in Gis_point::calculate_haversine > --echo # > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'01030000000400000004000000000000', multipoint(point(124,204)), 10); > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'010300000004000000040000', multipoint(point(124,204)), 10); > ---error ER_CANT_CREATE_GEOMETRY_OBJECT > +--error ER_CANT_CREATE_GEOMETRY_OBJECT,ER_INTERNAL_ERROR > select st_distance_sphere(x'010300000001000000040000', multipoint(point(124,204)), 10); > > --echo # > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: [Maria-developers] MDEV-28602 Wrong result with outer join, merged derived table and view
by Sergey Petrunia 04 Jan '23

04 Jan '23

Hi Rex, Please find below input for the commit. (I've actually committed an alternative fix now, but before doing that I wrote all the trivial comments below so I'm sending these anyway in order to show what is expected of a patch. I'm also including the rationale for creating an alternative patch) > commit 75af3195482d3da277825663d1150c0dfc55420a > Author: Rex <rex.johnston(a)mariadb.com> > Date: Fri Dec 23 05:28:59 2022 +1200 > > MDEV-28602 Wrong result with outer join, merged derived table and view > > const item reference being mishandled in outer to inner join on > filling in null values. > First, fixVersion. It is currently set to 10.11, but affectsVersion starts from 10.3 I think the bug and the fix are applicable for all versions. Second: commit comment. This is not descriptive enough: > const item reference being mishandled in outer to inner join on > filling in null values. Something like this would be better: <commit-comment> A LEFT JOIN with a constant as a column of the inner table produced wrong query result if the optimizer had to write the inner table column into a temp table: SELECT ... FROM (SELECT /*non-mergeable select*/ FROM t1 LEFT JOIN (SELECT 'Y' as Val) t2 ON ...) Fixed this by: 1. Making Item_ref::save_in_field() call check_null_ref() to see if the referred table has a NULL-complemented row. 2. In order to do #1, moved null_ref_table() and check_null_ref() from Item_direct_view_ref to Item_ref. </commit-comment> Note: it starts with a siccint problem description, which is followed by a description of the fix. > diff --git a/sql/sql_join_cache.cc b/sql/sql_join_cache.cc > index 620c52a3f40..65bedc3d337 100644 > --- a/sql/sql_join_cache.cc > +++ b/sql/sql_join_cache.cc > @@ -2622,7 +2622,7 @@ enum_nested_loop_state JOIN_CACHE::join_null_complements(bool skip_last) > get_record(); > /* The outer row is complemented by nulls for each inner table */ > restore_record(join_tab->table, s->default_values); > - mark_as_null_row(join_tab->table); > + mark_as_null_row(join_tab->table); In the future please make sure patches do not contain unrelated meaningless changes like this one. > rc= generate_full_extensions(get_curr_rec()); > if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS) > goto finish; > diff --git a/mysql-test/main/merge.test b/mysql-test/main/merge.test > index 0485f3ed1c3..05b05287e1b 100644 > --- a/mysql-test/main/merge.test > +++ b/mysql-test/main/merge.test Why is the patch in the merge.test ? merge.test at the top says clearly "Test of MERGE TABLES". That is, this is a test for tables with ENGINE=MERGE, a feature not related to this bug. The right test files for this bug would be main/join_outer.test and main/derived.test, I would prefer the first one. > @@ -2886,3 +2886,50 @@ drop table tm, t; > --echo # > --echo # End of 10.8 tests > --echo # > + > +--echo # > +--echo # MDEV-28602 Wrong result with outer join, merged derived table and view > +--echo # > + > +drop table if exists t1, t2; ^^^ This is not needed as there is a DROP TABLE right above > +create table t1 ( > + Election int(10) unsigned NOT NULL > +); > + > +insert into t1 (Election) values (1); > + +create table t2 ( + VoteID int(10), + ElectionID int(10), + UserID int(10) +); + +insert into t2 (ElectionID, UserID) values (2, 30), (3, 30); +# INSERT INTO t2 (ElectionID, UserID) VALUES (1, 30); ^^ No need to have commented-out lines in the commit. +drop view if exists v1; ^^^ same as above: this is not needed. +create view v1 as select * from t1 + left join ( select 'Y' AS Voted, ElectionID from t2 ) AS T + on T.ElectionID = t1.Election +limit 9; +# limit X causes merge algorithm select as opposed to temp table +select * from v1; ... > diff --git a/sql/item.h b/sql/item.h > index 2d598546b91..8684477b558 100644 > --- a/sql/item.h > +++ b/sql/item.h > @@ -5531,18 +5531,42 @@ class Item_sp > > class Item_ref :public Item_ident > { > +public: > + > +#define NO_NULL_TABLE (reinterpret_cast<TABLE *>(0x1)) > + > + void set_null_ref_table() > + { > + null_ref_table= NO_NULL_TABLE; > + } > + > + bool check_null_ref() This works, but looks like a change that is 1. Too big 2. Looks like a partial implementation I'm looking at methods of Item_direct_view_ref: - save_val() - save_org_in_field() - save_in_result_field() - val_real() and other val_XXX() and they all follow the same pattern: if (check_null_ref()) produce SQL NULL; else Call Item_direct_ref::same_method(); This suggests we could add Item_direct_view_ref::save_in_field() and use the same approach. Conversely, if we move check_null_ref() to be in Item_ref, we will end up with: - check_null_ref() is in Item_ref - methods that make use of it are in Item_direct_view_ref() - methods of Item_ref() do not make check_null_ref() check, except for save_in_field() which does it. I think the former looks more logical than the latter. I've made the patch for it and I'll ask Sanja (the Item_ref/views expert) to check it. BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

1 0