Re: [Maria-developers] acbe14b122c: Aria will now register it's transactions
Hi!
On Wed, May 20, 2020 at 10:52 AM Sergei Golubchik wrote:
>
> Hi, Michael!
> > +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()?
We already discussed this on slack.
- In some cases it's an advantage/a need to end the transaction before the
variable is destroyed.
- I don't like to have things happen automatically, especially not something
as important as doing a commit. I prefer to have it spelled out.
The commit in 'destroy()' is a there mainly as a safeguard if one forgets
to call restore_old_transaction(). I may at some point add an assert to
force one to call restore_old_transaction() at least in debug code.
> > +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.
There can't be any active transactions if there is no open tables.
So this is safe.
> > +
> > + /*
> > +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
Because of unlock tables calls external_lock which affects
transactions in some engines.
This is the fastest way to safely close a transaction.
The other option would be:
commit(stmt) - close() - commit(all)
but this may in some cases be not optimal if the table is evicted from
the table cache between close() and commit().
(At least Aria will keep the read view open until the table is
commited and the external unlock is called, even over close).
> > +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(_tables_state_backup);
> > + bzero(thd->ha_data, sizeof(thd->ha_data));
> > + old_transaction= thd->transaction;
> > + thd->transaction= _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 ?
Yes, I think that would be much better. We even discussed this and I suggested
that we do that in 10.6
> 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.
Yes, and that is how things works now. The engine threats this as a new
connection and when the new transactions ends, we call hton->free_connection()
on the ha_data before restoring it.
>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()
We already have in restore_old_transaction the code:
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
> committer: Michael Widenius
> 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(_tables_state_backup);
> + bzero(thd->ha_data, sizeof(thd->ha_data));
> + old_transaction= thd->transaction;
> + thd->transaction= _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
