maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Kristian!

On Fri, Jun 3, 2016 at 7:53 AM, Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>
wrote:

> Sergei Golubchik <serg@xxxxxxxxxxx> writes:
>
> > On May 26, Nirbhay Choubey wrote:
>
> >> > > +  if (wait_for_prior_commit())
>
> Please call this method tmptable_wait_for_prior_commit() or something like
> that. To avoid confusion with THD::wait_for_prior_commit().
>

I no longer have this wrapper and am calling THD::wait_for_prior_commit()
directly.


> >> > you're doing it in almost every method. And when one method calls
> another,
> >> > you're doing it twice. Or thrice. Isn't is too much? (I don't know)
> >>
> >> I added these additional waits to fix some failing replication tests.
> But,
> >> whit new TMP_TABLE_SHARE approach, this could have changed.
> >> I have now reverted them to the original state and will run rpl test to
> >> assure if we do not need additional waits.
>
> That won't mean much. Temporary table handling in statement-based
> replication is very nasty code, and we surely do not have full test
> coverage
> of possible issues. And bugs only turn up as rare race conditions that are
> hard to reproduce.
>

I kind of felt that. :)


>
> So you will have to make sure you understand in detail exactly where
> wait_for_prior_commit() (and possibly other needed code) is needed to
> ensure
> statement-based parallel replication will work in all cases (or at least
> work as well as the non-parallel case).
>
> The issue here is that temporary tables have no locking facilities. And the
> same temporary table can be used by multiple different transactions, which
> in parallel replication can be run in different threads. There is code that
> keeps track of a shared list of temporary tables, and shuffles it around
> between threads as needed (this is also needed for non-parallel
> replication,
> since temporary tables might need to be kept across SQL thread destruction
> and re-creation). See Relay_log_info::save_temporary_tables (as you
> probably
> already saw). Maybe Monty knows something about this (ISTR that he wrote
> that code originally), otherwise careful study of the code is probably the
> only way.
>
> Since there is no locking of temporary tables (at least before your
> patch?),
> it would be possible for two worker threads to simultaneously try to use
> the
> same temptable, which fails, obviously. This is handled rather crudely in
> the current code by simply serialising all transactions using temporary
> tables with all prior transactions. This is what wait_for_prior_commit()
> does - it waits for all transactions to commit that come before in the
> replication stream (within that replication domain).
>

Right, and as I see it, we wait at the _entry_ point when a thread tries
to access/create a temporary table.


>
> Hope this helps,
>


Thanks for sharing these details.

Best,
Nirbhay



>
>  - Kristian.
>