* Alfranio Correia <Alfranio.Correia@stripped> [10/04/26 23:54]:
> 3017 Alfranio Correia 2010-04-26
> BUG#50479 DDL stmt on row-only/stmt-only tables generate spurious binlog_format
> errors
Thank you for waiting for my review.
The patch looks good, a few nitpicks below, which are not required
for approval.
> === added file 'mysql-test/suite/binlog/r/binlog_spurious_ddl_errors.result'
> --- a/mysql-test/suite/binlog/r/binlog_spurious_ddl_errors.result 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_spurious_ddl_errors.result 2010-04-26 19:31:14
> +0000
> @@ -0,0 +1,53 @@
> +SET @old_binlog_format= @@global.binlog_format;
> +INSTALL PLUGIN example SONAME 'ha_example.so';
Are you sure you don't want to add explicit coverage for all
cases when the flag is set, not just the cases when it's not set?
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2010-04-26 09:02:29 +0000
> +++ b/sql/log_event.cc 2010-04-26 19:31:14 +0000
> @@ -2470,90 +2470,43 @@ Query_log_event::Query_log_event(THD* th
> cache.
> */
> LEX *lex= thd->lex;
> - bool implicit_commit= FALSE;
> - bool force_trans= FALSE;
> + bool use_cache= FALSE, force_trans= FALSE;
A comment about what each variable is supposed to mean would be
nice.
/*
TRUE if use the binlog transactional cache. We use the
transactional cache for statements that can generate row events
and use transactional engines (right?)
*/
bool use_cache= FALSE;
/* Kostja has no clue what it means. */
bool force_trans=FALSE;
> cache_type= Log_event::EVENT_INVALID_CACHE;
> switch (lex->sql_command)
> {
> - case SQLCOM_ALTER_DB:
> - case SQLCOM_CREATE_FUNCTION:
> - case SQLCOM_DROP_FUNCTION:
> - case SQLCOM_DROP_PROCEDURE:
> - case SQLCOM_INSTALL_PLUGIN:
> - case SQLCOM_UNINSTALL_PLUGIN:
> - case SQLCOM_ALTER_TABLESPACE:
> - implicit_commit= TRUE;
> - break;
> case SQLCOM_DROP_TABLE:
> - force_trans= lex->drop_temporary &&
> thd->in_multi_stmt_transaction();
> - implicit_commit= !force_trans;
> + use_cache= force_trans= (lex->drop_temporary &&
> + thd->in_multi_stmt_transaction());
> break;
> - case SQLCOM_ALTER_TABLE:
> +
> case SQLCOM_CREATE_TABLE:
> - force_trans= (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE)
> &&
> - thd->in_multi_stmt_transaction();
> - implicit_commit= !force_trans &&
> - !(lex->select_lex.item_list.elements &&
> - thd->is_current_stmt_binlog_format_row());
> + use_cache= force_trans=
> + ((lex->create_info.options & HA_LEX_CREATE_TMP_TABLE)
> &&
> + thd->in_multi_stmt_transaction()) ||
> + (lex->select_lex.item_list.elements &&
> + thd->is_current_stmt_binlog_format_row());
> break;
> case SQLCOM_SET_OPTION:
> - implicit_commit= (lex->autocommit ? TRUE : FALSE);
> + use_cache= force_trans= (lex->autocommit ? FALSE : TRUE);
> break;
> - /*
> - Replace what follows after CF_AUTO_COMMIT_TRANS is backported by:
> -
> - default:
> - implicit_commit= ((sql_command_flags[lex->sql_command] &
> - CF_AUTO_COMMIT_TRANS));
> - break;
> - */
> - case SQLCOM_CREATE_INDEX:
> - case SQLCOM_TRUNCATE:
> - case SQLCOM_CREATE_DB:
> - case SQLCOM_DROP_DB:
> - case SQLCOM_ALTER_DB_UPGRADE:
> - case SQLCOM_RENAME_TABLE:
> - case SQLCOM_DROP_INDEX:
> - case SQLCOM_CREATE_VIEW:
> - case SQLCOM_DROP_VIEW:
> - case SQLCOM_CREATE_TRIGGER:
> - case SQLCOM_DROP_TRIGGER:
> - case SQLCOM_CREATE_EVENT:
> - case SQLCOM_ALTER_EVENT:
> - case SQLCOM_DROP_EVENT:
> - case SQLCOM_REPAIR:
> - case SQLCOM_OPTIMIZE:
> - case SQLCOM_ANALYZE:
> - case SQLCOM_CREATE_USER:
> - case SQLCOM_DROP_USER:
> - case SQLCOM_RENAME_USER:
> - case SQLCOM_REVOKE_ALL:
> - case SQLCOM_REVOKE:
> - case SQLCOM_GRANT:
> - case SQLCOM_CREATE_PROCEDURE:
> - case SQLCOM_CREATE_SPFUNCTION:
> - case SQLCOM_ALTER_PROCEDURE:
> - case SQLCOM_ALTER_FUNCTION:
> - case SQLCOM_ASSIGN_TO_KEYCACHE:
> - case SQLCOM_PRELOAD_KEYS:
> - case SQLCOM_FLUSH:
> - case SQLCOM_RESET:
> - case SQLCOM_CHECK:
> - implicit_commit= TRUE;
> + case SQLCOM_RELEASE_SAVEPOINT:
> + case SQLCOM_ROLLBACK_TO_SAVEPOINT:
> + case SQLCOM_SAVEPOINT:
> + use_cache= force_trans= TRUE;
> break;
> default:
> - implicit_commit= FALSE;
> + use_cache= can_generate_rows(thd);
> break;
> }
>
> - if (implicit_commit || direct)
> + if (!use_cache || direct)
> {
> cache_type= Log_event::EVENT_NO_CACHE;
> }
> else
> {
> - cache_type= ((using_trans || stmt_has_updated_trans_table(thd) ||
> - force_trans || thd->thread_temporary_used)
> + cache_type= ((using_trans || stmt_has_updated_trans_table(thd)
> + || force_trans || thd->thread_temporary_used)
> ? Log_event::EVENT_TRANSACTIONAL_CACHE :
> Log_event::EVENT_STMT_CACHE);
> }
>
> === modified file 'sql/sql_class.cc'
> --- a/sql/sql_class.cc 2010-04-26 09:02:29 +0000
> +++ b/sql/sql_class.cc 2010-04-26 19:31:14 +0000
> @@ -3145,6 +3145,11 @@ extern "C" bool thd_binlog_filter_ok(con
> {
> return binlog_filter->db_ok(thd->db);
> }
> +
> +extern "C" bool thd_generates_rows(const MYSQL_THD thd)
> +{
> + return can_generate_rows(thd);
> +}
Suggest to use consistency in naming -- thd_generates_rows ->
thd_sqlcom_can_generate_row_events(), can_generate_rows ->
sqlcom_can_generate_row_events().
It would make the connection between the flag, the internal
method, and the plugin api more obvious.
> #endif // INNODB_COMPATIBILITY_HOOKS */
>
> /****************************************************************************
> @@ -3866,7 +3871,8 @@ int THD::decide_logging_format(TABLE_LIS
> */
> my_error((error= ER_BINLOG_ROW_INJECTION_AND_STMT_ENGINE), MYF(0));
> }
> - else if (variables.binlog_format == BINLOG_FORMAT_ROW)
> + else if (variables.binlog_format == BINLOG_FORMAT_ROW &&
> + can_generate_rows(this))
> {
> /*
> 2. Error: Cannot modify table that uses a storage engine
> @@ -3904,7 +3910,8 @@ int THD::decide_logging_format(TABLE_LIS
> */
> my_error((error= ER_BINLOG_ROW_INJECTION_AND_STMT_MODE), MYF(0));
> }
> - else if ((flags_write_all_set & HA_BINLOG_STMT_CAPABLE) == 0)
> + else if ((flags_write_all_set & HA_BINLOG_STMT_CAPABLE) == 0 &&
> + can_generate_rows(this))
> {
> /*
> 5. Error: Cannot modify table that uses a storage engine
>
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h 2010-04-26 09:02:29 +0000
> +++ b/sql/sql_class.h 2010-04-26 19:31:14 +0000
> @@ -3520,6 +3520,12 @@ public:
> */
> #define CF_PROTECT_AGAINST_GRL (1U << 10)
>
> +/**
> + Identifies statements that may generate row events
> + and that may end up in the binary log.
> +*/
> +#define CF_CAN_GENERATE_ROW_EVENTS (1U << 11)
> +
> /* Bits in server_command_flags */
>
> /**
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2010-04-26 09:02:29 +0000
> +++ b/sql/sql_parse.cc 2010-04-26 19:31:14 +0000
> @@ -247,8 +247,19 @@ void init_update_queries(void)
> /* Initialize the sql command flags array. */
> memset(sql_command_flags, 0, sizeof(sql_command_flags));
>
> + /*
> + In general, DDL statements do not generate row events and do not go
> + through a cache before being written to the binary log. However, the
> + CREATE TABLE...SELECT is an exception because it may generate row
> + events. For that reason, the SQLCOM_CREATE_TABLE which represents
> + a CREATE TABLE, including the CREATE TABLE...SELECT, has the
> + CF_CAN_GENERATE_ROW_EVENTS flag. The distinction between a regular
> + CREATE TABLE and the CREATE TABLE...SELECT is made in other parts of
> + the code, in particular in the Query_log_event's constructor.
> + */
> sql_command_flags[SQLCOM_CREATE_TABLE]= CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE
> |
> - CF_AUTO_COMMIT_TRANS |
> CF_PROTECT_AGAINST_GRL;
> + CF_AUTO_COMMIT_TRANS |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_CREATE_INDEX]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
> sql_command_flags[SQLCOM_ALTER_TABLE]= CF_CHANGES_DATA | CF_WRITE_LOGS_COMMAND
> |
> CF_AUTO_COMMIT_TRANS |
> CF_PROTECT_AGAINST_GRL;
> @@ -256,7 +267,7 @@ void init_update_queries(void)
> CF_AUTO_COMMIT_TRANS;
> sql_command_flags[SQLCOM_DROP_TABLE]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
> sql_command_flags[SQLCOM_LOAD]= CF_CHANGES_DATA | CF_REEXECUTION_FRAGILE
> |
> - CF_PROTECT_AGAINST_GRL;
> + CF_PROTECT_AGAINST_GRL |
> CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_CREATE_DB]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
> sql_command_flags[SQLCOM_DROP_DB]= CF_CHANGES_DATA | CF_AUTO_COMMIT_TRANS;
> sql_command_flags[SQLCOM_ALTER_DB_UPGRADE]= CF_AUTO_COMMIT_TRANS;
> @@ -275,24 +286,30 @@ void init_update_queries(void)
> sql_command_flags[SQLCOM_DROP_TRIGGER]= CF_AUTO_COMMIT_TRANS;
>
> sql_command_flags[SQLCOM_UPDATE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_UPDATE_MULTI]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_INSERT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_INSERT_SELECT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_DELETE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_DELETE_MULTI]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL;
> + CF_REEXECUTION_FRAGILE |
> CF_PROTECT_AGAINST_GRL |
> + CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_REPLACE]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE;
> + CF_REEXECUTION_FRAGILE |
> CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_REPLACE_SELECT]= CF_CHANGES_DATA | CF_HAS_ROW_COUNT |
> - CF_REEXECUTION_FRAGILE;
> - sql_command_flags[SQLCOM_SELECT]= CF_REEXECUTION_FRAGILE;
> + CF_REEXECUTION_FRAGILE |
> CF_CAN_GENERATE_ROW_EVENTS;
> + sql_command_flags[SQLCOM_SELECT]= CF_REEXECUTION_FRAGILE |
> CF_CAN_GENERATE_ROW_EVENTS;
> sql_command_flags[SQLCOM_SET_OPTION]= CF_REEXECUTION_FRAGILE |
> CF_AUTO_COMMIT_TRANS;
> - sql_command_flags[SQLCOM_DO]= CF_REEXECUTION_FRAGILE;
> + sql_command_flags[SQLCOM_DO]= CF_REEXECUTION_FRAGILE |
> CF_CAN_GENERATE_ROW_EVENTS;
>
> sql_command_flags[SQLCOM_SHOW_STATUS_PROC]= CF_STATUS_COMMAND |
> CF_REEXECUTION_FRAGILE;
> sql_command_flags[SQLCOM_SHOW_STATUS]= CF_STATUS_COMMAND |
> CF_REEXECUTION_FRAGILE;
> @@ -367,8 +384,10 @@ void init_update_queries(void)
> last called (or executed) statement is preserved.
> See mysql_execute_command() for how CF_ROW_COUNT is used.
> */
> - sql_command_flags[SQLCOM_CALL]= CF_HAS_ROW_COUNT | CF_REEXECUTION_FRAGILE;
> - sql_command_flags[SQLCOM_EXECUTE]= CF_HAS_ROW_COUNT;
> + sql_command_flags[SQLCOM_CALL]= CF_HAS_ROW_COUNT | CF_REEXECUTION_FRAGILE |
> + CF_CAN_GENERATE_ROW_EVENTS;
> + sql_command_flags[SQLCOM_EXECUTE]= CF_HAS_ROW_COUNT |
> + CF_CAN_GENERATE_ROW_EVENTS;
>
> /*
> The following admin table operations are allowed
> @@ -393,7 +412,12 @@ void init_update_queries(void)
> sql_command_flags[SQLCOM_CHECK]= CF_AUTO_COMMIT_TRANS;
> }
>
> -
> +bool can_generate_rows(const THD *thd)
> +{
> + return (sql_command_flags[thd->lex->sql_command] &
> + CF_CAN_GENERATE_ROW_EVENTS);
> +}
> +
> bool is_update_query(enum enum_sql_command command)
> {
> DBUG_ASSERT(command >= 0 && command <= SQLCOM_END);
>
> === modified file 'sql/sql_parse.h'
> --- a/sql/sql_parse.h 2010-04-12 13:17:37 +0000
> +++ b/sql/sql_parse.h 2010-04-26 19:31:14 +0000
> @@ -79,6 +79,7 @@ bool check_host_name(LEX_STRING *str);
> bool check_identifier_name(LEX_STRING *str, uint max_char_length,
> uint err_code, const char *param_for_err_msg);
> bool mysql_test_parse_for_slave(THD *thd,char *inBuf,uint length);
> +bool can_generate_rows(const THD *thd);
> bool is_update_query(enum enum_sql_command command);
> bool is_log_table_write_query(enum enum_sql_command command);
> bool alloc_query(THD *thd, const char *packet, uint packet_length);
>
> === modified file 'storage/innobase/handler/ha_innodb.cc'
> --- a/storage/innobase/handler/ha_innodb.cc 2010-04-20 22:29:30 +0000
> +++ b/storage/innobase/handler/ha_innodb.cc 2010-04-26 19:31:14 +0000
> @@ -7929,6 +7929,9 @@ ha_innobase::external_lock(
> #if MYSQL_VERSION_ID > 50140
> && thd_binlog_filter_ok(thd)
> #endif /* MYSQL_VERSION_ID > 50140 */
> +#if MYSQL_VERSION_ID >= 50505
> + && thd_generates_rows(thd)
> +#endif /* MYSQL_VERSION_ID >= 50505 */
> )
> {
> char buf[256];
>
> === modified file 'storage/innobase/handler/ha_innodb.h'
> --- a/storage/innobase/handler/ha_innodb.h 2009-11-30 12:11:36 +0000
> +++ b/storage/innobase/handler/ha_innodb.h 2010-04-26 19:31:14 +0000
> @@ -266,6 +266,15 @@ void thd_mark_transaction_to_rollback(MY
> */
> bool thd_binlog_filter_ok(const MYSQL_THD thd);
> #endif /* MYSQL_VERSION_ID > 50140 */
> +#if MYSQL_VERSION_ID >= 50505
> +/**
> + Check if the query may generate row changes which
> + may end up in the binary.
> + @param thd Thread handle
> + @return 1 the query may generate row changes, 0 otherwise.
> +*/
> +bool thd_generates_rows(const MYSQL_THD thd);
> +#endif /* MYSQL_VERSION_ID >= 50505 */
> }
>
> typedef struct trx_struct trx_t;
>
--
