Re: bzr commit into mysql-5.1-bugteam branch (davi:2741) Bug#37016

[Guilhem Bichot]

Hello,

Davi Arnaut a écrit, Le 12/13/2008 11:58 AM:
> # At a local mysql-5.1-bugteam repository of davi
> 
>  2741 Davi Arnaut	2008-12-13
>       Bug#37016: TRUNCATE TABLE removes some rows but not all
>       
>       The special TRUNCATE TABLE (DDL) transaction wasn't being properly
>       rolled back if a error occurred during row by row deletion. The
>       error can be caused by a foreign key restriction imposed by InnoDB
>       SE and woudl cause the server to erroneously issue a implicit
>       commit.
>       
>       The solution is to rollback the transaction if a truncation via row
>       by row deletion fails, otherwise commit. If the truncation is
>       emulated by a table recreation, always issue a implicit commit.
>       
>       This makes TRUNCATE TABLE a special case of DDL because a implicit
>       commit is NOT issued regardless of the outcome of the operation --
>       a implicit rollback is issued if row by row deletion fails.

As I was nit-picking on IRC, that last paragraph will be rephrased.

> modified:
>   mysql-test/include/commit.inc
>   mysql-test/r/commit_1innodb.result
>   mysql-test/r/innodb_mysql.result
>   mysql-test/t/innodb_mysql.test
>   sql/sql_delete.cc
> 
> per-file messages:
>   mysql-test/include/commit.inc
>     Truncate always starts a transaction and commits at the end.
>   mysql-test/r/commit_1innodb.result
>     Update test case results.
>   mysql-test/r/innodb_mysql.result
>     Add test case result for Bug#37016
>   mysql-test/t/innodb_mysql.test
>     Add test case for Bug#37016
>   sql/sql_delete.cc
>     Move truncation using row by row deletion to its own function.
>     If row by row deletion fails, rollback the transaction.
>     
>     Truncate is considered a DDL, hence it should have it's own special
>     transaction. Regardless of the outcome of the truncation operation,
>     always commit the transaction. If the truncation was emulated via
>     a row by row deletion, the transaction is already finished.
> === modified file 'mysql-test/include/commit.inc'
> --- a/mysql-test/include/commit.inc	2008-12-12 12:52:20 +0000
> +++ b/mysql-test/include/commit.inc	2008-12-13 10:58:32 +0000
> @@ -617,10 +617,10 @@ call p_verify_status_increment(0, 0, 0, 
>  --echo
>  --echo # No test because of Bug#8729 "rename table fails on temporary table"
>  
> ---echo # 24. DDL: TRUNCATE TEMPORARY TABLE, does not start a transaction
> +--echo # 24. DDL: TRUNCATE TEMPORARY TABLE
>  --echo
>  truncate table t2;
> -call p_verify_status_increment(2, 0, 2, 0);
> +call p_verify_status_increment(4, 0, 4, 0);

So in mixed and row-based, the patch adds 2 commits.

>  commit;
>  --echo # There is nothing left to commit
>  call p_verify_status_increment(0, 0, 0, 0);
> @@ -733,7 +733,7 @@ call p_verify_status_increment(1, 0, 1, 
>  rename table t4 to t3;
>  call p_verify_status_increment(1, 0, 1, 0);
>  truncate table t3;
> -call p_verify_status_increment(2, 2, 2, 2);
> +call p_verify_status_increment(4, 4, 2, 2);


So in mixed, the patch adds 2 commits, but in row-based 0?
I wonder what's the cause of mixed!=row here. Both hunks test like this:
set autocommit=0;
truncate;
call p_verify_status_increment;
In the first hunk table is temporary... Why would that explain the 
observation...

It's also interesting to note that before as well as after the patch, 
the two hunks behaved differently regarding the prepares: first hunk 
does no prepares, second does. Mystery.

Probably nothing to worry about. And TRUNCATE is rare, so expensing two 
commits more is fine with me.

>  create view v1 as select * from t2;
>  call p_verify_status_increment(1, 0, 1, 0);
>  check table t1;



> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2008-11-03 13:08:42 +0000
> +++ b/sql/sql_delete.cc	2008-12-13 10:58:32 +0000
> @@ -951,6 +951,26 @@ bool multi_delete::send_eof()
>  ****************************************************************************/
>  
>  /*
> +  Row-by-row truncation if the engine does not support table recreation.
> +  Probably a InnoDB table.
> +*/
> +
> +static bool mysql_truncate_by_delete(THD *thd, TABLE_LIST *table_list)
> +{
> +  bool error, save_binlog_row_based= thd->current_stmt_binlog_row_based;
> +  DBUG_ENTER("mysql_truncate_by_delete");
> +  table_list->lock_type= TL_WRITE;
> +  mysql_init_select(thd->lex);
> +  thd->clear_current_stmt_binlog_row_based();
> +  error= mysql_delete(thd, table_list, NULL, NULL, HA_POS_ERROR, LL(0), TRUE);
> +  ha_autocommit_or_rollback(thd, error);
> +  end_trans(thd, error ? ROLLBACK : COMMIT);

Ok.
When you merge into 6.0, given that SQLCOM_TRUNCATE has 
CF_AUTO_COMMIT_TRANS in 6.0, as you explained, you may not need the 
end_trans() above (even if error==false: ha_autocommit_or_rollback() has 
left an empty transaction, for which doing COMMIT is as good as doing 
ROLLBACK). But I think it is safer to keep it, maybe it matters that 
commit/rollback is done *before* restoring binlog properties below 
(haven't dug, but could well be; row-based binlogging follows closely 
what happens in transaction commits/rollbacks).

> +  thd->current_stmt_binlog_row_based= save_binlog_row_based;
> +  DBUG_RETURN(error);
> +}
> +
> +
> +/*
>    Optimize delete of all rows by doing a full generate of the table
>    This will work even if the .ISM and .ISD tables are destroyed
>  
> @@ -978,7 +998,7 @@ bool mysql_truncate(THD *thd, TABLE_LIST
>      handlerton *table_type= table->s->db_type();
>      TABLE_SHARE *share= table->s;
>      if (!ha_check_storage_engine_flag(table_type, HTON_CAN_RECREATE))
> -      goto trunc_by_del;
> +      DBUG_RETURN(mysql_truncate_by_delete(thd, table_list));
>  
>      table->file->info(HA_STATUS_AUTO | HA_STATUS_NO_LOCK);
>      
> @@ -1014,7 +1034,7 @@ bool mysql_truncate(THD *thd, TABLE_LIST
>      }
>      if (!ha_check_storage_engine_flag(ha_resolve_by_legacy_type(thd, table_type),
>                                        HTON_CAN_RECREATE))
> -      goto trunc_by_del;
> +      DBUG_RETURN(mysql_truncate_by_delete(thd, table_list));

If we compare
DBUG_RETURN(func());
to
bool res= func();
DBUG_RETURN(res);
The former is more compact, but has the defect that in the --debug trace 
there will be:
 >caller
<caller
 >func
<func
which is misleading as what actually happens is
 >caller
 >func
<func
<caller

This is a minor remark which I follow in my own code, you don't need to 
change if you prefer the form which you used.

>  
>      if (lock_and_wait_for_table_name(thd, table_list))
>        DBUG_RETURN(TRUE);
> @@ -1052,27 +1072,9 @@ end:
>      unlock_table_name(thd, table_list);
>      VOID(pthread_mutex_unlock(&LOCK_open));
>    }
> -  DBUG_RETURN(error);
>  
> -trunc_by_del:
> -  /* Probably InnoDB table */
> -  ulonglong save_options= thd->options;
> -  table_list->lock_type= TL_WRITE;
> -  thd->options&= ~(OPTION_BEGIN | OPTION_NOT_AUTOCOMMIT);
> -  ha_enable_transaction(thd, FALSE);
> -  mysql_init_select(thd->lex);
> -  bool save_binlog_row_based= thd->current_stmt_binlog_row_based;
> -  thd->clear_current_stmt_binlog_row_based();
> -  error= mysql_delete(thd, table_list, (COND*) 0, (SQL_LIST*) 0,
> -                      HA_POS_ERROR, LL(0), TRUE);
> -  ha_enable_transaction(thd, TRUE);
> -  /*
> -    Safety, in case the engine ignored ha_enable_transaction(FALSE)
> -    above. Also clears thd->transaction.*.
> -  */
> -  error= ha_autocommit_or_rollback(thd, error);
> -  ha_commit(thd);
> -  thd->options= save_options;
> -  thd->current_stmt_binlog_row_based= save_binlog_row_based;
> +  ha_autocommit_or_rollback(thd, 0);
> +  end_active_trans(thd);
> +
>    DBUG_RETURN(error);
>  }

The last 3 added lines above, are for a situation which is not 
row-by-row deletion, so what is the reason they are added (how do they 
relate to the bug report)?
I believe you describe this change with "If the truncation is 
emulated by a table recreation, always issue a implicit commit", but, 
was there a problem at all when using table recreation? Why was the 
addition needed?

-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com