Konstantin Osipov wrote:
> * 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).
There are no changes to the ha_* methods, expect for
ha_autocommit_or_rollback that shouldn't really be part of the storage
engine API.
> 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?
OK.
> I don't understand some changes that you've made, since I don't
> understand the problems that you encountered on your way.
Which ones you don't understand? Please ask!
> 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?
Truncate enables and disables transactions, while expecting the lock to
be kept around.
>> 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"?
The rename table statement does not commit at the end of the statement.
> 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?
The alter table rename command failed to commit it's changes at the end
of the statement. Since it failed to do so, the commit would be done and
accounted for in the next command that issued a implicit commit
(end_active_trans) before it's execution (rename table in this case).
>> 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)
Agree.
>> === 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?
Explained above.
>> 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?
What? I'm just removing a useless variable.
>> === 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?
It does not implicit commit. It does only implicit commit for the
Lex::autocommit is true for the SQLCOM_SET_OPTION. In sql_yacc.yy you
may see under which conditions it is set to true and "set autocommit" is
not one of then.
>> 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?
It separates the low level api (ha_*) from the higher level one
(trans_*) and as a consequence, make it less error prone to use and
easier to read.
>> @@ -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?
None the I know of. I guess you are referring to change to rollback and
it's fine because the error is always set in that branch:
if (thd->is_error())
{
/*
Rollback the effect of the statement, since next instruction
will clear the error and the rollback in the end of
dispatch_command() won't work.
*/
trans_rollback_stmt(thd);
thd->clear_error(); // DO always is OK
}
>> @@ -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?
Kind of, but it doesn't make a difference.
> What happens for COM_ commands that don't involve parsing? What's
> the SQLCOM value for them?
This is one of the defects I told you about. It's historically wrong as
it will fail to implicit commit for a multi-statement query. I plan to
move it to mysql_execute_command and introduce a server_command_flags.
> 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.
Agreed.
>
>> @@ -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.
This is a rollback, we don't to implicit rollback at the end.
>
>> @@ -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.
This is inside a for loop.. it might do other stuff before the end of
the statement.
>
>> +++ 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.
If this file is a separate one, we *must* pull in the server stuff. Or
are you suggestion that we shouldn't make new files? Please make
suggestions...
> 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?
>
No, but it's easy to change. Do you suggest to include it only in the
files that need it?
