- developers - lists.mariadb.org

Fwd: 2342aaf46eb: MDEV-30973 Implement debug logging for slave_transaction_retries
by Aleksey Midenkov 28 Feb '24

28 Feb '24

Hi Sergei, the commits are in bb-11.4-midenok-MDEV-30973 New changes are in 4 top commits. There is also a small CMake change "Columnstore empty submodule fix" which I would push ASAP. On Tue, Nov 7, 2023 at 8:35 PM Sergei Golubchik <serg(a)mariadb.org> wrote: > > Hi, Aleksey, > > On Nov 07, Aleksey Midenkov wrote: > > revision-id: 2342aaf46eb (mariadb-11.0.1-256-g2342aaf46eb) > > parent(s): 80bc5df1c80 > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2023-10-10 13:11:19 +0300 > > message: > > > > MDEV-30973 Implement debug logging for slave_transaction_retries > > > > Configuration options: > > > > log_slave_retries > > log_slave_retries_max > > 1. > a commit comment of >100 lines that's a bit too much, I think. > > 2. > comments in MDEV-30973 say that one should be able to have everything in > one file, namely in the error log. I don't see how it's possible in your > commit. Fixed. > > 3. > one of the reasons for not simply writing into erorr log was > "you get what are relatively large entries in there which can make it > hard/annoying to also look for the normal, one-line warning and error > output". But as far as I can see your slave retry entries are not large, > but normal one-line messages. I'd say they can fit into the error log > just fine. "large entries" here is not fully correct. "large number of entries" is more correct. > > I've still commented below in the assumption that the separate log > implementation will stay. But I honestly don't see a reason for it > anymore. > > > diff --git a/mysql-test/suite/rpl/t/slave_deadlock.test b/mysql-test/suite/rpl/t/slave_deadlock.test > > new file mode 100644 > > index 00000000000..9fcda7c9b71 > > --- /dev/null > > +++ b/mysql-test/suite/rpl/t/slave_deadlock.test > > @@ -0,0 +1,194 @@ > > +--source include/have_debug.inc > > +--source include/have_innodb.inc > > +--source include/have_binlog_format_statement.inc > > + > > +# On --repeat test infinitely increments thread_id and finally fails --sync_with_master at log_pos 1754: > > +# > > +# 231009 20:58:15 server id 1 end_log_pos 1754 CRC32 0x7e5aecd1 Query thread_id=561 exec_time=0 error_code=0 xid=0 > > I'm sorry, I don't understand that. Could you elaborate please? I don't know, I've just shown what I observed. Some replication bug? I tried to retest and couldn't reproduce. Removed. > > > +# > > +# force_restart seems to fix this > > + > > +--source include/force_restart.inc > > + > > +# Note that test requires big enough --debug-sync-timeout unfair scheduling at high load. > > Sorry, cannot parse that. Could you elaborate please? I missed *due to* unfair scheduling at high load. That is: thread scheduling latency may reach up to 1 hour. It is executed on VM and actually VM is not always good for concurrency testing (this is quite an omission in our CI system). > > > +# slave_deadlock.opt sets this to 1 hour > > + > > +--let $rpl_topology=1->2 > > +--source include/rpl_init.inc > > + > > +--echo # > > +--echo # MDEV-30973 Enhance output of innodb deadlocks (SAMU-124, SAMU-131) > > +--echo # > > +--connection server_2 > > +SET @old_dbug= @@global.debug_dbug; > > +set @old_parallel_threads= @@global.slave_parallel_threads; > > +set @old_domain_parallel_threads= @@global.slave_domain_parallel_threads; > > +set @old_parallel_mode= @@global.slave_parallel_mode; > > +set @old_slave_transaction_retries= @@global.slave_transaction_retries; > > +set @old_innodb_lock_wait_timeout= @@global.innodb_lock_wait_timeout; > > +set @old_log_slave_retries= @@global.log_slave_retries; > > +set @old_log_slave_retries_file= @@global.log_slave_retries_file; > > +set @old_log_slave_retries_max= @@global.log_slave_retries_max; > > All this makes little sense, it's simpler to put all settings into the .opt > file > > generally it's better to do your way, to have less > server restarts, but you have an .opt file already, you can as well > use it. > > except if you decide to remove the .opt file - then nevermind, the > above it good. I'm taking it for future. > > > +--source include/stop_slave.inc > > +change master to master_use_gtid= slave_pos; > > +set global slave_parallel_threads= 10; > > +set global slave_domain_parallel_threads= 1; > > +set global slave_parallel_mode= optimistic; > > +set global slave_transaction_retries= 4; > > +set global innodb_lock_wait_timeout= 1; > ... > > diff --git a/mysql-test/suite/versioning/r/rpl_row.result b/mysql-test/suite/versioning/r/rpl_row.result > > index c161f68d0bd..6343fbecdec 100644 > > --- a/mysql-test/suite/versioning/r/rpl_row.result > > +++ b/mysql-test/suite/versioning/r/rpl_row.result > > @@ -37,12 +37,7 @@ select *, unix_timestamp(row_start) as row_start, unix_timestamp(row_end) as row > > x y row_start row_end > > 1 1 12345.000000 2147483647.999999 > > set timestamp= default; > > -### INSERT INTO `test`.`t1` > > -### SET > > -### @1=1 > > -### @2=1 > > -### @3=12345.000000 > > -### @4=2147483647.999999 > > why? That was a bug in grep.inc (fixed). I guess this commit was temporary which I forgot to deal with. > > > +/*!50530 SET @@SESSION.PSEUDO_SLAVE_MODE=1*/; > > connection slave; > > select * from t1; > > x y > > diff --git a/sql/log.cc b/sql/log.cc > > index 877315c60de..78e874fda5b 100644 > > --- a/sql/log.cc > > +++ b/sql/log.cc > > @@ -1188,9 +1188,17 @@ bool Log_to_file_event_handler:: > > log_error(enum loglevel level, const char *format, > > va_list args) > > { > > - return vprint_msg_to_log(level, format, args); > > + return vprint_msg_to_log(stderr, level, format, args); > > } > > > > +#ifdef HAVE_REPLICATION > > +bool Log_to_file_event_handler::log_slave_retry(const char *format, va_list args) > > +{ > > + DBUG_ASSERT(slave_retries_file.get()); > > + return vprint_msg_to_log(slave_retries_file.get(), NO_LEVEL, format, args); > > I don't think you should use vprint_msg_to_log and add a NO_LEVEL hack. > vprint_msg_to_log also prints into the windows event log, into > wsrep json log, and it locks LOCK_error_log. You don't need any of that, > just create your own function, it's just one fprintf() anyway. I still think formatting log message in single function is a good idea. Any other above deficiencies were fixed. > > > +} > > +#endif > > + > > void Log_to_file_event_handler::init_pthread_objects() > > { > > mysql_log.init_pthread_objects(); > > @@ -9752,13 +9787,15 @@ static void print_buffer_to_file(enum loglevel level, const char *buffer, > > } > > } > > > > +#ifndef EMBEDDED_LIBRARY > > mysql_mutex_lock(&LOCK_error_log); > > +#endif > > > > skr= my_time(0); > > localtime_r(&skr, &tm_tmp); > > start=&tm_tmp; > > > > - fprintf(stderr, "%d-%02d-%02d %2d:%02d:%02d %lu [%s] %.*s%.*s\n", > > + fprintf(file, "%d-%02d-%02d %2d:%02d:%02d [T%lu] %s%.*s%.*s\n", > > eh... no. don't change the format of existing log files, please. This change is because of a serious deficiency in logging format: you cannot grep by thread number (with simple expression) and grepping by thread number is important. I understand this may cause incompatibilities but I hope the drawbacks are less than the advantage. What if we keep it? > > > start->tm_year + 1900, > > start->tm_mon+1, > > start->tm_mday, > > @@ -9834,6 +9872,22 @@ void sql_print_error(const char *format, ...) > > } > > > > > > +#ifdef HAVE_REPLICATION > > +void sql_print_error2(const char *format, ...) > > s/sql_print_error2/print_slave_error/ ? Fixed (slave_print_error). > > > +{ > > + va_list args; > > + DBUG_ENTER("sql_print_error"); > > + > > + va_start(args, format); > > + error_log_print(ERROR_LEVEL, format, args); > > + slave_retries_print(format, args); > > + va_end(args); > > + > > + DBUG_VOID_RETURN; > > +} > > +#endif > > + > > + > > void sql_print_warning(const char *format, ...) > > { > > va_list args; > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > > index 8cc3ad51ee9..4f6b2273597 100644 > > --- a/sql/mysqld.cc > > +++ b/sql/mysqld.cc > > @@ -765,6 +765,90 @@ char *opt_relay_logname = 0, *opt_relaylog_index_name=0; > > char *opt_logname, *opt_slow_logname, *opt_bin_logname; > > char *opt_binlog_index_name=0; > > > > +#ifdef HAVE_REPLICATION > > +char default_slave_retries_path[FN_REFLEN]; > > +bool opt_slave_retries= false; > > +char *opt_slave_retries_path= NULL; > > +uint opt_slave_retries_max_log= 0; > > +#endif > > +Slave_retries_file slave_retries_file; > > + > > +bool Slave_retries_file::open() > > +{ > > +#ifdef HAVE_REPLICATION > > + DBUG_ASSERT(opt_slave_retries); > > + DBUG_ASSERT(opt_slave_retries_path); > > + file= fopen(opt_slave_retries_path, "a"); > > my_fopen or my_open > > and anyway, why a new class, why not to use MYSQL_LOG like general and slow > logs do? I rewrote the code via MYSQL_LOG. > > > + if (!file) > > + { > > + sql_print_error("Open of log_slave_retries '%s' failed: %s (%d)", > > + opt_slave_retries_path, strerror(errno), errno); > > This is inconsistent, the server doesn't print this for other log files. > > I've just tried with general log and got > > 2023-11-06 17:58:19 0 [ERROR] mysqld: File '/tmp' not found (Errcode: 21 "Is a directory") > 2023-11-06 17:58:19 0 [ERROR] Could not use /tmp for logging (error 21). Turning logging off for the whole duration of the MariaDB server process. To turn it on again: fix the cause, shutdown the MariaDB server and restart it. > > Please, do the same Looks like wrong behavior: you supplied wrong arguments and the server was started anyway! What is the justification for this? It is easy to miss: if you don't read the error log, you don't even know something is wrong. And if you got lucky to notice that, the server is already up and running (and loaded with clients). Anyway since my logging now works like general log it now works the same. > > > + return true; > > + } > > + else > > + { > > + slave_retries_print("%s (mariadbd %s) starting as process %lu ...", > > + my_progname, server_version, (ulong) getpid()); > > Any reason, why it shouldn't be the same as in the error log? > > sql_print_information("Starting MariaDB %s source revision %s as process %lu", > server_version, SOURCE_REVISION, (ulong) getpid()); > I think it would be nice to add my_progname into the message. Made it now like in the original code. > > + } > > +#endif > > + return false; > > +} > > + > > +void Slave_retries_file::close() > > +{ > > +#ifdef HAVE_REPLICATION > > + if (file) > > + { > > + acquire(); > > + fclose(file); > > + release(); > > + } > > + file= NULL; > > +#endif > > +} > > + > > +void Slave_retries_file::init() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_init(key_LOCK_global_system_variables, &mutex, MY_MUTEX_INIT_FAST); > > +#endif > > +} > > + > > +void Slave_retries_file::destroy() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_destroy(&mutex); > > +#endif > > +} > > + > > +bool Slave_retries_file::acquire() > > +{ > > +#ifdef HAVE_REPLICATION > > + if (!file) > > + return false; > > + mysql_mutex_lock(&mutex); > > + return true; > > +#else > > + return false; > > +#endif > > +} > > + > > +void Slave_retries_file::release() > > +{ > > +#ifdef HAVE_REPLICATION > > + mysql_mutex_unlock(&mutex); > > +#endif > > +} > > + > > +void Slave_retries_file::flush() > > +{ > > + if (acquire()) > > + { > > + fflush(file); > > + release(); > > + } > > +} > > + > > /* Static variables */ > > > > my_bool opt_stack_trace; > > @@ -5055,6 +5145,35 @@ static int init_server_components() > > "this server. However this will be ignored as the " > > "--log-bin option is not defined."); > > } > > + > > + slave_retries_file.init(); > > + if (!log_error_file_ptr[0]) > > + fn_format(default_slave_retries_path, pidfile_name, mysql_data_home, > > + "-retries.err", MY_REPLACE_EXT); > > + else > > + fn_format(default_slave_retries_path, log_error_file_ptr, mysql_data_home, > > + "-retries.err", MY_REPLACE_EXT); > > that's not how all other log names work, > grep mysqld.cc for opt_slow_logname Now it works like other logs. > > > + if (opt_slave_retries_path && !opt_abort) > > + { > > + if (!opt_slave_retries_path[0]) > > + { > > + if (!default_slave_retries_path[0]) > > + { > > + /* fn_format failed */ > > + opt_slave_retries_path= NULL; > > + opt_slave_retries= false; > > + } > > + else > > + { > > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > > + default_slave_retries_path, MYF(0)); > > + } > > + } > > + > > + if (opt_slave_retries && > > + slave_retries_file.open()) > > + unireg_abort(1); > > + } > > #endif > > > > if (opt_bin_log) > > diff --git a/sql/mysqld.h b/sql/mysqld.h > > index 69cfc0cad56..5f1561ca10d 100644 > > --- a/sql/mysqld.h > > +++ b/sql/mysqld.h > > @@ -822,6 +848,8 @@ enum options_mysqld > > OPT_KEY_CACHE_CHANGED_BLOCKS_HASH_SIZE, > > OPT_LOG_BASENAME, > > OPT_LOG_ERROR, > > + OPT_SLAVE_RETRIES, > > + OPT_SLAVE_RETRIES_LOG, > > why do you need them? Removed. > > > OPT_LOWER_CASE_TABLE_NAMES, > > OPT_PLUGIN_LOAD, > > OPT_PLUGIN_LOAD_ADD, > > diff --git a/sql/sql_show.cc b/sql/sql_show.cc > > index d742bc6c1c3..c53ca0f0097 100644 > > --- a/sql/sql_show.cc > > +++ b/sql/sql_show.cc > > @@ -10662,6 +10663,15 @@ char *thd_get_error_context_description(THD *thd, char *buffer, > > /* Don't wait if LOCK_thd_data is used as this could cause a deadlock */ > > if (!mysql_mutex_trylock(&thd->LOCK_thd_data)) > > { > > + if (thd->rgi_slave) > > is rgi_slave and its content protected by thd->LOCK_thd_data ? AFAICS they are per-thread and don't require protection. > > > + { > > + rpl_group_info *rgi= thd->rgi_slave; > > + len= my_snprintf(header, sizeof(header), > > + ", GTID %u-%u-%llu", rgi->current_gtid.domain_id, > > + rgi->current_gtid.server_id, rgi->current_gtid.seq_no); > > + str.append(header, len); > > + } > > + > > if (const char *info= thread_state_info(thd)) > > { > > str.append(' '); > > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > > index ebb64b340b0..11c321f06dc 100644 > > --- a/sql/sys_vars.cc > > +++ b/sql/sys_vars.cc > > @@ -5862,6 +5862,61 @@ Sys_slave_trans_retry_interval( > > "slave_transaction_retry_errors", > > GLOBAL_VAR(slave_trans_retry_interval), CMD_LINE(REQUIRED_ARG), > > VALID_RANGE(0, 3600), DEFAULT(0), BLOCK_SIZE(1)); > > + > > +static bool update_slave_retries_log(sys_var *self, THD* thd, enum_var_type type) > > +{ > > + if (opt_slave_retries) > > + { > > + if (!slave_retries_file.get() && slave_retries_file.open()) > > + { > > + my_error(ER_CANT_OPEN_FILE, MYF(0), opt_slave_retries_path, errno); > > + opt_slave_retries= false; > > + return true; > > + } > > + } > > + else > > + slave_retries_file.close(); > > + return false; > > +} > > + > > +static Sys_var_mybool Sys_slave_retries_log( > > + "log_slave_retries", "Log transaction retries", > > + GLOBAL_VAR(opt_slave_retries), > > + CMD_LINE(OPT_ARG, OPT_SLAVE_RETRIES), DEFAULT(false), > > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), ON_UPDATE(update_slave_retries_log)); > > + > > + > > +static bool check_slave_retries_file(sys_var *self, THD *thd, set_var *var) > > +{ > > + if (opt_slave_retries) > > + { > > + my_error(ER_NOT_SUPPORTED_YET, MYF(0), "modifying while log_slave_retries is ON"); > > as always, please don't hard-code error messages. > you can use > > my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "log_slave_retries=ON"); Fixed. > > > + return true; > > + } > > + return false; > > +} > > + > > +static bool update_slave_retries_file(sys_var *self, THD* thd, enum_var_type type) > > +{ > > + if (!opt_slave_retries_path) > > + opt_slave_retries_path= my_strdup(key_memory_Sys_var_charptr_value, > > + default_slave_retries_path, MYF(0)); > > sys_var.cc can handle that for you, see PREALLOCATED > in e.g. opt_slow_logname Yes. > > also, you don't reopen the log when its name changes, how comes? It reopens. > > > + return false; > > +} > > + > > +static Sys_var_charptr Sys_slave_retries_log_path( > > + "log_slave_retries_file", "Log file path for transaction retries", > > + GLOBAL_VAR(opt_slave_retries_path), > > + CMD_LINE(REQUIRED_ARG, OPT_SLAVE_RETRIES_LOG), DEFAULT(NULL), > > + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(check_slave_retries_file), > > + ON_UPDATE(update_slave_retries_file)); > > + > > + > > +static Sys_var_uint Sys_slave_retries_log_max( > > + "log_slave_retries_max", "Log max N transaction retries " > > + "(default 0 means infinity)", > > just "(0 means infinity)". Default is printed separately > Fixed. > > + GLOBAL_VAR(opt_slave_retries_max_log), CMD_LINE(REQUIRED_ARG), > > + VALID_RANGE(0, UINT_MAX), DEFAULT(0), BLOCK_SIZE(1)); > > #endif > > > > static bool check_locale(sys_var *self, THD *thd, set_var *var) > > diff --git a/storage/innobase/lock/lock0lock.cc b/storage/innobase/lock/lock0lock.cc > > index 0e9cef669d4..8a8eeaf39f6 100644 > > --- a/storage/innobase/lock/lock0lock.cc > > +++ b/storage/innobase/lock/lock0lock.cc > > @@ -49,6 +49,7 @@ Created 5/7/1996 Heikki Tuuri > > #include <debug_sync.h> > > > > #include <set> > > +#include <stdio.h> > > I presume Marko has reviewed (or will review) InnoDB changes I didn't receive his explicit confirmation. > > > > > #ifdef WITH_WSREP > > #include <mysql/service_wsrep.h> > > diff --git a/mysql-test/main/mysqld--help.result b/mysql-test/main/mysqld--help.result > > index 77d6d10b3e6..274c034d455 100644 > > --- a/mysql-test/main/mysqld--help.result > > +++ b/mysql-test/main/mysqld--help.result > > @@ -1673,6 +1678,9 @@ log-isam myisam.log > > log-output FILE > > log-queries-not-using-indexes FALSE > > log-short-format FALSE > > +log-slave-retries FALSE > > +log-slave-retries-file (No default value) > > this is another sign that log-slave-retries-file doesn't behave like > other log filename variables. > > > +log-slave-retries-max 0 > > log-slave-updates FALSE > > log-slow-admin-statements TRUE > > log-slow-disabled-statements sp > > Regards, > Sergei > Chief Architect, MariaDB Server > and security(a)mariadb.org Thanks! -- @midenok

