- developers - lists.mariadb.org

Re: [MariaDB commits] [PATCH] MDEV-31872: Add --old_mode=COMPAT_DISCOURAGED
by Sergei Golubchik 12 Sep '23

12 Sep '23

Hi, Kristian, On Sep 12, Kristian Nielsen wrote: > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index e25d293bbb7..18d08996aa8 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -3863,12 +3863,13 @@ static const char *old_mode_names[]= > "IGNORE_INDEX_ONLY_FOR_JOIN", // deprecated since 11.3 > "COMPAT_5_1_CHECKSUM", // deprecated since 11.3 > "LOCK_ALTER_TABLE_COPY", // deprecated since 11.3 > + "COMPAT_DISCOURAGED", > 0 > }; > > void old_mode_deprecated_warnings(THD *thd, ulonglong v) > { > - v &= ~OLD_MODE_DEFAULT_VALUE; > + v &= ~(OLD_MODE_DEFAULT_VALUE | OLD_MODE_COMPAT_DISCOURAGED); No, this is wrong. old-mode *by definition* is something that is a temporary workaround for something new, old-mode values are by design deprecated from the day one. So, OLD_MODE_COMPAT_DISCOURAGED shouldn't be on the line above, and thus it'll itself be deprecated and will cause a deprecation warning. Which is rather silly, as I wrote in a previous email, and that's why an old-mode is not a good solution for turning deprecated warnings away. > for (uint i=0; old_mode_names[i]; i++) > if ((1ULL<<i) & v) > { > diff --git a/sql/item_strfunc.cc b/sql/item_strfunc.cc > index 4b96271c331..339430a2794 100644 > --- a/sql/item_strfunc.cc > +++ b/sql/item_strfunc.cc > @@ -2720,7 +2720,16 @@ bool Item_func_encode::seed() > hash_password(rand_nr, key->ptr(), key->length()); > sql_crypt.init(rand_nr); > > - warn_deprecated<1103>(current_thd, func_name_cstring().str); > + if (!(current_thd->variables.old_behavior & OLD_MODE_COMPAT_DISCOURAGED)) > + { > + /* > + This function should not be removed despite the deprecation warning. > + That would cause problems for users who can then no longer access > + columns in their database that might have been inserted as > + ENCODE(str,pass_str). > + */ > + warn_deprecated<1103>(current_thd, func_name_cstring().str); > + } No, this is exactly what I wanted to prevent by adding a warn_deprecated<>() wrapper. If something is giving a warning "... is deprecated and will be removed" - it *has to be removed*. We don't want to lie to users to simply scare them. If we promise to do something, we should do it. If we aren't going to remove ENCODE/DECODE functions, we shouldn't deprecate them at all. Which is the result I'm leaning to right now. > > return FALSE; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 75ea482c4fc: MDEV-30973 GTID in deadlock output
by Sergei Golubchik 11 Sep '23

11 Sep '23

Hi, Aleksey, On Sep 11, Aleksey Midenkov wrote: > revision-id: 75ea482c4fc (mariadb-11.0.1-230-g75ea482c4fc) > parent(s): fe62f70fb4b > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2023-09-01 20:30:24 +0300 > message: > > MDEV-30973 GTID in deadlock output > > Example: > > TRANSACTION 40, ACTIVE 0 sec starting index read > mysql tables in use 1, locked 1 > LOCK WAIT 2 lock struct(s), heap size 1152, 1 row lock(s) > MySQL thread id 23, OS thread handle 140201491547840, query id 310, GTID 3-1-1 Updating > delete from tu where a = 9999 and b = 'xxxx' > 2023-04-12 19:53:58 22 [Note] InnoDB: *** (1) WAITING FOR THIS LOCK TO BE GRANTED: > > Deterministic slave_deadlock.test > > Instead of "MDEV-10962 Deadlock with 3 concurrent DELETEs by unique key" > this one uses "MDEV-22698 Deadlock on concurrent acquisition from multiple indexes" > > --- > mysql-test/suite/rpl/t/slave_deadlock.test | 93 ++++++++++++++++++++++++++++++ > sql/sql_show.cc | 10 ++++ please move slave_deadlock.result from the following commit to this one. otherwise ok Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Implementing Binlog GTID Indexes (MDEV-4991)
by Kristian Nielsen 10 Sep '23

10 Sep '23

I am implementing Binlog GTID Indexes, to fix an old performance regression for GTID when slaves connect to the master. I now have a solid design and a working prototype, so I wanted to describe the work to encourage early comments and suggestions. I'm hoping this will make it to the next release (where is the place to actively follow/participate in the release planning?) Ever since I implemented GTID, connecting slaves needed to sequentially scan the leading portion of one binlog file to locate the GTID position to start at. This can be slow as binlog files default to 1GB in size, even worse so when binlog files can now be encrypted. I files MDEV-4991 for this 10 years ago, and it is embarrassing that this has somehow remained unfixed for so long :-/ But now I'm fixing it. The current implementation is in the work-in-progress branch knielsen_mdev4991: https://github.com/MariaDB/server/tree/knielsen_mdev4991 The code is already functional, with the testsuite passing. Still missing is binlog purge, index crash recovery, async path, and general testing and cleanup. The basic idea is for each binlog file master-bin.000001 to write an index file master-bin.000001.idx. The index contains a B+-Tree in which the keys are pairs of (GTID state, binlog offset). A connecting slave's GTID position can be looked up quickly in the tree to find the corresponding binlog offset (and gtid_binlog_state) to start replicating at. Similarly, a non-GTID connecting slave can quickly look up its starting offset (BINLOG_GTID_POS()) to obtain the GTID position corresponding to its starting offset. The index is written out to disk concurrently with the writing of the binlog file. Since the tree is written in sorted order and append-only, the implementation is significantly simplified over a general B+-Tree, and can also be described as a Log-structed Merge Tree with a Btree search structure on top. All GTID state keys (except the first) in the index are delta-compressed, storing only the GTIDs that changed since the last record (which typically will be only one). Additionally the index is sparse, storing only say every 1-in-10 GTIDs in the index (this will be configurable of course). Using a sparse index, the disk space is reduced at the cost only of scanning a few extra events of the binlog file to find the exact position requested. Actual benchmarking of space usage is TBO, but we can expect a typical binlog file of 1 GB containing say 3 million GTIDs to require something like 10 MB of index disk space (1% increase). The file format is page-based (unlike the binlog file), so that it can be written efficiently and easily read using random-access. The index is written in parallel with the binlog file, but no extra fsync()s are needed (except one at the end of the file when the index is closed and synced to disk). The writing of the index will happen asynchroneously from the binlog background thread; this way it will have minimal impact on the performance and scalability of the binlog and the contested LOCK_log. Connecting slaves can read from the currently-being-written ("hot") GTID index by accessing internal memory buffers of pages not yet written to disk. If the server crashes, the existing binlog recovery and binlog checkpoint mechanism will be used to re-create any incomplete indexes as part of the normal recovery scan of binlog files. If somehow a binlog index should be found corrupt or missing (could be eg. old server upgrade or backup script that omits the new .idx files), the code will gracefully fall back to the old (slower) way of sequentially scanning the binlog file to locate the slave starting position. I think this will be a great (if rather late) improvement to one remaining corner of the GTID implementation that currently has sub-optimal performance, and I'm exited to get this completed and reviewed/tested and added to hopefully the next MariaDB release. - Kristian.

1 0

Re: 365faf7229f: MDEV-15935 Clean up string sys_var types.
by Sergei Golubchik 08 Sep '23

08 Sep '23

Hi, Yuchen, On Sep 08, Yuchen Pei wrote: > revision-id: 365faf7229f (mariadb-11.0.1-103-g365faf7229f) > parent(s): 4f39ae75606 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-09-08 13:12:22 +1000 > message: > > MDEV-15935 Clean up string sys_var types. > > Merge sys_var_charptr with sys_var_charptr_base, as well as merge > Sys_var_session_lexstring into Sys_var_lexstring. Also refactored > update methods of sys_var_charptr accordingly. > > Because the class is more generic, session_update() calls > sys_var_charptr::session_update() which does not assume a buffer field > associated with THD, but instead call strdup/free, we get rid of > THD::default_master_connection_buff accordingly > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 18e796655bd..52fa6c27112 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -1233,10 +1233,12 @@ void THD::init() > avoid temporary tables replication failure. > */ > variables.pseudo_thread_id= thread_id; > - variables.default_master_connection.str= default_master_connection_buff; > - ::strmake(default_master_connection_buff, > - global_system_variables.default_master_connection.str, > - variables.default_master_connection.length); > + variables.default_master_connection.str= > + my_strndup(PSI_INSTRUMENT_ME, may be key_memory_Sys_var_charptr_value instead of PSI_INSTRUMENT_ME? > + global_system_variables.default_master_connection.str, > + global_system_variables.default_master_connection.length, > + MYF(MY_WME | MY_THREAD_SPECIFIC)); > + > mysql_mutex_unlock(&LOCK_global_system_variables); > > user_time.val= start_time= start_time_sec_part= 0; > diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc > index d39589ed1ab..4724e2200e6 100644 > --- a/sql/sql_plugin.cc > +++ b/sql/sql_plugin.cc > @@ -3279,6 +3279,9 @@ void plugin_thdvar_init(THD *thd) > #ifndef EMBEDDED_LIBRARY > thd->session_tracker.sysvars.deinit(thd); > #endif > + my_free((char*) thd->variables.default_master_connection.str); > + thd->variables.default_master_connection.str= 0; > + thd->variables.default_master_connection.length= 0; shouldn't this go inside cleanup_variables() ? > > thd->variables= global_system_variables; > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index 4843450352e..064c60ab2b0 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -1355,12 +1355,11 @@ static bool check_master_connection(sys_var *self, THD *thd, set_var *var) > return false; > } > > -static Sys_var_session_lexstring Sys_default_master_connection( > +static Sys_var_lexstring Sys_default_master_connection( > "default_master_connection", > "Master connection to use for all slave variables and slave commands", > - SESSION_ONLY(default_master_connection), > - NO_CMD_LINE, > - DEFAULT(""), MAX_CONNECTION_NAME, ON_CHECK(check_master_connection)); > + SESSION_ONLY(default_master_connection), NO_CMD_LINE, DEFAULT(""), > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_master_connection)); some length limit would still be nice, I'd say. Doesn't have to be per variable, may be all Sys_var_lexstring could have some common limit, like, arbitrarily, 1K or 10K. > #endif > > static Sys_var_charptr_fscs Sys_init_file( > diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl > index 31ebbae01ca..0f2a07d92fc 100644 > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -576,6 +571,21 @@ class Sys_var_charptr_base: public sys_var > new_val= 0; > return new_val; > } > + char *session_update_prepare(set_var *var) do you need this session_update_prepare method? global_update_prepare() and global_update_finish() are used by Sys_var_sesvartrack. Getting rid of them is a task for another day :) > + { > + return update_prepare(var, MYF(MY_WME | MY_THREAD_SPECIFIC)); > + } > + bool session_update(THD *thd, set_var *var) override > + { > + char *new_val= session_update_prepare(var); > + my_free(session_var(thd, char*)); > + session_var(thd, char*)= new_val; > + return (new_val == 0 && var->save_result.string_value.str != 0); > + } > + char *global_update_prepare(set_var *var) > + { > + return update_prepare(var, MYF(MY_WME)); > + } > void global_update_finish(char *new_val) > { > if (flags & ALLOCATED) > @@ -583,50 +593,28 @@ class Sys_var_charptr_base: public sys_var > flags|= ALLOCATED; > global_var(char*)= new_val; > } > - bool global_update(THD *thd, set_var *var) > + bool global_update(THD *thd, set_var *var) override > { > - char *new_val= global_update_prepare(thd, var); > + char *new_val= global_update_prepare(var); > global_update_finish(new_val); > return (new_val == 0 && var->save_result.string_value.str != 0); > } > - void session_save_default(THD *thd, set_var *var)= 0; > - void global_save_default(THD *thd, set_var *var) > + void save_default(set_var *var) > { > char *ptr= (char*)(intptr)option.def_value; > var->save_result.string_value.str= ptr; > var->save_result.string_value.length= ptr ? strlen(ptr) : 0; > } > -}; > - > -class Sys_var_charptr: public Sys_var_charptr_base > -{ > -public: > - Sys_var_charptr(const char *name_arg, > - const char *comment, int flag_args, ptrdiff_t off, size_t size, > - CMD_LINE getopt, > - const char *def_val, PolyLock *lock=0, > - enum binlog_status_enum binlog_status_arg=VARIABLE_NOT_IN_BINLOG, > - on_check_function on_check_func=0, > - on_update_function on_update_func=0, > - const char *substitute=0) : > - Sys_var_charptr_base(name_arg, comment, flag_args, off, size, getopt, > - def_val, lock, binlog_status_arg, > - on_check_func, on_update_func, substitute) > + void session_save_default(THD *, set_var *var) override > { > - SYSVAR_ASSERT(scope() == GLOBAL); > - SYSVAR_ASSERT(size == sizeof(char *)); > + return save_default(var); no, session's default value is global's value. See any other Sys_var_xxx class, e.g. for Sys_var_mybool: void session_save_default(THD *thd, set_var *var) { var->save_result.ulonglong_value= (ulonglong)*(my_bool *)global_value_ptr(thd, 0); } You cannot have a test for that now, but let's have one in the redirect_url commit. > } > - > - bool session_update(THD *thd, set_var *var) > + void global_save_default(THD *, set_var *var) override > { > - DBUG_ASSERT(FALSE); > - return true; > + return save_default(var); > } > - void session_save_default(THD *thd, set_var *var) > - { DBUG_ASSERT(FALSE); } > }; > > - > class Sys_var_charptr_fscs: public Sys_var_charptr > { > using Sys_var_charptr::Sys_var_charptr; > diff --git a/mysql-test/suite/sys_vars/r/mdev_15935.result b/mysql-test/suite/sys_vars/r/mdev_15935.result > new file mode 100644 > index 00000000000..d20c2ad2f19 > --- /dev/null > +++ b/mysql-test/suite/sys_vars/r/mdev_15935.result > @@ -0,0 +1,12 @@ > +# > +# test that the global/session-only enforcement is still in place despite the merge of string sys_var classes > +# > +set session init_connect="whatever"; > +ERROR HY000: Variable 'init_connect' is a GLOBAL variable and should be set with SET GLOBAL > +set session wsrep_auto_increment_control="whatever"; > +ERROR HY000: Variable 'wsrep_auto_increment_control' is a GLOBAL variable and should be set with SET GLOBAL > +set global default_master_connection="whatever"; > +ERROR HY000: Variable 'default_master_connection' is a SESSION variable and can't be used with SET GLOBAL not sure you need that, it's checked in the very base class, you could've have affected that I'd say it's enough if existing tests for Sys_var_session_lexstring and Sys_var_sesvartrack variables succeed and ASAN/MSAN are happy > +# > +# end of test mdev_15935 > +# Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 04b99a30a3f: MDEV-15935 Adding global/session var redirect_url
by Sergei Golubchik 08 Sep '23

