maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On May 27, Sergey Vojtovich wrote:
> 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?

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