1 0

Re: f22662c36b1: MDEV-33442 REPAIR TABLE corrupts UUIDs
by Sergei Golubchik 26 Feb '24

26 Feb '24

Hi, Alexander, On Feb 24, Alexander Barkov wrote: > revision-id: f22662c36b1 (mariadb-10.11.7-6-gf22662c36b1) > parent(s): 656f8867720 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-02-15 18:11:03 +0400 > message: > > MDEV-33442 REPAIR TABLE corrupts UUIDs Looks quite good. A question: you removed a hard-coded check for MYSQL_TYPE_NEWDECIMAL, but I don't see that you added Type_handler_olddecimal::type_handler_for_implicit_upgrade() A suggestion, rename Column_definition_implicit_upgrade(). You've changed its semantics, it's good to make sure that the code using old Column_definition_implicit_upgrade() (added e.g. in 10.6 and merged up) won't compile. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 2

Re: 7ff9e91a7d2: MDEV-32975 Default charset doesn't work with PHP MySQLi extension
by Sergei Golubchik 26 Feb '24

26 Feb '24

Hi, Alexander, the fix is good, thanks. a couple of notes about comments, see below. ok to push after fixing them On Feb 25, Alexander Barkov wrote: > revision-id: 7ff9e91a7d2 (mariadb-10.4.32-179-g7ff9e91a7d2) > parent(s): 1070575a890 > author: Alexander Barkov > committer: Alexander Barkov > timestamp: 2024-01-26 13:12:03 +0400 > message: > > MDEV-32975 Default charset doesn't work with PHP MySQLi extension > > When sending the server default collation ID to the client > in the handshake packet, translate a 2-byte collation ID > to the ID of the default collation for the character set. > > diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc > index 1949a0c02b6..5103ff02105 100644 > --- a/sql/sql_acl.cc > +++ b/sql/sql_acl.cc > @@ -12831,7 +12831,32 @@ static bool send_server_handshake_packet(MPVIO_EXT *mpvio, > > int2store(end, thd->client_capabilities); > /* write server characteristics: up to 16 bytes allowed */ may be move this comment to be directly above end[2]=... line, as before? Otherwise they're separated by another comment and an if() block, so the comment looks out of place > - end[2]= (char) default_charset_info->number; > + > + /* > + Send the default server collation ID. > + Note, since MySQL-4.1 it's actually the > + client responsibility to send its character set to the server, > + so this value means nothing and is ignored by most connectors. > + But some rare connectors (e.g. PHP) still read it. where did you get it from? protocol description doesn't say that the charset field in the handshake packet is obsolete and means nothing. better to remove this claim. If you want to obsolete this field, don't do it in an obscure comment in the code, but change the protocol description in KB. > + */ > + CHARSET_INFO *handshake_cs= default_charset_info; > + if (handshake_cs->number > 0xFF) > + { > + /* > + A workaround for a 2-byte collation ID: translate it into > + the ID of the primary collation of this character set. > + */ > + CHARSET_INFO *cs= get_charset_by_csname(handshake_cs->csname, > + MY_CS_PRIMARY, MYF(MY_WME)); > + /* > + cs should not normally be NULL, however it may be possible > + with a dynamic character set incorrectly defined in Index.xml. > + For safety let's fallback to latin1 in case cs is NULL. > + */ > + handshake_cs= cs ? cs : &my_charset_latin1; > + } > + end[2]= (char) handshake_cs->number; > + > int2store(end+3, mpvio->auth_info.thd->server_status); > int2store(end+5, thd->client_capabilities >> 16); > end[7]= data_len; > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 367c87a34f8: Optimize performance of my_bitmap
by Sergei Golubchik 24 Feb '24

24 Feb '24

Hi, Monty, On Feb 23, Michael Widenius wrote: > commit 367c87a34f8 > Author: Michael Widenius <monty(a)mariadb.org> > Date: Sun Feb 18 17:30:01 2024 +0200 > > diff --git a/include/my_bitmap.h b/include/my_bitmap.h > --- a/include/my_bitmap.h > +++ b/include/my_bitmap.h > @@ -81,14 +79,15 @@ extern void bitmap_union(MY_BITMAP *map, const MY_BITMAP *map2); > extern void bitmap_xor(MY_BITMAP *map, const MY_BITMAP *map2); > extern void bitmap_invert(MY_BITMAP *map); > extern void bitmap_copy(MY_BITMAP *map, const MY_BITMAP *map2); > +extern void bitmap_copy_data(MY_BITMAP *map, const uchar *ptr, uint bits); Why? In all cases where you use it, the source is a bitmap too. you can use a function that copies MY_BITMAP to MY_BITMAP. > > extern uint bitmap_lock_set_next(MY_BITMAP *map); > extern void bitmap_lock_clear_bit(MY_BITMAP *map, uint bitmap_bit); > /* Fast, not thread safe, bitmap functions */ > -#define bitmap_buffer_size(bits) (((bits)+31)/32)*4 > +#define bitmap_buffer_size(bits) (((bits)+63)/64)*8 may be MY_ALIGN(bits, sizeof(my_bitmap_map)*8)*sizeof(my_bitmap_map) ? and the same below? less error-prone or, even better #define my_bitmap_map_bytes sizeof(my_bitmap_map) #define my_bitmap_map_bits (my_bitmap_map_bytes*8) #define bitmap_buffer_size(bits) MY_ALIGN(bits,my_bitmap_map_bits)*my_bitmap_map_bytes > #define no_bytes_in_map(map) (((map)->n_bits + 7)/8) > -#define no_words_in_map(map) (((map)->n_bits + 31)/32) > -#define bytes_word_aligned(bytes) (4*((bytes + 3)/4)) > +#define no_words_in_map(map) (((map)->n_bits + 63)/64) > +#define bytes_word_aligned(bytes) (8*((bytes + 7)/8)) > /* The following functions must be compatible with create_last_word_mask()! */ > static inline void > bitmap_set_bit(MY_BITMAP *map,uint bit) > diff --git a/mysys/my_bitmap.c b/mysys/my_bitmap.c > --- a/mysys/my_bitmap.c > +++ b/mysys/my_bitmap.c > @@ -31,95 +42,82 @@ > New version written and test program added and some changes to the interface > was made by Mikael Ronstrom 2005, with assistance of Tomas Ulin and Mats > Kindahl. > + Updated to 64 bits and use my_find_first_bit() to speed up > + bitmap_get_next_set() by Monty in 2024 > */ > > #include "mysys_priv.h" > #include <my_bitmap.h> > #include <m_string.h> > #include <my_bit.h> > +#include <my_byteorder.h> > + > + > +/* Defines to check bitmaps */ > + > +#define DBUG_ASSERT_BITMAP(M) \ > + DBUG_ASSERT((M)->bitmap); \ > + DBUG_ASSERT((M)->n_bits > 0); \ > + DBUG_ASSERT((M)->last_word_ptr == (M)->bitmap + no_words_in_map(M)-1); \ > + DBUG_ASSERT((*(M)->last_word_ptr & (M)->last_word_mask) == 0); > + > +#define DBUG_ASSERT_BITMAP_AND_BIT(M,B) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT((B) < (M)->n_bits); > + > +#define DBUG_ASSERT_DIFFERENT_BITMAPS(M,N) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT_BITMAP(N); > + > +#define DBUG_ASSERT_IDENTICAL_BITMAPS(M,N) \ > + DBUG_ASSERT_BITMAP(M); \ > + DBUG_ASSERT_BITMAP(N); \ > + DBUG_ASSERT((M)->n_bits == (N)->n_bits); > > /* > - Create a mask with the upper 'unused' bits set and the lower 'used' > - bits clear. The bits within each byte is stored in big-endian order. > + Create a mask for the usable bits on the LAST my_bitmap_map position for > + a bitmap with 'bits' number of bits. > + > + The lowest 'bits' bits are set to zero and the rest bits are set to 1. > + For (bits & 63) == 0 , 0 is returned as in this case all bits in > + the my_bitmap_position are significant > + > + This code assumes the underlying storage is a 64 bit ulonglong. doesn't have to be if you'll use my_bitmap_map_bits > */ > > -static inline uchar invers_last_byte_mask(uint bits) > -{ > - return last_byte_mask(bits) ^ 255; > -} > +static inline my_bitmap_map last_word_mask(uint bits) > +{ > + uint bits_in_last_map= (bits & 63); > + return bits_in_last_map ? ~((1ULL << bits_in_last_map)-1) : 0ULL; you sure a condition is faster than a linear code of few bit operations? > +} > > > +static inline my_bitmap_map first_word_mask(uint bits) > +{ > + uint bits_in_last_map= (bits & 63); > + return ~((1ULL << bits_in_last_map)-1); and no condition here. strange. and shouldn't one of these two functions be inverted? supposedly one masks all bits before the first and the other - all bits after the last. > +} > -void create_last_word_mask(MY_BITMAP *map) > -{ > - unsigned char const mask= invers_last_byte_mask(map->n_bits); > - > - /* > - The first bytes are to be set to zero since they represent real bits > - in the bitvector. The last bytes are set to 0xFF since they represent > - bytes not used by the bitvector. Finally the last byte contains bits > - as set by the mask above. > - */ > - unsigned char *ptr= (unsigned char*)&map->last_word_mask; > - > - map->last_word_ptr= map->bitmap + no_words_in_map(map)-1; > - switch (no_bytes_in_map(map) & 3) { > - case 1: > - map->last_word_mask= ~0U; > - ptr[0]= mask; > - return; > - case 2: > - map->last_word_mask= ~0U; > - ptr[0]= 0; > - ptr[1]= mask; > - return; > - case 3: > - map->last_word_mask= 0U; > - ptr[2]= mask; > - ptr[3]= 0xFFU; > - return; > - case 0: > - map->last_word_mask= 0U; > - ptr[3]= mask; > - return; > - } > -} > > > -static inline my_bitmap_map last_word_mask(uint bit) > -{ > - my_bitmap_map last_word_mask; > - uint n_bits= bit + 1; > - unsigned char const mask= invers_last_byte_mask(n_bits); > - > - /* > - The first bytes are to be set to zero since they represent real bits > - in the bitvector. The last bytes are set to 0xFF since they represent > - bytes not used by the bitvector. Finally the last byte contains bits > - as set by the mask above. > - */ > - unsigned char *ptr= (unsigned char*)&last_word_mask; > - > - switch ((n_bits + 7)/8 & 3) { > - case 1: > - last_word_mask= ~0U; > - ptr[0]= mask; > - break; > - case 2: > - last_word_mask= ~0U; > - ptr[0]= 0; > - ptr[1]= mask; > - break; > - case 3: > - last_word_mask= 0U; > - ptr[2]= mask; > - ptr[3]= 0xFFU; > - break; > - case 0: > - last_word_mask= 0U; > - ptr[3]= mask; > - break; > - } > - return last_word_mask; > -} > +/* > + Update the bitmap's last_word_ptr and last_word_mask > + Also ensure that the last world is all zero to make it > + easy to find the next set bit. > + > + Note that if n_bits is 0, then last_word_ptr will point to > + bitmap (safetly). The bitmap will not be usable for almost any operation. ether "safety" or "safely" > +*/ > + > +void create_last_word_mask(MY_BITMAP *map) > +{ > + my_bitmap_map mask= last_word_mask(map->n_bits); > + map->last_word_mask= mask; > + map->last_word_ptr= map->bitmap + MY_MAX(no_words_in_map(map),1) -1; > + if (map->n_bits > 0) > + { > + *map->last_word_ptr&= ~mask; /* Set not used bits to 0 */ > + DBUG_ASSERT_BITMAP(map); > + } > +} > > > @@ -326,37 +312,45 @@ void bitmap_set_prefix(MY_BITMAP *map, uint prefix_size) > } > if ((d= no_bytes_in_map(map)-prefix_bytes)) > memset(m, 0, d); > + DBUG_ASSERT_BITMAP(map); > } > > > +/** > + Check if bitmap is a bitmap of prefix bits set in the beginning > + > + @param map bitmap > + @param prefix_size number of bits that should be set. 0 is allowed. > + > + @return 1 Yes, prefix bits where set or prefix_size == 0. > + @return 0 No > +*/ > + > my_bool bitmap_is_prefix(const MY_BITMAP *map, uint prefix_size) > { > uint prefix_mask= last_byte_mask(prefix_size); > uchar *m= (uchar*) map->bitmap; > uchar *end_prefix= m+(prefix_size-1)/8; > uchar *end; > - DBUG_ASSERT(m); > - DBUG_ASSERT(prefix_size <= map->n_bits); > > /* Empty prefix is always true */ > if (!prefix_size) > return 1; > > + DBUG_ASSERT_BITMAP_AND_BIT(map, prefix_size-1); > + > while (m < end_prefix) > if (*m++ != 0xff) > return 0; > > - end= ((uchar*) map->bitmap) + no_bytes_in_map(map) - 1; > - if (m == end) > - return ((*m & last_byte_mask(map->n_bits)) == prefix_mask); > - > + end= ((uchar*) map->last_word_ptr) + sizeof(*map->last_word_ptr)-1; good, here you use sizeof(), not just 7. > if (*m != prefix_mask) > return 0; > > - while (++m < end) > + while (++m <= end) > if (*m != 0) > return 0; > - return ((*m & last_byte_mask(map->n_bits)) == 0); > + return 1; > } > > > @@ -447,50 +442,51 @@ void bitmap_intersect(MY_BITMAP *map, const MY_BITMAP *map2) > > /* > Check if there is some bit index between start_bit and end_bit, such that > - this is bit is set for all bitmaps in bitmap_list. > + this is atleast on bit that set for all bitmaps in bitmap_list. "at least" and "one" and "that is set" ? > > SYNOPSIS > bitmap_exists_intersection() > bitmpap_array [in] a set of MY_BITMAPs > - bitmap_count [in] number of elements in bitmpap_array > + bitmap_count [in] number of elements in bitmap_array > start_bit [in] beginning (inclusive) of the range of bits to search > end_bit [in] end (inclusive) of the range of bits to search, must be > no bigger than the bits of the shortest bitmap. > > - NOTES > - This function assumes that for at least one of the bitmaps in bitmap_array all > - bits outside the range [start_bit, end_bit] are 0. As a result is not > - necessary to take care of the bits outside the range [start_bit, end_bit]. > - > RETURN > TRUE if an intersecion exists > FALSE no intersection > */ > > -my_bool bitmap_exists_intersection(const MY_BITMAP **bitmap_array, > +my_bool bitmap_exists_intersection(MY_BITMAP **bitmap_array, > uint bitmap_count, > uint start_bit, uint end_bit) > { > uint i, j, start_idx, end_idx; > - my_bitmap_map cur_res; > + my_bitmap_map cur_res, first_map; > > DBUG_ASSERT(bitmap_count); > DBUG_ASSERT(end_bit >= start_bit); > for (j= 0; j < bitmap_count; j++) > - DBUG_ASSERT(end_bit < bitmap_array[j]->n_bits); > + { > + DBUG_ASSERT_BITMAP_AND_BIT(bitmap_array[j], end_bit); > + } > > start_idx= start_bit/8/sizeof(my_bitmap_map); > end_idx= end_bit/8/sizeof(my_bitmap_map); > > + first_map= first_word_mask(start_bit); > + cur_res= first_map; > for (i= start_idx; i < end_idx; i++) > { > - cur_res= ~0; > for (j= 0; cur_res && j < bitmap_count; j++) > cur_res &= bitmap_array[j]->bitmap[i]; > if (cur_res) > return TRUE; > + cur_res= ~(my_bitmap_map) 0; > } > - cur_res= ~last_word_mask(end_bit); > + cur_res= ~last_word_mask(end_bit+1); > + if (start_idx == end_idx) > + cur_res&= first_map; > for (j= 0; cur_res && j < bitmap_count; j++) > cur_res &= bitmap_array[j]->bitmap[end_idx]; > return cur_res != 0; > @@ -501,21 +497,26 @@ my_bool bitmap_exists_intersection(const MY_BITMAP **bitmap_array, > > my_bool bitmap_union_is_set_all(const MY_BITMAP *map1, const MY_BITMAP *map2) > { > - my_bitmap_map *m1= map1->bitmap, *m2= map2->bitmap, *end; > + my_bitmap_map *m1= map1->bitmap, *m2= map2->bitmap, *end= map1->last_word_ptr; > + DBUG_ASSERT_IDENTICAL_BITMAPS(map1,map2); > > - DBUG_ASSERT(map1->bitmap); > - DBUG_ASSERT(map2->bitmap); > - DBUG_ASSERT(map1->n_bits==map2->n_bits); > - end= map1->last_word_ptr; > while ( m1 < end) > - if ((*m1++ | *m2++) != 0xFFFFFFFF) > + if ((*m1++ | *m2++) != 0xFFFFFFFFFFFFFFFFULL) better ~(my_bitmap_map)0 > return FALSE; > /* here both maps have the same number of bits - see assert above */ > - return ((*m1 | *m2 | map1->last_word_mask) != 0xFFFFFFFF); > + return ((*m1 | *m2 | map1->last_word_mask) != 0xFFFFFFFFFFFFFFFFULL); > } > > > > +#ifdef NOT_USED > + > +/* > + If we re-introduce this one one, we should change the interface to > + use_from_bit and not from_byte. We should also add testing for this > + in bitmap-t.c > +*/ just remove it, it's easier to write it from scratch again when needed then to undertand and fix this old dead code > + > /* > Set/clear all bits above a bit. > > @@ -603,45 +601,51 @@ uint bitmap_bits_set(const MY_BITMAP *map) > my_bitmap_map *data_ptr= map->bitmap; > my_bitmap_map *end= map->last_word_ptr; > uint res= 0; > - DBUG_ASSERT(map->bitmap); > + DBUG_ASSERT_BITMAP(map); > > - for (; data_ptr < end; data_ptr++) > - res+= my_count_bits_uint32(*data_ptr); > + for (; data_ptr <= end; data_ptr++) > + res+= my_count_bits(*data_ptr); > > - /*Reset last bits to zero*/ > - res+= my_count_bits_uint32(*map->last_word_ptr & ~map->last_word_mask); > return res; > } > > void bitmap_copy(MY_BITMAP *map, const MY_BITMAP *map2) > { > - my_bitmap_map *to= map->bitmap, *from= map2->bitmap, *end; > - > - DBUG_ASSERT(map->bitmap); > - DBUG_ASSERT(map2->bitmap); > - DBUG_ASSERT(map->n_bits == map2->n_bits); > - end= map->last_word_ptr; > + my_bitmap_map *to= map->bitmap, *from= map2->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_IDENTICAL_BITMAPS(map,map2); > > while (to <= end) > *to++ = *from++; > } > > > +/* > + Copy data into the bitmap from a byte array > +*/ > + > +void bitmap_copy_data(MY_BITMAP *map, const uchar *ptr, uint bits) not needed, as far as I understand > +{ > + my_bitmap_map *to= map->bitmap; > + uint length= (bits + 7)/8; > + uint map_length= no_bytes_in_map(map); > + DBUG_ASSERT_BITMAP_AND_BIT(map, bits-1); > + > + memcpy(to, ptr, length); > + if (map_length != length) > + bzero(to + length, map_length - length); > + *map->last_word_ptr&= ~map->last_word_mask; > +} > + > + > uint bitmap_get_first_set(const MY_BITMAP *map) > { > - uint i; > my_bitmap_map *data_ptr= map->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_BITMAP(map); > > - DBUG_ASSERT(map->bitmap); > - > - for (i=0; data_ptr < end; data_ptr++, i++) > + for (uint i=0; data_ptr <= end; data_ptr++, i++) > if (*data_ptr) > - goto found; > - if (!(*data_ptr & ~map->last_word_mask)) > - return MY_BIT_NONE; > - > -found: > - return get_first_set(*data_ptr, i); > + return my_find_first_bit(*data_ptr) + i * sizeof(my_bitmap_map)*8; > + return MY_BIT_NONE; > } > > > @@ -703,35 +696,18 @@ uint bitmap_get_next_set(const MY_BITMAP *map, uint bitmap_bit) > > uint bitmap_get_first(const MY_BITMAP *map) this should be named bitmap_get_first_clear(). because it's always is_bit_set/is_bit_clear, is_set_all/is_clear_all, test_and_set/test_and_clear, set_bit/clear_bit, etc. there is bitmap_get_first_set, so this one must be bitmap_get_first_clear > { > - uchar *byte_ptr; > - uint i,j,k; > - my_bitmap_map *data_ptr, *end= map->last_word_ptr; > - > - DBUG_ASSERT(map->bitmap); > - data_ptr= map->bitmap; > - *map->last_word_ptr|= map->last_word_mask; > + uint i; > + my_bitmap_map *data_ptr= map->bitmap, *end= map->last_word_ptr; > + DBUG_ASSERT_BITMAP(map); > > - for (i=0; data_ptr < end; data_ptr++, i++) > - if (*data_ptr != 0xFFFFFFFF) > + for (i= 0; data_ptr < end; data_ptr++, i++) > + if (*data_ptr != 0xFFFFFFFFFFFFFFFFULL) > goto found; > - if ((*data_ptr | map->last_word_mask) == 0xFFFFFFFF) > + if ((*data_ptr | map->last_word_mask) == 0xFFFFFFFFFFFFFFFFULL) > return MY_BIT_NONE; > - > found: > - byte_ptr= (uchar*)data_ptr; > - for (j=0; ; j++, byte_ptr++) > - { > - if (*byte_ptr != 0xFF) > - { > - for (k=0; ; k++) > - { > - if (!(*byte_ptr & (1 << k))) > - return (i*32) + (j*8) + k; > - } > - } > - } > - DBUG_ASSERT(0); > - return MY_BIT_NONE; /* Impossible */ > + /* find first zero bit by reverting all bits and find first bit */ > + return my_find_first_bit(~*data_ptr) + i * sizeof(my_bitmap_map)*8; > } > > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: 264aef71a37: Have ha_partition ignore HA_EXTRA..CHILDREN extra() calls if no myisamrg
by Sergei Golubchik 23 Feb '24

