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(a)mariadb.com>
> committer: Michael Widenius <monty(a)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(a)mariadb.org
