Hello!
Here are my comments about your patch:
* Davi Arnaut <Davi.Arnaut@stripped> [08/09/14 03:42]:
> # At a local mysql-5.1 repository of davi
>
> 2683 Davi Arnaut 2008-09-13
> Bug#34306: Can't make copy of log tables when server binary log is enabled
>
...
> sql/sql_yacc.yy
> The lock type is now decided at open_tables time. This was actually
You probably has meant "The old behavior was actually..." here, right ?
> misleading as the binary log format can be dynamically switched and this
> would not change for statements that have already been parsed when the
> binary log format is changed (ie: prepared statements).
...
> === modified file 'include/thr_lock.h'
> --- a/include/thr_lock.h 2008-02-18 22:29:39 +0000
> +++ b/include/thr_lock.h 2008-09-13 23:31:20 +0000
> @@ -29,6 +29,12 @@ extern ulong locks_immediate,locks_waite
>
> enum thr_lock_type { TL_IGNORE=-1,
> TL_UNLOCK, /* UNLOCK ANY LOCK */
> + /*
> + Parser only! At open_tables() becomes TL_READ or
> + TL_READ_NO_INSERT depending on the binary log format
> + (SBR/RBR) and on the table category (log table).
> + */
> + TL_READ_DEFAULT,
> TL_READ, /* Read lock */
> TL_READ_WITH_SHARED_LOCKS,
> /* High prior. than TL_WRITE. Allow concurrent insert */
Probably it makes sense to mention in this comment in which situations
we use this lock type. I.e. for tables that read by statements which
modify tables.
> === added file 'mysql-test/suite/binlog/r/binlog_stm_row.result'
> --- a/mysql-test/suite/binlog/r/binlog_stm_row.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_stm_row.result 2008-09-13 23:31:20 +0000
> @@ -0,0 +1,58 @@
> +DROP TABLE IF EXISTS t1;
> +DROP TABLE IF EXISTS t2;
> +SET GLOBAL BINLOG_FORMAT = STATEMENT;
> +SET SESSION BINLOG_FORMAT = STATEMENT;
> +CREATE TABLE t1 (a INT);
> +CREATE TABLE t2 LIKE t1;
> +select @@SESSION.BINLOG_FORMAT;
> +@@SESSION.BINLOG_FORMAT
> +STATEMENT
> +INSERT INTO t1 VALUES(1);
> +INSERT INTO t2 VALUES(2);
> +#
> +# Ensure that INSERT INTO .. SELECT FROM under SBR takes a read
> +# lock that will prevent the source table from being modified.
> +#
> +SELECT GET_LOCK('Bug#34306', 120);
> +GET_LOCK('Bug#34306', 120)
> +1
> +PREPARE stmt FROM "INSERT INTO t1 SELECT * FROM t2 WHERE GET_LOCK('Bug#34306',
> 120)";
> +EXECUTE stmt;;
> +INSERT INTO t2 VALUES (3);;
> +SELECT RELEASE_LOCK('Bug#34306');
> +RELEASE_LOCK('Bug#34306')
> +1
> +SELECT RELEASE_LOCK('Bug#34306');
> +RELEASE_LOCK('Bug#34306')
> +1
...
The above test results can be made much more readable by adding
something like --echo # switching to connection "con1"
to all places where you switch between connections.
> === added file 'mysql-test/suite/binlog/t/binlog_stm_row.test'
> --- a/mysql-test/suite/binlog/t/binlog_stm_row.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/binlog/t/binlog_stm_row.test 2008-09-13 23:31:20 +0000
> @@ -0,0 +1,90 @@
> +--source include/have_log_bin.inc
> +--source include/have_binlog_format_row_or_statement.inc
> +
> +# Get rid of previous tests binlog
> +--disable_query_log
> +reset master;
> +--enable_query_log
> +
> +#
> +# Bug#34306: Can't make copy of log tables when server binary log is enabled
> +#
Could you please mention that this is actually an _additional_ test for
this bug? As otherwise it is not obvious how the following test case is
related to the problem described in bug report.
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +DROP TABLE IF EXISTS t2;
> +--enable_warnings
> +
> +SET GLOBAL BINLOG_FORMAT = STATEMENT;
> +SET SESSION BINLOG_FORMAT = STATEMENT;
> +
> +CREATE TABLE t1 (a INT);
> +CREATE TABLE t2 LIKE t1;
> +select @@SESSION.BINLOG_FORMAT;
> +INSERT INTO t1 VALUES(1);
> +INSERT INTO t2 VALUES(2);
> +
> +--connect(con1,localhost,root,,)
> +--connect(con2,localhost,root,,)
> +
> +--echo #
> +--echo # Ensure that INSERT INTO .. SELECT FROM under SBR takes a read
> +--echo # lock that will prevent the source table from being modified.
> +--echo #
...
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc 2008-09-03 14:45:40 +0000
> +++ b/sql/sql_base.cc 2008-09-13 23:31:20 +0000
> @@ -5036,6 +5061,9 @@ int decide_logging_format(THD *thd, TABL
> void* prev_ht= NULL;
> for (TABLE_LIST *table= tables; table; table= table->next_global)
> {
> + TABLE_SHARE *share= table->table ? table->table->s : NULL;
> + if (share && share->table_category == TABLE_CATEGORY_PERFORMANCE)
> + thd->lex->set_stmt_unsafe();
> if (!table->placeholder() && table->lock_type >=
> TL_WRITE_ALLOW_WRITE)
> {
> ulonglong const flags= table->table->file->ha_table_flags();
>
Could you please explain in which cases you need this check on
TABLE_LIST::table? Is it for view? May be it is better to be
consistent and use TABLE_LIST::placeholder() in this case ?
Also I think you use the opportunity and move the following check
from lock_tables() code:
/*
If we have >= 2 different tables to update with auto_inc columns,
statement-based binlogging won't work. We can solve this problem in
mixed mode by switching to row-based binlogging:
*/
if (thd->variables.binlog_format == BINLOG_FORMAT_MIXED &&
has_two_write_locked_tables_with_auto_increment(tables))
{
...
to the decide_logging_format() function (probably in separate
patch for 6.0 tree).
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy 2008-09-05 10:44:16 +0000
> +++ b/sql/sql_yacc.yy 2008-09-13 23:31:20 +0000
> @@ -4299,7 +4299,7 @@ create_select:
> SELECT_SYM
> {
> LEX *lex=Lex;
> - lex->lock_option= using_update_log ? TL_READ_NO_INSERT : TL_READ;
> + lex->lock_option= TL_READ_DEFAULT;
> if (lex->sql_command == SQLCOM_INSERT)
> lex->sql_command= SQLCOM_INSERT_SELECT;
> else if (lex->sql_command == SQLCOM_REPLACE)
>
IMO it also makes sense to fix another place in sql_yacc.yy where we
have similar choice as well. After all there is no good reason why
something like "INSERT INTO t1 SELECT * FROM mysql.slow_log" should
work and something like "INSERT INTO t1 (count) VALUES ((SELECT count(*)
FROM mysql.slow_log" should not.
Also what about other places where we have similar choice -
mysql_init_multi_delete() and mysql_multi_update_prepare() ?
And if we decide that we need to get rid of those places as well we
can consider getting rid of using_update_log variable completely...
Overall I think it is OK to push this patch after considering and
discussing above comments!
--
Dmitry Lenev, Software Developer
MySQL AB, www.mysql.com
Are you MySQL certified? http://www.mysql.com/certification