08 Sep '23

Hi, Yuchen, On Sep 01, Yuchen Pei wrote: > revision-id: 04b99a30a3f (mariadb-11.0.1-103-g04b99a30a3f) > parent(s): 4f39ae75606 > author: Yuchen Pei > committer: Yuchen Pei > timestamp: 2023-08-16 11:31:16 +1000 > message: > > MDEV-15935 Adding global/session var redirect_url > > Adding a global/session var `redirect_url' of string type. The initial > value is empty. Can be supplied in mysqld with --redirect-url. A valid > redirect_url should be of the format > > {mysql,mariadb}://host[:port] > > where <host> is an arbitrary string not containing colons, and <port> > is a number between 0 and 65535 inclusive. > > diff --git a/mysql-test/main/redirect.opt b/mysql-test/main/redirect.opt move this to the sys_vars suite, please > --- /dev/null > +++ b/mysql-test/main/redirect.opt > @@ -0,0 +1 @@ > +--redirect_url=mysql://foobar ok, as long as this isn't a complete feature, but just one piece of it. but when client support will be implemented (CONC-599) and the server will actually send this to the connecting clients (MDEV-31609), this test will need to be fixed, as foobar is not something a client can be redirected to. Also, it'd be more interesting to try something like --init-connect='set @@redirect_url="mysql://foobar"' instead of the simple --redirect_url=mysql://foobar because init-connect is much more flexible, it can generate the url on the fly, it can send different users to different locations. So it'd make sense to test init-connect approach. > diff --git a/mysql-test/main/redirect.test b/mysql-test/main/redirect.test > new file mode 100644 > --- /dev/null > +++ b/mysql-test/main/redirect.test > @@ -0,0 +1,56 @@ > +--echo # > +--echo # MDEV-15935 Connection Redirection Mechanism in MariaDB Client/Server Protocol > +--echo # > + > +set @old_redirect_url=@@redirect_url; > +select @@global.redirect_url; > +set global redirect_url=default; > +select @@global.redirect_url; > +--error ER_WRONG_VALUE_FOR_VAR > +set global redirect_url="mariadb.org"; > +--error ER_WRONG_VALUE_FOR_VAR > +set global redirect_url="https://mariadb.org"; before doing all this fun, please remove redirect_url from the session_track_system_variables, or just set it to the empty string. > diff --git a/sql/sql_class.h b/sql/sql_class.h > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3585,6 +3587,7 @@ class THD: public THD_count, /* this must be first */ > */ > LEX_CSTRING connection_name; > char default_master_connection_buff[MAX_CONNECTION_NAME+1]; > + char redirect_url_buff[MAX_CONNECTION_NAME+1]; This isn't such a good idea (I'm afraid, I missed it when default_master_connection_buff was added). A couple of years ago Monty have spent significant efforts trying to make THD smaller as it has grown to be too large. Now THD keeps growing again. Let's try not to encourage it, if we can help it. > uint8 password; /* 0, 1 or 2 */ > uint8 failed_com_change_user; > bool slave_thread; > diff --git a/sql/sys_vars.inl b/sql/sys_vars.inl > --- a/sql/sys_vars.inl > +++ b/sql/sys_vars.inl > @@ -637,6 +637,129 @@ class Sys_var_charptr_fscs: public Sys_var_charptr ... > +class Sys_var_redirect_url: public Sys_var_charptr_base please, no. new classes are needed only for variables with very special, unlike anything else, behavior. redirect_url is just a session string variable. See the comment at the top of sys_vars.cc But (!) there are no other session string variables yet. So I'd recomment to fix Sys_var_charptr to support session variables. And then create redirect_url out of it. Practically it might need no changes at all, if you put explicit strmake (or strdup/free) into THD::init like you did anyway. After that, I suspect, you might be able to remove Sys_var_charptr_base, Sys_var_sesvartrack, and Sys_var_session_lexstring, because Sys_var_charptr/Sys_var_lexstring will be able to serve all those use cases. > +{ > + static const size_t max_length = MAX_CONNECTION_NAME; > +public: > + Sys_var_redirect_url(const char *name_arg, > + const char *comment, > + CMD_LINE getopt, > + const char *def_val, PolyLock *lock= 0) : > + Sys_var_charptr_base(name_arg, comment, > + SESSION_VAR(redirect_url), > + getopt, def_val, lock, > + VARIABLE_NOT_IN_BINLOG, 0, 0, 0) > + { > + global_var(LEX_CSTRING).length= strlen(def_val); > + option.var_type|= GET_STR; > + *const_cast<SHOW_TYPE*>(&show_val_type)= SHOW_LEX_STRING; > + } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: 44e692d0dbc: MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
by Sergei Golubchik 06 Sep '23

06 Sep '23

Hi, Oleksandr, On Sep 04, Oleksandr Byelkin wrote: > revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) > parent(s): 9cb75f333fa > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-09-04 14:12:29 +0200 > message: > > MDEV-29167 new db-level SHOW CREATE ROUTINE privilege > > -- Create general_log if CSV is enabled. > diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql > --- a/scripts/mariadb_system_tables_fix.sql > +++ b/scripts/mariadb_system_tables_fix.sql > @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_ > > UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0; > > +ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; > +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N'; Why two statements? > + > ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, > ADD authentication_string TEXT NOT NULL AFTER plugin; > ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, > NULL, db, > sctx->priv_role, > name, sph, 0))) > - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); > + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS); Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have" And now I see that it means to "show procedure without definition". Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But SHOW_PROC_NO_DEF is rather misleading. > } > mysql_rwlock_unlock(&LOCK_grant); > return no_routine_acl; > @@ -9237,7 +9242,7 @@ static const char *command_array[]= > "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", > "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", > "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", > - "BINLOG REPLAY", "SLAVE MONITOR" > + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" > }; > > static uint command_lengths[]= > @@ -9250,7 +9255,7 @@ static uint command_lengths[]= > 11, 5, 7, 17, 14, > 8, 15, 16, 15, > 23, 24, 12, > - 13, 13 > + 13, 13, 19 May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course. > }; > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, > DBUG_RETURN(0); > > if (!full_access) > - full_access= !strcmp(sp_user, definer.str); > + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, > + db.str, name.str, sph, TRUE); Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE. As far as I can see check_db_routine_access doesn't check for the user being the definer. > if (!full_access && > check_some_routine_access(thd, db.str, name.str, sph)) > DBUG_RETURN(0); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

3 3

Re: 82a49812507: MDEV-22597 Add views for periods in information_schema
by Sergei Golubchik 05 Sep '23

05 Sep '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 82a49812507 (mariadb-11.0.1-228-g82a49812507) > parent(s): 23b15d24eb3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 22:49:36 +0400 > message: > > MDEV-22597 Add views for periods in information_schema > > Two new information_schema views are added: > * PERIOD table -- columns TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, > PERIOD_NAME, START_COLUMN_NAME, END_COLUMN_NAME. > * KEY_PERIOD_USAGE -- works similar to KEY_COLUMN_USAGE, but for periods. > Columns CONSTRAINT_CATALOG, CONSTRAINT_SCHEMA, CONSTRAINT_NAME, > TABLE_CATALOG, TABLE_SCHEMA, TABLE_NAME, PERIOD_NAME > > Two new columns are added to the COLUMNS view: > IS_SYSTEM_TIME_PERIOD_START, IS_SYSTEM_TIME_PERIOD_END - contain YES/NO. > > diff --git a/mysql-test/main/information_schema-big.result b/mysql-test/main/information_schema-big.result > index 94c8d274fad..ad0451293c9 100644 > --- a/mysql-test/main/information_schema-big.result > +++ b/mysql-test/main/information_schema-big.result good, you didn't forget to update big and embedded tests > diff --git a/mysql-test/suite/period/r/i_s_notembedded.result b/mysql-test/suite/period/r/i_s_notembedded.result > new file mode 100644 > index 00000000000..879376cc8b4 > --- /dev/null > +++ b/mysql-test/suite/period/r/i_s_notembedded.result > @@ -0,0 +1,52 @@ > +select * from information_schema.periods; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +create or replace table t1 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)); > +create or replace table t2 (id int primary key, s timestamp(6), e timestamp(6), > +period for mytime(s,e)) with system versioning; > +show columns from t1; > +Field Type Null Key Default Extra > +id int(11) NO PRI NULL > +s timestamp(6) NO NULL > +e timestamp(6) NO NULL > +select * from information_schema.periods where table_schema = 'test'; > +TABLE_CATALOG TABLE_SCHEMA TABLE_NAME PERIOD START_COLUMN_NAME END_COLUMN_NAME > +def test t2 SYSTEM_TIME row_start row_end I don't think so, they're INVISIBLE_SYSTEM. for all practical purposes they don't exist as columns in the table. Shouldn't be shown either. But don't remove this test, let's keep it to show that row_start/row_end are *not* shown here. And add table t3 with full standard definition of period for system_time. it should be shown all right. > +def test t2 mytime s e > +def test t1 mytime s e > +create user periods_hidden@localhost; ... > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > index 1dc60a01f38..876d6c6f244 100644 > --- a/sql/sql_show.cc > +++ b/sql/sql_show.cc > @@ -6165,11 +6166,83 @@ static void store_variable_type(THD *thd, const sp_variable *spvar, > } > } > > +static int store_schema_period_record(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name, > + const TABLE_SHARE::period_info_t &period) > +{ > + TABLE_SHARE *s= tl->table->s; > + const CHARSET_INFO *cs= system_charset_info; > + > + int err= schema_table->field[0]->store(STRING_WITH_LEN("def"), cs); > + if(!err) err= schema_table->field[1]->store(db_name, cs); > + if(!err) err= schema_table->field[2]->store(table_name, cs); > + if(!err) err= schema_table->field[3]->store(period.name, cs); well, ok. but as you've seen elsewhere these go without checks, as these calls generally cannot fail, they don't even allocate any memory. > + > + if (err) return err; > + > + int period_field= 4; > + for (auto *field: {period.start_field(s), period.end_field(s)}) > + { > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + bool col_granted= get_column_grant(thd, &tl->grant, > + db_name->str, table_name->str, > + field->field_name.str) & COL_ACLS; > + if (col_granted) > +#endif > + { > + schema_table->field[period_field]->set_notnull(); > + err= schema_table->field[period_field]->store(field->field_name, cs); > + } > + > + if (err) > + return err; > + period_field++; > + } > + > + return schema_table_store_record(thd, schema_table); > +} > + > + > +static int > +get_schema_period_records(THD *thd, TABLE_LIST *tl, > + TABLE *schema_table, //!< @ref > + //!< Show::periods_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > +{ > + TABLE *table= tl->table; > + > + if (!table || (!table->s->period.name && !table->versioned())) > + return 0; > + > +#ifndef NO_EMBEDDED_ACCESS_CHECKS > + check_access(thd, SELECT_ACL, db_name->str, > + &tl->grant.privilege, 0, 0, MY_TEST(tl->schema_table)); > + if (is_temporary_table(tl)) > + { > + tl->grant.privilege|= TMP_TABLE_ACLS; > + } > +#endif > + int err= 0; > + if (table->versioned()) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->vers); > + if (!err && table->s->period.name) > + err= store_schema_period_record(thd, tl, schema_table, db_name, table_name, > + table->s->period); > + > + return err; > +} > > -static int get_schema_column_record(THD *thd, TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static > +int get_schema_column_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, //!< @ref Show::columns_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > LEX *lex= thd->lex; > const char *wild= lex->wild ? lex->wild->ptr() : NullS; > @@ -7513,30 +7592,41 @@ static int get_schema_triggers_record(THD *thd, TABLE_LIST *tables, > DBUG_RETURN(0); > } > > +static int > +store_key_column_usage(TABLE *table, const LEX_CSTRING &db_name, > + const LEX_CSTRING &table_name, > + const LEX_CSTRING &key_name, > + const LEX_CSTRING &col_name) > +{ > + CHARSET_INFO *cs= system_charset_info; > + static const LEX_CSTRING def{STRING_WITH_LEN("def")}; > + const LEX_CSTRING *values[] { &def, &db_name, &key_name, > + &def, &db_name, &table_name, &col_name }; > + int err = 0; > + for (uint i = 0; i < array_elements(values) && !err; i++) > + table->field[i]->store(values[i], cs); you forgot `err=` here :) but as I wrote above, it's not needed here anyway > + return 0; > +} > > static void > store_key_column_usage(TABLE *table, const LEX_CSTRING *db_name, > const LEX_CSTRING *table_name, const char *key_name, > - size_t key_len, const char *con_type, size_t con_len, > + size_t key_len, const char *col_name, size_t col_len, > longlong idx) > { > - CHARSET_INFO *cs= system_charset_info; > - table->field[0]->store(STRING_WITH_LEN("def"), cs); > - table->field[1]->store(db_name->str, db_name->length, cs); > - table->field[2]->store(key_name, key_len, cs); > - table->field[3]->store(STRING_WITH_LEN("def"), cs); > - table->field[4]->store(db_name->str, db_name->length, cs); > - table->field[5]->store(table_name->str, table_name->length, cs); > - table->field[6]->store(con_type, con_len, cs); > + store_key_column_usage(table, *db_name, *table_name, {key_name, key_len}, > + {col_name, col_len}); pretty, but better, please, put it back. You can use the old store_key_column_usage from your get_schema_key_period_usage_record() just the same. There's no need to have two store_key_column_usage() functions. > table->field[7]->store((longlong) idx, TRUE); > } > > > -static int get_schema_key_column_usage_record(THD *thd, > - TABLE_LIST *tables, > - TABLE *table, bool res, > - const LEX_CSTRING *db_name, > - const LEX_CSTRING *table_name) > +static int > +get_schema_key_column_usage_record(THD *thd, TABLE_LIST *tables, > + TABLE *table, > + //!< @ref Show::key_column_usage_fields_info > + bool res, > + const LEX_CSTRING *db_name, > + const LEX_CSTRING *table_name) > { > DBUG_ENTER("get_schema_key_column_usage_record"); > if (res) > @@ -9703,6 +9824,18 @@ ST_FIELD_INFO key_column_usage_fields_info[]= > CEnd() > }; > > +ST_FIELD_INFO key_period_usage_fields_info[]= > +{ > + Column("CONSTRAINT_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("CONSTRAINT_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_CATALOG", Catalog(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_SCHEMA", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("TABLE_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), > + Column("PERIOD_NAME", Name(), NOT_NULL, OPEN_FULL_TABLE), Cannot they all be OPEN_FRM_ONLY? > + CEnd() > +}; > + > > ST_FIELD_INFO table_names_fields_info[]= > { > @@ -10208,6 +10358,8 @@ ST_SCHEMA_TABLE schema_tables[]= > {0, 0, 0, 0, 0, 0, 0, 0, 0, 0} > }; > > +static_assert(array_elements(schema_tables) == SCH_ENUM_SIZE + 1, > + "Update enum_schema_tables as well."); +1 > > int initialize_schema_table(st_plugin_int *plugin) > { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 6

Re: 8114fb63bbb: MDEV-31684: Add timezone information to DATE_FORMAT
by Sergei Golubchik 04 Sep '23

04 Sep '23

Hi, Rucha, Few comments, after I saw the commit in the branch: On Sep 04, Rucha Deodhar wrote: > revision-id: 8114fb63bbb (mariadb-11.0.1-229-g8114fb63bbb) > parent(s): 7ba9c7fb84b > author: Rucha Deodhar > committer: Rucha Deodhar > timestamp: 2023-09-01 11:01:09 +0530 > message: > > MDEV-31684: Add timezone information to DATE_FORMAT > > Before starting to go over the format string, prepare the current time > zone information incase '%z' or '%Z' is encountered. > This information can be obtained as given below: > > A) If timezone is not set ( meaning we are working with system timezone): > Get the MYSQL_TIME representation for current time and GMT time using > current thread variable for timezone and timezone variable for UTC > respectively. This MYSQL_TIME variable will be used to calculate time > difference. Also convert current time in second to tm structure to > get system timezone information. > > B) If timezone is set as offset: > Get timezone information using current timezone information and store > in appropriate variable. > > C) If timezone is set as some place (example: Europe/Berlin) > Get timezone information by searching the timezone. During internal > timezone search, information like timeoffset from UTC and abbrevation > is stored in another relevant structure. Hence use the same information. > > diff --git a/mysql-test/main/date_formats.result b/mysql-test/main/date_formats.result > index 463cce39520..23aeffedef7 100644 > --- a/mysql-test/main/date_formats.result > +++ b/mysql-test/main/date_formats.result > @@ -573,3 +573,67 @@ time_format('01 02:02:02', '%T') > select time_format('2001-01-01 02:02:02', '%T'); > time_format('2001-01-01 02:02:02', '%T') > 02:02:02 > +# > +# Beginning of 11.3 test > +# > +# MDEV-31684: Add timezone information to DATE_FORMAT > +# > +SET @old_timezone= @@time_zone; > +SET TIME_ZONE='Japan'; > +SELECT DATE_FORMAT('2009-10-04 22:23:00', '%z %Z') AS current_timezone; > +current_timezone > ++0900 CJT > +SELECT DATE_FORMAT(CURDATE(), '%z %Z') AS current_tz; 1) please, don't use CURDATE/CURRENT_TIMESTAMP/etc. The time zone abbreviation depends on the date: $ TZ=Europe/Berlin date -d '2020-01-01 1:2:3' Wed Jan 1 01:02:03 CET 2020 $ TZ=Europe/Berlin date -d '2020-06-01 1:2:3' Mon Jun 1 01:02:03 CEST 2020 See? Could be CET or CEST. Japan doesn't seem to have daylight saving, so it'll be the same abbreviation throughout the year, but it can still change at some point. So always use a fixed date, like in your first test. 2) why is it CJT at all? $ TZ=Japan date -d '2009-10-04 22:23:00' +'%z %Z' +0900 JST it looks like it should be JST? ... 3) also, please test that CET/CEST example. Or, rather, MET/MEST/MST/MMT example - the data for these abbreviations are already present in mariadb_test_data_timezone.sql That is, add lines like SET @@time_zone='Europe/Moscow'; SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); SELECT DATE_FORMAT('....', '%z %Z'); and with different dates it should return different abbreviations for the same time zone. > +# > +# output depends on system time information. Hence replacing > +# to avoid result diff test failures. > +# > +SET @@time_zone= default; > +SELECT DATE_FORMAT('2009-10+|-HH:MM ABC22:23:00', '%z %Z') AS current_timezone; 1) I don't understand what you're testing here :( 2) Anyway, note that you can put, say, --timezone=Europe/Berlin into the date_formats.opt file and the server will be restarted with this value in TZ. That is, you can force the system time zone to be a specific fixed value. > diff --git a/scripts/mariadb_test_data_timezone.sql b/scripts/mariadb_test_data_timezone.sql > index 8d07d413cef..83bb548478d 100644 > --- a/scripts/mariadb_test_data_timezone.sql > +++ b/scripts/mariadb_test_data_timezone.sql > @@ -14,8 +14,8 @@ > -- along with this program; if not, write to the Free Software > -- Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1335 USA > > -INSERT INTO time_zone_name (Name, Time_Zone_id) ... > -INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > -INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > -INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > -INSERT INTO time_zone_leap_second (Transition_time, Correction) ... > +INSERT INTO time_zone_name (Name, Time_Zone_id) VALUES ... ('India/Kolkata', 6); > +INSERT INTO time_zone (Time_zone_id, Use_leap_seconds) ... > +INSERT INTO time_zone_transition (Time_zone_id, Transition_time, Transition_type_id) ... > +INSERT INTO time_zone_transition_type (Time_zone_id, Transition_type_id, `Offset`, Is_DST, Abbreviation) ... > +INSERT INTO time_zone_leap_second (Transition_time, Correction) ... 1. Why? I didn't see where you use this new value in tests. 2. Is it really India/Kolkata? The official https://www.iana.org/time-zones seems to have Asia/Kolkata. > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 26adc4eddaa..f8d3fe7dfe8 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -468,20 +468,111 @@ static bool extract_date_time(THD *thd, DATE_TIME_FORMAT *format, > DBUG_RETURN(1); > } > > +/* > + > + Gets local time in MYSQL_TIME structure and tm struct. > + > +*/ > +void get_timezone_offset_information(THD *thd, Time_zone *tz, int &hr, > + int &min, char* abbrevation, > + bool &is_neg) > +{ > + if (!tz) > + return; > + > + if (tz->get_name()->length() == tz_SYSTEM_name.length() && > + !strncmp((const char*)(tz->get_name()->ptr()), > + (const char*)(tz_SYSTEM_name.ptr()), 6)) > + { > + struct tm tm_local_time; > + time_t time_sec= my_time(0); > + MYSQL_TIME local_TIME, gmt_TIME; > + ulonglong seconds_diff; > + ulong microsec_diff; > + > + /* Get local time in MYSQL_TIME struct. */ > + thd->variables.time_zone->gmt_sec_to_TIME(&local_TIME, > + thd->query_start()); > + /* Get GMT time in MYSQL_TIME struct. */ > + my_tz_UTC->gmt_sec_to_TIME(&gmt_TIME, thd->query_start()); > + > + /* We need above two MYSQL_TIME to get time difference. */ > + is_neg= calc_time_diff(&local_TIME, &gmt_TIME, 1, > + &seconds_diff, &microsec_diff); > + > + /* > + once we have time difference in seconds_diff, > + convert it into hr and min. > + */ > + hr= (int)(seconds_diff/3600L); > + int temp= (int)(seconds_diff%3600L); > + min= temp/60L; > + > + /* > + Get timezone abbrevation. It is stored in tm structure, > + so convert time to tm struct and copy it. > + */ 1) tm structure also has the offset, you don't need to calculate it manually. 2) you should use not my_time(0) but the value of the first DATE_FORMAT() argument > + localtime_r(&time_sec, &tm_local_time); > +# ifdef __USE_MISC > + int len= strlen((tm_local_time.tm_zone)); > + strncpy(abbrevation, (tm_local_time.tm_zone), len); > + abbrevation[len]= '\0'; > +# endif > + } > + else > + { > + /* > + Information for "named" timezone and offset can be obtained from their > + relevant class. So get the timezone information first. > + */ > + struct tz tmp; > + tz->get_timezone_information(&tmp); > + is_neg= tmp.is_behind; > + > + /* Get time in hour and min. */ > + time_t time_in_sec= abs(tmp.seconds_offset); > + hr= (int)(time_in_sec/3600L); > + int temp= (int)(time_in_sec%3600L); > + min= temp/60L; > + > + /* Copy the abbrevation. */ > + strncpy(abbrevation, (const char*)(tmp.abbrevation), > + strlen((const char*)(tmp.abbrevation))); > + abbrevation[strlen((const char*)(tmp.abbrevation))+1]='\0'; > + } > + > + return; > +} > + > > /** > Create a formatted date/time value in a string. > */ > > -static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > - timestamp_type type, const MY_LOCALE *locale, > - String *str) > +static bool make_date_time(THD *thd, const String *format, > + const MYSQL_TIME *l_time, timestamp_type type, > + const MY_LOCALE *locale, String *str) > { > char intbuff[15]; > uint hours_i; > uint weekday; > ulong length; > const char *ptr, *end; > + int diff_hr=0, diff_min=0; > + bool curr_time_behind_utc= false; > + char abbrevation[8]; > + > + Time_zone* curr_timezone= my_tz_find(thd, > + thd->variables.time_zone->get_name()); > + > + /* > + precalculate local time and timezone information because > + %z or %Z maybe present in the format. > + */ > + memset(abbrevation, 0, sizeof(abbrevation)); > + get_timezone_offset_information(thd, curr_timezone, diff_hr, > + diff_min, abbrevation, > + curr_time_behind_utc); Just a thought. get_timezone_offset_information() doesn't seem to be very cheap. May be do it only when %z or %Z is requested? Like case 'z': if (!have_tz_info) { memset(abbrevation, 0, sizeof(abbrevation)); get_timezone_offset_information(thd, curr_timezone, diff_hr, diff_min, abbrevation, curr_time_behind_utc); have_tz_info= true; } Or better something with less code duplication :) > str->length(0); > > @@ -699,6 +790,31 @@ static bool make_date_time(const String *format, const MYSQL_TIME *l_time, > str->append_with_prefill(intbuff, length, 1, '0'); > break; > > + case 'z': > + { > + if (curr_time_behind_utc) > + str->append("-", 1, system_charset_info); > + else > + str->append("+", 1, system_charset_info); > + > + if (diff_hr/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_hr, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); > + if (diff_min/10 == 0) > + str->append("0", 1, system_charset_info); > + length= (uint) (int10_to_str(diff_min, intbuff, 10) - intbuff); > + str->append_with_prefill(intbuff, length, 1, '0'); why do you append "0" manually, if you use append_with_prefill(), which, I suppose, does it automatically? or, if you add "0" manually, you should use simple append(). > + } > + break; > + > + case 'Z': > + { > + str->append(abbrevation, > + (uint) strlen(abbrevation), > + system_charset_info); > + } > + break; > default: > str->append(*ptr); > break; > @@ -1912,7 +2028,7 @@ String *Item_func_date_format::val_str(String *str) > > /* Create the result string */ > str->set_charset(collation.collation); > - if (!make_date_time(format, &l_time, > + if (!make_date_time(thd, format, &l_time, > is_time_format ? MYSQL_TIMESTAMP_TIME : > MYSQL_TIMESTAMP_DATE, > lc, str)) > diff --git a/sql/tztime.cc b/sql/tztime.cc > index e77a7529332..4662fa082e0 100644 > --- a/sql/tztime.cc > +++ b/sql/tztime.cc > @@ -55,15 +55,10 @@ > > /* > Now we don't use abbreviations in server but we will do this in future. > + Edit: Startd needing abbrevation in server as part of task MDEV-31684. typo > */ > -#if defined(TZINFO2SQL) || defined(TESTTIME) > -#define ABBR_ARE_USED > -#else > -#if !defined(DBUG_OFF) > -/* Let use abbreviations for debug purposes */ > -#undef ABBR_ARE_USED > -#define ABBR_ARE_USED > -#endif /* !defined(DBUG_OFF) */ > +#ifndef ABBR_ARE_USED > +#define ABBR_ARE_USED > #endif /* defined(TZINFO2SQL) || defined(TESTTIME) */ > > /* Structure describing local time type (e.g. Moscow summer time (MSD)) */ > @@ -1008,11 +1003,6 @@ TIME_to_gmt_sec(const MYSQL_TIME *t, const TIME_ZONE_INFO *sp, uint *error_code) > > #if !defined(TESTTIME) && !defined(TZINFO2SQL) > > -/* > - String with names of SYSTEM time zone. > -*/ > -static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); > - > > /* > Instance of this class represents local time zone used on this system > @@ -1031,6 +1021,7 @@ class Time_zone_system : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1097,6 +1088,12 @@ Time_zone_system::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_system::get_timezone_information(struct tz* curr_tz) const > +{ this is weird. You have a special if (tz->get_name()->length() == tz_SYSTEM_name.length() && !strncmp((const char*)(tz->get_name()->ptr()), (const char*)(tz_SYSTEM_name.ptr()), 6)) to detect SYSTEM time zone by name in get_timezone_offset_information(). why not to put that logic here, in the dedicated method, like you did for other time zones? > + return; > +} > + > > /* > Get name of time zone > @@ -1128,6 +1125,7 @@ class Time_zone_utc : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > }; > > > @@ -1175,6 +1173,12 @@ Time_zone_utc::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_utc::get_timezone_information(struct tz* curr_tz) const > +{ what is printed for UTC? Please, add a test for that. > + return; > +} > + > > /* > Get name of time zone > @@ -1210,6 +1214,7 @@ class Time_zone_db : public Time_zone > virtual my_time_t TIME_to_gmt_sec(const MYSQL_TIME *t, uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > private: > TIME_ZONE_INFO *tz_info; > const String *tz_name; > @@ -1280,6 +1285,16 @@ Time_zone_db::gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const > adjust_leap_second(tmp); > } > > +void > +Time_zone_db::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= tz_info->revtis->rt_offset; > + curr_tz->is_behind= tz_info->revtis->rt_offset < 0 ? true : false; > + strncpy((char*)(curr_tz->abbrevation), (const char*)(tz_info->chars), > + strlen((const char*)(tz_info->chars))+1); > + curr_tz->abbrevation[strlen((const char*)(tz_info->chars))+1]='\0'; here and below (in Time_zone_offset) 1. why casts? 2. you use strncpy incorrectly, the third argument specifies the size of the buffer, like strncpy(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)); but better use strmake, it'll do the zero-termination for you strmake(curr_tz->abbrevation, tz_info->chars, sizeof(curr_tz->abbrevation)-1); > +} > + > > /* > Get name of time zone > @@ -1309,6 +1324,7 @@ class Time_zone_offset : public Time_zone > uint *error_code) const; > virtual void gmt_sec_to_TIME(MYSQL_TIME *tmp, my_time_t t) const; > virtual const String * get_name() const; > + virtual void get_timezone_information(struct tz* curr_tz) const; > /* > This have to be public because we want to be able to access it from > my_offset_tzs_get_key() function > @@ -1434,6 +1450,16 @@ Time_zone_offset::get_name() const > return &name; > } > > +void > +Time_zone_offset::get_timezone_information(struct tz* curr_tz) const > +{ > + curr_tz->seconds_offset= offset; > + const char *name= get_name()->ptr(); > + curr_tz->is_behind= name[0] == '+' ? false : true; > + strncpy((char*)(curr_tz->abbrevation), name, this->get_name()->length()); > + curr_tz->abbrevation[this->get_name()->length()]= '\0'; > +} > + > > static Time_zone_utc tz_UTC; > static Time_zone_system tz_SYSTEM; > diff --git a/sql/tztime.h b/sql/tztime.h > index 6d8af62ecd4..57e4aa2021b 100644 > --- a/sql/tztime.h > +++ b/sql/tztime.h > @@ -26,6 +26,11 @@ > #include "sql_list.h" /* Sql_alloc */ > #include "sql_string.h" /* String */ > > +/* > + String with names of SYSTEM time zone. > +*/ > +static const String tz_SYSTEM_name("SYSTEM", 6, &my_charset_latin1); No, sorry. It's a .h file included in many .cc files. You'll have a separate `static const String` in every single source file that includes this header. > + > class THD; Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: 23b15d24eb3: cleanup: add Field::store_yesno
by Sergei Golubchik 31 Aug '23

31 Aug '23

Hi, Nikita, On Aug 31, Nikita Malyavin wrote: > revision-id: 23b15d24eb3 (mariadb-11.0.1-227-g23b15d24eb3) > parent(s): cb37c99dd87 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-30 13:18:52 +0400 > message: > > cleanup: add Field::store_yesno I agree, it's a good idea to have a helper for storing YES/NO in the field. But I think Field class itself is a wrong place for it. YES/NO is exclusively INFORMATION_SCHEMA thing and it feels kind of wrong to have it in the base class. I'd do a separate function, like store_yes_no(). Or something. Also, note that you missed few places where it's needed, like pos=(uchar*) ((field->flags & NOT_NULL_FLAG) ? "NO" : "YES"); table->field[6]->store((const char*) pos, strlen((const char*) pos), cs); or if (updatable_view) table->field[5]->store(STRING_WITH_LEN("YES"), cs); else table->field[5]->store(STRING_WITH_LEN("NO"), cs); both in sql_show.cc. There are more. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

Re: Client-server protocol improvement proposal: Connection redirection
by Otto Kekäläinen 28 Aug '23

28 Aug '23

Hi Pali! On Fri, 18 Aug 2023 at 11:47, Pali <pali(a)cpan.org> wrote: > CCing choroba (for Perl's DBD::MariaDB) and replying to developers@ ML. > > I have read proposal and the first thing which worries me is how the > proposal handles security aspect of the whole solution. > ... > If TLS is used then any protocol redirection information must happen > inside of the TLS tunnel. Thanks for the comments! Actually Daniel's submission has already this covered and the redirect works only if the connection is TLS protected: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation

1 0

Re: [MariaDB commits] [PATCH 4/4] MDEV-31273: Precompute binlog checksums
by Marko Mäkelä 25 Aug '23

25 Aug '23

On Fri, Aug 25, 2023 at 10:16 AM Kristian Nielsen via commits <commits(a)lists.mariadb.org> wrote: > > Compute binlog checksums (when enabled) already when writing events > into the statement or transaction caches, where before it was done > when the caches are copied to the real binlog file. This moves the > checksum computation outside of holding LOCK_log, improving > scalabitily. > > At stmt/trx cache write time, the final end_log_pos values are not > known, so with this patch these will be set to 0. [...] > Checksums cannot be pre-computed when binlog encryption is enabled, as > encryption relies on correct end_log_pos to provide part of the > nonce/IV. Checksum pre-computation is also disabled for WSREP/Galera. Have you thought about a format change that would address these issues? In MDEV-14425 https://github.com/MariaDB/server/commit/685d958e38b825ad9829be311f26729ccc… I redesigned the InnoDB log file format in a way that allows checksums to be computed before the final position of the records (the log sequence number of LSN) is known. For encryption, an additional 64-bit nonce will be written; this currently is the LSN at the time the encryption was scheduled. Tablespace identifiers, page numbers and file names are not encrypted, because these were never fully encrypted in innodb_encrypt_tables=ON either. This allows the metadata to be used as initialization vectors, and also avoids the need for backup tools to know any encryption keys. Thanks to MDEV-27774, not only InnoDB computes checksums while not holding any mutex, but multiple threads can copy their mini-transactions to the shared log_sys.buf while holding a shared lock_sys.latch. An exclusive latch will only be needed during a log checkpoint when we need to guarantee that all changes up to a particular LSN have been written to the log buffer. This would not be possible without the format change that was implemented in MDEV-14425. > +/* Convenience macros since ulong is 32 or 64 bit depending on platform. */ > +#if SIZEOF_LONG == 4 > +#define my_atomic_storeul(P, D) my_atomic_store32((int32 volatile *)(P), (D)) In all currently supported MariaDB Server versions, C++11 is available. Is there a particular reason why you would not use std::atomic<size_t> here? Marko -- Marko Mäkelä, Lead Developer InnoDB MariaDB plc

2 3

Client-server protocol improvement proposal: Connection redirection
by Lenski, Daniel 18 Aug '23

18 Aug '23

Hi! I would like to propose a new feature in the MariaDB client-server protocol<https://mariadb.com/kb/en/clientserver-protocol>: application-layer redirection of client connections. We want the MariaDB server to be able to tell clients connecting to it, “Sorry, this server is unavailable. Connect to an alternate server instead.” This mechanism is inspired by HTTP 302 (“temporary redirect”) mechanism familiar to all developers of web applications, and is intended to have similar semantics and security properties, since these have now been widely deployed and tested for decades. I have submitted a minimal but viable implementation of this at: * https://github.com/MariaDB/server/pull/2681, server-side implementation * https://github.com/mariadb-corporation/mariadb-connector-c/pull/226, MariaDB Connector/C implementation I am seeking advice and opinions on these, and will proceed to finalize this in the following 1-2 months. In its current form, this implementation allows the MariaDB server administrator to set two variables: * SET GLOBAL SERVER_REDIRECT_MODE={ON,OFF} (the default is OFF) * SET GLOBAL SERVER_REDIRECT_TARGET='my-new-server.example.com' (or 192.168.0.123:3307, or new-server.com:3308, etc) When SERVER_REDIRECT_MODE is set to on, the server will stop accepting new TCP-based connections (unless they are to the extra port<https://mariadb.com/kb/en/thread-pool-in-mariadb/#configuring-the-extra-port>, which is intended for emergency administrative use), and it will instead respond to all clients with an error packet<https://mariadb.com/kb/en/err_packet/>: $ mariadb --host my-server.example.com ERROR 4189 (HY000): |Server is redirecting clients to 'my-new-server.example.com:3307'|my-new-server.example.com:3307 When an appropriately-updated client receives this error packet (error code 4196, message formatted as |Human readable message|<value of SERVER_REDIRECT_TARGET>), it will parse the redirection target out from the error message and attempt a new connection: $ updated-mariadb --host my-server.example.com -e 'select @@hostname' Got server redirect to 'my-new-server.example.com' (port 3307) +---------------------------+ | @@hostname | +---------------------------+ | my-new-server.example.com | +---------------------------+ The feature is gracefully backwards-compatible. Old clients that do not support redirection will fail and show the message in a form from which humans can deduce what happened. New clients that do support it will follow it to the new server. The feature is also designed to be extensible in a forwards-compatible way. The SERVER_REDIRECT_MODE system variable could be expanded to support additional redirection modes. For example, it could be augmented to redirect new connections only from specific IP subnets, or to direct the client to interpret the SERVER_REDIRECT_TARGET variable in a new form (such as a<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> JDBC-compatible mysql:// URL<https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-reference-jdbc-url…> as proposed in MDEV-15935<https://jira.mariadb.org/browse/MDEV-15935>). While researching and designing this connection redirection mechanism, I discovered or rediscovered a couple of major TLS-related security vulnerabilities in the MariaDB Connector/C client [1, 2]. These vulnerabilities affect all MariaDB users who care about the confidentiality and authenticity of their client/server connections, and they become even more severe if a connection redirection mechanism exists — because this entails the client trusting the server to send it instructions for creating a new network connection. Unless these are resolved, any connection redirection mechanism will be susceptible to hijacking and redirection to an attacker-controlled server even if the client thinks it is using TLS/SSL. What are the use cases for this connection redirection feature? Even in this simple, initial form, the feature is already useful for database administrators who want to gracefully shut down a MariaDB server (for example, to upgrade it), and to redirect new client connections to a replica or alternate server that can accept the traffic. Server-side connection redirection can be combined with existing MariaDB server features, such as SET GLOBAL READ_ONLY=ON and KILLing of idle or long-running connections, in order to build a full switchover solution for shunting traffic from one server to another as gracefully as possible. Currently, MariaDB database administrators tend to use proxies and wrappers (such as HikariCP) to redirect client connections from one MariaDB server to another. Supporting this redirection mechanism directly in the server, and in the client-server protocol, will allow many simple use cases of redirection without extra software, making MariaDB easier and simpler to manage for minimal downtime situations. Please post on this mailing list what you think about this suggestion. I am looking forward to polishing this feature and submitting it in a form that meets the quality requirements for new features from MariaDB core developers. Thank you! Daniel Lenski Amazon RDS MySQL/MariaDB engine team --- Notes on TLS vulnerabilities in MariaDB Connector/C: [1] CONC-648<https://jira.mariadb.org/browse/CONC-648>, reported by me on 5 June: when a Connector/C-based client is using connections secured by TLS/SSL, Connector/C will nevertheless receive plain-text, unencrypted error packets prior to the TLS handshake, and present them to the client in a way that’s indistinguishable from TLS-protected error packets sent by the server after the TLS handshake. This longstanding vulnerability is a consequence of the tangled and unsystematic way that the Connector/C codebase interleaves the MariaDB application-layer authentication protocol with the transport-layer security protocol. My standalone mariadb-connector-c PR#223<https://github.com/mariadb-corporation/mariadb-connector-c/pull/223> fixes the most straightforward manifestation of this vulnerability. Until this issue is resolved in Connector/C, it will be trivial for MITM attackers to inject plaintext connection redirection packets into MariaDB client/server connections, and thereby redirect clients to attacker-controlled servers despite those clients’ intention to use only TLS-secured connections. [2] MDEV-28634<https://jira.mariadb.org/browse/MDEV-28634>, reported almost 3 years ago by Geoff Montee: Connector/C clients which specify --ssl without --ssl-verify-server-cert will allow their connections to be silently downgraded to unencrypted, plaintext connections. This vulnerability can be used by on-path attackers to trivially downgrade most MariaDB client-server connections from TLS-encrypted to plaintext. My standalone mariadb-connector-c PR#224<https://github.com/mariadb-corporation/mariadb-connector-c/pull/224> fixes this. It is a backwards-incompatible change in that clients which specify --ssl (and thus clearly expect TLS-secured connections) will no longer succeed in connecting to servers which don’t support TLS. This should be considered a good thing: MariaDB Connector/C-based clients have been misleading their users about whether their connections are guaranteed to be encrypted, and will no longer do so. Essentially all software that purports to secure network connections with TLS have implemented similar changes, most of it about a decade ago when the pervasiveness of such attacks<https://en.wikipedia.org/wiki/Global_surveillance_disclosures_(2013%E2%80%9…> (by ISPs, censors, intelligenceagencies, etc) became very widely-known.

8 28

Re: [Commits] cb06612a9da: MDEV-31655: Parallel replication deadlock victim preference code errorneously removed
by Kristian Nielsen 15 Aug '23

15 Aug '23

Hi Marko, I'd like to ask you to review the below patch for MDEV-31655. https://jira.mariadb.org/browse/MDEV-31655 https://github.com/MariaDB/server/commit/cb06612a9da09a7981ada84768793f2ff3… This patch restores code in InnoDB that was originally part of the parallel replication implementation. It makes InnoDB choose the right victim when two transactions in parallel replication deadlock with each other. The code was erroneously removed as part of the VATS work in InnoDB. I saw that you later deprecated and eventually removed the VATS code. I saw that the InnoDB deadlock code is substantially changed in 10.6, so I will need to rework the patch in that version. But I wanted to ask your opinion about this 10.4 version first. In the patch, note in particular the modification to has_higher_priority(), which I think has the effect of choosing which of two waiting transactions to grant a lock to. I don't remember adding this myself, so it might have been added as part of VATS work. But it sounds appropriate; if T1 and T2 are both waiting for a lock, there is no point in granting it to T2 over T1, as then T2 will just be immediately killed and rolled back by replication. - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > revision-id: cb06612a9da09a7981ada84768793f2ff3ef842c (mariadb-10.4.30-3-gcb06612a9da) > parent(s): dbe5c20b755b87f67d87990c3baabc866667e41b > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2023-07-11 00:31:29 +0200 > message: > > MDEV-31655: Parallel replication deadlock victim preference code errorneously removed > > Restore code to make InnoDB choose the second transaction as a deadlock > victim if two transactions deadlock that need to commit in-order for > parallel replication. This code was erroneously removed when VATS was > implemented in InnoDB. > > Also add a test case for InnoDB choosing the right deadlock victim. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > > --- > .../suite/binlog_encryption/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/r/rpl_parallel.result | 42 ++++++++++++- > mysql-test/suite/rpl/t/rpl_parallel.test | 71 +++++++++++++++++++++- > sql/rpl_parallel.cc | 7 ++- > sql/sql_class.cc | 43 +++++++++++++ > storage/innobase/lock/lock0lock.cc | 12 ++++ > storage/innobase/trx/trx0trx.cc | 12 ++++ > 7 files changed, 225 insertions(+), 4 deletions(-) > > diff --git a/mysql-test/suite/binlog_encryption/rpl_parallel.result b/mysql-test/suite/binlog_encryption/rpl_parallel.result > index b75a66a634a..b24ff7ba53d 100644 > --- a/mysql-test/suite/binlog_encryption/rpl_parallel.result > +++ b/mysql-test/suite/binlog_encryption/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1680,13 +1681,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/r/rpl_parallel.result b/mysql-test/suite/rpl/r/rpl_parallel.result > index 9b2e68d366e..ef89d954faa 100644 > --- a/mysql-test/suite/rpl/r/rpl_parallel.result > +++ b/mysql-test/suite/rpl/r/rpl_parallel.result > @@ -2,6 +2,7 @@ include/master-slave.inc > [connection master] > connection server_2; > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > SET GLOBAL slave_parallel_threads=10; > ERROR HY000: This operation cannot be performed as you have a running slave ''; run STOP SLAVE '' first > include/stop_slave.inc > @@ -1679,13 +1680,52 @@ a > 2000 > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > a > +MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +connection server_1; > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +COMMIT; > +include/save_master_gtid.inc > +connection server_2; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 5; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > +connection master; > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > +SET @commit_id= 1+1000; > +SET @commit_id= 2+1000; > +SET @commit_id= 3+1000; > +SET @commit_id= 4+1000; > +SET @commit_id= 5+1000; > +SET @commit_id= 6+1000; > +SET @commit_id= 7+1000; > +SET @commit_id= 8+1000; > +SET @commit_id= 9+1000; > +SET @commit_id= 10+1000; > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > +include/save_master_gtid.inc > +connection server_2; > +include/start_slave.inc > +include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > +COUNT(*) SUM(a*100*b) > +10 225000 > connection server_2; > include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > connection server_1; > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_parallel.test b/mysql-test/suite/rpl/t/rpl_parallel.test > index 9ba7a30f2eb..d43cec4df34 100644 > --- a/mysql-test/suite/rpl/t/rpl_parallel.test > +++ b/mysql-test/suite/rpl/t/rpl_parallel.test > @@ -13,6 +13,7 @@ > > --connection server_2 > SET @old_parallel_threads=@@GLOBAL.slave_parallel_threads; > +SET @old_parallel_mode=@@GLOBAL.slave_parallel_mode; > --error ER_SLAVE_MUST_STOP > SET GLOBAL slave_parallel_threads=10; > --source include/stop_slave.inc > @@ -2203,16 +2204,84 @@ SELECT * FROM t1 WHERE a>=2000 ORDER BY a; > SELECT * FROM t2 WHERE a>=2000 ORDER BY a; > > > +--echo MDEV-31655: Parallel replication deadlock victim preference code erroneously removed > +# The problem was that InnoDB would choose the wrong deadlock victim. > +# Create a lot of transactions that can cause deadlocks, and use error > +# injection to check that the first transactions in each group is never > +# selected as deadlock victim. > +--let $rows= 10 > +--let $transactions= 5 > +--let $gcos= 10 > + > +--connection server_1 > +CREATE TABLE t7 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +BEGIN; > +--disable_query_log > +--let $i= 0 > +while ($i < 10) { > + eval INSERT INTO t7 VALUES ($i, 0); > + inc $i; > +} > +--enable_query_log > +COMMIT; > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/sync_with_master_gtid.inc > +--source include/stop_slave.inc > +eval set @@global.slave_parallel_threads= $transactions; > +set @@global.slave_parallel_mode= conservative; > +SET @old_dbug= @@GLOBAL.debug_dbug; > +# This error injection will allow no retries for GTIDs divisible by 1000. > +SET GLOBAL debug_dbug= "+d,rpl_mdev31655_zero_retries"; > + > +--connection master > +SET @old_dbug= @@SESSION.debug_dbug; > +SET SESSION debug_dbug="+d,binlog_force_commit_id"; > + > +--let $j= 1 > +while ($j <= $gcos) { > + eval SET @commit_id= $j+1000; > + --let $i= 0 > + while ($i < $transactions) { > + --disable_query_log > + eval SET SESSION gtid_seq_no= 1000 + 1000*$j + $i; > + BEGIN; > + --let $k= 0 > + while ($k < $rows) { > + eval UPDATE t7 SET b=b+1 WHERE a=(($i+$k) MOD $rows); > + inc $k; > + } > + COMMIT; > + --enable_query_log > + inc $i; > + } > + inc $j; > +} > + > +SET SESSION debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > +--source include/save_master_gtid.inc > + > +--connection server_2 > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc > +SET GLOBAL debug_dbug= @old_dbug; > +SELECT COUNT(*), SUM(a*100*b) FROM t7; > + > + > # Clean up. > --connection server_2 > --source include/stop_slave.inc > SET GLOBAL slave_parallel_threads=@old_parallel_threads; > +SET GLOBAL slave_parallel_mode=@old_parallel_mode; > --source include/start_slave.inc > SET DEBUG_SYNC= 'RESET'; > > --connection server_1 > DROP function foo; > -DROP TABLE t1,t2,t3,t4,t5,t6; > +DROP TABLE t1,t2,t3,t4,t5,t6,t7; > SET DEBUG_SYNC= 'RESET'; > > --source include/rpl_end.inc > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index b550315d69f..1aeb1257c4a 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1385,8 +1385,13 @@ handle_rpl_parallel_thread(void *arg) > err= dbug_simulate_tmp_error(rgi, thd);); > if (unlikely(err)) > { > + ulong max_retries= slave_trans_retries; > convert_kill_to_deadlock_error(rgi); > - if (has_temporary_error(thd) && slave_trans_retries > 0) > + DBUG_EXECUTE_IF("rpl_mdev31655_zero_retries", > + if ((rgi->current_gtid.seq_no % 1000) == 0) > + max_retries= 0; > + ); > + if (has_temporary_error(thd) && max_retries > 0) > err= retry_event_group(rgi, rpt, qev); > } > } > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 8ed3f8a9c5e..e6ed7ca1cc4 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5247,6 +5247,49 @@ thd_need_ordering_with(const MYSQL_THD thd, const MYSQL_THD other_thd) > return 0; > } > > + > +/* > + If the storage engine detects a deadlock, and needs to choose a victim > + transaction to roll back, it can call this function to ask the upper > + server layer for which of two possible transactions is prefered to be > + aborted and rolled back. > + > + In parallel replication, if two transactions are running in parallel and > + one is fixed to commit before the other, then the one that commits later > + will be prefered as the victim - chosing the early transaction as a victim > + will not resolve the deadlock anyway, as the later transaction still needs > + to wait for the earlier to commit. > + > + The return value is -1 if the first transaction is prefered as a deadlock > + victim, 1 if the second transaction is prefered, or 0 for no preference (in > + which case the storage engine can make the choice as it prefers). > +*/ > +extern "C" int > +thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2) > +{ > + rpl_group_info *rgi1, *rgi2; > + > + if (!thd1 || !thd2) > + return 0; > + > + /* > + If the transactions are participating in the same replication domain in > + parallel replication, then request to select the one that will commit > + later (in the fixed commit order from the master) as the deadlock victim. > + */ > + rgi1= thd1->rgi_slave; > + rgi2= thd2->rgi_slave; > + if (rgi1 && rgi2 && > + rgi1->is_parallel_exec && > + rgi1->rli == rgi2->rli && > + rgi1->current_gtid.domain_id == rgi2->current_gtid.domain_id) > + return rgi1->gtid_sub_id < rgi2->gtid_sub_id ? 1 : -1; > + > + /* No preferences, let the storage engine decide. */ > + return 0; > +} > + > + > extern "C" int thd_non_transactional_update(const MYSQL_THD thd) > { > return(thd->transaction.all.modified_non_trans_table); > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index b1cf2152cd6..469d03eaa06 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -49,6 +49,8 @@ Created 5/7/1996 Heikki Tuuri > #include <mysql/service_wsrep.h> > #endif /* WITH_WSREP */ > > +extern "C" int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** Lock scheduling algorithm */ > ulong innodb_lock_schedule_algorithm; > > @@ -1538,6 +1540,16 @@ static bool has_higher_priority(lock_t *lock1, lock_t *lock2) > } else if (!lock_get_wait(lock2)) { > return false; > } > + // Ask the upper server layer if any of the two trx should be prefered. > + int preference = thd_deadlock_victim_preference(lock1->trx->mysql_thd, > + lock2->trx->mysql_thd); > + if (preference == -1) { > + // lock1 is preferred as a victim, so lock2 has higher priority > + return false; > + } else if (preference == 1) { > + // lock2 is preferred as a victim, so lock1 has higher priority > + return true; > + } > return lock1->trx->start_time_micro <= lock2->trx->start_time_micro; > } > > diff --git a/storage/innobase/trx/trx0trx.cc b/storage/innobase/trx/trx0trx.cc > index 7cd95878b0c..0771d764fb6 100644 > --- a/storage/innobase/trx/trx0trx.cc > +++ b/storage/innobase/trx/trx0trx.cc > @@ -52,6 +52,9 @@ Created 3/26/1996 Heikki Tuuri > #include <set> > #include <new> > > +extern "C" > +int thd_deadlock_victim_preference(const MYSQL_THD thd1, const MYSQL_THD thd2); > + > /** The bit pattern corresponding to TRX_ID_MAX */ > const byte trx_id_max_bytes[8] = { > 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff > @@ -1906,6 +1909,15 @@ trx_weight_ge( > { > ibool a_notrans_edit; > ibool b_notrans_edit; > + int pref; > + > + /* First ask the upper server layer if it has any preference for which > + to prefer as a deadlock victim. */ > + pref= thd_deadlock_victim_preference(a->mysql_thd, b->mysql_thd); > + if (pref < 0) > + return FALSE; > + else if (pref > 0) > + return TRUE; > > /* If mysql_thd is NULL for a transaction we assume that it has > not edited non-transactional tables. */ > _______________________________________________ > commits mailing list -- commits(a)lists.mariadb.org > To unsubscribe send an email to commits-leave(a)lists.mariadb.org

2 7

Re: 9c52eede2c8: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 15 Aug '23

15 Aug '23

Hi, Nikita, On Aug 14, Nikita Malyavin wrote: > revision-id: 9c52eede2c8 (mariadb-11.0.1-175-g9c52eede2c8) > parent(s): 209c894b431 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-11 16:29:37 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > > 1. Make online disk writes unlimited, same as filesort does. > 2. Make proper error handling -- in 32-bit build IO_CACHE capacity limit is > 4GB, so it is quite possible to overfill there. > 3. Event_log::write_cache complicated with event reparsing, and as it was > proven by QA, contains some mistakes. Rewrite introbuce a simpler and much > faster version, not featuring reparsing and therefore copying a whole > buffer at once. This also disables checksums and crypto. > > As a result, online alter is untied of several binlog variables, which was > a second aim of this patch. > > diff --git a/mysql-test/main/alter_table_online_debug.result b/mysql-test/main/alter_table_online_debug.result > index 021267a15f5..3a615372ed5 100644 > --- a/mysql-test/main/alter_table_online_debug.result > +++ b/mysql-test/main/alter_table_online_debug.result > @@ -409,23 +409,23 @@ set debug_sync= 'now SIGNAL start_replication'; > set debug_sync= 'now WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > -3 51.220 > +3 53.390 why is that? > set debug_sync= 'now SIGNAL proceed WAIT_FOR applied'; > select stage, progress from INFORMATION_SCHEMA.PROCESSLIST where id = @con; > stage progress > diff --git a/sql/log.cc b/sql/log.cc > index d9e81700463..8c57487d57e 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2286,13 +2286,21 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > MY_BITMAP *old_rpl_write_set= table->rpl_write_set; > table->rpl_write_set= &table->s->all_set; > > + table->online_alter_cache->store_prev_position(); > int error= (*log_func)(thd, table, table->s->online_alter_binlog, > - table->online_alter_cache, true, > + table->online_alter_cache, > + table->file->has_transactions_and_rollback(), > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + if (unlikely(error)) > + { > + table->online_alter_cache->restore_prev_position(); what is this for? > + return HA_ERR_RBR_LOGGING_FAILED; > + } > + > + return 0; > } > > static void > @@ -7748,7 +7763,7 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > } > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Re: ca64ddcc709: MDEV-31646 Online alter applies binlog cache limit to cache writes
by Sergei Golubchik 10 Aug '23

10 Aug '23

Hi, Nikita, This is a review of all the three commits On Jul 20, Nikita Malyavin wrote: > revision-id: ca64ddcc709 (mariadb-11.0.1-144-gca64ddcc709) > parent(s): f34c7419cb8 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-19 02:31:32 +0400 > message: > > MDEV-31646 Online alter applies binlog cache limit to cache writes > diff --git a/sql/log.cc b/sql/log.cc > index afd0643fe82..7466d63d325 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -2291,8 +2294,14 @@ int binlog_log_row_online_alter(TABLE* table, const uchar *before_record, > before_record, after_record); > > table->rpl_write_set= old_rpl_write_set; > + thd->pop_internal_handler(); > + > + if (unlikely(error)) > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, ER_DISK_FULL, > + "Broken online alter log. " > + "ALTER TABLE will finish with error."); I don't think so. Why would a DML statement get a warning about the concurrently running ALTER TABLE? It didn't do anything wrong, I don't think there should be a warning here Like, someone is running a series of DML statements, or the same statement many times. At some point another connection runs ALTER TABLE and suddenly those perfectly good DML statements start spewing out warnings. ALTER ends and all is clear again - looks wrong to me > > - return unlikely(error) ? HA_ERR_RBR_LOGGING_FAILED : 0; > + return 0; > } > > static void > @@ -2592,7 +2601,7 @@ void Event_log::set_write_error(THD *thd, bool is_transactional) > DBUG_VOID_RETURN; > } > > -bool Event_log::check_write_error(THD *thd) > +bool MYSQL_BIN_LOG::check_write_error(THD *thd) What's the difference? it's a static method anyway, FWIW it could be outside of any class. What's the point of moving it from one class to another? > { > DBUG_ENTER("MYSQL_BIN_LOG::check_write_error"); > > @@ -3806,9 +3815,9 @@ bool MYSQL_BIN_LOG::open_index_file(const char *index_file_name_arg, > } > > > -bool Event_log::open(enum cache_type io_cache_type_arg) > +bool Event_log::open(enum cache_type io_cache_type_arg, size_t buffer_size) > { > - bool error= init_io_cache(&log_file, -1, LOG_BIN_IO_SIZE, io_cache_type_arg, > + bool error= init_io_cache(&log_file, -1, buffer_size, io_cache_type_arg, why? it's only used by online alter, where would you need a different buffer size? only in your DBUG_EXECUTE_IF? > 0, 0, MYF(MY_WME | MY_NABP | MY_WAIT_IF_FULL)); > > log_state= LOG_OPENED; > @@ -6544,7 +6562,8 @@ Event_log::flush_and_set_pending_rows_event(THD *thd, Rows_log_event* event, > if (writer.write(pending)) > { > set_write_error(thd, is_transactional); > - if (check_write_error(thd) && cache_data && > + if (dynamic_cast<MYSQL_BIN_LOG*>(this) && why? > + MYSQL_BIN_LOG::check_write_error(thd) && cache_data && > stmt_has_updated_non_trans_table(thd)) > cache_data->set_incident(); > delete pending; > @@ -7721,8 +7749,11 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > { > DBUG_ASSERT(cache.cache_log.type != READ_CACHE); > mysql_mutex_lock(binlog->get_log_lock()); > - error= binlog->write_cache(thd, &cache.cache_log); > + error= binlog->write_cache_raw(thd, &cache.cache_log); > mysql_mutex_unlock(binlog->get_log_lock()); > + > + if (unlikely(error)) > + binlog->set_write_error(my_errno); shouldn't binlog->write_cache() call set_write_error() internally? why would the caller have to do it? > } > } > else if (!commit) // rollback > @@ -7736,14 +7767,14 @@ static int binlog_online_alter_end_trans(THD *thd, bool all, bool commit) > cache.store_prev_position(); > } > > + thd->pop_internal_handler(); > > if (error) > { > - my_error(ER_ERROR_ON_WRITE, MYF(ME_ERROR_LOG), > - binlog->get_name(), errno); > - binlog_online_alter_cleanup(thd->online_alter_cache_list, > - is_ending_transaction); > - DBUG_RETURN(error); > + push_warning_printf(thd, Sql_state_errno_level::WARN_LEVEL_WARN, > + ER_ERROR_ON_WRITE, ER_THD(thd, ER_ERROR_ON_WRITE), > + binlog->get_name(), errno); > + DBUG_ASSERT(binlog->get_write_error()); same comment as above about not showing ALTER problems in concurrent threads > } > } > > diff --git a/sql/log.h b/sql/log.h > index 83e323cb6fb..39512b01991 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -444,17 +445,20 @@ class Event_log: public MYSQL_LOG > */ > class Cache_flip_event_log: public Event_log { > IO_CACHE alt_buf; > +IF_DBUG(public:,) yuck I don't see you using online_write_error or ref_count in DBUG macros > IO_CACHE *current, *alt; > std::atomic<uint> ref_count; > + std::atomic<int> online_write_error; > public: > > Cache_flip_event_log() : Event_log(), alt_buf{}, > - current(&log_file), alt(&alt_buf), ref_count(1) {} > - bool open(enum cache_type io_cache_type_arg) > + current(&log_file), alt(&alt_buf), ref_count(1), > + online_write_error(0) {} > + bool open(size_t buffer_size) looks like Cache_flip_event_log::open() doesn't need any arguments at all > { > log_file.dir= mysql_tmpdir; > alt_buf.dir= log_file.dir; > - bool res= Event_log::open(io_cache_type_arg); > + bool res= Event_log::open(WRITE_CACHE); > if (res) > return res; > > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 10180a95d99..0c4c284a40f 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -761,16 +761,21 @@ Log_event::Log_event(const uchar *buf, > > int Log_event::read_log_event(IO_CACHE* file, String* packet, > const Format_description_log_event *fdle, > - enum enum_binlog_checksum_alg checksum_alg_arg) > + enum enum_binlog_checksum_alg checksum_alg_arg, > + size_t max_allowed_packet) > { > ulong data_len; > char buf[LOG_EVENT_MINIMAL_HEADER_LEN]; > uchar ev_offset= packet->length(); > #if !defined(MYSQL_CLIENT) > - THD *thd=current_thd; > - ulong max_allowed_packet= thd ? thd->slave_thread ? slave_max_allowed_packet > - : thd->variables.max_allowed_packet > - : ~(uint)0; > + if (max_allowed_packet == 0) may be better pass THD as an argument? or even correct value of max_allowed_packet. As ulong max_allowed_packet(THD *thd) { return thd ? ... : ~(uint)0; } on the second thought, do you need to ignore max_allowed_packet here for online ALTER? Is there no event size limit when the event is written into binlog? > + { > + THD *thd=current_thd; > + max_allowed_packet= thd ? thd->slave_thread > + ? slave_max_allowed_packet > + : thd->variables.max_allowed_packet > + : ~(uint)0; > + } > #endif > DBUG_ENTER("Log_event::read_log_event(IO_CACHE*,String*...)"); > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 7

Re: 88a77860a8b: MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails
by Sergei Golubchik 09 Aug '23

09 Aug '23

Hi, Nikita, On Aug 08, Nikita Malyavin wrote: > revision-id: 88a77860a8b (mariadb-11.0.1-194-g88a77860a8b) > parent(s): 73a677f0306 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-07 22:45:11 +0400 > message: > > MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails > > ... upon replicating online ALTER > > When an online event is applied and slave_exec_mode is idempotent, > Write_rows_log_event::do_before_row_operations had reset > thd->lex->sql_command to SQLCOM_REPLACE. > > This led to that a statement was detected as a row-type during > binlogging, and was logged as not standalone. > > So the corresponding Gtid_log_event, when applied on replica, did not > exit early and created a new PSI transaction. Hence the difference > with non-online ALTER. Nice find. I vaguely believe I've seen in one of the commits I've reviewed a change that makes every row event set thd->lex->sql_command to the corresponding SQLCOM_INSERT/UPDATE/DELETE. But be it was in an unpushed commit. Anyway, I think it's a bit risky to assume that row events never change thd->lex->sql_command (and never will). Perhaps you should simply restore it after the applying part? Like --- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -12181,6 +12181,7 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *> if (error) from->s->tdc->flush_unused(1); // to free the binlog to->pos_in_table_list= NULL; // Safety + thd->lex->sql_command= SQLCOM_ALTER_TABLE; } else if (online) // error was on copy stage { Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 5

Re: 63c93f89af9: MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails
by Sergei Golubchik 07 Aug '23

07 Aug '23

Hi, Nikita, On Aug 07, Nikita Malyavin wrote: > revision-id: 63c93f89af9 (mariadb-11.0.1-192-g63c93f89af9) > parent(s): a75ba4caa51 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-08-05 20:17:41 +0400 > message: > > MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails ... > index 1c8d56cafd1..66b2b1ee4cc 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -1459,7 +1459,7 @@ void trans_register_ha(THD *thd, bool all, handlerton *ht_arg, ulonglong trxid) > Do not register transactions in which binary log is the only participating > transactional storage engine. > */ > - if (thd->m_transaction_psi == NULL && ht_arg->db_type != DB_TYPE_BINLOG) > + if (thd->m_transaction_psi == NULL && ht_arg != binlog_hton) it would've been better to fix it in 10.4, but if it doesn't show before online alter then ok > { > thd->m_transaction_psi= MYSQL_START_TRANSACTION(&thd->m_transaction_state, > thd->get_xid(), trxid, thd->tx_isolation, thd->tx_read_only, > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 88302096272..944807ea422 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -65,6 +65,7 @@ > #include "rpl_rli.h" > #include "log.h" > #include "sql_debug.h" > +#include "mysql/psi/mysql_transaction.h" > > #ifdef _WIN32 > #include <io.h> > @@ -11653,6 +11654,9 @@ bool mysql_trans_commit_alter_copy_data(THD *thd) > > DEBUG_SYNC(thd, "alter_table_copy_trans_commit"); > > + MYSQL_COMMIT_TRANSACTION(thd->m_transaction_psi); > + thd->m_transaction_psi= NULL; I don't understand it. ha_enable_transaction() commits the transaction it calls ha_commit_trans(). And ha_commit_trans() does the above psi manipulations. Why do you need to do that explicitly? Also, as a cleanup, I suspect, you can remove explicit commit that follows ha_enable_transaction() in mysql_trans_commit_alter_copy_data(). Seems to be redundant. > if (ha_enable_transaction(thd, TRUE)) > DBUG_RETURN(TRUE); Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: b508107c7a3: MDEV-31631 Adding auto-increment to table with history online misbehaves
by Sergei Golubchik 04 Aug '23

04 Aug '23

Hi, Nikita, Looks good. One comment below: On Aug 04, Nikita Malyavin wrote: > revision-id: b508107c7a3 (mariadb-11.0.1-189-gb508107c7a3) > parent(s): e9a75c98781 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-29 00:13:48 +0400 > message: > > MDEV-31631 Adding auto-increment to table with history online misbehaves > > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index eabee13e8d3..3a70ed672a6 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -9953,6 +9953,37 @@ const char *online_alter_check_supported(const THD *thd, > } > } > > + for (auto &c: alter_info->create_list) > + { > + *online= c.field || !(c.flags & AUTO_INCREMENT_FLAG); > + if (!*online) > + return "ADD COLUMN ... AUTO_INCREMENT"; > + > + auto *def= c.default_value; > + *online= !(def && def->flags & VCOL_NEXTVAL > + // either it's a new field, or a NULL -> NOT NULL change > + && (!c.field || (!(c.field->flags & NOT_NULL_FLAG) > + && (c.flags & NOT_NULL_FLAG)))); > + if (!*online) > + { > + const char *fmt= ER_THD(thd, ER_GENERATED_COLUMN_FUNCTION_IS_NOT_ALLOWED); > + > + Item::vcol_func_processor_result res; > + IF_DBUG(bool ret=,) > + def->expr->walk(&Item::check_vcol_func_processor, 0, &res); I don't think you need to walk here, res.name is always "nextval()" isn't it? > + DBUG_ASSERT(!ret); > + DBUG_ASSERT(res.errors & VCOL_NEXTVAL); > + > + LEX_CSTRING dflt{STRING_WITH_LEN("DEFAULT")}; > + size_t len= strlen(fmt) + strlen(res.name) + c.field_name.length > + + dflt.length; > + char *resp= (char*)thd->alloc(len); > + // expression %s cannot be used in the %s clause of %`s > + my_snprintf(resp, len, fmt, res.name, dflt.str, c.field_name.str); > + return resp; > + } > + } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1

