Re: [Maria-developers] acbe14b122c: Aria will now register it's transactions

Hi, Michael! See below On May 20, Michael Widenius wrote:

revision-id: acbe14b122c (mariadb-10.5.2-255-gacbe14b122c) parent(s): 48a758696bf author: Michael Widenius <monty@mariadb.com> committer: Michael Widenius <monty@mariadb.com> timestamp: 2020-05-19 17:52:17 +0300 message:

Aria will now register it's transactions

MDEV-22531 Remove maria::implicit_commit()

diff --git a/sql/sql_class.h b/sql/sql_class.h index d4a95fa3fd8..a7a071f7cdb 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -5149,6 +5151,40 @@ class THD: public THD_count, /* this must be first */

};

+ +/* + Start a new independent transaction for the THD. + The old one is stored in this object and restored when calling + restore_old_transaction() or when the object is freed +*/ + +class start_new_trans +{ + /* container for handler's private per-connection data */ + Ha_data old_ha_data[MAX_HA]; + struct THD::st_transactions *old_transaction, new_transaction; + Open_tables_backup open_tables_state_backup; + MDL_savepoint mdl_savepoint; + PSI_transaction_locker *m_transaction_psi; + THD *org_thd; + uint in_sub_stmt; + uint server_status; + +public: + start_new_trans(THD *thd); + ~start_new_trans() + { + destroy(); + } + void destroy() + { + if (org_thd) // Safety + restore_old_transaction(); + new_transaction.free(); + } + void restore_old_transaction();

interesting. You made restore_old_transaction() public and you use it in many places. Why not to use destroy() instead? When one would want to use restore_old_transaction() instead of destroy()?

+}; + /** A short cut for thd->get_stmt_da()->set_ok_status(). */

inline void diff --git a/sql/sql_class.cc b/sql/sql_class.cc index dda8e00f6bf..51d7380f622 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -5742,6 +5744,90 @@ void THD::mark_transaction_to_rollback(bool all) }

+/** + Commit the whole transaction (both statment and all) + + This is used mainly to commit an independent transaction, + like reading system tables. + + @return 0 0k + @return <>0 error code. my_error() has been called() +*/ + +int THD::commit_whole_transaction_and_close_tables() +{ + int error, error2; + DBUG_ENTER("THD::commit_whole_transaction_and_close_tables"); + + /* + This can only happened if we failed to open any table in the + new transaction + */ + if (!open_tables) + DBUG_RETURN(0);

Generally, I think, it should still end the transaction here. Or add an assert that there is no active transaction at the moment.

+ + /* + Ensure table was locked (opened with open_and_lock_tables()). If not + the THD can't be part of any transactions and doesn't have to call + this function. + */ + DBUG_ASSERT(lock); + + error= ha_commit_trans(this, FALSE); + /* This will call external_lock to unlock all tables */ + if ((error2= mysql_unlock_tables(this, lock))) + { + my_error(ER_ERROR_DURING_COMMIT, MYF(0), error2); + error= error2; + } + lock= 0; + if ((error2= ha_commit_trans(this, TRUE))) + error= error2; + close_thread_tables(this);

I wonder why you're doing it in that specific order. commit(stmt)-unlock-commit(all)-close

+ DBUG_RETURN(error); +} + +/** + Start a new independent transaction +*/ + +start_new_trans::start_new_trans(THD *thd) +{ + org_thd= thd; + mdl_savepoint= thd->mdl_context.mdl_savepoint(); + memcpy(old_ha_data, thd->ha_data, sizeof(old_ha_data)); + thd->reset_n_backup_open_tables_state(&open_tables_state_backup); + bzero(thd->ha_data, sizeof(thd->ha_data)); + old_transaction= thd->transaction; + thd->transaction= &new_transaction; + new_transaction.on= 1; + in_sub_stmt= thd->in_sub_stmt; + thd->in_sub_stmt= 0; + server_status= thd->server_status; + m_transaction_psi= thd->m_transaction_psi; + thd->m_transaction_psi= 0; + thd->server_status&= ~(SERVER_STATUS_IN_TRANS | + SERVER_STATUS_IN_TRANS_READONLY); + thd->server_status|= SERVER_STATUS_AUTOCOMMIT; +}

Few thoughts: 1. If you need to save and restore _all that_ then, perhaps, all that should be inside st_transactions ? 2. strictly speaking, ha_data is _per connection_. If you just bzero it, the engine will think it's a new connection, and you cannot just overwrite it on restore without hton->close_connection. A strictly "proper" solution would be to introduce ha_data per transaction in addition to what we have now. But it looks like an overkill. So I'd just add close_system_tables() now into restore_old_transaction() A strictly "proper" solution would be to introduce ha_data per transaction in addition to what we have now. But it looks like an overkill. So I'd just add ha_close_connection() now into restore_old_transaction() and that's all. I see that you've added free_transaction() call, but isn't it redundant? ha_close_connection() does that now. Otherwise very good, thanks, I cannot wait to start using it more for other features.

+ + +void start_new_trans::restore_old_transaction() +{ + org_thd->transaction= old_transaction; + org_thd->restore_backup_open_tables_state(&open_tables_state_backup); + ha_free_transactions(org_thd); + memcpy(org_thd->ha_data, old_ha_data, sizeof(old_ha_data)); + org_thd->mdl_context.rollback_to_savepoint(mdl_savepoint); + org_thd->in_sub_stmt= in_sub_stmt; + org_thd->server_status= server_status; + if (org_thd->m_transaction_psi) + MYSQL_COMMIT_TRANSACTION(org_thd->m_transaction_psi); + org_thd->m_transaction_psi= m_transaction_psi; + org_thd= 0; +} + + /** Decide on logging format to use for the statement and issue errors or warnings as needed. The decision depends on the following diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index af43d92dea7..82b3968de85 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -742,7 +748,8 @@ Event_db_repository::create_event(THD *thd, Event_parse_data *parse_data, ret= 0;

end: - close_thread_tables(thd); + if (table) + thd->commit_whole_transaction_and_close_tables();

You're checking twice. Here in the caller, and the first thing inside commit_whole_transaction_and_close_tables(). Here and in many places. If you want to check in the caller, there's no need to check inside? You can add an assert instead, can't you?

thd->mdl_context.rollback_to_savepoint(mdl_savepoint);

thd->variables.sql_mode= saved_mode; @@ -1117,22 +1124,20 @@ update_timing_fields_for_event(THD *thd, TABLE *table= NULL; Field **fields; int ret= 1; - enum_binlog_format save_binlog_format; MYSQL_TIME time; DBUG_ENTER("Event_db_repository::update_timing_fields_for_event");

- /* - Turn off row binlogging of event timing updates. These are not used - for RBR of events replicated to the slave. - */ - save_binlog_format= thd->set_current_stmt_binlog_format_stmt(); - DBUG_ASSERT(thd->security_ctx->master_access & PRIV_IGNORE_READ_ONLY);

if (open_event_table(thd, TL_WRITE, &table)) goto end;

Why do you reuse a current transaction, instead of creating a new one as everywhere above?

fields= table->field; + /* + Turn off row binlogging of event timing updates. These are not used + for RBR of events replicated to the slave. + */ + table->file->row_logging= 0;

if (find_named_event(event_db_name, event_name, table)) goto end; diff --git a/sql/handler.h b/sql/handler.h index e4903172c33..ebe23b97062 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1765,6 +1766,12 @@ handlerton *ha_default_tmp_handlerton(THD *thd); */ #define HTON_TABLE_MAY_NOT_EXIST_ON_SLAVE (1 << 15)

+/* + True if handler cannot roolback transactions. If not true, the transaction + will be put in the transactional binlog cache. +*/ +#define HTON_NO_ROLLBACK (1 << 16)

How is it different from HA_PERSISTENT_TABLE?

+ class Ha_trx_info;

struct THD_TRANS diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 582c9bb110b..0c53bdc5bbe 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -11016,7 +11016,7 @@ int ha_partition::check_misplaced_rows(uint read_part_id, bool do_repair) If the engine supports transactions, the failure will be rollbacked.

if you're changing it anyway, can you also change rollbacked -> rolled back ? thanks.

*/ - if (!m_file[correct_part_id]->has_transactions()) + if (!m_file[correct_part_id]->has_transactions_and_rollback()) { /* Log this error, so the DBA can notice it and fix it! */ sql_print_error("Table '%-192s' failed to move/insert a row" diff --git a/sql/handler.cc b/sql/handler.cc index 39841cc28d7..e3f5773717d 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -4514,7 +4515,6 @@ void handler::mark_trx_read_write_internal() */ if (ha_info->is_started()) { - DBUG_ASSERT(has_transaction_manager());

there should be *some* assert here, I think

/* table_share can be NULL in ha_delete_table(). See implementation of standalone function ha_delete_table() in sql_base.cc. diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 250da9948a0..1f3bf212d3f 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7961,3 +7961,5 @@ ER_KEY_CONTAINS_PERIOD_FIELDS eng "Key %`s cannot explicitly include column %`s" ER_KEY_CANT_HAVE_WITHOUT_OVERLAPS eng "Key %`s cannot have WITHOUT OVERLAPS" +ER_DATA_WAS_COMMITED_UNDER_ROLLBACK + eng "Engine %s does not support rollback. Changes where commited during rollback call"

A confusing error message. I'd just say "table is not transactional". This is the user point of view difference between a "transactional engine" like InnoDB and "crash safe engine" like Aria. It's very confusing to invent a new category of "transactional engines that cannot roll back" even if internally Aria is treated like one.

diff --git a/sql/sp.cc b/sql/sp.cc index 51bbeeef368..1629290eb73 100644 --- a/sql/sp.cc +++ b/sql/sp.cc @@ -470,27 +471,29 @@ static Proc_table_intact proc_table_intact; currently open tables will be saved, and from which will be restored when we will end work with mysql.proc.

+ NOTES + On must have a start_new_trans object active when calling this function

I think this: DBUG_ASSERT(thd->transcation != &thd->default_transaction); could be a bit safer than a NOTE :)

+ @retval 0 Error @retval \# Pointer to TABLE object of mysql.proc */

-TABLE *open_proc_table_for_read(THD *thd, Open_tables_backup *backup) +TABLE *open_proc_table_for_read(THD *thd) { TABLE_LIST table; - DBUG_ENTER("open_proc_table_for_read");

table.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PROC_NAME, NULL, TL_READ);

- if (open_system_tables_for_read(thd, &table, backup)) + if (open_system_tables_for_read(thd, &table)) DBUG_RETURN(NULL);

if (!proc_table_intact.check(table.table, &proc_table_def)) DBUG_RETURN(table.table);

- close_system_tables(thd, backup); + thd->commit_whole_transaction_and_close_tables();

DBUG_RETURN(NULL); } @@ -517,7 +520,8 @@ static TABLE *open_proc_table_for_update(THD *thd) MDL_savepoint mdl_savepoint= thd->mdl_context.mdl_savepoint(); DBUG_ENTER("open_proc_table_for_update");

same? also need start_new_trans?

- table_list.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PROC_NAME, NULL, TL_WRITE); + table_list.init_one_table(&MYSQL_SCHEMA_NAME, &MYSQL_PROC_NAME, NULL, + TL_WRITE);

if (!(table= open_system_table_for_update(thd, &table_list))) DBUG_RETURN(NULL); diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 6c938670fdc..325a0f1d41d 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4259,7 +4259,7 @@ bool open_tables(THD *thd, const DDL_options_st &options, list, we still need to call open_and_process_routine() to take MDL locks on the routines. */ - if (thd->locked_tables_mode <= LTM_LOCK_TABLES) + if (thd->locked_tables_mode <= LTM_LOCK_TABLES && *sroutine_to_open)

why?

{ /* Process elements of the prelocking set which are present there @@ -8887,17 +8887,16 @@ bool is_equal(const LEX_CSTRING *a, const LEX_CSTRING *b) open_system_tables_for_read() thd Thread context. table_list List of tables to open. - backup Pointer to Open_tables_state instance where - information about currently open tables will be - saved, and from which will be restored when we will - end work with system tables.

NOTES + Caller should have used start_new_trans object to start a new + transcation when reading system tables.

assert, may be?

+ Thanks to restrictions which we put on opening and locking of system tables for writing, we can open and lock them for reading - even when we already have some other tables open and locked. One - must call close_system_tables() to close systems tables opened - with this call. + even when we already have some other tables open and locked. + One should call thd->commit_whole_transaction_and_close_tables() + to close systems tables opened with this call.

NOTES In some situations we use this function to open system tables for diff --git a/sql/sql_help.cc b/sql/sql_help.cc index c9307b578fc..3ccab553bfe 100644 --- a/sql/sql_help.cc +++ b/sql/sql_help.cc @@ -709,8 +709,9 @@ static bool mysqld_help_internal(THD *thd, const char *mask) Reset and backup the current open tables state to make it possible. */ - Open_tables_backup open_tables_state_backup; - if (open_system_tables_for_read(thd, tables, &open_tables_state_backup)) + start_new_trans new_trans(thd);

I'm starting to think that this (and in some other places) is rather redundant. Transactions can be expensive, the overhead being quite big. It seems like a overkill to wrap read-only system table accesses in a separate transaction, they could as well be performed as a part of the parent transaction. Write accesses has to be wrapped in a dedicated transaction, of course.

+ + if (open_system_tables_for_read(thd, tables)) goto error2;

/* diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 2528134f4ee..db5b4d1c5fd 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6105,7 +6105,7 @@ static my_bool iter_schema_engines(THD *thd, plugin_ref plugin, table->field[1]->store(option_name, strlen(option_name), scs); table->field[2]->store(plugin_decl(plugin)->descr, strlen(plugin_decl(plugin)->descr), scs); - tmp= &yesno[MY_TEST(hton->commit)]; + tmp= &yesno[MY_TEST(hton->commit && !(hton->flags & HTON_NO_ROLLBACK))];

Why not to say that hton->rollback==NULL means no rollback? Then you won't need a new flag for that. And you'll remove redundancy, what would it mean if HTON_NO_ROLLBACK is present, but hton->rollback!=NULL? Or the other way around, HTON_NO_ROLLBACK is not present, but hton->rollback==NULL?

table->field[3]->store(tmp->str, tmp->length, scs); table->field[3]->set_notnull(); tmp= &yesno[MY_TEST(hton->prepare)]; diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 55e8e52c052..7a817aea97e 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -230,17 +230,17 @@ index_stat_def= {INDEX_STAT_N_FIELDS, index_stat_fields, 4, index_stat_pk_col}; Open all statistical tables and lock them */

-static int open_stat_tables(THD *thd, TABLE_LIST *tables, - Open_tables_backup *backup, bool for_write) +static int open_stat_tables(THD *thd, TABLE_LIST *tables, bool for_write) { int rc; - Dummy_error_handler deh; // suppress errors + DBUG_ASSERT(thd->transaction != &thd->default_transaction);

I see, you already use such an assert :)

+ thd->push_internal_handler(&deh); init_table_list_for_stat_tables(tables, for_write); init_mdl_requests(tables); thd->in_sub_stmt|= SUB_STMT_STAT_TABLES; - rc= open_system_tables_for_read(thd, tables, backup); + rc= open_system_tables_for_read(thd, tables); thd->in_sub_stmt&= ~SUB_STMT_STAT_TABLES; thd->pop_internal_handler();

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org