23 Feb '24

Hi, Michael, On Feb 23, Michael Widenius wrote: > revision-id: 264aef71a37 (mariadb-10.6.17-15-g264aef71a37) > parent(s): 042c3fc432b > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-02-22 09:20:51 +0200 > message: > > Have ha_partition ignore HA_EXTRA..CHILDREN extra() calls if no myisamrg > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc > index e174ef8d219..f2b3ab3aff9 100644 > --- a/sql/ha_partition.cc > +++ b/sql/ha_partition.cc > @@ -3030,6 +3031,10 @@ bool ha_partition::create_handlers(MEM_ROOT *mem_root) > DBUG_PRINT("info", ("InnoDB")); > m_innodb= TRUE; > } > + else if (ha_legacy_type(hton0) == DB_TYPE_MRG_MYISAM) > + { > + m_myisammrg= TRUE; > + } > DBUG_RETURN(FALSE); > } > > @@ -9419,9 +9424,14 @@ int ha_partition::extra(enum ha_extra_function operation) > } > /* Category 9) Operations only used by MERGE */ > case HA_EXTRA_ADD_CHILDREN_LIST: > + if (!m_myisammrg) > + DBUG_RETURN(0); > DBUG_RETURN(loop_partitions(extra_cb, &operation)); > case HA_EXTRA_ATTACH_CHILDREN: > { > + if (!m_myisammrg) > + DBUG_RETURN(0); > + > int result; This is, of course, quite horrible. Checking the legacy type, doing something for a specify underlying engine. But I don't see a better way of doing it, short of changing the storage engine API. What if we redefine ::extra() call to return HA_ERR_WRONG_COMMAND for unsupported extra operations? The caller will not abort on it, but it'll allow the caller to know whether the operation was executed or ignored. This will allow partitioning handler to skip all extra() methods that are not implemented in the underlying engine. For all engines and all extra operations. Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Re: Second part of review of XA patches in bb-10.6-MDEV-31949
by Kristian Nielsen 17 Feb '24

17 Feb '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > > I think it's better to define a proper API for this. > > It feels an overkill in the current case. "Define a proper API" means that you think through how this should work for different storage engines and TC_LOG implementations. And then you document those thoughts. I don't understand how doing this can be overkill, you are re-defining how durability of external XA transactions are coordinated between the server layer and the storage engines. >> And vice versa, for every xid >> X_COMPLETE found, roll it back if X is found prepared in InnoDB, > > Actually >> else commit it. > safer must be to commit both X_COMPLETE and X. Generally even if > X_COMPLETE and X commits in this order, X can race to be durable before > crash. Right, and "commit X" would mean "XA ROLLBACK X" if the "complete" operation is a rollback. I didn't follow why rolling back the X_COMPLETE would be a problem, but it's not important for now; I can see how committing should work as well. >> One reason I'm asking is I'm wondering how this will affect the scheduling >> of XA in parallel replication? Like, in case of duplicate XID, the X_PREPARE >> or X_COMPLETE could conflict between two transactions, so maybe the >> wakeup_subsequent_commits can only be done after the XA COMMIT of the >> X_PREPARE and X_COMMIT? Which I think would require changes to the current >> patch. > > Hmm, right now in the branch (see Sliding_window etc) for the duplicate > XID case, like > P_1,C_2,P_3,C_4 > P - XA-PREPARE, C - ... commit) P_3 can start only when C_2 wakes it up. Yes. So my point is: In this case, the C_2 cannot do wakeup_subsequent_commits inside the group commit along with run_commit_ordered(), that would be too early. C_2 must first complete the extra internal XA transaction on the mysql.gtid_slave_pos table before it can wakeup P_3. Or is this not a problem? > My bad I left out a mention of the plan, to treat - call it > XAC_0 -> XAP_1 > dependency - by the optimistic retry. That's in order to minimize the Aha. So if we optimistically apply the XA PREPARE, and we get an error ER_XAER_DUPID that the XID already exists as prepared, then we would do a retry like other temporary errors, is that what you mean? Is this implemented in the current patch? Maybe it is automatically handled by convert_kill_to_deadlock_error() ? If this XAC_0 -> XAP_1 conflict can be handled by normal optimistic rollback/retry, then I agree there is no need to handle the dependency explicitly in the sliding window. That was not clear to me from the code. >> This I don't understand at all. >> How can an XA PREPARE/COMMIT/ROLLBACK be marked as "ddl" ? > > That's a binlogging whim. E.g a temp table "DDL" involved into > transaction. I was not sure I could quickly eliminate the flagging, so > decided to live with it. Aha, right, CREATE TEMPORARY TABLE can be part of a transaction and thus DDL (in statement-based binlogging). I missed that. > But I could not get the meaning of > > ... handler.cc to avoid special cases for whether the binlog is enabled > The burden of checking whether the binlog is enabled, e.g for taking a > TC_LOG->log_and_order() path is on the programmer. If binlog is globally enabled (--log-bin option), then tc_log is an object of class MYSQL_BIN_LOG. Else it is class TC_LOG_MMAP (or TC_LOG_DUMMY). So a virtual function can be used instead of a conditional on opt_bin_log. But binlogging can be disabled on a transaction-by-transaction basis by setting SQL_LOG_BIN, is that the case here? Then yes, it seems an explicit condition will be needed. - Kristian.

1 0

[Andrei Elkin] Re: Third/last part of review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 12 Feb '24

12 Feb '24

Kristian, Using the chance to forwarding the reply that did not make to the developers list, let me comment on binlog_commit() that I might hastily rate as a big subject. > Hi Andrei, > > Here is the last part of my review of the changes in branch > bb-10.6-MDEV-31949: > >> --- a/sql/log_event_server.cc >> +++ b/sql/log_event_server.cc >> @@ -3324,6 +3324,11 @@ Gtid_log_event::Gtid_log_event(THD *thd_arg, uint64 seq_no_arg, >> flags2|= thd->lex->sql_command == SQLCOM_XA_PREPARE ? >> FL_PREPARED_XA : FL_COMPLETED_XA; >> xid.set(xid_state.get_xid()); >> + >> + // multi-engine external completion is unaware of the prepared engine #. >> + extra_engines= thd->transaction->all.ha_list ? >> + ha_count_rw_2pc(thd_arg, >> + thd_arg->in_multi_stmt_transaction_mode()) - 1 : 0; > > I cannot understand the comment. > I assume it's about that an XA COMMIT of a detached xid (different > session > that the XA PREPARE, or replication slave) - right? Correct. > > But what is the consequences of not knowing extra_engines in this > case? > Is the extra_engines used for anything than the optional truncate of > the > binlog after master crash? In the multi-engine case when the # of prepared in Engine is greater than in the value in GTID event then the trx is partially committed, so is decided for to be completed. > >> @@ -4175,7 +4173,8 @@ int XA_prepare_log_event::do_commit() >> thd->lex->xid= &xid; >> if (!one_phase) >> { >> - if ((res= thd->wait_for_prior_commit())) >> + if (thd->is_current_stmt_binlog_disabled() && >> + (res= thd->wait_for_prior_commit())) > > Why?000 > Is it to allow XA_PREPARE_LOG_EVENT to group commit with other event > groups? > That makes sense, but then shouldn't the wait_for_prior_commit() be > somewhere in handler.cc (or maybe xa.cc)? > Look at Xid_log_event::do_commit(), it doesn't have any > wait_for_prior_commit(), that is in ha_commit_one_phase(). Right. > (The wait-for-prior-commit facility is designed as a server-level > feature, > not a replication-level, it could in principle be used outside of > replication, even though it not currently (IIRC).) I think I understood what you mean which is follow the ha_commit_one_phase() pattern. [todo] > >> diff --git a/sql/sql_array.h b/sql/sql_array.h >> index 8610e971016..94c6d86ac05 100644 >> --- a/sql/sql_array.h >> +++ b/sql/sql_array.h >> @@ -172,6 +172,8 @@ template <class Elem> class Dynamic_array >> >> size_t size() const { return array.elements; } >> >> + size_t max_size() const { return array.max_element; } >> + > > Not that it hurts that much, but I think you can avoid adding this. Left over of a cleanup from the previous version. [todo] > >> --- a/sql/log.cc >> +++ b/sql/log.cc >> @@ -85,6 +85,7 @@ const char *log_bin_index= 0; >> const char *log_bin_basename= 0; >> >> MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period); >> +XID_cache_element *xid_cache_search(THD *thd, XID *xid); > > This duplicates the same declaration in sql/xa.h > So probably this can be removed now? [todo] > >> @@ -2186,16 +2202,13 @@ int binlog_commit(THD *thd, bool all, bool >> ro_1pc) >> } >> >> if (cache_mngr->trx_cache.empty() && >> - (thd->transaction->xid_state.get_state_code() != XA_PREPARED || >> - !(thd->ha_data[binlog_hton->slot].ha_info[1].is_started() && >> - thd->ha_data[binlog_hton->slot].ha_info[1].is_trx_read_write()))) >> + (!(is_preparing_xa(thd) || is_prepared_xa(thd)) || >> + !is_binlog_read_write(thd))) > > This binlog_commit function started out as the "commit" method for the > binlog handlerton. But that is not how it is used anymore, now it is > called > directly from several different places, including some places specific > to > external user XA. > > A large part of the function has become complex conditions like this > that > are basically impossible for the developer (or reviewer) to understand > exactly why this set of conditions is the correct one. Again, the > function > is trying to guess what the caller wants, depending on the state of > various > thd properties etc. > > Instead, the function should be split into multiple functions. One > that is > called to handle XA PREPARE; one for binlog_commit_by_xid(), etc. Then > each > version should be able to greatly reduce the number of conditions > needed. > > And if any logic is shared between the different versions, this can > then be > put in smaller functions that are called from each version. [my initial response was: Your proposal leads to a significant refactoring, which I add only one hunk. I'd really consider that as a separate exercise I think I've understood. Sure, binlog_prepare() that logs XA-PREPARE can call a common function with binlog_commit() that originally intended to log only normal trx:s. I can look into that. [todo] >> >> @@ -11050,10 +11124,30 @@ bool >> Recovery_context::decide_or_assess(xid_recovery_member *member, int >> round, >> } >> else if (member->in_engine_prepare < last_gtid_engines) >> { >> - DBUG_ASSERT(member->in_engine_prepare > 0); >> + DBUG_ASSERT(member->in_engine_prepare > 0 || >> + /* XA event over xid that was completed in engines */ >> + (member->xid == 0 && >> + member->xa_binlog_state >= xid_recovery_member::XA_PREPARE)); >> + if (member->in_engine_prepare > 0 && >> + member->xa_binlog_state == xid_recovery_member::XA_PREPARE) >> + { > > The function decide_or_assess() is hard to understand. > I can see that it's called in 3 cases: XID_LOG_EVENT, XA PREPARE, XA > COMMIT/ROLLBACK. > So again here, I think this should be split into 3 different > functions, one > for each case (or maybe two, one for internal XID and one for external > XA, > whatever makes the more sense). > > I don't understand the comment "XA event over xid that was completed > in > engines". But I remember seeing something in log_event_server.cc that > sets > extra_engines to 0 in the external user XA case. > > So is that the case here, that last_gtid_engines==0 marks this is user > XA? > That would be much clearer if the logic for internal and external XA > was not > mixed in the same function. > >> + /* >> + the first of the two options below can occur when round 1 turns to 2 >> + so event sequences across binlog files are like the following >> + g_current(B_0, xid), ... g_last(B^*, xid), >> + where `g`:s are xa event groups associated with the same member via >> + common `xid`. As elsewhere B_0 is the 2nd round's first and, >> + B^* the 1st round binlogs. >> + */ >> if (reset_truncate_coord(member->binlog_coord < last_gtid_coord ? >> member->binlog_coord.second : pos)) > > I'm trying to understand what is going on here. I think this is refering to > the way binlog files are scanned? > First we scan the most recent binlog file. At the end of it, we may find > that the latest binlog checkpoint is in an earlier file, then we continue to > scan those older files. So for example we may scan in order: > > master-bin.000004, master-bin.000002, master-bin.000003 > > This means that we may see the XA PREPARE and XA COMMIT out-of-order as we > go from the first scan to the remaining - "round 1 turns to 2". > > But I don't understand that this is "the first of the two options below": > > member->binlog_coord < last_gtid_coord ? member->binlog_coord.second : pos > > If last_gtid_coord is in master-bin.000002, but member->binlog_coord is in > master-bin.000004 ("round 1 turns to 2"), then I would think we would have > > member->binlog_coord > last_gtid_coord > > So it would be the *second* of the options below? What am I missing here? > > Also, I would suggest a simplification here. After we do the initial scan in > round 1, if we find that we need to scan more files, then reset all the > state and hashes. And then do a full scan of all necessary binlog files > in-order. In my example, that would be: > > master-bin.000004 > <reset state> > master-bin.000002 > master-bin.000003 > master-bin.000004 > > This way, the logic becomes simpler, as we don't need logic to handle seeing > events out-of-order. > > This will mean that occasionally we will need to scan one more binlog file, > but I think that is a good trade-off. The recovery code is notoriously > difficult to test and get correct, because it's an unusual occurence to need > recovery in the first place, and very hard to reproduce any problems. And > robust and correct recovery is the whole point of having XA in the first > palce. So making the code as simple as possible is crucial. > > The extra cost is small: > > - For one, recovery is a rare event > - Re-scanning only costs CPU, the file is already read from storage into OS > buffers. > - Normally, the binlog checkpoint completes shortly after rotation, so the > last binlog file that must be rescanned will be short. > > So simply redo the scan from the correct point when a "round 2" is needed, > to avoid any logic to handle seeing events out-of-order during recovery. > > >> @@ -11182,7 +11311,9 @@ void Recovery_context::process_gtid(int round, Gtid_log_event *gev, >> last_gtid_no2pc= false; >> last_gtid_standalone= >> (gev->flags2 & Gtid_log_event::FL_STANDALONE) ? true : false; >> - if (do_truncate && last_gtid_standalone) >> + /* a standalone XA-COMPLETE is truncation-safe */ >> + if (do_truncate && last_gtid_standalone >> + && !(gev->flags2 & Gtid_log_event::FL_COMPLETED_XA)) >> update_binlog_unsafe_coord_if_needed(linfo); > > I don't understand the comment here. > What is "a standalone XA-COMPLETE" ? > What does it mean to be "truncation-safe"? > Please reword the comment to be comprehensible. > >> + /* >> + the user XA events are decided similary to the normal transaction, >> + difference is just in the timing which is Gtid event read now. >> + */ > > I don't understand this comment. Please reformulate to make it > understandable. What does normal transactions do, and what does user > XA do? > How does the timing differ, and why? > >> + sql_print_warning("GTID %u-%u-%s XA transaction state % " >> + "in binlog file:%s pos:%llu is inconsistent " >> + "with the former same state; " >> + "consider filtered binary loggig.", >> + ctx.last_gtid.domain_id, ctx.last_gtid.server_id, >> + buf, is_xac ? "'COMPLETE'" : "'PREPARE'", >> + linfo->log_file_name, ctx.last_gtid_coord.second); > > Typo: s/loggig/logging/ > > Also, the phrase "the former same state" is not understandable (at > least to > me), what does it mean? Please reformulate. > I also have to be short now in replying to the rest, which I'll expand on tomorrow morning. Now just this. >> @@ -909,14 +910,16 @@ ha_commit_checkpoint_request(void *cookie, void (*pre_hook)(void *)) >> there's no need to rollback here as all transactions must >> be rolled back already >> */ >> -void ha_close_connection(THD* thd) >> +void ha_close_connection(THD* thd, bool skip_binlog) > > I tried to understand what the purpose of this change is. > ha_close_connection() is now called from two new places related to > replication of XA PREPARE. And in one place with this mysterious > skip_binlog=true condition. > > My guess is that this is to detach the XA PREPARE from the current > THD? The reason is described in 6cbb63f592 commit name II.1. ``` OTHER CHANGES, to ha_close_connection() are done to keep the binlog hton connection alive for the end of XA-prepare's binlogging on the parallel slave. Other than the binlog hton:s are disconnected from the slave transaction handler in TC_LOG::run_commit_ordered() which makes the prepared transaction engine objects available to binlog-commit-order followers. ``` In other words XAP_k in run_commit_ordered() releases with ha_close_connection(thd, true) all resources that the following XAC_k+i need. The latter does not need XAP_k to release binlog which it still need while being is this stack of TC_LOG::run_commit_ordered MYSQL_BIN_LOG::trx_group_commit_leader MYSQL_BIN_LOG::write_transaction_to_binlog_events MYSQL_BIN_LOG::write_transaction_to_binlog binlog_flush_cache binlog_commit_flush_xa_prepare binlog_commit binlog_prepare prepare_or_error ha_prepare > So > that it becomes visible in XA RECOVER to other threads, and (maybe > more > important) to make the THD available to run another transaction? One > of the > things InnoDB does in close_connection is: trx_disconnect_prepared(). > > But I think the purpose of the close_connection handlerton call is > different And you must have meant 'ha_close_connection', right? > - it is to free things at the end of a client connection? Well, with the ha- prefix, it's to disconnect THD from all transaction branches/htons. > So it can do > more > than detach the current external XA - which is presumably the reason > for the > need to hack it to not call the specific binlog_close_connection()? > > So this shouldn't hijack the close_connection to mean > "detach_xa_trx". It > looks like we need an explicit handlerton call for detaching an xa > transaction, that can then be used instead of this hack in > ha_close_connection(). I need to digest that in the morning. Generally I don't see a problem to conduct hton release flexibly. > >> -static my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, >> - void *arg) >> +my_bool xacommit_handlerton(THD *unused1, plugin_ref plugin, void *arg) >> { >> handlerton *hton= plugin_hton(plugin); >> - if (hton->recover) >> + if (hton->recover && hton != binlog_hton) >> { >> hton->commit_by_xid(hton, ((struct xahton_st *)arg)->xid); > >> -int ha_commit_or_rollback_by_xid(XID *xid, bool commit) >> +int ha_commit_or_rollback_by_xid(XID *xid, bool commit, THD *thd) >> { > >> + if (!skip_binlog) >> + { >> + // when binlog is ON start from its transaction branch >> if (commit) >> binlog_commit_by_xid(binlog_hton, xid); >> else >> binlog_rollback_by_xid(binlog_hton, xid); > > I think the purpose if this is to ensure that commit/rollback of XA > calls > into the binlog code first (to binlog before doing it in the engines). > > But as I think Serg also remarked already, this should not be > needed. The > binlog is the (internal) transaction coordinator when it is enabled, > we have > a dedicated interface for that, class TC_LOG. This is used elsewhere > in > handler.cc to avoid special cases for whether the binlog is enabled or > not. > > So it looks like there needs to be extra virtual functions in TC_LOG > for > the XA commit/rollback, and these can then be called on the tc_log > object > and do what is required. I need to process that tomorrow. > Then some logic in handler.cc can be omitted/simplified. > > ---- > > I think with this, I got through all of the code changes in branch > bb-10.6-MDEV-31949 as of commit > 1bdc9bc8077c79e55cb4dbfc7cc47a193d17d54f > from January 20, 2024. So this concludes the current review. I have > not > looked at the test cases in this round of review. Thanks again! Cheers, Andrei

1 1

Re2: Starting on review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 12 Feb '24

12 Feb '24

Salve Kristian, this is the 2nd follow-up replay to your 1st review, to cover few deferred notes in particular. I mark with [x] any addressed [todo] note. > Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: > >> Hi Andrei, > So you're in this context > > 1 innobase_commit_by_xid( > 2 /*===================*/ > 3 handlerton* hton, > 4 XID* xid) /*!< in: X/Open XA transaction > identification */ > 5 { > 6 DBUG_ASSERT(hton == innodb_hton_ptr); > 7 > 8 DBUG_EXECUTE_IF("innobase_xa_fail", > 9 return XAER_RMFAIL;); > 10 > 11 if (high_level_read_only) { > 12 return(XAER_RMFAIL); > 13 } > 14 > 15 if (trx_t* trx = trx_get_trx_by_xid(xid)) { > 16 THD *thd = current_thd; > 17 if (thd) > 18 thd_binlog_pos(thd, &trx->mysql_log_file_name, > 19 &trx->mysql_log_offset); > 20 if (trx->mysql_log_offset > 0) > 21 trx->flush_log_later = true; > > and actually considering `trx->mysql_log_offset` as the return value? > ... I could not grasp your way of thinking in this place. > Could you please expand on? In another mail yesterday concerning with this part I suggested to replace if (trx->mysql_log_offset > 0) with if (thd_binlog_format(thd) != UNSPEC) > > >> >> It would be much better if this could be done similar to how normal >> commits >> are done. We have the commit_ordered handlerton which does the fast >> part of >> commit-to-memory. And then the slow part which does the rest in commit >> handlerton. > > Notice that the *slow* part of the XA commit is the same as in the > normal > case: > > trx_commit_complete_for_mysql > *innobase_commit* > commit_one_phase_2 > ha_commit_one_phase > > The fast parts of the two - the XA and normal - could be unified - into > > innobase_commit_ordered() > > but I did not think (had time for) towards that. > >> >> So we could have commit_by_xid_ordered() and commit_by_xid(). The >> problem is >> that commit_by_xid_ordered() needs to release the xid, so it becomes >> harder >> to get hold of the transaction (which the commit handlerton just finds >> from >> the thd). Maybe commit_by_xid_ordered() would need to return some trx >> handle. >> >> What part of the commit is skipped by this in InnoDB? Is it _only_ >> flushing >> the log up to the LSN of the commit record, or are there more things? > > > So the xa's fast innobase_commit_by_xid() skips a "slow" > trx_commit_complete_for_mysql() > and the same does the normal's innobase_commit_ordered(). > >> >> How is another storage engine supposed to handle this? This also needs >> to be >> documented somewhere, eg. in comments on the handlerton calls. >> >> This also means that the --innodb-flush-log-at-trx-commit=3 is not >> supported >> for XA COMMIT when it's done from a different session that the XA >> PREPARE. >> For example, with --innodb-flush-log-at-trx-commit=3 but >> --sync-binlog=0, >> normal transactions are still durable in InnoDB. But external XA >> transactions will not be. This could be solved if the commit_by_xid() >> was >> split into a fast and slow part similar to normal commit. > > [todo] Let me process that carefully later. [x] My 1st reply already said, just missed to underscore that, that the XA also the slow and fast commit paths, therefore iflatc=3 is good. The past paths are commit_one_phase_2 and innobase_commit_by_xid functionally equivalent. One would vote to unify them, and I'd accept that. > >> >> Probably you'll say that this works for now, and perhaps that's true. >> But at >> least it really _really_ needs some detailed comments explaining what >> is >> going on, and what should be kept in mind if modifying/improving this >> code >> in the future, otherwise it will be impossible for the next developer >> to >> understand. > > [todo] Agreed. > >> >> >>> diff --git a/sql/log.cc b/sql/log.cc >>> index e54d4087d46..7e3dbeaef29 100644 >>> --- a/sql/log.cc >>> +++ b/sql/log.cc >> >>> @@ -9365,18 +9381,84 @@ TC_LOG::run_commit_ordered(THD *thd, bool >>> all) >>> all ? thd->transaction->all.ha_list : >>> thd->transaction->stmt.ha_list; >>> >>> mysql_mutex_assert_owner(&LOCK_commit_ordered); >>> - for (; ha_info; ha_info= ha_info->next()) >>> + >>> + if (!ha_info) >> >> Your patch puts a lot of XA/binlog logic into run_commit_ordered(). >> But >> that's not appropriate. The run_commit_ordered() function has the >> purpose >> solely to call the commit_ordered() handlerton in each engine, and >> it's also >> called from TC_LOG_MMAP (that's why it's TC_LOG::, not >> MYSQL_BIN_LOG::). >> >> Instead make a new function that handles the part of binlog commit >> that >> needs to be done under LOCK_commit_ordered. Be sure to comment it that >> this >> runs under this highly contended mutex, and needs to do as little work >> as >> possible. Agreed. [todo] >> >> This function can then call run_commit_ordered(). It can also be used >> to >> share a few bits of code that are now duplicated between the >> opt_optimize_thread_scheduling and !opt_optimize_thread_scheduling >> cases, >> like: >> >> ++num_commits; >> run_commit_ordered(...) >> entry->thd->wakeup_subsequent_commits() >> if (entry->queued_by_other) { ... } >> >> (if it makes sense to do so in the actual code, only if makes things >> simpler, that's easier to decide when working with the actual code). Will see. [todo] >> >> There sesems to be 3 new cases: >> >> - XA PREPARE >> - XA COMMIT from same session as XA PREPARE >> - XA COMMIT of detached XA transaction >> >> The logic to decide this was already done earlier, when the event >> group was >> queued for group commit, eg. to decide which end_event to use. For >> example >> there is no need to call xid_cache_search() here, that can be done >> earlier >> (and it's also wrong to call my_error() here, as we're in the wrong >> thread >> context). Indeed. The error's thread context was overlooked. Thanks for catching it. >> >> So the logic about what to do should be placed higher up in the call >> stack, >> ideally where we already know, eg. binlog_commit_by_xid(). And errors >> such >> as ER_XAER_NOTA can be thrown already there, and the decision on what >> to do >> put consisely as a flag into the struct group_commit_entry, so the >> logic >> during binlog group commit becomes as simple as possible. >> >> (It might even make sense to simply store a function pointer to the >> method >> that should be called to handle this; normal, attached XA COMMIT, >> detached >> XA COMMIT, XA PREPARE. But again it's easier when actually working >> with the >> code to decide if this is simpler than a couple flags or case on enum >> value). > > > [todo] Let me process the above suggestion block a bit later. [x] To the need of xid_cache_search() etc, let me combine this part to reply to the 2nd review mail's notes around a Dummy XID though ... (read on) > >> >>> + if (thd->rgi_slave && thd->rgi_slave->is_parallel_exec) >>> + { >>> + DBUG_ASSERT(thd->transaction->xid_state.is_dummy_XA()); >>> + >>> + auto xs= xid_cache_search(thd, xid); >>> + if(!xs) >>> + { >>> + my_error(ER_XAER_NOTA, MYF(0)); >>> + return; >>> + } >>> + else >>> + { >>> + thd->transaction->xid_state.xid_cache_element= xs; >>> + } >>> + DBUG_ASSERT(thd->transaction->xid_state.is_recovered()); >>> + } >>> + >>> plugin_foreach(NULL, commit ? xacommit_handlerton : >>> xarollback_handlerton, >>> MYSQL_STORAGE_ENGINE_PLUGIN, &xaop); >>> xid_cache_delete(thd); >> >> Why this check for thd->rgi_slave->is_parallel_exec here deep in >> binlog >> group commit? That seems very wrong, and again no comment whatsoever >> explaining what is going on, so again I have to try to guess... >> >> I'm guessing that it's something with calling xid_cache_delete() which >> needs >> to find thd->transaction->xid_state.xid_cache_element ? > > Right. > >> >> But this could be done much better in a higher-level part of the code >> that's >> specific to xa and (parallel) replication, not here deep down in >> binlog >> group commit code. >> >> And why should this be needed only for parallel replication, not for >> non-parellel replication or non-replication SQL? I don't understand. > > > It's a kind of optimization for the slave side, in that `xid` of XAP > (Prepare) gets released for a XAC (Commit) the soonest. > ... the reason's said here. >> >> >> A couple remarks about the tests I pushed to >> knielsen_mdev31949_review: >> > >> >> I think the failures in rpl.rpl_recovery_xa I linked are from >> duplicate XID >> in XA PREPARE ; XA COMMIT ; XA PREPARE. There comes an ambiguity >> between >> whether the transaction prepared in the engine at recovery is from the >> first >> XA PREPARE (so it should be committed) or the second (so it should be >> rolled >> back). This happens when the second XA PREPARE is in the engine but >> not >> binlogged. >> >> One way to fix could actually be to use the binlog position stored >> inside >> InnoDB. That's a neat reminder on the Innodb durably committed binlog position! Indeed with the duplicate xid case there exists this trouble of identification and the Engine is able and must come to rescue. >> If the XA COMMIT is before this binlog position, then we know the >> first transaction became durable in InnoDB, so we know we have to roll >> back >> the current prepared transaction in the engine. Otherwise we should >> commit >> it. I see the recovery code already uses binlog positions to decide >> the fate >> of pending XA transactions in the engines, but I did not yet fully >> understand what is going on there. >> >> Another way could be to binlog an extra event before the second XA >> PREPARE >> with the duplicate XID. Well, this does not cover the case of prior to crash XA PREPARE only made to Engine. The above consulting with Innodb resolves the ambiguity generally. >> This event would mean "This xid is now durable >> committed in the engine", so we would know not to commit any prepared >> trx in >> the engine with the same xid. This event would only be needed in case >> of XA >> PREPARE with a duplicate xid. This though requires the prior XA COMMIT >> to >> fsync inside InnoDB first, I think. > > Thanks for this piece of analysis! > [todo] I am processing this a bit later. [x] > >> >> >> Another test failure in rpl.slave_provision_xa I think happens because >> the >> XA PREPARE does not update the binlog position inside innodb. >> Maybe the way >> to solve this problem is to have the slave be idempotent, and silently >> ignore any duplicate XA PREPARE if the xid is already prepared? Indeed, the fact that XAP does not update the innodb's binlog position must cause few concerns. I still need to have a close look. [todo] > > [todo] I am processing this a bit later. [x] > >> >> >> About the "ToDo" of using some Xid_list_log_event. I think this is a >> way >> during recovery to get the state of the XA PREPARE/COMMIT's in earlier >> binlog files before the last binlog checkpoint, if I understood things >> correctly? > > True. > >> >> Another way to do this is to delay the binlog checkpoints until all XA >> PREPARE in a binlog file have been XA COMMIT'ted (or XA ROLLBACK'ed), >> so >> that they will all be scanned during recovery. I actually implemented >> this >> in my branch knielsen_mdev32020. The advantage of this is that we may >> anyway >> need this to provide the XA PREPARE events to a slave set up from a >> logical >> backup (like mysqldump), to avoid purging XA PREPARE events too early. >> And >> then it's a bit simpler to simply scan an extra file rather than both >> scan >> and read Xid_list_log_event. But I think Xid_list_log_event could work >> fine >> as well, just mentioning it for you to consider. > > Xlle can't be generally avoided so we have to take conservatively > first. > To add up to a separate thread discussion over this problematics, I've grown more inclined with the first implementation basing on your idea to defer Binlog-CheckPoint. That might work for a majority of XA users, Xid list to be added later if time permits or will be convincingly :-) requested. Now I am turning to account the rest - 2,3 reviews you sent that only glanced through - to reply within few hours. Cheers, Andrei

