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

Reply via email to