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 <mo...@mariadb.com> > committer: Michael Widenius <mo...@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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp