Sergei Golubchik added a comment - 2023-10-13 13:12 - edited 8d21bf9f5 2e38c6cb8d8c (new commit with GPL headers) is ok to push, thanks!

Nikita Malyavin added a comment - 2023-10-11 10:09 Minor issues are resolved, head is now 8d21bf9f5 . Waiting for the final verdict.

Nikita Malyavin added a comment - 2023-09-25 17:45 The high-level reason is that the assumption about that thd->binlog_get_cache_mngr() is, sufficiently, NULL, when we shouldn't run the binlog part of binlog_commit/binlog_rollback , is wrong, i.e. this condition: binlog_cache_mngr * const cache_mngr= thd->binlog_get_cache_mngr();   if (!cache_mngr) { ... DBUG_RETURN(0); } is not enough. I don't know whether cache_mgr is used for statement logging or not. Here we have a mixed logging, and that is, some operations may have no row-base logging. However, cache_mngr exists, which makes me think, that a statement logging is done differently, and cache_mngr only serves row-based logging, since trx_cache is also empty at the moment of check. What's definite is that binlog_hton wouldn't have been set for SBR. It gets enabled during binlog_log_row_to_binlog , in THD::binlog_start_trans_and_stmt . In particular, trx_cache.restore_prev_position() is called during binlog_rollback , when it actually shouldn't have to – bacause it's SBR and binlog_hton is not set: 0x000055f154ef0ac8 in reinit_io_cache (info=0x61b000060638, type=WRITE_CACHE, seek_offset=18446744073709551615, use_async_io=0 '\000', clear_cache=0 '\000') at /home/nik/mariadb/mysys/mf_iocache.c:483 483 info->pos_in_file=seek_offset; (gdb) bt #0 0x000055f154ef0ac8 in reinit_io_cache (info=0x61b000060638, type=WRITE_CACHE, seek_offset=18446744073709551615, use_async_io=0 '\000', clear_cache=0 '\000') at /home/nik/mariadb/mysys/mf_iocache.c:483 #1 0x000055f152b2b072 in binlog_cache_data::truncate (this=0x61b000060638, pos=18446744073709551615, reset_cache=false) at /home/nik/mariadb/sql/log.cc:492 #2 0x000055f152b2341a in binlog_cache_data::restore_prev_position (this=0x61b000060638) at /home/nik/mariadb/sql/log.cc:364 #3 0x000055f152b1c2d3 in binlog_truncate_trx_cache (thd=0x62b0001c0218, cache_mngr=0x61b000060498, all=false) at /home/nik/mariadb/sql/log.cc:2109 #4 0x000055f152ac7c80 in binlog_rollback (hton=0x615000002118, thd=0x62b0001c0218, all=false) at /home/nik/mariadb/sql/log.cc:2535 #5 0x000055f152617f6e in ha_rollback_trans (thd=0x62b0001c0218, all=false) at /home/nik/mariadb/sql/handler.cc:2264 #6 0x000055f1537c4d75 in trans_rollback_stmt (thd=0x62b0001c0218) at /home/nik/mariadb/sql/transaction.cc:535 #7 0x000055f15313e5ab in mysql_execute_command (thd=0x62b0001c0218, is_called_from_prepared_stmt=false) at /home/nik/mariadb/sql/sql_parse.cc:5854 #8 0x000055f15311ccfb in mysql_parse (thd=0x62b0001c0218, rawbuf=0x629000316238 "INSERT INTO t2 VALUES (3,3,'a','x'), (3,3,'a','x')", length=50, parser_state=0x7f2f140497f0) at /home/nik/mariadb/sql/sql_parse.cc:7811 #9 0x000055f15311674e in dispatch_command (command=COM_QUERY, thd=0x62b0001c0218, packet=0x62900030c219 "INSERT INTO t2 VALUES (3,3,'a','x'), (3,3,'a','x')", packet_length=50, blocking=true) at /home/nik/mariadb/sql/sql_parse.cc:1893 #10 0x000055f15311eb2a in do_command (thd=0x62b0001c0218, blocking=true) at /home/nik/mariadb/sql/sql_parse.cc:1406 I tried a few approaches to fix that problem. I tried to only exit when trx_cache->before_stmt_pos is MY_OFF_T_UNDEF , but then DBUG_ASSERT(WSREP(thd) || rollback_online) breaks, meaning, that sometimes it's fine to have undefined trx_cache. But in that case, binlog_commit/binlog_rollback is supposed to work somehow – since the handlerton was installed for some reason. I tried to go deeper and take MY_OFF_T_UNDEF more carefully into account – actually, I tried to modify binlog_cache_data::empty() and write a more careful exit condition. That failed tests in random places, like it had an assertion failure in MYSQL_BIN_LOG::trx_group_commit_leader . So overall this demonstrates that binlog state consistency is quite fragile, and I don't want to play around with it. I had a success in extracting everything in a separate hton with no much price, but some benefits, like online_alter_cache_list is moved out of thd, and I call it debloating (yes, first I bloated it ). And what's more important is more independency from binlog, which will definitely come quite handy.

Sergei Golubchik added a comment - 2023-09-25 11:14 The overall problem is a deep interference with the effect of an installed binlog_hton. this doesn't really explain anything. Please explain why the assert actually fires. You can explain here to avoid recommitting everything just to change a comment. Later, when everything is "ok to push" you can fix the comment in commit too together with other changes, if any

Nikita Malyavin added a comment - 2023-09-23 15:36 There is a lot of interference with binlog_hton enabled by online alter, which I couldn't fight by adhoc state deductions. So I decided to carry online alter out, into a separate handlerton. This gave me a possibility to store connection-local online alter data as ha_data. Also I reworked the savepoints handling as handlerton functions. Please review the following commits: 217a7a7c online alter: rework savepoints ad6a305d online alter: use thd->ha_data to store cache_list 132c56f1 online alter: extract the source to a separate file a71b1343 MDEV-32126 Assertion fails upon online ALTER and binary log enabled Also please consider my 132c56f1 commit among then, which creates online_alter.cc and moves all the producer side code there. It's optional, but really helps reading and editing.

Nikita Malyavin added a comment - 2023-09-19 14:21 Please review branch bb-11.2-nikita , commit b96b093d

Nikita Malyavin added a comment - 2023-09-19 09:02 The smaller test, leading to the same problem, however a different assertion, has been found: --source include/have_binlog_format_mixed.inc --source include/have_innodb.inc --source include/have_debug_sync.inc   CREATE TEMPORARY TABLE tmp (id INT , PRIMARY KEY (id)) ENGINE=InnoDB; CREATE TABLE t1 (a INT , b TEXT) ENGINE=InnoDB; CREATE TABLE t2 (a INT , b INT , c CHAR (8), d TEXT, UNIQUE (a)) ENGINE=InnoDB; INSERT INTO t2 VALUES (1,1, 'f' , 'e' ),(1000,1000, 'c' , 'b' ); --connection default SET debug_sync= 'alter_table_online_before_lock SIGNAL go_trx WAIT_FOR go_alter' ; send ALTER TABLE t2 FORCE , ALGORITHM=COPY, LOCK=NONE;   --connect (con1,localhost,root,,test)   SET debug_sync= 'now WAIT_FOR go_trx' ; START TRANSACTION ; INSERT INTO t1 VALUES (3, 'a' ); --error ER_DUP_ENTRY INSERT INTO t2 VALUES (3,3, 'a' , 'x' ), (3,3, 'a' , 'x' ); INSERT INTO t2 VALUES (3,3, 'a' , 'x' ); COMMIT ; set debug_sync= 'now SIGNAL go_alter' ;