Re: 63c93f89af9: MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails
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@mariadb.org
Hi, Nikita,
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
On Aug 07, Nikita Malyavin wrote: 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.
Me neither☺
On Mon, 7 Aug 2023 at 18:25, Sergei Golubchik <serg@mariadb.org> wrote: psi commit is done under if(is_real_trans) condition. That is, maybe it's not supposed to start the psi transaction at all for non-trans engines? But since there are DMLs running concurrently to ALTER TABLE, maybe binlog/replication recognizes it as needed for some "binlog transactioning"? *Before* ALTER TABLE replication begins, replica receives a Gtid_log_event: Thread 2 hit Hardware watchpoint 2: -location thd->m_transaction_psi Old value = (PSI_transaction_locker *) 0x62b0001d21d8 New value = (PSI_transaction_locker *) 0x0 0x00005622d5c3c76c in trans_begin (thd=0x62b0001ce218, flags=0) at /home/nik/mariadb/sql/transaction.cc:235 235 thd->m_transaction_psi= MYSQL_START_TRANSACTION(&thd->m_transaction_state, (gdb) bt #0 0x00005622d5c3c76c in trans_begin (thd=0x62b0001ce218, flags=0) at /home/nik/mariadb/sql/transaction.cc:235 #1 0x00005622d5026c32 in Gtid_log_event::do_apply_event (this=0x616000a2a598, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/log_event_server.cc:3205 #2 0x00005622d4fe632f in Log_event::apply_event (this=0x616000a2a598, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/log_event.cc:3880 #3 0x00005622d5212a4f in apply_event_and_update_pos_apply (ev=0x616000a2a598, thd=0x62b0001ce218, rgi=0x61d0002dfa80, reason=0) at /home/nik/mariadb/sql/slave.cc:3877 #4 0x00005622d5211e72 in apply_event_and_update_pos (ev=0x616000a2a598, thd=0x62b0001ce218, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/slave.cc:4043 #5 0x00005622d522bed3 in exec_relay_log_event (thd=0x62b0001ce218, rli=0x62a0000202c0, serial_rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/slave.cc:4440 #6 0x00005622d5201589 in handle_slave_sql (arg=0x62a00001e200) at /home/nik/mariadb/sql/slave.cc:5626 Which doesn't happen during lock=shared copy.
if (ha_enable_transaction(thd, TRUE)) DBUG_RETURN(TRUE);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
Hi again, I rewrote the solution completely, please review the following commits: 88a77860 <https://github.com/MariaDB/server/commit/88a77860a8bf78bf058e3315c27d14d72eb196ba> MDEV-31804 Assertion `thd->m_transaction_psi == __null' fails 73a677f0 <https://github.com/MariaDB/server/commit/73a677f03061fa188329b8e1148cbacbe6549729> Cleanup: make slave_exec_mode of its enum type and pack Log_event better On Mon, 7 Aug 2023 at 19:54, Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
On Mon, 7 Aug 2023 at 18:25, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
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
On Aug 07, Nikita Malyavin wrote: 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.
Me neither☺ psi commit is done under if(is_real_trans) condition. That is, maybe it's not supposed to start the psi transaction at all for non-trans engines? But since there are DMLs running concurrently to ALTER TABLE, maybe binlog/replication recognizes it as needed for some "binlog transactioning"?
*Before* ALTER TABLE replication begins, replica receives a Gtid_log_event:
Thread 2 hit Hardware watchpoint 2: -location thd->m_transaction_psi
Old value = (PSI_transaction_locker *) 0x62b0001d21d8 New value = (PSI_transaction_locker *) 0x0 0x00005622d5c3c76c in trans_begin (thd=0x62b0001ce218, flags=0) at /home/nik/mariadb/sql/transaction.cc:235 235 thd->m_transaction_psi= MYSQL_START_TRANSACTION(&thd->m_transaction_state, (gdb) bt #0 0x00005622d5c3c76c in trans_begin (thd=0x62b0001ce218, flags=0) at /home/nik/mariadb/sql/transaction.cc:235 #1 0x00005622d5026c32 in Gtid_log_event::do_apply_event (this=0x616000a2a598, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/log_event_server.cc:3205 #2 0x00005622d4fe632f in Log_event::apply_event (this=0x616000a2a598, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/log_event.cc:3880 #3 0x00005622d5212a4f in apply_event_and_update_pos_apply (ev=0x616000a2a598, thd=0x62b0001ce218, rgi=0x61d0002dfa80, reason=0) at /home/nik/mariadb/sql/slave.cc:3877 #4 0x00005622d5211e72 in apply_event_and_update_pos (ev=0x616000a2a598, thd=0x62b0001ce218, rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/slave.cc:4043 #5 0x00005622d522bed3 in exec_relay_log_event (thd=0x62b0001ce218, rli=0x62a0000202c0, serial_rgi=0x61d0002dfa80) at /home/nik/mariadb/sql/slave.cc:4440 #6 0x00005622d5201589 in handle_slave_sql (arg=0x62a00001e200) at /home/nik/mariadb/sql/slave.cc:5626
Which doesn't happen during lock=shared copy.
if (ha_enable_transaction(thd, TRUE)) DBUG_RETURN(TRUE);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
-- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik