Re: bzr commit into mysql-6.0 branch (davi:2682) WL#4284

[Konstantin Osipov]

* Davi Arnaut <Davi.Arnaut@stripped> [08/07/19 05:32]:
>  2682 Davi Arnaut	2008-07-18
>       WL#4284: Transactional DDL locking
>       
>       Chapter I - SQL statements' effect on transactions.

A patch that renames code, moves it around and on top of that introduce
semantical changes to it is very hard to review. 
ha_* methods are part of the storage engine api, we need to make
sure we don't break storage engines with this change (or if we do,
we're fine doing that - I will check with Serg).

Could you please split the patch into two, one that moves
transaction-related functions from handler.h to transaction.h and renames them
(if you indeed need to), and another that changes implementation
of implicit commit?

I don't understand some changes that you've made, since I don't 
understand the problems that you encountered on your way.
Have you kept track of those? I would like to know the full list
of problems you encountered after the naive implementation, which simply 
adds an implicit commit at the beginning of certain ddl statements
and at the end of them. Which ddl statements issue an
implicit commit inside them? What is the problem with truncate?


>   mysql-test/include/commit.inc
>     Alter table rename was not committing the normal transaction and
>     the commit was being issued in the next DDL command (rename table)
>     that actually doesn't commit at all. Other changes are to take
>     into account the implicit commits issued before and after the DDL
>     command execution.

What do you mean "actually doesn't commit at all"? 
Why did statistics counters change? The counters change only if
the commit was not a no-op, did these statements start a
transaction? If so, how did it happen?

>   mysql-test/include/wrap_trans_log.inc
>     Add auxiliary test that shows if a statement issued a
>     implicit commit.

This is a nice tool, but I think it's output is a bit too verbose.
There are other ways to check if there was an implicit commit:

set @@autocommit=off;
insert into trans_table (a) values (1);
<statement in question>
rollback;
select 1 as yes from trans_table;

Ideally all what needs to be in the test file is:

<statement in question>
call test_if_commit();

And in the log:

<statement in question>
call test_if_commit();
yes (or no)

