Hi Andrei, Ingo,
Ingo you basically say it all in your review. I have little room
for comments now!
Andrei, the patch looks sound!
Let me just post some comments of my own below.
On Thu, 2010-03-04 at 19:24 +0100, Ingo Strüwing wrote:
> Hi Andrei,
> the idea is good. At the moment it may be the best way to accomplish
> synchronization with system threads.
> However, in the long run it might not be the best to combine two
> otherwise completely independent facilities: DBUG and DEBUG_SYNC.
> DEBUG_SYNC has the downside that it cannot synchronize non-connection
> threads directly. The actions are stored in THD memory. To have
> efficient sync points, no locking of access to the actions is done.
> Hence, another thread cannot modify/set a thread's actions.
I think this is an important feature to have, especially in the context
of replication threads: IO, SQL and dump. Eventually, in multi-threaded
slave, debug_sync with setting actions on non-connection threads
dynamically would be great advantage. (IIUIC)
> To overcome this limitation, you set an action directly in the code and
> execute it. This is controlled by the DBUG facility. It has the
> advantage that one can set a global variable, which will be copied to
> the thread's local storage at thread start time.
You have put it in very clear wording. Thanks Ingo!
> Long term, an equivalent operation could be done by allowing to set
> DEBUG_SYNC globally. This would maintain a global set of actions, which
> would be copied to THD at thread start. Proper cleanup of the global
> action set must be obeyed to avoid inheritance by other starting
> threads. (This applies to the global DEBUG variable too, btw.)
I wish we could go further than just setting globally. IIUIC, setting
it globally for non-connection threads, means that we only have control
over the actions at one specific point in time (before the thread is
started). We can't do nothing wrt the list of actions later! But I want
to have this ;) .
For instance, I want to be able to instruct the IO thread to start
corrupting events, and then stop and then start corruption again ...
I may require that corruption is synchronized with some other
connection thread or server thread through DBUG_SYNC... This is just an
example that global scope can be a step forward, but to me it feels
like there will be more room to improve.
> The proposed patch looks good. Just the optimization to use the pseudo
> sync point "NOW" as discussed on IRC would be nice.
> Another change would be nice too: If the eliminated DEBUG_SYNC_POINTs
> are the last one, a complete wipe out of the facility seems desirable.
> Another option could be to improve DEBUG_SYNC_POINT by making it
> dependent on DBUG_OFF, not EXTRA_DEBUG. Though user level locks have
> their problems for synchronization, as described in
> they might still have merit. But if the last few existing uses of the
> facility are wiped out now, it might be better to get rid of it altogether.
> Andrei Elkin, 03.03.2010 17:51:
> > 3369 Andrei Elkin 2010-03-03
> > Bug #51648 DBUG_SYNC_POINT is not defined on all platforms and mtr cant
> pre-check that
> > a prototype of fixes
> > modified:
> > mysql-test/suite/rpl/r/rpl_show_slave_running.result
> > mysql-test/suite/rpl/t/rpl_show_slave_running.test
> > sql/debug_sync.cc
> > sql/debug_sync.h
> > sql/slave.cc
> Ingo Strüwing, Database Group
> Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
> Geschäftsführer: Thomas Schröder, Wolfgang Engels
> Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028