2 2

Re: 64-bit time_t support in MariaDB?
by Otto Kekäläinen 12 Feb '24

12 Feb '24

> it's not related to time, it's just an assert that's verified at compile > time, that is, during compilation. Like static_assert in C++. > > I just need to find 32-bit arm somewhere. Unfortunately I don't have any with SSH access to give. Seems the reported of MDEV-33429 has armhf hardware and is helping out. Once you have a patch, I can push it to Launchpad builders (or you can do it yourself, anyone with an Launchpad can push code to their own PPA). Related, Cfarm is a project that gives free accounts to open source developers in case you want to have access to play around with multiple architectures: https://portal.cfarm.net/machines/list/ Unfortunately they don't seem to have armhf/armel.

2 1

Re2: thd_binlog_pos / Re: Starting on review of XA patches in bb-10.6-MDEV-31949
by andrei.elkin＠pp.inet.fi 09 Feb '24

09 Feb '24

>> So you're in this context >> >> 1 innobase_commit_by_xid( > >> 18 thd_binlog_pos(thd, &trx->mysql_log_file_name, >> 19 &trx->mysql_log_offset); >> 20 if (trx->mysql_log_offset > 0) >> 21 trx->flush_log_later = true; >> >> and actually considering `trx->mysql_log_offset` as the return value? >> Could you please expand on? > > My point was just, that what we want to do here is for the (internal) > transaction coordinator to tell the storage engine that the engine does > not > need to fsync() for the commit to be durable, another fsync was already > done > to ensure this. This is what we in the normal commit path do by calling > commit_ordered(), as documented sql/handler.h. > > The thd_binlog_pos() is completely unrelated, it's a way for the > storage... I see your point. My patch's block of if (trx_t* trx = trx_get_trx_by_xid(xid)) { + THD *thd = current_thd; + if (thd) + thd_binlog_pos(thd, &trx->mysql_log_file_name, + &trx->mysql_log_offset); + if (trx->mysql_log_offset > 0) + trx->flush_log_later = true; + /* use cases are: disconnected xa, slave xa, recovery */ innobase_commit_low(trx); + trx->mysql_log_file_name = NULL; should not indeed rely on anything that thd_binlog_pos() computes. I think I wanted to spare --skip-log-bin setups from the flush_log_later policy. I see now that alternatively thd_binlog_format(thd) could be used which will always return BINLOG_FORMAT_UNSPEC when that's the case (or @@skip_log_bin=1). You think it's better to covert the block to use that?

1 0