Running msqld with thread sanitizer
by Thejaka Amila J Kanewala 04 Aug '23

04 Aug '23

Hello, I am trying to build and run a thread sanitized mysqld binary. In the CMake configuration, I see the WITH_ASAN option, however, I do not see an option to build mysqld with the thread sanitizer. After making some custom configurations, I was able to build a mysqld binary with the thread sanitizer (by linking static thread sanitizer), however, when trying to run it, I got the following error: "ThreadSanitizer: sync allocator overflow (65536*1024). Dying." This error seems to be coming from the tsan library and seems this error is caused by too many synchronizations [1]. Reducing the number of threads also didn't help me with this error. So my questions to you experts: 1. Do you run mysqld with the thread sanitizer ? 2. If yes, could you please share the details. (e.g., CMake configurations, compiler/linker flags etc.) 3. If not, any suggestions to overcome the above error ? [1] https://groups.google.com/g/thread-sanitizer/c/AX5twD_IPP4 Thank you. Best Regards, Thejaka Amila Kanewala, PhD https://github.com/thejkane/agm

2 3

Re: 44b4a3e64fa: MDEV-31777 ER_GET_ERRNO upon online alter on CONNECT table
by Sergei Golubchik 03 Aug '23

03 Aug '23

Hi, Nikita, Generally, please, prefer table flags over new virtual function. E.g. *online= new_table->file->ha_table_flags() & HA_CAN_ONLINE_ALTER; But in this particular case there's no reason for a engine to not support online alter, it's just CONNECT being very weird. Like you wrote, it doesn't support REPLACE either and we didn't introduce a flag for that. This should be solved inside CONNECT. E.g it can throw an error just like it does for REPLACE. It can upgrade the lock: --- a/storage/connect/ha_connect.cc +++ b/storage/connect/ha_connect.cc @@ -5125,10 +5125,12 @@ @see get_lock_data() in lock.cc */ -THR_LOCK_DATA **ha_connect::store_lock(THD *, +THR_LOCK_DATA **ha_connect::store_lock(THD *thd, THR_LOCK_DATA **to, enum thr_lock_type lock_type) { + if (thd_sql_command(thd) == SQLCOM_ALTER_TABLE && lock_type == TL_READ) + lock_type= TL_READ_NO_INSERT; if (lock_type != TL_IGNORE && lock.type == TL_UNLOCK) lock.type=lock_type; *to++= &lock; or it can implement reading and inserting to the same table :) Either way, I don't think we need to extend the storage engine API for this specific case. Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org On Aug 03, Nikita Malyavin wrote: > revision-id: 44b4a3e64fa (mariadb-11.0.1-190-g44b4a3e64fa) > parent(s): b508107c7a3 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-29 00:13:48 +0400 > message: > > MDEV-31777 ER_GET_ERRNO upon online alter on CONNECT table > > Forbid Online for CONNECT. > > diff --git a/sql/handler.h b/sql/handler.h > index 480c79017ab..4a1363d2e93 100644 > --- a/sql/handler.h > +++ b/sql/handler.h > @@ -4911,6 +4911,8 @@ class handler :public Sql_alloc > bool commit); > > > + virtual bool is_online_alter_supported() const { return true; } > + > protected:

1 0

Re: dbe5c20b755: MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication
by Kristian Nielsen 03 Aug '23

03 Aug '23

Hi Marko, I'd like to ask your opinion/review of the below patch for MDEV-31482: https://github.com/MariaDB/server/commit/dbe5c20b755b87f67d87990c3baabc8666… https://jira.mariadb.org/browse/MDEV-31482 As you know, in-order parallel replication has InnoDB report lock waits. If T1 must replicate before T2, and InnoDB reports that T1 is waiting for a lock held by T2, then T2 will be killed (and retried later) so that T1 can replicate. As an optimisation, auto-increment lock waits where _not_ reported, but this turns out to be a bug. This patch just removes the exception so that such waits are reported like other waits. My only concern is the possible performance impact of this. I'm hoping it will not be very big, because in many cases the auto-increment lock is not taken for replicated transactions? My understanding is: - It is not taken for INSERT INTO t VALUES (...) (and maybe not for multi-row "INSERT ... VALUES (...), (...), ..." ?) - SELECT ... INSERT is not safe for statement-based replication, so will be logged as row in MIXED mode and give a warning in STATEMENT. But I wanted to hear you if you know any case and/or MariaDB version where a significant amount of auto-increment lock waits could be expected in parallel replication? And if so, maybe this fix should be pushed only to later MariaDB versions? - Kristian. Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > revision-id: dbe5c20b755b87f67d87990c3baabc866667e41b (mariadb-10.4.30-2-gdbe5c20b755) > parent(s): a6114df595eeb7aeed4b050c9a3f4640c4320b5f > author: Kristian Nielsen > committer: Kristian Nielsen > timestamp: 2023-07-09 16:45:47 +0200 > message: > > MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > > Remove the exception that InnoDB does not report auto-increment locks waits > to the parallel replication. > > There was an assumption that these waits could not cause conflicts with > in-order parallel replication and thus need not be reported. However, this > assumption is wrong and it is possible to get conflicts that lead to hangs > for the duration of --innodb-lock-wait-timeout. This can be seen with three > transactions: > > 1. T1 is waiting for T3 on an autoinc lock > 2. T2 is waiting for T1 to commit > 3. T3 is waiting on a normal row lock held by T2 > > Here, T3 needs to be deadlock killed on the wait by T1. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > > --- > mysql-test/suite/rpl/r/rpl_parallel_autoinc.result | 95 ++++++++++++++ > mysql-test/suite/rpl/t/rpl_parallel_autoinc.test | 140 +++++++++++++++++++++ > sql/sql_class.cc | 6 - > storage/innobase/lock/lock0lock.cc | 8 +- > 4 files changed, 236 insertions(+), 13 deletions(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result b/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result > new file mode 100644 > index 00000000000..c1829bafa1a > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_parallel_autoinc.result > @@ -0,0 +1,95 @@ > +include/master-slave.inc > +[connection master] > +MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > +include/rpl_connect.inc [creating slave2] > +include/rpl_connect.inc [creating slave3] > +connection master; > +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; > +CREATE TABLE t1 (a INT PRIMARY KEY AUTO_INCREMENT, b INT, c INT, INDEX (c)) ENGINE=InnoDB; > +INSERT INTO t1 (b,c) VALUES (0, 1), (0, 1), (0, 2), (0,3), (0, 5), (0, 7), (0, 8); > +CREATE TABLE t2 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t2 VALUES (10,1), (20,2), (30,3), (40,4), (50,5); > +CREATE TABLE t3 (a VARCHAR(20) PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t3 VALUES ('row for T1', 0), ('row for T2', 0), ('row for T3', 0); > +include/save_master_gtid.inc > +connection slave; > +include/sync_with_master_gtid.inc > +include/stop_slave.inc > +set @@global.slave_parallel_threads= 3; > +set @@global.slave_parallel_mode= OPTIMISTIC; > +set @@global.innodb_lock_wait_timeout= 20; > +connection master; > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T1"; > +INSERT INTO t1(b, c) SELECT 1, t2.b FROM t2 WHERE a=10; > +Warnings: > +Note 1592 Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave > +COMMIT; > +DELETE FROM t1 WHERE c >= 4 and c < 6; > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T3"; > +INSERT INTO t1(b, c) SELECT 3, t2.b FROM t2 WHERE a >= 20 AND a <= 40; > +Warnings: > +Note 1592 Unsafe statement written to the binary log using statement format since BINLOG_FORMAT = STATEMENT. Statements writing to a table with an auto-increment column after selecting from another table are unsafe because the order in which rows are retrieved determines what (if any) rows will be written. This order cannot be predicted and may differ on master and the slave > +COMMIT; > +include/save_master_gtid.inc > +connection slave1; > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T1" FOR UPDATE; > +a b > +row for T1 0 > +connection slave2; > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T3" FOR UPDATE; > +a b > +row for T3 0 > +connection slave3; > +BEGIN; > +DELETE FROM t2 WHERE a=30; > +connection slave; > +include/start_slave.inc > +connection slave2; > +ROLLBACK; > +connection slave1; > +ROLLBACK; > +connection slave3; > +ROLLBACK; > +connection slave; > +include/sync_with_master_gtid.inc > +SELECT * FROM t1 ORDER BY a; > +a b c > +1 0 1 > +2 0 1 > +3 0 2 > +4 0 3 > +6 0 7 > +7 0 8 > +8 1 1 > +9 3 2 > +10 3 3 > +11 3 4 > +SELECT * FROM t2 ORDER BY a; > +a b > +10 1 > +20 2 > +30 3 > +40 4 > +50 5 > +SELECT * FROM t3 ORDER BY a; > +a b > +row for T1 1 > +row for T2 0 > +row for T3 1 > +connection master; > +CALL mtr.add_suppression("Unsafe statement written to the binary log using statement format"); > +DROP TABLE t1, t2, t3; > +connection slave; > +include/stop_slave.inc > +SET @@global.slave_parallel_threads= 0; > +SET @@global.slave_parallel_mode= conservative; > +SET @@global.innodb_lock_wait_timeout= 50; > +include/start_slave.inc > +SELECT @@GLOBAL.innodb_autoinc_lock_mode; > +@@GLOBAL.innodb_autoinc_lock_mode > +1 > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test b/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test > new file mode 100644 > index 00000000000..0e96b4dfb80 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_parallel_autoinc.test > @@ -0,0 +1,140 @@ > +--source include/have_binlog_format_statement.inc > +--source include/have_innodb.inc > +--source include/master-slave.inc > + > +--echo MDEV-31482: Lock wait timeout with INSERT-SELECT, autoinc, and statement-based replication > + > +# The scenario is transactions T1, T2, T3: > +# > +# T1 is waiting for T3 on an autoinc lock > +# T2 is waiting for T1 to commit > +# T3 is waiting on a normal row lock held by T2 > +# > +# This caused a hang until innodb_lock_wait_timeout, because autoinc > +# locks were not reported to the in-order parallel replication, so T3 > +# was not deadlock killed. > + > +--let $lock_wait_timeout=20 > + > +--let $rpl_connection_name= slave2 > +--let $rpl_server_number= 2 > +--source include/rpl_connect.inc > + > +--let $rpl_connection_name= slave3 > +--let $rpl_server_number= 2 > +--source include/rpl_connect.inc > + > +--connection master > +ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; > + > +# A table as destination for INSERT-SELECT > +CREATE TABLE t1 (a INT PRIMARY KEY AUTO_INCREMENT, b INT, c INT, INDEX (c)) ENGINE=InnoDB; > +INSERT INTO t1 (b,c) VALUES (0, 1), (0, 1), (0, 2), (0,3), (0, 5), (0, 7), (0, 8); > + > +# A table as source for INSERT-SELECT. > +CREATE TABLE t2 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t2 VALUES (10,1), (20,2), (30,3), (40,4), (50,5); > + > +# A table to help order slave worker threads to setup the desired scenario. > +CREATE TABLE t3 (a VARCHAR(20) PRIMARY KEY, b INT) ENGINE=InnoDB; > +INSERT INTO t3 VALUES ('row for T1', 0), ('row for T2', 0), ('row for T3', 0); > +--source include/save_master_gtid.inc > + > +--connection slave > +--source include/sync_with_master_gtid.inc > +--source include/stop_slave.inc > +--let $save_innodb_lock_wait_timeout= `SELECT @@global.innodb_lock_wait_timeout` > +--let $save_slave_parallel_threads= `SELECT @@global.slave_parallel_threads` > +--let $save_slave_parallel_mode= `SELECT @@global.slave_parallel_mode` > +set @@global.slave_parallel_threads= 3; > +set @@global.slave_parallel_mode= OPTIMISTIC; > +eval set @@global.innodb_lock_wait_timeout= $lock_wait_timeout; > + > +--connection master > +# Transaction T1. > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T1"; > +INSERT INTO t1(b, c) SELECT 1, t2.b FROM t2 WHERE a=10; > +COMMIT; > + > +# Transaction T2. > +DELETE FROM t1 WHERE c >= 4 and c < 6; > + > +# Transaction T3. > +BEGIN; > +UPDATE t3 SET b=b+1 where a="row for T3"; > +INSERT INTO t1(b, c) SELECT 3, t2.b FROM t2 WHERE a >= 20 AND a <= 40; > +COMMIT; > + > +--source include/save_master_gtid.inc > + > +--connection slave1 > +# Temporarily block T1 to create the scheduling that triggers the bug. > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T1" FOR UPDATE; > + > +--connection slave2 > +# Temporarily block T3 from starting (so T2 can reach commit). > +BEGIN; > +SELECT * FROM t3 WHERE a="row for T3" FOR UPDATE; > + > +--connection slave3 > +# This critical step blocks T3 after it has inserted its first row, > +# and thus taken the auto-increment lock, but before it has reached > +# the point where it gets a row lock wait on T2. Even though > +# auto-increment lock waits were not reported due to the bug, > +# transitive lock waits (T1 waits on autoinc of T3 which waits on row > +# on T2) _were_ reported as T1 waiting on T2, and thus a deadlock kill > +# happened and the bug was not triggered. > +BEGIN; > +DELETE FROM t2 WHERE a=30; > + > +--connection slave > +--source include/start_slave.inc > + > +# First let T2 complete until it is waiting for T1 to commit. > +--let $wait_condition= SELECT count(*)=1 FROM information_schema.processlist WHERE state='Waiting for prior transaction to commit' and command LIKE 'Slave_worker'; > +--source include/wait_condition.inc > + > +# Then let T3 reach the point where it has obtained the autoinc lock, > +# but it is not yet waiting for a row lock held by T2. > +--connection slave2 > +ROLLBACK; > +--let $wait_condition= SELECT count(*)=1 FROM information_schema.processlist WHERE state='Sending data' and info LIKE 'INSERT INTO t1(b, c) SELECT 3, t2.b%' and time_ms > 500 and command LIKE 'Slave_worker'; > +--source include/wait_condition.inc > + > +# Now let T1 continue, while T3 is holding the autoinc lock but before > +# it is waiting for T2. Wait a short while to give the hang a chance to > +# happen; T1 needs to get to request the autoinc lock before we let T3 > +# continue. (There's a small chance the sleep will be too small, which will > +# let the test occasionally pass on non-fixed server). > +--connection slave1 > +ROLLBACK; > +--sleep 0.5 > + > +# Now let T3 continue; the bug was that this lead to an undetected > +# deadlock that remained until innodb lock wait timeout. > +--connection slave3 > +ROLLBACK; > + > +--connection slave > +--let $slave_timeout= `SELECT $lock_wait_timeout/2` > +--source include/sync_with_master_gtid.inc > +--let $slave_timeout= > +SELECT * FROM t1 ORDER BY a; > +SELECT * FROM t2 ORDER BY a; > +SELECT * FROM t3 ORDER BY a; > + > +# Cleanup. > +--connection master > +CALL mtr.add_suppression("Unsafe statement written to the binary log using statement format"); > +DROP TABLE t1, t2, t3; > + > +--connection slave > +--source include/stop_slave.inc > +eval SET @@global.slave_parallel_threads= $save_slave_parallel_threads; > +eval SET @@global.slave_parallel_mode= $save_slave_parallel_mode; > +eval SET @@global.innodb_lock_wait_timeout= $save_innodb_lock_wait_timeout; > +--source include/start_slave.inc > +SELECT @@GLOBAL.innodb_autoinc_lock_mode; > +--source include/rpl_end.inc > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 73bb654080a..8ed3f8a9c5e 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5119,12 +5119,6 @@ thd_need_wait_reports(const MYSQL_THD thd) > deadlock with the pre-determined commit order, we kill the later > transaction, and later re-try it, to resolve the deadlock. > > - This call need only receive reports about waits for locks that will remain > - until the holding transaction commits. InnoDB auto-increment locks, > - for example, are released earlier, and so need not be reported. (Such false > - positives are not harmful, but could lead to unnecessary kill and retry, so > - best avoided). > - > Returns 1 if the OTHER_THD will be killed to resolve deadlock, 0 if not. The > actual kill will happen later, asynchronously from another thread. The > caller does not need to take any actions on the return value if the > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > index 26388ad95e2..b1cf2152cd6 100644 > --- a/storage/innobase/lock/lock0lock.cc > +++ b/storage/innobase/lock/lock0lock.cc > @@ -6872,13 +6872,7 @@ DeadlockChecker::search() > return m_start; > } > > - /* We do not need to report autoinc locks to the upper > - layer. These locks are released before commit, so they > - can not cause deadlocks with binlog-fixed commit > - order. */ > - if (m_report_waiters > - && (lock_get_type_low(lock) != LOCK_TABLE > - || lock_get_mode(lock) != LOCK_AUTO_INC)) { > + if (m_report_waiters) { > thd_rpl_deadlock_check(m_start->mysql_thd, > lock->trx->mysql_thd); > }

2 1

Proposal: mariadbd should automatically upgrade system tables and data directory on startup
by Otto Kekäläinen 03 Aug '23

03 Aug '23

Hi! I frequently see users running into various problems after upgrading MariaDB and starting the upgraded mariadbd server: - Server might refuse to start, asking users to manually run mariadb-upgrade - Server might start, but misbehave as on-disk data format for regular tables or the system tables has not updated - On systems where mariadb-upgrade runs automatically on MariaDB service startup there might be unnecessary delay if mariadb-upgrade is run multiple times - If running mariadb-upgrade fails or is slow, users might end up in a situation where they re-restart the server and have two mariadb-upgrade scripts running and blocking each other - In some cases user might attempt to run mariadb-upgrade but it fails it the MariaDB server was not running, forcing user to figure out how to manually start the server for upgrades - Users might end up starting server in a mode that does not accept connections with the intent to only run mariadb-upgrade, but then don't understand they need to restart the server to have it serve connections again - Users planning upgrades get confused because because reading about Fedora based distros the DBA is requires to manually run mariadb-upgrade themselves, and reading about Debian based distros DBAs are not required to do this as it is automatic, but they might end up trying to run it manually anyway because of prior experience with Fedora Some of the above issues can be remediated by running 'mariadb_upgrade --check-if-upgrade-is-needed' before actually running mariadb_upgrade in service scripts, but it does not work if server is offline (https://jira.mariadb.org/browse/MDEV-27636) Making users aware that their database is running but mariadb-upgrade has not run can be done via https://github.com/MariaDB/server/pull/2464. However this are just duct tape to the original problem. All of the above could be permanently solved if mariadbd simply did the upgrade by itself automatically. Can we please have this? Can we agree that this would be a desired feature in next MariaDB 11.x version, and if somebody implements this, it would be timely reviewed and merged? There is already design outlined at https://jira.mariadb.org/browse/MDEV-30499

8 19

Problem Upgrading Database
by jashley＠deadacated.com 25 Jul '23

25 Jul '23

Greetings, MariaDB 10.11.4 on Server 2022 Upgrading from 10.11.4 to 11.0.2 or 11.1.1 fails on Phase 5/10: Waiting for startup to complete. Is this a known problem? Thank you, mux

3 2

Re: 64ac6ed3e35: MDEV-31742 incorrect examined rows in case of stored function usage
by Sergei Golubchik 21 Jul '23

21 Jul '23

Hi, Oleksandr, On Jul 21, Oleksandr Byelkin wrote: > revision-id: 64ac6ed3e35 (mariadb-10.4.30-31-g64ac6ed3e35) > parent(s): 55a27bac80c > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2023-07-19 16:14:39 +0200 > message: > > MDEV-31742 incorrect examined rows in case of stored function usage > > The counter is global so we do not need add backup to it > if we do not zero it after taking the backup. > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index c9e86b6142c..7911c78b6e0 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5568,7 +5568,6 @@ void THD::restore_sub_statement_state(Sub_statement_state *backup) > The following is added to the old values as we are interested in the > total complexity of the query > */ > - inc_examined_row_count(backup->examined_row_count); may be backup->examined_row_count should be simply removed? Sub_statement_state::examined_row_count, I mean. Not saved, not restored? There's also void THD::add_slow_query_state(Sub_statement_state *backup) { affected_rows+= backup->affected_rows; bytes_sent_old= backup->bytes_sent_old; m_examined_row_count+= backup->examined_row_count; it's not even using inc_examined_row_count(). Also examined_row_count is mentioned in comments in sql_class.cc, if you'll be removing it from Sub_statement_state, don't forget to fix comments to match. > cuted_fields+= backup->cuted_fields; > DBUG_VOID_RETURN; > } Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 2

Re: 08403d85b00: MDEV-31677 Assertion failed upon online ALTER with binlog_row_image=NOBLOB
by Sergei Golubchik 21 Jul '23

21 Jul '23

Hi, Nikita, ok to push after making few small changes, see inline: On Jul 20, Nikita Malyavin wrote: > revision-id: 08403d85b00 (mariadb-11.0.1-147-g08403d85b00) > parent(s): a31b2a52119 > author: Nikita Malyavin > committer: Nikita Malyavin > timestamp: 2023-07-19 19:08:51 +0400 > message: > > MDEV-31677 Assertion failed upon online ALTER with binlog_row_image=NOBLOB > > Make binlog_prepare_row_images accept image type as an argument. > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index d4ad0efbf3a..bb2453af6a0 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -7070,7 +7071,7 @@ int THD::binlog_update_row(TABLE* table, Event_log *bin_log, > not needed for binlogging. This is done according to the: > binlog-row-image option. the comment is outdated, should say This is done according to the row_image argument > */ > - binlog_prepare_row_images(table); > + binlog_prepare_row_images(table, row_image); > > size_t const before_maxlen= max_row_length(table, table->read_set, > before_record); > @@ -7194,7 +7195,7 @@ void THD::binlog_prepare_row_images(TABLE *table) > and the handler involved supports this. > */ > if (table->s->primary_key < MAX_KEY && > - (thd->variables.binlog_row_image < BINLOG_ROW_IMAGE_FULL) && > + row_image < BINLOG_ROW_IMAGE_FULL && > !ha_check_storage_engine_flag(table->s->db_type(), > HTON_NO_BINLOG_ROW_OPT)) > { please also repace thd->variables.binlog_row_image in the switch() below. doesn't make any practical difference, just for clarity Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

2 1