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.
>>>
>>>
>>
>> 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'.
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.
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?
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.
>>>
>>
>> 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
