- developers - lists.mariadb.org

Re: d8e04ef367b: MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared to never be null in my_strdup
by Sergei Golubchik 07 Jul '23

07 Jul '23

Hi, Oleksandr, On Jul 07, Oleksandr Byelkin wrote: > revision-id: d8e04ef367b (mariadb-10.4.30-9-gd8e04ef367b) > parent(s): 423c28f0aa4 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-03 17:50:49 +0200 > message: > > MDEV-25237 Assertion `global_system_variables.session_track_system_variables' failed in Session_sysvars_tracker::init | SIGSEGV's in __strlen_avx2 | UBSAN: runtime error: null pointer passed as argument 1, which is declared to never be null in my_strdup first, if you see a bug with a very long summary, particularly if it's basically makes no sense to a user - in that case, please, don't hesitate to make it shorter and more meaningful. For example "crash after setting session_track_system_variables to an invalid value" > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -620,7 +620,12 @@ class Sys_var_sesvartrack: public Sys_var_charptr_base > { > if (sysvartrack_global_update(thd, new_val, > var->save_result.string_value.length)) > - new_val= 0; > + { > + if (new_val) > + my_free(new_val); > + // keep the old value in case of error > + new_val= (char*)my_strdup(global_var(const char*), MYF(MY_WME)); > + } No, this is wrong. Well, not wrong, but an incorrect bug fix. update should not fail, this is the contract. check() is supposed to verify that the value is valid, so basically update must succeed. technically it can fail because of OOM, I suppose, so restoring the value here might be needed still. But in the case of OOM, strdup() might be not such a great idea. Perhaps you can make sure that OOM is impossible and update truly cannot fail? Anyway, the actual fix for MDEV-25237 is to make check fail on an invalid value. In fact, it was a simple typo: @@ -221,7 +221,7 @@ bool sysvartrack_validate_value(THD *thd, const char *str, > /* Remove leading/trailing whitespace. */ trim_whitespace(system_charset_info, &var); - if (!strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) + if (strcmp(var.str, "*") && !find_sys_var(thd, var.str, var.length)) return true; > } > global_update_finish(new_val); > return (new_val == 0 && var->save_result.string_value.str != 0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: d23555b1920: MDEV-30984 Online ALTER table is denied with non-informative error messages
by Sergei Golubchik 06 Jul '23

06 Jul '23

Hi, Nikita, On Jun 24, Nikita Malyavin wrote: > revision-id: d23555b1920 (mariadb-11.0.1-138-gd23555b1920) > parent(s): df90bf34644 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-06-20 16:10:48 +0300 > message: > > MDEV-30984 Online ALTER table is denied with non-informative error messages > > diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result > --- a/mysql-test/main/alter_table_online.result > +++ b/mysql-test/main/alter_table_online.result > @@ -179,25 +179,27 @@ t CREATE TABLE `t` ( > UNIQUE KEY `b` (`b`) > ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci > alter table t drop b, change c c serial, algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > # Check existed unique keys. > create or replace table t(a int, b int not null, c int not null, d int); > # No unique in the old table; > alter table t add unique(b, c), modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > alter table t add unique(a, b); > # Unique in the old table has nulls; > alter table t modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > alter table t add unique(b, c); > # Change unique'scolumn; Typo > -alter table t change b x int, modify d int auto_increment, add key(d), > +alter table t change b x bigint, modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > -ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > -# Finally good. > -alter table t modify d int auto_increment, add key(d), > +ERROR 0A000: LOCK=NONE is not supported. Reason: CHANGE COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +# Finally good. Simple renames with a type unchenged will not affect typo s/unchenged/unchanged/ > +# the result. Also NOT NULL -> NULL transform is fine. > +alter table t modify d int auto_increment, add key(d), > +change b x int null, > algorithm=copy, lock=none; > drop table t; > # MDEV-31172 Server crash or ASAN errors in online_alter_check_autoinc > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -1228,6 +1228,15 @@ class Query_arena > { return strdup_root(mem_root,str); } > inline char *strmake(const char *str, size_t size) const > { return strmake_root(mem_root,str,size); } > + inline LEX_CSTRING strcat(const LEX_CSTRING &a, const LEX_CSTRING &b) const > + { > + char *buf= (char*)alloc(a.length + b.length + 1); > + if (unlikely(!buf)) > + return null_clex_str; > + strncpy(buf, a.str, a.length); memcpy both ↑ and ↓ (and then you might need to append \0 explicitly) > + strncpy(buf + a.length, b.str, b.length + 1); > + return {buf, a.length + b.length}; > + } > inline void *memdup(const void *str, size_t size) const > { return memdup_root(mem_root,str,size); } > inline void *memdup_w_gap(const void *str, size_t size, size_t gap) const > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9869,26 +9869,21 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > */ > for (uint k= 0; k < table->s->keys; k++) > { > - const KEY &key= table->s->key_info[k]; > + const KEY &key= table->key_info[k]; This is generally incorrect. User specified keys are in table->s->key_info[] Keys in table->key_info[] can be modified to match what indexes are actually created, in particular, for long uniques table->key_info[] differs from table->s->key_info[] > if ((key.flags & HA_NOSAME) == 0 || key.flags & HA_NULL_PART_KEY) > continue; > bool key_parts_good= true; > for (uint kp= 0; kp < key.user_defined_key_parts && key_parts_good; kp++) > { > - const char *field_name= key.key_part[kp].field->field_name.str; > - for (const auto &c: alter_info->drop_list) > - if (c.type == Alter_drop::COLUMN > - && my_strcasecmp(system_charset_info, c.name, field_name) == 0) > - { > - key_parts_good= false; > - break; > - } > + const Field *f= key.key_part[kp].field; > + // tmp_set contains dropped fields after mysql_prepare_alter_table > + key_parts_good= !bitmap_is_set(&table->tmp_set, f->field_index); > + > if (key_parts_good) > for (const auto &c: alter_info->create_list) > - if (c.change.str && my_strcasecmp(system_charset_info, c.change.str, > - field_name) == 0) > + if (c.field == f) > { > - key_parts_good= false; > + key_parts_good= f->is_equal(c); > break; > } > } > @@ -9896,25 +9891,63 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > return true; > } > > - const char *old_autoinc= table->found_next_number_field > - ? table->found_next_number_field->field_name.str > - : ""; > - bool online= true; > for (const auto &c: alter_info->create_list) > { > - if (c.change.str && c.flags & AUTO_INCREMENT_FLAG) > + if (c.flags & AUTO_INCREMENT_FLAG) > { > - if (my_strcasecmp(system_charset_info, c.change.str, old_autoinc) != 0) > + if (c.field && !(c.field->flags & AUTO_INCREMENT_FLAG)) > + return false; > + break; > + } > + } > + return true; > +} > + > +static > +const char *online_alter_check_supported(const THD *thd, > + const Alter_info *alter_info, > + const TABLE *table, bool *online) > { > - if (c.create_if_not_exists // check IF EXISTS option > - && table->find_field_by_name(&c.change) == NULL) > - continue; > - online= false; > + DBUG_ASSERT(!table->s->tmp_table); that's confusing. *online can be false here already, then return "DROP SYSTEM VERSIONING" will be wrong and you use some other condition to decide whether "DROP SYSTEM VERSIONING" is correct or not. it'd be much clearer not to call online_alter_check_supported in this case. and add here DBUG_ASSERT(*online); > + > + *online= *online && (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) == 0; and then you won't need to use && above, just as you don't use it below > + if (!*online) > + return "DROP SYSTEM VERSIONING"; > + > + *online= !thd->lex->ignore; > + if (!*online) > + return "ALTER IGNORE TABLE"; > + > + *online= !table->versioned(VERS_TRX_ID); > + if (!*online) > + return "BIGINT GENERATED ALWAYS AS ROW_START"; > + > + List<FOREIGN_KEY_INFO> fk_list; > + table->file->get_foreign_key_list(thd, &fk_list); > + for (auto &fk: fk_list) > + { > + if (fk_modifies_child(fk.delete_method) || > + fk_modifies_child(fk.update_method)) > + { > + *online= false; > + // Don't fall to a common unsupported case to avoid heavy string ops. > + if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE) > + { > + return fk_modifies_child(fk.delete_method) > + ? thd->strcat({STRING_WITH_LEN("ON DELETE ")}, > + *fk_option_name(fk.delete_method)).str > + : thd->strcat({STRING_WITH_LEN("ON UPDATE ")}, > + *fk_option_name(fk.update_method)).str; > } > - break; > + return NULL; > } > } > - return online; > + > + *online= online_alter_check_autoinc(thd, alter_info, table); > + if (!*online) > + return "CHANGE COLUMN ... AUTO_INCREMENT"; > + > + return NULL; > } > > > @@ -10979,14 +10994,14 @@ do_continue:; > > if (!table->s->tmp_table) > { > - // COPY algorithm doesn't work with concurrent writes. > + auto *reason= online_alter_check_supported(thd, alter_info, table, &online); > + // COPY algorithm works with concurrent writes only when online is true. as I wrote above, better put it inside if (online) { > if (!online && > alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE) > { > + DBUG_ASSERT(reason); > my_error(ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, MYF(0), > - "LOCK=NONE", > - ER_THD(thd, ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY), > - "LOCK=SHARED"); > + "LOCK=NONE", reason, "LOCK=SHARED"); > goto err_new_table_cleanup; > } > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 7

[MariaDB developers]Re: a2e71bc8e89: MDEV-22979 MDEV-31400 Fixing spider init bugs
by Yuchen Pei 03 Jul '23

03 Jul '23

Hi Sergei, Thanks for the comments. I have addressed them and the url of the new patch can be found in the new request for review comment in the ticket. On Mon 2023-06-12 18:07:03 +0200, Sergei Golubchik wrote: > Hi, Yuchen, > > See comments inline. Here I'm only reviewing MDEV-31400, > please, tell me if you'd like me to review spider part too > Given that Alexey (cc'd) has already been working on the review of the spider init bug fix, I think it makes sense to ask Alexey to continue on the spider part, which I just did on MDEV-22979. Given that the spider part requires the patch for MDEV-31400 to work, I have the spider part on top of the init dependency part. > On Jun 12, Yuchen Pei wrote: >> revision-id: a2e71bc8e89 (mariadb-10.9.5-19-ga2e71bc8e89) >> parent(s): cb0e0c915f6 >> author: Yuchen Pei >> committer: Yuchen Pei >> timestamp: 2023-06-08 16:13:56 +1000 >> message: >> >> MDEV-22979 MDEV-31400 Fixing spider init bugs > > please, make a separate commit for MDEV-31400 Done. > >> diff --git a/sql/handler.cc b/sql/handler.cc >> index a12e9ea18f5..60080f1da6a 100644 >> --- a/sql/handler.cc >> +++ b/sql/handler.cc >> @@ -646,10 +648,14 @@ int ha_initialize_handlerton(st_plugin_int *plugin) >> hton->slot= HA_SLOT_UNDEF; >> /* Historical Requirement */ >> plugin->data= hton; // shortcut for the future >> - if (plugin->plugin->init && plugin->plugin->init(hton)) >> + if (plugin->plugin->init && (ret= plugin->plugin->init(hton))) >> { >> - sql_print_error("Plugin '%s' init function returned error.", >> - plugin->name.str); >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' init function returned error but >> may be retried.", >> + plugin->name.str); >> + else >> + sql_print_error("Plugin '%s' init function returned error.", >> + plugin->name.str); > > let's treat all plugins identically. Meaning, in particular, no warnings > or errors in ha_initialize_handlerton(). They belong to sql_plugin.cc > Done. >> goto err; >> } >> >> diff --git a/sql/mysqld.cc b/sql/mysqld.cc >> index 32392ab882e..caf85736770 100644 >> --- a/sql/mysqld.cc >> +++ b/sql/mysqld.cc >> @@ -365,6 +365,7 @@ bool opt_disable_networking=0, opt_skip_show_db=0; >> bool opt_skip_name_resolve=0; >> my_bool opt_character_set_client_handshake= 1; >> bool opt_endinfo, using_udf_functions; >> +bool udf_initialized= 0; > > I don't think you need that. You can use exising mysqld_server_started. > That won't work, because mysqld_server_started will only be turned on very late, towards the end of mysqld_main(). If spider is initialised between udf_init() and mysqld_server_started is 1 (for example in an init-file), then the udf will fail. See https://github.com/MariaDB/server/commit/91176a7be7c where the test udf_mysql_func_early fails with "query 'SELECT SPIDER_DIRECT_SQL('select * from tbl_a', 'results', 'srv "s_2_1", database "auto_test_remote"')' failed: ER_SP_DOES_NOT_EXIST (1305): FUNCTION auto_test_local.SPIDER_DIRECT_SQL does not exist" because of this. Now, I assume MDEV-31401, the task to include the udf insertion trick in early calls CREATE FUNCTION, will fix this issue without introduce any new global booleans because it should has access to the internal variable `initialized`, though I do not know how complex or doable this task is. So I guess there's a trade-off here between fixing the spider init bugs now and investigating MDEV-31401. Would you like me to explore MDEV-31401 first? >> my_bool locked_in_memory; >> bool opt_using_transactions; >> bool volatile abort_loop; >> diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc >> index 5a077a934ac..e564ab5a38d 100644 >> --- a/sql/sql_plugin.cc >> +++ b/sql/sql_plugin.cc >> @@ -1462,10 +1462,16 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, >> >> if (plugin_type_initialize[plugin->plugin->type]) >> { >> - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) >> + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin); >> + if (ret) >> { >> - sql_print_error("Plugin '%s' registration as a %s failed.", >> - plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + /* Plugin init failed but requested a retry if possible */ >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + else >> + sql_print_error("Plugin '%s' registration as a %s failed.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > > 1. "may be" is confusing, we should be less vague in the error > messages Done. I have expanded it a bit more to the following but we still really use "will be" here without either losing accuracy or being overly verbose. Even if it fails and returns the retry code, the retry will not happen if no new plugins are successfully initialised this round. --8<---------------cut here---------------start------------->8--- sql_print_warning("Plugin '%s' registration as a %s failed and may " "be retried provided it is being loaded with " "plugin-load[-add]", plugin->name.str, plugin_type_names[plugin->plugin->type].str); --8<---------------cut here---------------end--------------->8--- > > 2. about (ret == -1). I looked at what plugin init functions return now: > 33 plugins don't have an init function > 76 plugins always return 0 > 13 plugins return 0 or 1 > 3 plugins return 0 or -1 > 3 plugins return 0 or HA_ERR_INITIALIZATION, HA_ERR_OUT_OF_MEM > and innodb inconsistently return 0, HA_ERR_xxx as above, or 1 > > so, I suggest to introduce, like, HA_ERR_DEPENDENCIES (or whatever) > and only retry plugins that return that specific error code. > Done. The HA_ERR_ namespace is tightly packed and bounded by HA_ERR_FIRST (120) and HA_ERR_LAST (198), and I crammed in a #define HA_ERR_RETRY_INIT 129. >> goto err; >> } >> } >> @@ -1462,8 +1462,14 @@ static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, >> >> if (plugin_type_initialize[plugin->plugin->type]) >> { >> - if ((*plugin_type_initialize[plugin->plugin->type])(plugin)) >> + ret= (*plugin_type_initialize[plugin->plugin->type])(plugin); > > there's a second branch below, with plugin->plugin->init(), > please, handle it too. May be, do as with deinit? > > plugin_type_init deinit= plugin_type_deinitialize[plugin->plugin->type]; > if (!deinit) > deinit= (plugin_type_init)(plugin->plugin->deinit); > > if (deinit && deinit(plugin)) > > Less code duplication here. > Done. >> + if (ret) >> { >> + /* Plugin init failed but requested a retry if possible */ >> + if (unlikely(ret == -1)) >> + sql_print_warning("Plugin '%s' registration as a %s failed but may be retried.", >> + plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> + else >> sql_print_error("Plugin '%s' registration as a %s failed.", >> plugin->name.str, plugin_type_names[plugin->plugin->type].str); >> goto err; >> @@ -1737,11 +1743,34 @@ int plugin_init(int *argc, char **argv, int flags) >> */ >> >> mysql_mutex_lock(&LOCK_plugin); >> + /* List of plugins to reap */ >> reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); >> *(reap++)= NULL; >> + /* List of plugins to retry */ >> + retry= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); >> + *(retry++)= NULL; >> >> for(;;) >> { >> + /* Number of plugins that is successfully initialised in a round */ >> + int num_initialized; >> + do >> + { >> + num_initialized= 0; >> + /* If any plugins failed and requested a retry, clean up before >> + retry */ >> + while ((plugin_ptr= *(--retry))) >> + { >> + mysql_mutex_unlock(&LOCK_plugin); >> + plugin_deinitialize(plugin_ptr, true); >> + mysql_mutex_lock(&LOCK_plugin); >> + /** Needed to satisfy assertions in `test_plugin_options()` */ > > Hmm, I don't think it'll work. I mean, test_plugin_options() - as far as > I remember - will remove processed options from the command line array, > meaning you cannot call it twice for the same plugin. > > May be it'd be better to repeat only plugin init function call and not > the complete initialize/deinitialize cycle? Done. I separated out a plugin_do_initialize() function that skips the test_options() part and largely mirrors the plugin_deinitialize() function, and calls this function instead of the full plugin_initialize() during a retry. I also made sure the code path outside of plugin_init() does not change, i.e. if plugin_initialize() is called from an INSTALL statement. The contract of init and deinit is not clear, but I suppose it is a reasonable expectation that if a plugin init wants to be retried, it will clean up before returning init instead of relying the server to call the deinit before retrying. > >> + my_afree(plugin_ptr->ptr_backup); >> + plugin_ptr->ptr_backup= NULL; >> + plugin_ptr->nbackups= 0; >> + plugin_ptr->state= PLUGIN_IS_UNINITIALIZED; >> + } >> + retry++; >> for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) >> { >> HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > > Regards, > Sergei > VP of MariaDB Server Engineering > and security(a)mariadb.org Best, Yuchen

2 3

Re: 8b5de389ab1: MDEV-31400 Simple plugin dependency resolution
by Sergei Golubchik 03 Jul '23

03 Jul '23

Hi, Yuchen, On Jul 01, Yuchen Pei wrote: > revision-id: 8b5de389ab1 (mariadb-10.9.5-17-g8b5de389ab1) > parent(s): d8997f875e2 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-06-13 20:08:28 +1000 > message: > > MDEV-31400 Simple plugin dependency resolution > > We introduce simple plugin dependency. A plugin init function may > return HA_ERR_RETRY_INIT. If this happens during server startup when > the server is trying to initialise all plugins, the failed plugins > will be retried, until no more plugins succeed in initialisation or > want to be retried. > > This will fix spider init bugs which is caused in part by its > dependency on Aria for initialisation. > > The reason we need a new return code, instead of treating every > failure as a request for retry, is that it may be impossible to clean > up after a failed plugin initialisation. Take InnoDB for example, it > has a global variable `buf_page_cleaner_is_active`, which may not > satisfy an assertion during a second initialisation try, probably > because InnoDB does not expect the initialisation to be called > twice. A test that may fail because of this is > `encryption.corrupted_during_recovery`, see for example[1], which is > tested at 73835f64b7fc245d38812380685aca03bef72bb5, a previous commit again, commits that you are going to push into the main branche should not refer to commits that you aren't going to push. these are dangling references leading nowhere. > where we retry every failed plugin. > > [1] https://buildbot.mariadb.org/#/builders/369/builds/10107/steps/7/logs/stdio nor references to buildbot logs, they're very short lived, while I can see commits in the server git repo going to 2000. > > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index 5a077a934ac..92d74aa51e8 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -1435,6 +1435,49 @@ void plugin_unlock_list(THD *thd, plugin_ref *list, size_t count) > DBUG_VOID_RETURN; > } > > +static int plugin_do_initialize(struct st_plugin_int *plugin, uint &state) > +{ > + DBUG_ENTER("plugin_do_initialize"); > + mysql_mutex_assert_not_owner(&LOCK_plugin); > + plugin_type_init init= plugin_type_initialize[plugin->plugin->type]; > + if (!init) > + init= (plugin_type_init) plugin->plugin->init; > + if (init) > + if (int ret= init(plugin)) > + { > + /* Plugin init failed but requested a retry if possible */ > + if (unlikely(ret== HA_ERR_RETRY_INIT)) > + sql_print_warning("Plugin '%s' registration as a %s failed and may " please, don't write "may". It's saying "eh, I don't know, you, user, figure it out". But you should know better than the user. So either say "will be retried" or "won't be retried" (in which case, you can just use the error message below and not mention retry at all). > + "be retried provided it is being loaded with " > + "plugin-load[-add]", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + else > + sql_print_error("Plugin '%s' registration as a %s failed.", > + plugin->name.str, plugin_type_names[plugin->plugin->type].str); > + DBUG_RETURN(ret); > + } > + state= PLUGIN_IS_READY; // plugin->init() succeeded > + > + if (plugin->plugin->status_vars) > + { > + /* > + historical ndb behavior caused MySQL plugins to specify > + status var names in full, with the plugin name prefix. > + this was never fixed in MySQL. > + MariaDB fixes that but supports MySQL style too. > + */ > + SHOW_VAR *show_vars= plugin->plugin->status_vars; > + SHOW_VAR tmp_array[2]= {{plugin->plugin->name, > + (char *) plugin->plugin->status_vars, SHOW_ARRAY}, > + {0, 0, SHOW_UNDEF}}; > + if (strncasecmp(show_vars->name, plugin->name.str, plugin->name.length)) > + show_vars= tmp_array; > + > + if (add_status_vars(show_vars)) > + DBUG_RETURN(1); > + } > + DBUG_RETURN(0); > +} > > static int plugin_initialize(MEM_ROOT *tmp_root, struct st_plugin_int *plugin, > int *argc, char **argv, bool options_only) > @@ -1737,32 +1738,71 @@ int plugin_init(int *argc, char **argv, int flags) > */ > > mysql_mutex_lock(&LOCK_plugin); > + /* List of plugins to reap */ > reap= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > *(reap++)= NULL; > + /* List of plugins to retry */ > + retry= (st_plugin_int **) my_alloca((plugin_array.elements+1) * sizeof(void*)); > + *(retry++)= NULL; > > for(;;) > { > + /* Number of plugins that is successfully initialised in a round */ > + int num_initialized; > + do > + { > + num_initialized= 0; > + /* If any plugins failed and requested a retry, clean up before > + retry */ > + while ((plugin_ptr= *(--retry))) > + plugin_ptr->state= PLUGIN_IS_TO_BE_RETRIED; Why do you need a special state for that? You haven't fixed the SHOW PLUGINS command to show it correctly. And I didn't check other conditions, may be some of them need to take the new state into account too, I don't know. Wouldn't it be easier to have a special retry look after the main for() without all its complexity and without a new state. Like (pseudocode, and note that here I've changed the creative trick of walking retry[] backwards until NULL, so now it iterated forward until end): while (retry_start < retry_end) { retry_ptr= retry= retry_start; while ((plugin_ptr= *(retry++))) { mysql_mutex_unlock(&LOCK_plugin); error= plugin_do_initialize(plugin_ptr, state); mysql_mutex_lock(&LOCK_plugin); if (error == HA_ERR_RETRY_INIT) *(retry_ptr++)= plugin_ptr; else if (error) *(reap++)= plugin_ptr; } if (retry == retry_ptr) while (retry_ptr > retry_start) *(reap++)= *(--retry_ptr); retry_end= retry_ptr; } > + retry++; > for (i=0; i < MYSQL_MAX_PLUGIN_TYPE_NUM; i++) > { > HASH *hash= plugin_hash + plugin_type_initialization_order[i]; > for (uint idx= 0; idx < hash->records; idx++) > { > plugin_ptr= (struct st_plugin_int *) my_hash_element(hash, idx); > + if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED || > + plugin_ptr->state == PLUGIN_IS_TO_BE_RETRIED) > + { > + int error; > if (plugin_ptr->state == PLUGIN_IS_UNINITIALIZED) > { > bool plugin_table_engine= > lex_string_eq(&plugin_table_engine_name, &plugin_ptr->name); > bool opts_only= flags & PLUGIN_INIT_SKIP_INITIALIZATION && > (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE || > !plugin_table_engine); > - if (plugin_initialize(&tmp_root, plugin_ptr, argc, argv, opts_only)) > + error= plugin_initialize(&tmp_root, plugin_ptr, argc, argv, > + opts_only); > + } else > { > + uint state= plugin_ptr->state; > + mysql_mutex_unlock(&LOCK_plugin); > + error= plugin_do_initialize(plugin_ptr, state); > + mysql_mutex_lock(&LOCK_plugin); > + plugin_ptr->state= state; > + } > + if (error) > + { > plugin_ptr->state= PLUGIN_IS_DYING; > + /* The plugin wants a retry of the initialisation, > + possibly due to dependency on other plugins */ > + if (unlikely(error == HA_ERR_RETRY_INIT)) > + *(retry++)= plugin_ptr; > + else > *(reap++)= plugin_ptr; > } > + else > + num_initialized++; > } > } > } > + /* Only retry if at least one plugin has been initialised > + successfully and at least one has requested a retry during this > + round */ > + } while (num_initialized > 0 && *(retry - 1)); > > /* load and init plugins from the plugin table (unless done already) */ > if (flags & PLUGIN_INIT_SKIP_PLUGIN_TABLE) > @@ -1775,8 +1815,11 @@ int plugin_init(int *argc, char **argv, int flags) > } > > /* > - Check if any plugins have to be reaped > + Merge the retry list to the reap list, then reap the failed > + plugins. Note that during the merge we reverse the order in retry > */ > + while ((plugin_ptr= *(--retry))) > + *(reap++) = plugin_ptr; I already have that in my pseudocode above > while ((plugin_ptr= *(--reap))) > { > mysql_mutex_unlock(&LOCK_plugin); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Numeric datatype with large decimal values
by brajmohan saxena 23 Jun '23

23 Jun '23

Hi Folks, Do we have Or someone already working on creating a Numeric (decimal) data type plugin in MariaDB, Which supports large decimal digits and may be upto infinite. It would be great if someone already has some pointers on it. And let me know the best approach to achieve this type of data type implementation in MariaDB. Thanks in advance. Thanks Braj mohan saxena

1 0

Re: f3aa4d0a215: MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func
by Sergei Golubchik 22 Jun '23

22 Jun '23

Hi, Yuchen, On Jun 20, Yuchen Pei wrote: > revision-id: f3aa4d0a215 (mariadb-10.4.30-4-gf3aa4d0a215) > parent(s): f5dceafd0ba > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-06-14 15:02:38 +1000 > message: > > MDEV-29447 MDEV-26285 MDEV-31338 Refactor spider_db_mbase_util::open_item_func > > spider_db_mbase_util::open_item_func() is a monster function. > It is difficult to maintain while it is expected that we need to > modify it when a new SQL function or a new func_type is added. > > We split the function into two distinct functions: one handles the > case of str != NULL and the other handles the case of str == NULL. > > This refactoring was done in a conservative way because we do not > have comprehensive tests on the function. > > It also fixes MDEV-29447 and MDEV-31338 where field items that are > arguments of a func item may be used before created / initialised. > > Note this commit is a port of > 3836098c29ef1b7ff9d5fbde99b690eab73a0df1 (MDEV-26285) to current > versions 10.3+. As I wrote in the first review, please, don't mention transient commit hashes in a permanent commit comment. This commit will, supposedly, be pushed into the main branch. While 3836098c29ef1 will, on the other hand, disappear, as it's not in any of the main branches. You'll have a dangling pointer > Signed-off-by: Yuchen Pei <yuchen.pei(a)mariadb.com> also, it looks strange to sign-off your own commits > > diff --git a/storage/spider/spd_db_mysql.cc b/storage/spider/spd_db_mysql.cc > index a51c1497b02..1ed733105c6 100644 > --- a/storage/spider/spd_db_mysql.cc > +++ b/storage/spider/spd_db_mysql.cc > @@ -5848,94 +5992,12 @@ int spider_db_mbase_util::open_item_func( > { > if (!strncasecmp("utc_timestamp", func_name, func_name_length)) > { > - 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 Why did you removed this and not the #else part? > - 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) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 3

DBA in Tokyo
by James Tobin 20 Jun '23

20 Jun '23

Hello, I'm working with an employer that is looking to hire a DBA in Tokyo. Consequently, I had hoped some members of this list may like to discuss. I can be reached using "JamesBTobin (at) Gmail (dot) Com". Kind regards, James

1 0

[Maria-developers] Review input for: MDEV-22534 Decorrelate IN subquery
by Sergey Petrunia 01 Jun '23

01 Jun '23

Hi Yuchen, Please find below review input for > commit ba9cf8efeb0b1e1c132d565adb79c98156783f15 > Author: Yuchen Pei <yuchen.pei(a)mariadb.com> > Date: Fri May 19 20:06:31 2023 +1000 > > MDEV-22534 Decorrelate IN subquery > First, general questions: Q1: It seems there is some overlap between Item_in_subselect::exists2in_processor() and Item_exists_subselect::exists2in_processor() Did you consider factoring out common code rather than copying it? (if yes, I'd like to know why it couldn't be done). Q2: Where is the coe that removes the correlated equalities from the subquery's WHERE? (I assume Item_exists_subselect code does it?) > diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc > index 9e6c205ca76..8d893794da3 100644 > --- a/sql/item_subselect.cc > +++ b/sql/item_subselect.cc > @@ -3099,6 +3099,132 @@ static bool find_inner_outer_equalities(Item **conds, > return TRUE; > } > > +/* Checks whether item tree intersects with the free list */ > +static bool intersects_free_list(Item *item, THD *thd) > +{ > + for (const Item *to_find= thd->free_list; to_find; to_find= to_find->next) > + if (item->walk(&Item::find_item_processor, 1, (void *) to_find)) > + return true; > + return false; > +} > + Please try moving this function into a separate file. First name that came to my mind: opt_subq_decorrelate.cc Feel free to suggest better names. The idea is that we should aim for better structure. (This request is only valid if the factoring out suggested above doesn't work out). > +/* De-correlate where conditions in an IN subquery */ This needs a bigger comment describing what the rewrite does. > +bool Item_in_subselect::exists2in_processor(void *opt_arg) > +{ > + THD *thd= (THD *)opt_arg; > + SELECT_LEX *first_select= unit->first_select(); > + JOIN *join= first_select->join; > + bool will_be_correlated; > + Dynamic_array<EQ_FIELD_OUTER> eqs(PSI_INSTRUMENT_MEM, 5, 5); > + List<Item> outer; > + Query_arena *arena= NULL, backup; > + int res= FALSE; > + DBUG_ENTER("Item_in_subselect::exists2in_processor"); > + Please move comments out of the if statement and number the conditions. For examples, see best_access_path(), large if-statement starting with "Don't test table scan if it can't be better." or "EXISTS-to-IN coversion and ORDER BY ... LIMIT clause" in Item_exists_subselect::exists2in_processor(). > + if (!optimizer_flag(thd, OPTIMIZER_SWITCH_EXISTS_TO_IN) || > + /* proceed only if I'm a toplevel IN or a toplevel NOT IN */ > + !(is_top_level_item() || > + (upper_not && upper_not->is_top_level_item())) || what is the reason behind this condition? If we're able to handle top-level NOT IN, why can't we handle subquery in any context? Can we really handle the NOT IN? (I suspect not, it will suffer from the semantics of IN subqueries with NULLs.. I can't find the name for this, it's described e.g. here: https://petrunia.net/2006/11/16/inany-subqueries-null-woes/ ) > + first_select->is_part_of_union() || /* skip if part of a union */ > + first_select->group_list.elements || /* skip if with group by */ > + first_select->with_sum_func || /* skip if aggregation */ > + join->having || /* skip if with having */ > + !join->conds || /* skip if no conds */ > + /* skip if left expr is a single nonscalar subselect */ > + (left_expr->type() == Item::SUBSELECT_ITEM && > + !left_expr->type_handler()->is_scalar_type())) The above seems to be some exotic SQL. Is it something like (select col1,lol2 from t1) IN (select col3,col4 from t2 where ..) and does MariaDB really support this? Please add a comment about this. > + DBUG_RETURN(FALSE); > + /* iterate over conditions, and check whether they can be moved out. */ > + if (find_inner_outer_equalities(&join->conds, eqs)) > + DBUG_RETURN(FALSE); > + /* If we are in the execution of a prepared statement, check for > + intersection with the free list to avoid segfault. Note that the > + check for prepared statement execution is necessary, otherwise it > + will likely always find intersection thus abort the > + transformation. > + > + fixme: this only works when HAVE_PSI_STATEMENT_INTERFACE is > + defined. It causes CI's like amd64-debian-10-debug-embedded to > + fail. Are there other ways to find out we are in the execution of a > + prepared statement? */ I'm looking at the code of activate_stmt_arena_if_needed(). It seems "stmt_arena->is_conventional()" is the check you're looking for. > + if (thd->m_statement_state.m_parent_prepared_stmt) > + { > + for (uint i= 0; i < (uint) eqs.elements(); i++) > + { > + if (intersects_free_list(*eqs.at(i).eq_ref, thd)) > + DBUG_RETURN(FALSE); This check doesn't look good. As far as I understand, it is a temporary measure until related re-execution bug(s) are fixed by Igor (and/or Sanja)? > + } > + } > + /* Determines whether the result will be correlated */ > + { > + List<Item> unused; > + Collect_deps_prm prm= {&unused, // parameters > + unit->first_select()->nest_level_base, // nest_level_base > + 0, // count > + unit->first_select()->nest_level, // nest_level > + FALSE // collect > + }; > + walk(&Item::collect_outer_ref_processor, TRUE, &prm); > + DBUG_ASSERT(prm.count > 0); > + DBUG_ASSERT(prm.count >= (uint)eqs.elements()); > + will_be_correlated= prm.count > (uint)eqs.elements(); See the example pasted in the MDEV: https://jira.mariadb.org/browse/MDEV-22534?focusedCommentId=259705&page=com… Somehow, I get here will_be_correlated==FALSE but Materialization is still not considered for the subquery? > + } > + /* Back up the free list */ > + arena= thd->activate_stmt_arena_if_needed(&backup); > + /* Construct the items for left_expr */ > + if (left_expr->type() == Item::ROW_ITEM) > + for (uint i= 0; i < left_expr->cols(); i++) > + outer.push_back(left_expr->element_index(i)); > + else > + outer.push_back(left_expr); > + const uint offset= first_select->item_list.elements; > + /* Move items to outer and select item list */ > + for (uint i= 0; i < (uint)eqs.elements(); i++) > + { > + Item **eq_ref= eqs.at(i).eq_ref; > + Item_ident *local_field= eqs.at(i).local_field; > + Item *outer_exp= eqs.at(i).outer_exp; > + first_select->item_list.push_back(local_field, thd->mem_root); > + first_select->ref_pointer_array[offset + i]= (Item *)local_field; How do we know that ref_pointer_array has enough room for the new elements? (We can discuss this with Sanja, he's an expert on the topic). I see this check in Item_exists_subselect::exists2in_processor if ((uint)eqs.elements() > (first_select->item_list.elements + first_select->select_n_reserved)) Is it how Item_exists_subselect handles this issue? > + outer.push_back(outer_exp); > + *eq_ref= new (thd->mem_root) Item_int(thd, 1); > + if((*eq_ref)->fix_fields(thd, (Item **)eq_ref)) > + { > + res= TRUE; > + goto out; > + } > + } > + /* Update the left_expr */ > + left_expr= new (thd->mem_root) Item_row(thd, outer); > + if (left_expr->fix_fields(thd, &left_expr)) > + { > + res= TRUE; > + goto out; > + } > + left_expr_orig= left_expr; > + is_correlated= will_be_correlated; > + /* Update any Item_in_optimizer wrapper if exists */ > + if (optimizer) > + { > + optimizer->reset_cache(); > + if (optimizer->fix_left(thd)) > + { > + res= TRUE; > + goto out; > + } > + } > + { > + OPT_TRACE_TRANSFORM(thd, trace_wrapper, trace_transform, > + get_select_lex()->select_number, "IN (SELECT)", > + "decorrelation"); > + } > +out: > + /* Restores the free list */ > + if (arena) > + thd->restore_active_arena(arena, &backup); > + DBUG_RETURN(res); > +} On Mon, Dec 20, 2021 at 09:29:47PM +0300, Sergey Petrunia wrote: > Hello Igor, > > I'm trying examples with this patch: > > create table t1 (a int not null, key(a)); > insert into t1 select seq from seq_1_to_1000; > > First, I debug this example: > > explain select * from t1 where a <10 or a>=10; > > The execution enters key_or(), passes through the new code this patch has added > and key_or() returns NULL. Good. > > But if I modify the example slightly: > > explain select * from t1 where a <20 or a>=10 ; > > Here, key_or() still returns a SEL_ARG object that represents an infinite > range: > > (gdb) fini > Run till exit from #0 key_or (...) > 0x0000555555f1fca2 in tree_or (...) > Value returned is $24 = (SEL_ARG *) 0x7fff70064d88 > (gdb) p dbug_print_sel_arg($24) > $26 = 0x7fff70024390 "SEL_ARG(-inf<=a<=+inf)" > > Should this case be fixed as well? > > On Fri, Dec 17, 2021 at 02:11:40PM -0800, IgorBabaev wrote: > > revision-id: 677643a80986107491b7886441f2828384f0494b (mariadb-10.2.31-1286-g677643a) > > parent(s): 8bb55633699612279744c055e22eeca8d4058273 > > author: Igor Babaev > > committer: Igor Babaev > > timestamp: 2021-12-17 14:11:39 -0800 > > message: > > > > MDEV-27262 Unexpected index intersection with full index scan for an index > > > > If when extracting a range condition foran index from the WHERE condition > > I think the comment wording could be improved also. > > > Range Optimizer sees that the range condition covers the whole index then > > such condition should be discarded because cannot it be used in any range > > scan. In some cases Range Optimizer really does it, but there remained some > > conditions for which it was not done. As a result the optimizer could > > produce index merge plans with the full index scan for one of the indexes > > participating in the index merge. > > This could be observed in one of the test cases from index_merge1.inc > > where a plan with index_merge_sort_union was produced and in the test case > > reported for this bug where a plan with index_merge_sort_intersect was > > produced. In both cases one of two index scans participating in index merge > > ran over the whole index. > > The patch slightly changes the original above mentioned test case from > > index_merge1.inc to be able to produce an intended plan employing > > index_merge_sort_union. The original query was left to show that index > > merge is not used for it anymore. > > It should be noted that for the plan with index_merge_sort_intersect could > > be chosen for execution only due to a defect in the InnoDB code that > > returns wrong estimates for the cardinality of big ranges. > > > > This bug led to serious problems in 10.4+ where the optimization using > > Rowid filters is employed (see mdev-26446). > > > > Approved by Oleksandr Byelkin <sanja(a)mariadb.com> > > > > --- > > mysql-test/include/index_merge1.inc | 8 +++- > > mysql-test/r/index_merge_myisam.result | 12 +++-- > > mysql-test/r/range_innodb.result | 81 ++++++++++++++++++++++++++++++++++ > > mysql-test/t/range_innodb.test | 78 ++++++++++++++++++++++++++++++++ > > sql/opt_range.cc | 7 +++ > > 5 files changed, 181 insertions(+), 5 deletions(-) > > > > diff --git a/mysql-test/include/index_merge1.inc b/mysql-test/include/index_merge1.inc > > index b168a76..440f1f7 100644 > > --- a/mysql-test/include/index_merge1.inc > > +++ b/mysql-test/include/index_merge1.inc > > @@ -150,15 +150,19 @@ explain select * from t0 where > > (((key3 <7 and key7 < 6) or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > explain select * from t0 where > > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > - ((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + ((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > > > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > + ((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > + or > > + ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > # 8. Verify that "order by" after index merge uses filesort > > select * from t0 where key1 < 5 or key8 < 4 order by key1; > > > > diff --git a/mysql-test/r/index_merge_myisam.result b/mysql-test/r/index_merge_myisam.result > > index 5a23092..a096c34 100644 > > --- a/mysql-test/r/index_merge_myisam.result > > +++ b/mysql-test/r/index_merge_myisam.result > > @@ -173,17 +173,23 @@ or > > id select_type table type possible_keys key key_len ref rows Extra > > 1 SIMPLE t0 index_merge i1,i2,i3,i5,i6,i7 i3,i5 4,4 NULL 11 Using sort_union(i3,i5); Using where > > explain select * from t0 where > > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > id select_type table type possible_keys key key_len ref rows Extra > > 1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > > explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > -((key3 <5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +((key3 < 4 or key5 < 4) and (key1 < 4 or key2 < 4)) > > or > > ((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > id select_type table type possible_keys key key_len ref rows Extra > > -1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 0,4 NULL 1024 Using sort_union(i3,i5); Using where > > +1 SIMPLE t0 index_merge i1,i2,i3,i5,i6 i3,i5 4,4 NULL 1024 Using sort_union(i3,i5); Using where > > +explain select * from t0 force index(i1, i2, i3, i4, i5, i6 ) where > > +((key3 < 5 or key5 < 4) and (key1 < 4 or key2 < 4)) > > +or > > +((key3 >=5 or key5 < 2) and (key5 < 5 or key6 < 6)); > > +id select_type table type possible_keys key key_len ref rows Extra > > +1 SIMPLE t0 ALL i1,i2,i3,i5,i6 NULL NULL NULL 1024 Using where > > select * from t0 where key1 < 5 or key8 < 4 order by key1; > > key1 key2 key3 key4 key5 key6 key7 key8 > > 1 1 1 1 1 1 1 1023 > > diff --git a/mysql-test/r/range_innodb.result b/mysql-test/r/range_innodb.result > > index f2349f2..ccb6da3 100644 > > --- a/mysql-test/r/range_innodb.result > > +++ b/mysql-test/r/range_innodb.result > > @@ -108,3 +108,84 @@ DROP TABLE t0,t1; > > SET @@GLOBAL.debug_dbug = @saved_dbug; > > set @@optimizer_switch= @optimizer_switch_save; > > # End of 10.1 tests > > +# > > +# MDEV-27262: Index intersection with full scan over an index > > +# > > +CREATE TABLE t1 ( > > +id int(10) unsigned NOT NULL AUTO_INCREMENT, > > +p char(32) DEFAULT NULL, > > +es tinyint(3) unsigned NOT NULL DEFAULT 0, > > +er tinyint(3) unsigned NOT NULL DEFAULT 0, > > +x mediumint(8) unsigned NOT NULL DEFAULT 0, > > +PRIMARY KEY (id), > > +INDEX es (es), > > +INDEX x (x), > > +INDEX er (er,x), > > +INDEX p (p) > > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > > +update t1 set p='foobar'; > > +update t1 set x=0; > > +set @save_isp=@@innodb_stats_persistent; > > +set global innodb_stats_persistent= 1; > > +analyze table t1; > > +Table Op Msg_type Msg_text > > +test.t1 analyze status OK > > +set optimizer_switch='index_merge_sort_intersection=on'; > > +SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id p es er x > > +14645 foobar 4 4 0 > > +14646 foobar 4 4 0 > > +EXPLAIN EXTENDED SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id select_type table type possible_keys key key_len ref rows filtered Extra > > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > > +Warnings: > > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > > +set optimizer_switch='index_merge_sort_intersection=off'; > > +SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id p es er x > > +14645 foobar 4 4 0 > > +14646 foobar 4 4 0 > > +EXPLAIN EXTENDED SELECT * FROM t1 > > +WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > +id select_type table type possible_keys key key_len ref rows filtered Extra > > +1 SIMPLE t1 range es,er,p es 1 NULL # 100.00 Using index condition; Using where > > +Warnings: > > +Note 1003 select `test`.`t1`.`id` AS `id`,`test`.`t1`.`p` AS `p`,`test`.`t1`.`es` AS `es`,`test`.`t1`.`er` AS `er`,`test`.`t1`.`x` AS `x` from `test`.`t1` where (`test`.`t1`.`p` = 'foo' and `test`.`t1`.`er` <> 4 or `test`.`t1`.`er` = 4) and `test`.`t1`.`es` >= 4 limit 2 > > +set optimizer_switch='index_merge_sort_intersection=default'; > > +set global innodb_stats_persistent= @save_isp; > > +DROP TABLE t1; > > +# End of 10.2 tests > > diff --git a/mysql-test/t/range_innodb.test b/mysql-test/t/range_innodb.test > > index 428e5c2..7420e72 100644 > > --- a/mysql-test/t/range_innodb.test > > +++ b/mysql-test/t/range_innodb.test > > @@ -116,3 +116,81 @@ SET @@GLOBAL.debug_dbug = @saved_dbug; > > set @@optimizer_switch= @optimizer_switch_save; > > > > --echo # End of 10.1 tests > > + > > +--echo # > > +--echo # MDEV-27262: Index intersection with full scan over an index > > +--echo # > > + > > +--source include/have_sequence.inc > > + > > +CREATE TABLE t1 ( > > + id int(10) unsigned NOT NULL AUTO_INCREMENT, > > + p char(32) DEFAULT NULL, > > + es tinyint(3) unsigned NOT NULL DEFAULT 0, > > + er tinyint(3) unsigned NOT NULL DEFAULT 0, > > + x mediumint(8) unsigned NOT NULL DEFAULT 0, > > + PRIMARY KEY (id), > > + INDEX es (es), > > + INDEX x (x), > > + INDEX er (er,x), > > + INDEX p (p) > > +) ENGINE=InnoDB DEFAULT CHARSET=latin1; > > + > > +insert into t1(es,er) select 0, 1 from seq_1_to_45; > > +insert into t1(es,er) select 0, 2 from seq_1_to_49; > > +insert into t1(es,er) select 0, 3 from seq_1_to_951; > > +insert into t1(es,er) select 0, 3 from seq_1_to_1054; > > +insert into t1(es,er) select 0, 6 from seq_1_to_25; > > +insert into t1(es,er) select 0, 11 from seq_1_to_1; > > +insert into t1(es,er) select 1, 1 from seq_1_to_45; > > +insert into t1(es,er) select 1, 2 from seq_1_to_16; > > +insert into t1(es,er) select 1, 3 from seq_1_to_511; > > +insert into t1(es,er) select 1, 4 from seq_1_to_687; > > +insert into t1(es,er) select 1, 6 from seq_1_to_50; > > +insert into t1(es,er) select 1, 7 from seq_1_to_4; > > +insert into t1(es,er) select 1, 11 from seq_1_to_1; > > +insert into t1(es,er) select 2, 1 from seq_1_to_82; > > +insert into t1(es,er) select 2, 2 from seq_1_to_82; > > +insert into t1(es,er) select 2, 3 from seq_1_to_1626; > > +insert into t1(es,er) select 2, 4 from seq_1_to_977; > > +insert into t1(es,er) select 2, 6 from seq_1_to_33; > > +insert into t1(es,er) select 2, 11 from seq_1_to_1; > > +insert into t1(es,er) select 3, 1 from seq_1_to_245; > > +insert into t1(es,er) select 3, 2 from seq_1_to_81; > > +insert into t1(es,er) select 3, 3 from seq_1_to_852; > > +insert into t1(es,er) select 3, 4 from seq_1_to_2243; > > +insert into t1(es,er) select 3, 6 from seq_1_to_44; > > +insert into t1(es,er) select 3, 11 from seq_1_to_1; > > +insert into t1(es,er) select 4, 1 from seq_1_to_91; > > +insert into t1(es,er) select 4, 2 from seq_1_to_83; > > +insert into t1(es,er) select 4, 3 from seq_1_to_297; > > +insert into t1(es,er) select 4, 4 from seq_1_to_2456; > > +insert into t1(es,er) select 4, 6 from seq_1_to_19; > > +insert into t1(es,er) select 4, 11 from seq_1_to_1; > > +update t1 set p='foobar'; > > +update t1 set x=0; > > +set @save_isp=@@innodb_stats_persistent; > > +set global innodb_stats_persistent= 1; > > +analyze table t1; > > + > > +let $q= > > +SELECT * FROM t1 > > + WHERE ((p = 'foo' AND er != 4) OR er = 4 ) AND (es >= 4) LIMIT 2; > > + > > +set optimizer_switch='index_merge_sort_intersection=on'; > > +eval $q; > > +--replace_column 9 # > > +eval EXPLAIN EXTENDED $q; > > + > > +set optimizer_switch='index_merge_sort_intersection=off'; > > +# execution of $q and explain for it led to an assertion failure in 10.4 > > +# (with the optimizer switch rowid_filter set to 'on') > > +eval $q; > > +--replace_column 9 # > > +eval EXPLAIN EXTENDED $q; > > +set optimizer_switch='index_merge_sort_intersection=default'; > > + > > +set global innodb_stats_persistent= @save_isp; > > +DROP TABLE t1; > > + > > +--echo # End of 10.2 tests > > diff --git a/sql/opt_range.cc b/sql/opt_range.cc > > index f3f1843..2e05b88 100644 > > --- a/sql/opt_range.cc > > +++ b/sql/opt_range.cc > > @@ -9413,6 +9413,13 @@ key_or(RANGE_OPT_PARAM *param, SEL_ARG *key1,SEL_ARG *key2) > > key2->copy_min(tmp); > > if (!(key1=key1->tree_delete(tmp))) > > { // Only one key in tree > > + if (key2->min_flag & NO_MIN_RANGE && > > + key2->max_flag & NO_MAX_RANGE) > > + { > > + if (key2->maybe_flag) > > + return new SEL_ARG(SEL_ARG::MAYBE_KEY); > > + return 0; // Always true OR > > + } > > key1=key2; > > key1->make_root(); > > key2=key2_next; > > BR > Sergei > -- > Sergei Petrunia, Software Developer > MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net > > -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://petrunia.net

2 3

Re: [Maria-developers] c457f237511: MDEV-30984 Online ALTER table is denied with non-informative error messages
by Sergei Golubchik 29 May '23

29 May '23

Hi, Nikita, comments below refer to a combined diff of both commits On May 23, Nikita Malyavin wrote: > revision-id: c457f237511 (mariadb-11.0.1-124-gc457f237511) > parent(s): f4b04ec534d > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-15 21:07:50 +0300 > message: > > MDEV-30984 Online ALTER table is denied with non-informative error messages Two main thoughts. First, users will generally don't do show warnings after an error - mariadb already puts some useful info into a warning after an error in some (few) cases, and it's always a surprise when I write in a bug report "use SHOW WARNINGS". It's not very intuitive. And in your case it's even unnecessary, because your error message is LOCK=NONE is not supported. Reason: %s. Try LOCK=SHARED and "Reason" part is perfectly suited for an explanation that you generate, instead of a misleading "COPY algorithm requires a lock" (because it doesn't). Second thought, your messages are hard-coded and won't be translated even if the user will configure the server to use, say, spanish or Japanese error messages. See how it was solved for "COPY algorithm requires a lock", ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY. But I think the message will still be comprehensible if you'll simply print the offending sql clause as the reason, this won't need any localization. See below where I've shown what your error messages would look like if you do that. If you think that's too cryptic, then, please, use the ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY approach. Two more comments at the very end but I'll repeat them here, because they're easy to overlook: AUTO_INCREMENT instead of AUTOINC, and a test for DROP SYSTEM VERSIONING. > diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result > index 8b726ccb591..bcf7afea9df 100644 > --- a/mysql-test/main/alter_table_online.result > +++ b/mysql-test/main/alter_table_online.result > @@ -4,6 +4,10 @@ > create table t (a int); > alter ignore table t add primary key (a), algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: ALTER IGNORE. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 ALTER IGNORE TABLE is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > drop table t; > # > # MDEV-28771 Assertion `table->in_use&&tdc->flushed' failed after ALTER > @@ -92,12 +96,20 @@ on update cascade) engine=InnoDB; > insert into t2 values (1),(2),(3); > alter table t2 add c int, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON UPDATE CASCADE. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=inplace, lock=none; > create or replace table t2 (b int, foreign key (b) > references t1 (a) > on delete set null) engine=InnoDB; > alter table t2 add c int, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=inplace, lock=none; > create or replace table t2 (b int, foreign key (b) > references t1 (a) > @@ -117,6 +129,10 @@ b int references t1 (b) on update cascade) engine=InnoDB; > insert into t2 values (1, 1),(2, 2); > alter table t2 add c int, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t2 add c int, algorithm=copy; > alter table t2 add d int, algorithm=inplace; > drop table t2, t1; > @@ -132,6 +148,10 @@ period for system_time (row_start, row_end)) > engine=innodb with system versioning; > alter table t1 add c int, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: BIGINT GENERATED ALWAYS AS ROW_START. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Transaction-versioned tables are incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t1 add c int, algorithm=inplace; > alter table t1 add d int, lock=none; > set system_versioning_alter_history= default; > @@ -143,6 +163,10 @@ drop table t1; > create table t (a serial, b int) engine=innodb; > alter table t drop a, modify b serial, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > set statement sql_mode= NO_AUTO_VALUE_ON_ZERO for > alter table t drop a, modify b serial, algorithm=copy, lock=none; > create or replace table t (a serial, b int) engine=innodb; > @@ -157,6 +181,10 @@ t CREATE TABLE `t` ( > # Only unsafe approach is fine because of possible collisions. > alter table t modify a int, modify b serial, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > # > # Check that we treat autoinc columns correctly modify old autoinc is > # fine, adding new autoinc for existed column is unsafe. > @@ -180,22 +208,38 @@ t CREATE TABLE `t` ( > ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci > alter table t drop b, change c c serial, algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > # Check existed unique keys. > create or replace table t(a int, b int not null, c int not null, d int); > # No unique in the old table; > alter table t add unique(b, c), modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t add unique(a, b); > # Unique in the old table has nulls; > alter table t modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > alter table t add unique(b, c); > # Change unique'scolumn; > alter table t change b x int, modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED > +show warnings; > +Level Code Message > +Note 1846 Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY > +Error 1846 LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED > # Finally good. > alter table t modify d int auto_increment, add key(d), > algorithm=copy, lock=none; > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 99570008ddb..22447db42af 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9916,6 +9916,67 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info, > return online; > } > > +bool online_alter_is_supported(THD *thd, const Alter_info *alter_info, > + const TABLE *table) > +{ > + const char *reason= NULL; > + List<FOREIGN_KEY_INFO> fk_list; > + > + if (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING) > + { > + reason= "DROP SYSTEM VERSIONING is"; no test for that > + goto unsupported; > + } > + > + if (thd->lex->ignore) > + { > + reason= "ALTER IGNORE TABLE is"; > + goto unsupported; > + } > + > + if (table->s->tmp_table) > + { > + reason= "Temporary tables are"; > + goto unsupported; > + } > + > + if (table->versioned(VERS_TRX_ID)) > + { > + reason= "Transaction-versioned tables are"; > + goto unsupported; > + } > + > + table->file->get_foreign_key_list(thd, &fk_list); > + for (auto &fk: fk_list) > + { > + if (fk_modifies_child(fk.delete_method) > + || fk_modifies_child(fk.update_method)) > + { > + reason= "Tables with CASCADE/SET NULL foreign keys are"; > + goto unsupported; > + } > + } > + > + if (!online_alter_check_autoinc(thd, alter_info, table)) > + { > + reason= "Adding AUTOINC to an existing column for a table without a " AUTO_INCREMENT, please, don't abbrev user visible messages > + "primary key is"; > + goto unsupported; > + } > + > + return true; > + > +unsupported: > + if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE > + && alter_info->algorithm(thd) == Alter_info::ALTER_TABLE_ALGORITHM_COPY) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, > + ER_ALTER_OPERATION_NOT_SUPPORTED_REASON, > + "%s incompatible with " > + "LOCK=NONE, ALGORITHM=COPY", > + reason); > + return false; > +} > + > > /** > Alter table Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 4

