maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

a few thoughts/questions inline.

On Tue, May 26, 2015 at 03:37:36PM +0200, serg@xxxxxxxxxxx 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