> === modified file 'mysql-test/include/commit.inc'
> --- a/mysql-test/include/commit.inc	2008-04-01 15:13:57 +0000
> +++ b/mysql-test/include/commit.inc	2008-07-19 01:23:51 +0000
> @@ -726,15 +726,15 @@ call p_verify_status_increment(4, 4, 4, 
>  alter table t3 add column (b int);
>  call p_verify_status_increment(2, 0, 2, 0);
>  alter table t3 rename t4;
> -call p_verify_status_increment(1, 0, 1, 0);
> +call p_verify_status_increment(2, 0, 2, 0);

Number of "handler_commit" calls doubled. 
Could you explain where the commit is invoked from?

>  int ha_prepare(THD *thd)
>  {
> -  int error=0, all=1;
> -  THD_TRANS *trans=all ? &thd->transaction.all :
> &thd->transaction.stmt;
> +  int error= 0;
> +  THD_TRANS *trans= &thd->transaction.all;
>    Ha_trx_info *ha_info= trans->ha_list;
>    DBUG_ENTER("ha_prepare");
>  
> @@ -932,10 +932,10 @@ int ha_prepare(THD *thd)
>        status_var_increment(thd->status_var.ha_prepare_count);
>        if (ht->prepare)
>        {
> -        if ((err= ht->prepare(ht, thd, all)))
> +        if ((err= ht->prepare(ht, thd, TRUE)))
>          {
>            my_error(ER_ERROR_DURING_COMMIT, MYF(0), err);
> -          ha_rollback_trans(thd, all);
> +          ha_rollback_trans(thd, TRUE);
>            error=1;
>            break;

Why did you stop using the two-phase commit protocol for statement
transactions?


> === modified file 'sql/set_var.cc'
> --- a/sql/set_var.cc	2008-07-04 10:48:08 +0000
> +++ b/sql/set_var.cc	2008-07-19 01:23:51 +0000
> @@ -2973,7 +2973,7 @@ static bool set_option_autocommit(THD *t
>     */
>    if (var->save_result.ulong_value != 0 &&
>        (thd->options & OPTION_NOT_AUTOCOMMIT) &&
> -      ha_commit(thd))
> +      trans_commit(thd))
>      return 1;

What's the point of this commit if you do an implicit commit at
start of the statement now?

>    if (!(thd->state_flags & Open_tables_state::BACKUPS_AVAIL))
>    {
>      thd->main_da.can_overwrite_status= TRUE;
> -    ha_autocommit_or_rollback(thd, thd->is_error());
> +    thd->is_error() ? trans_rollback_stmt(thd) : trans_commit_stmt(thd);

What's the advantage of the new naming scheme?

> @@ -36,7 +36,7 @@ bool mysql_do(THD *thd, List<Item> &valu
>        will clear the error and the rollback in the end of
>        dispatch_command() won't work.
>      */
> -    ha_autocommit_or_rollback(thd, thd->is_error());
> +    trans_rollback_stmt(thd);

A bug?

> @@ -1403,7 +1322,8 @@ bool dispatch_command(enum enum_server_c
>  
>    /* If commit fails, we should be able to reset the OK status. */
>    thd->main_da.can_overwrite_status= TRUE;
> -  ha_autocommit_or_rollback(thd, thd->is_error());
> +  thd->is_error() ? trans_rollback_stmt(thd) : trans_commit_stmt(thd);
> +  opt_implicit_commit(thd, CF_IMPLICIT_COMMIT_END);
>    thd->main_da.can_overwrite_status= FALSE;

Why opt_implicit_commit(begin) is in mysql_execute_command(), and
opt_implicit_commit(end) is in dispatch_command()?
Ah, that's because it got to be after close_thread_tables?
What happens for COM_ commands that don't involve parsing? What's
the SQLCOM value for them?
Ideally, even though I realize it's a scope creep, we need to make
sure that everything that touches the transaction or tables
goes through mysql_execute_command. After that we can move
close_thread_tables() to it.

> @@ -4219,8 +4217,8 @@ static bool mysql_admin_table(THD* thd, 
>        DBUG_PRINT("admin", ("calling prepare_func"));
>        switch ((*prepare_func)(thd, table, check_opt)) {
>        case  1:           // error, message written to net
> -        ha_autocommit_or_rollback(thd, 1);
> -        end_trans(thd, ROLLBACK);
> +        trans_rollback_stmt(thd);
> +        trans_rollback(thd);

Why do you need these, you already have an implicit commit at the
end? This at least deserves a comment.

> @@ -4278,8 +4276,8 @@ static bool mysql_admin_table(THD* thd, 
>        length= my_snprintf(buff, sizeof(buff), ER(ER_OPEN_AS_READONLY),
>                            table_name);
>        protocol->store(buff, length, system_charset_info);
> -      ha_autocommit_or_rollback(thd, 0);
> -      end_trans(thd, COMMIT);
> +      trans_commit_stmt(thd);
> +      trans_commit(thd);

Same here.

> +++ b/sql/transaction.cc	2008-07-19 01:23:51 +0000
> @@ -0,0 +1,365 @@
> +/* Copyright (C) 2008 Sun/MySQL
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
> +
> +
> +#ifdef USE_PRAGMA_IMPLEMENTATION
> +#pragma implementation				// gcc: Class implementation
> +#endif
> +
> +#include "mysql_priv.h"

I would love to move it to a separate implementation file
if not for this include.

Unfortunately, your current split only adds a new header to the
system, but does not increase the modularity much, only time
to compile has grown.

> --- a/sql/transaction.h	1970-01-01 00:00:00 +0000
> +++ b/sql/transaction.h	2008-07-19 01:23:51 +0000
> @@ -0,0 +1,38 @@

Is this header file self-contained? Can I include it without other
headers?


--