Re: [Maria-developers] 56e2cd20ed3: MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
by Sergei Golubchik 29 May '23

29 May '23

Hi, Nikita, On May 24, Nikita Malyavin wrote: > revision-id: 56e2cd20ed3 (mariadb-11.0.1-114-g56e2cd20ed3) > parent(s): 47f5c7ae7e4 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-05-10 01:49:42 +0300 > message: > > MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table > > The row events were applied "twice": once for the ha_partition, and one > more time for the underlying storage engine. > > There's no such problem in binlog/rpl, because ha_partiton::row_logging > is normally set to false. > > The fix makes the events replicate only when the handler is a root handler. > We will try to *guess* this by comparing it to table->file. The same > approach is used in the MDEV-21540 fix, 231feabd. The assumption is made, > that the row methods are only called for table->file (and never for a > cloned handler), hence the assertions are added in ha_innobase and > ha_myisam to make sure that this is true at least for those engines > > Also closes MDEV-31040, however the test is not included, since we have no > convenient way to construct a deterministic version. > > diff --git a/sql/handler.cc b/sql/handler.cc > index 3fcbb5171d6..d8aca5bc80b 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7280,7 +7280,7 @@ int handler::binlog_log_row(const uchar *before_record, > log_func, row_logging_has_trans); > > #ifdef HAVE_REPLICATION > - if (unlikely(!error && table->s->online_alter_binlog)) > + if (unlikely(!error && table->s->online_alter_binlog && is_root_handler())) > error= binlog_log_row_online_alter(table, before_record, after_record, > log_func); > #endif // HAVE_REPLICATION > @@ -7800,7 +7800,7 @@ int handler::ha_write_row(const uchar *buf) > DBUG_RETURN(error); > > /* > - NOTE: this != table->file is true in 3 cases: > + NOTE: is_root_handler() is false in 3 cases: may be move this comment closer to is_root_handler() ? > > 1. under copy_partitions() (REORGANIZE PARTITION): that does not > require long unique check as it does not introduce new rows or new index. > @@ -7810,7 +7810,7 @@ int handler::ha_write_row(const uchar *buf) > 3. under ha_mroonga::wrapper_write_row() > */ > > - if (table->s->long_unique_table && this == table->file) > + if (table->s->long_unique_table && is_root_handler()) > { > DBUG_ASSERT(inited == NONE || lookup_handler != this); > if ((error= check_duplicate_long_entries(buf))) > @@ -7862,13 +7862,13 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) > error= ha_check_overlaps(old_data, new_data); > > /* > - NOTE: this != table->file is true under partition's ha_update_row(): > + NOTE: is_root_handler() is false under partition's ha_update_row(): > check_duplicate_long_entries_update() was already done by > ha_partition::ha_update_row(), no need to check it again for each single > partition. Same applies to ha_mroonga wrapper. > */ > > - if (!error && table->s->long_unique_table && this == table->file) > + if (!error && table->s->long_unique_table && is_root_handler()) > error= check_duplicate_long_entries_update(new_data); > table->status= saved_status; > > @@ -7913,6 +7913,12 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) > return error; > } > > + > +bool handler::is_root_handler() const > +{ > + return this == table->file; > +} better make it inline in handler.h > + > /* > Update first row. Only used by sequence tables > */ > diff --git a/sql/handler.h b/sql/handler.h > index fb0eba83c38..9b903e7ebce 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -3502,6 +3502,10 @@ class handler :public Sql_alloc > return extra(HA_EXTRA_NO_KEYREAD); > } > > +protected: > + bool is_root_handler() const; > + > +public: > int check_collation_compatibility(); > int check_long_hash_compatibility() const; > int ha_check_for_upgrade(HA_CHECK_OPT *check_opt); > diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc > index f035437b1d7..94d9da3486b 100644 > --- a/storage/innobase/handler/ha_innodb.cc > +++ b/storage/innobase/handler/ha_innodb.cc > @@ -7750,6 +7750,8 @@ ha_innobase::write_row( > > DBUG_ENTER("ha_innobase::write_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); instead of putting this assert into every handler's implementation and in many methods, why not to have it in handler ha_ methods or may be even in the is_root_handler itself? You can compare this->ht with table->file->ht. For example, like DBUG_ASSERT(this == table->file || this->ht != table->file->ht); > + > trx_t* trx = thd_to_trx(m_user_thd); > > /* Validation checks before we commence write_row operation. */ > @@ -8479,6 +8481,8 @@ ha_innobase::update_row( > > DBUG_ENTER("ha_innobase::update_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); > + > if (is_read_only()) { > DBUG_RETURN(HA_ERR_TABLE_READONLY); > } else if (!trx_is_started(trx)) { > @@ -8653,6 +8657,8 @@ ha_innobase::delete_row( > > DBUG_ENTER("ha_innobase::delete_row"); > > + DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); > + > if (is_read_only()) { > DBUG_RETURN(HA_ERR_TABLE_READONLY); > } else if (!trx_is_started(trx)) { > diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc > index 4057c3ea859..b7fadd86a23 100644 > --- a/storage/myisam/ha_myisam.cc > +++ b/storage/myisam/ha_myisam.cc > @@ -983,6 +983,7 @@ int ha_myisam::close(void) > > int ha_myisam::write_row(const uchar *buf) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > /* > If we have an auto_increment column and we are writing a changed row > or a new row, then update the auto_increment value in the record. > @@ -1957,11 +1958,13 @@ bool ha_myisam::is_crashed() const > > int ha_myisam::update_row(const uchar *old_data, const uchar *new_data) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > return mi_update(file,old_data,new_data); > } > > int ha_myisam::delete_row(const uchar *buf) > { > + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); > return mi_delete(file,buf); > } > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5