← Back to team overview

maria-developers team mailing list archive

Re: [Commits] c285dbe: MDEV-5535: Cannot reopen temporary table


Hi Kristian!

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

> 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()

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


>  - Kristian.