Re: [Maria-developers] [Commits] 8a595a9: cleanup: LOAD DATA replication support in IO_CACHE
Hi Sergei, a few thoughts/questions inline. On Tue, May 26, 2015 at 03:37:36PM +0200, serg@mariadb.org wrote:
revision-id: 8a595a9825de286ad073f6f043db4c0dce88a2ea parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d committer: Sergei Golubchik branch nick: maria timestamp: 2015-05-26 15:09:25 +0200 message:
cleanup: LOAD DATA replication support in IO_CACHE
remove some 14-year old code that added support for LOAD DATA replication to IO_CACHE: * three callbacks, of which only two were actually used and that were only needed for LOAD DATA replication but were tested in every IO_CACHE instance * an additional opaque void * argument in IO_CACHE, also only used for LOAD DATA replication, but present everywhere * the code to close IO_CACHE prematurely in LOAD DATA to have these callbacks called in the correct order and a long comment explaining what will happen if IO_CACHE is not closed prematurely * a variable to track whether IO_CACHE was closed prematurely (to avoid double-closing it)
this also fixes a bug with replication of LOAD DATA LOCAL (IO_CACHE of READ_NET type was not calling these callbacks at all) I couldn't track this down. That is GET/my_b_get/pre_read was called in both cases as well as end_io_cache/pre_close. Could you elaborate?
...skip...
diff --git a/sql/sql_load.cc b/sql/sql_load.cc index 18982c5..62b6e17 100644 --- a/sql/sql_load.cc +++ b/sql/sql_load.cc @@ -1410,20 +1372,15 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs, } else { - /* - init_io_cache() will not initialize read_function member - if the cache is READ_NET. So we work around the problem with a - manual assignment - */ - need_end_io_cache = 1; - #ifndef EMBEDDED_LIBRARY if (get_it_from_net) cache.read_function = _my_b_net_read;
if (mysql_bin_log.is_open()) - cache.pre_read = cache.pre_close = - (IO_CACHE_CALLBACK) log_loaded_block; + { + cache.real_read_function= cache.read_function; + cache.read_function= log_loaded_block; + } #endif } } Will this compile with replication disabled? OTOH I guess it didn't compile even before: log_loaded_block() seem to be unavailable when replication is disabled.
IOCACHE::read_function may change, e.g. on error in _my_b_async_read(). Shouldn't we handle this?
@@ -1432,8 +1389,7 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs,
READ_INFO::~READ_INFO() { - if (need_end_io_cache) - ::end_io_cache(&cache); + ::end_io_cache(&cache); my_free(buffer); List_iterator<XML_TAG> xmlit(taglist); XML_TAG *t; Hmm... I may miss something, but shouldn't we call log_loaded_block() for last successfully read block?
Thanks, Sergey
Hi, Sergey! On May 27, Sergey Vojtovich wrote:
On Tue, May 26, 2015 at 03:37:36PM +0200, serg@mariadb.org wrote:
revision-id: 8a595a9825de286ad073f6f043db4c0dce88a2ea parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d committer: Sergei Golubchik branch nick: maria timestamp: 2015-05-26 15:09:25 +0200 message:
cleanup: LOAD DATA replication support in IO_CACHE
remove some 14-year old code that added support for LOAD DATA replication to IO_CACHE: * three callbacks, of which only two were actually used and that were only needed for LOAD DATA replication but were tested in every IO_CACHE instance * an additional opaque void * argument in IO_CACHE, also only used for LOAD DATA replication, but present everywhere * the code to close IO_CACHE prematurely in LOAD DATA to have these callbacks called in the correct order and a long comment explaining what will happen if IO_CACHE is not closed prematurely * a variable to track whether IO_CACHE was closed prematurely (to avoid double-closing it)
this also fixes a bug with replication of LOAD DATA LOCAL (IO_CACHE of READ_NET type was not calling these callbacks at all) I couldn't track this down. That is GET/my_b_get/pre_read was called in both cases as well as end_io_cache/pre_close. Could you elaborate?
Right, that part is incorrect. I'll remove it.
...skip...
diff --git a/sql/sql_load.cc b/sql/sql_load.cc index 18982c5..62b6e17 100644 --- a/sql/sql_load.cc +++ b/sql/sql_load.cc @@ -1410,20 +1372,15 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs, } else { - /* - init_io_cache() will not initialize read_function member - if the cache is READ_NET. So we work around the problem with a - manual assignment - */ - need_end_io_cache = 1; - #ifndef EMBEDDED_LIBRARY if (get_it_from_net) cache.read_function = _my_b_net_read;
if (mysql_bin_log.is_open()) - cache.pre_read = cache.pre_close = - (IO_CACHE_CALLBACK) log_loaded_block; + { + cache.real_read_function= cache.read_function; + cache.read_function= log_loaded_block; + } #endif } } Will this compile with replication disabled? OTOH I guess it didn't compile even before: log_loaded_block() seem to be unavailable when replication is disabled.
Yes, it compiles for embedded (which has replication disabled). I don't know if the server can be compiled with replication disabled anymore.
IOCACHE::read_function may change, e.g. on error in _my_b_async_read(). Shouldn't we handle this?
async is dead code, but theoretically it might change. how would you suggest to handle that? I'd leave it as is until we have a test case.
@@ -1432,8 +1389,7 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs,
READ_INFO::~READ_INFO() { - if (need_end_io_cache) - ::end_io_cache(&cache); + ::end_io_cache(&cache); my_free(buffer); List_iterator<XML_TAG> xmlit(taglist); XML_TAG *t; Hmm... I may miss something, but shouldn't we call log_loaded_block() for last successfully read block?
Yes, I do that by calling log_loaded_block() explicitly. But now end_io_cache() only closes the IO_CACHE it does not log anything. Which means I don't need to call end_io_cache() early. And don't need 'need_end_io_cache' variable - I can unconditionally call end_io_cache() at the end. Regards, Sergei
Hi Sergei, ok to push. On Wed, May 27, 2015 at 08:14:59PM +0200, Sergei Golubchik wrote:
Hi, Sergey!
On May 27, Sergey Vojtovich wrote:
On Tue, May 26, 2015 at 03:37:36PM +0200, serg@mariadb.org wrote:
revision-id: 8a595a9825de286ad073f6f043db4c0dce88a2ea parent(s): 3e55ef26d49a900782d2c2bb2c03470faed6ec9d committer: Sergei Golubchik branch nick: maria timestamp: 2015-05-26 15:09:25 +0200 message:
cleanup: LOAD DATA replication support in IO_CACHE ...skip...
IOCACHE::read_function may change, e.g. on error in _my_b_async_read(). Shouldn't we handle this?
async is dead code, but theoretically it might change. how would you suggest to handle that?
I'd leave it as is until we have a test case. Maybe add an assertion if you can foresee some practical cases when read_function may change. Up to you.
@@ -1432,8 +1389,7 @@ READ_INFO::READ_INFO(File file_par, uint tot_length, CHARSET_INFO *cs,
READ_INFO::~READ_INFO() { - if (need_end_io_cache) - ::end_io_cache(&cache); + ::end_io_cache(&cache); my_free(buffer); List_iterator<XML_TAG> xmlit(taglist); XML_TAG *t; Hmm... I may miss something, but shouldn't we call log_loaded_block() for last successfully read block?
Yes, I do that by calling log_loaded_block() explicitly.
But now end_io_cache() only closes the IO_CACHE it does not log anything. Which means I don't need to call end_io_cache() early. And don't need 'need_end_io_cache' variable - I can unconditionally call end_io_cache() at the end. Somehow I thought that the second log_loaded_block() was inside "if (error)" too. Sorry for confusion.
Thanks, Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich