Hi Andrei and Jasonh,
See some comments in-line.
Cheers.
Andrei Elkin wrote:
> Alfranio, hello.
>
> Thanks for addressing ealier review suggestions.
> Please find some questions.
>
>
>> Hi Andrei and Jasonh,
>>
>> There is a new patch at http://lists.mysql.com/commits/83803
>>
>> Cheers.
>>
>> Andrei Elkin wrote:
>>
>>> Alfranio, hello.
>>>
>>>
>>>
>>>> Hi Andrei and Jasonh,
>>>>
>>>> I've implemented what follows as we have discussed yesterday:
>>>>
>>>> 1 - The decision on what type of cache to use is delegated to
>>>> the log.cc file, specifically to MYSQL_BIN_LOG::write.
>>>>
>>>> 2 - In order to temporary identify a DDL, I've used
>>>> CF_AUTO_COMMIT_TRANS as suggested by Jasonh.
>>>>
>>>>
>>>>
The flag above does not really reflects what happens in practice.
CREATE TEMPORARY TABLES does not commit a transaction but has the flag
CF_AUTO_COMMIT_TRANS.
This makes it being logged outside the boundaries of a transaction. On
the other hand, DROP FUNCTION
commits a transaction but does not have the flag and that's why it is
wrapped up by a BEGIN/COMMIT.
So I was wondering if we should implement andrei's approach. What do you
think?
Find a test case in attachment.
Log_name Pos Event_type Server_id End_log_pos Info
master-bin.000001 4 Format_desc 1 107 Server
ver: 6.0.14-alpha-debug-log, Binlog ver: 4
master-bin.000001 107 Query 1 207 use `test`;
CREATE TABLE t1 (a int) engine=innodb
master-bin.000001 207 Query 1 436 use `test`;
CREATE DEFINER=`root`@`localhost` FUNCTION `fc_i_nt_5_suc`(p_trans_id
INTEGER, p_stmt_idINTEGER) RETURNS varchar(64) CHARSET latin1
BEGIN
RETURN "fc_i_nt_5_suc";
END
master-bin.000001 436 Query 1 532 use `test`;
CREATE TEMPORARY TABLE tt1(a int)
master-bin.000001 532 Query 1 600 BEGIN
master-bin.000001 600 Query 1 687 use `test`;
INSERT INTO t1 VALUES(1)
master-bin.000001 687 Query 1 774 use `test`;
INSERT INTO t1 VALUES(2)
master-bin.000001 774 Xid 1 801 COMMIT /* xid=16 */
master-bin.000001 801 Query 1 869 BEGIN
master-bin.000001 869 Query 1 959 use `test`; DROP
FUNCTION fc_i_nt_5_suc
master-bin.000001 959 Query 1 1028 COMMIT
master-bin.000001 1028 Query 1 1125 use `test`;
CREATE TEMPORARY TABLE tt2 (a int)
>>>>
>>> I still think it would be better if DDL callers marked THD instance
>>> specifically. However, my objection is not critical.
>>>
>>>
>>>
>>>> 3 - I am not calling thd->binlog_start_trans_and_stmt() when
>>>> writing directly to the binary log, e.g. DDL.
>>>>
>>>> 4 - To further influencing what type of cache to use, I've introduced
>>>> a flag (NO_CACHE) which is stored along with the flag is_transactional
>>>> in a bit map.
>>>>
>>>> 5 - I also addressed some other minor details requested by Andrei.
>>>>
>>>>
>
> As we talked today on #rep it seems to be possible to migrate by `pending'
> from the cache to the magnager class. There are some benefits we would gain
> specifically
> that would facilitate preserving the pre-patch no-arg
> signature to MYSQL_BIN_LOG::methods dealing with `pending'.
>
>
Although it is possible, this would generate unnecessary extra events.
Think about the case of M statement which updates several T-rows and
for each row a trigger is fired to update an N-row.
So, if you don't mind I would keep things as they are.
> Among other things I am still seeing a block
>
>
> + else if (!stmt_has_updated_trans_table(thd) &&
> + cache_mngr->trx_cache.empty() &&
> + !event_info->is_trans_event())
> + { ... }
>
> where !event_info->is_trans_event() is inferred by
> !stmt_has_updated_trans_table(thd).
> To remind, there was chatting on #rep ended with
> Sep 16 17:20:02 <alfranio> andrei, I agree. event_info->is_trans_event is
> reduntant.
> I suggest assert(!event_info->is_trans_event()) inside of the block.
>
Done.
> In the new patch
>
> === modified file 'sql/log_event.h'
> --- a/sql/log_event.h 2009-06-30 19:04:13 +0000
> +++ b/sql/log_event.h 2009-09-19 12:33:24 +0000
> @@ -870,6 +870,26 @@ public:
> EVENT_SKIP_COUNT
> };
>
> + enum enum_type_cache
> + {
> + /*
> + If possible the event should use a non-transactional cache before
> + being flushed to the binary log. This means that it must be flushed
> + right after its correspondent statement is completed.
> + */
> + EVENT_NON_TRANSACTIONAL_CACHE = 1,
> + /*
> + The event should use a transactional cache before being flushed to
> + the binary log. This means that it must be flushed upon commit or
> + rollback.
> + */
> + EVENT_TRANSACTIONAL_CACHE = 2,
> + /*
> + The event must be written directly to the binary log without going
> + through a cache.
> + */
> + EVENT_NO_CACHE = 4
> + };
>
> the enum value are set in a "bitmap" style. I did not get quite why not continuous
> numbering?
>
> Considering the definition
>
> + inline bool has_ignored_trans_event()
> + {
> + return (type_cache & Log_event::EVENT_NO_CACHE);
> + }
>
>
> and its use cases like
>
> + if (is_ddl || event_info->has_ignored_trans_event())
> + {
> + file= &log_file;
>
> maybe the name should just say event->use_direct_logging() or something?
>
Done.
> I did ask before although the question came to my mind really.
> Why the extra new arg to
>
> @@ -6427,14 +6612,11 @@ bool MYSQL_BIN_LOG::write_incident(THD *
> */
>
> bool MYSQL_BIN_LOG::write(THD *thd, IO_CACHE *cache, Log_event *commit_event,
> - bool incident)
> + bool incident, bool is_transactional)
>
> ?
>
> Indeed, for flushing the N-cache the arg is 0 as well as commit_event is 0
>
> @@ -2736,10 +2769,56 @@ static int binlog_prepare(handlerton *ht
> ...
>
> + bool const is_transactional= FALSE;
> + IO_CACHE *cache_log= &cache_mngr->stmt_cache.cache_log;
> + thd->binlog_flush_pending_rows_event(TRUE, is_transactional);
> + if ((error= mysql_bin_log.write(thd, cache_log, 0,
> + cache_mngr->stmt_cache.has_incident(),
> + is_transactional)))
>
> while for doing it to the T-cache the flag is 1 and commit_event is provided by the
> caller as
> one of COMMIT or ROLLBACK query events.
>
> Finally,
>
> +
> + /*
> + Set the statement as unsafe if:
> +
> + . it is a mixed statement, i.e. access transactional and non-transactional
> + tables, and updates at least one;
>
> We spoke on that matter on #rep on Fri and I still don't see why
> INSERT into N SELECT from T is unsafe when the query is the first after BEGIN.
> While the T-cache is empty any queries after BEGIN modifying solely a N-table should
> be safe.
> And we should log them in front of BEGIN.
>
>
>
>
Done.
>>>>
>>>>
>>> More important is to provide comments as in the code - e.g the
>>> heading synopsis lines for new functions - as well as per file
>>> comments in the cset.
>>>
>>>
>>>
>>>> I am running mtr to check if everything is ok.
>>>> I will commit a patch at the end of the day and ping you both.
>>>>
>>>>
>>>>
>>>>
>>> I am ready to look at it today.
>>>
>>> good luck,
>>>
>>> Andrei
>>>
>>>
>>>
>>>> Cheers.
>>>>
>>>>
>
> cheers,
>
> Andrei
>
>
--source include/have_log_bin.inc
--source include/have_innodb.inc
--source include/have_binlog_format_statement.inc
CREATE TABLE t1 (a int) engine=innodb;
DELIMITER |;
CREATE FUNCTION fc_i_nt_5_suc (p_trans_id INTEGER, p_stmt_id INTEGER) RETURNS VARCHAR(64)
BEGIN
RETURN "fc_i_nt_5_suc";
END|
DELIMITER ;|
BEGIN;
INSERT INTO t1 VALUES(1);
CREATE TEMPORARY TABLE tt1(a int);
INSERT INTO t1 VALUES(2);
DROP FUNCTION fc_i_nt_5_suc;
ROLLBACK;
CREATE TEMPORARY TABLE tt2 (a int);
SELECT * FROM t1;
SHOW BINLOG EVENTS;
DROP TABLE t1, tt1, tt2;
exit;
