Sergei Golubchik <serg@mariadb.org> writes:
On Apr 15, Kristian Nielsen wrote:
Any suggestions for how this is supposed to work? Or is it just broken by design, but saved because normally slave threads do not need to access SHOW STATUS or system variables?
commit f808030 Author: unknown <guilhem@mysql.com> Date: Thu Mar 11 16:23:35 2004 +0100
Fix for BUG#2921 "Replication problem on mutex lock in mySQL-4.0.18": re-using unused LOCK_active_mi to serialize all administrative commands related to replication: START SLAVE, STOP SLAVE, RESET SLAVE, CHANGE MASTER, init_slave() (replication autostart at server startup), end_slave() (replication autostop at server shutdown), LOAD DATA FROM MASTER. This protects us against a handful of deadlocks (like BUG#2921 when two START SLAVE, but when two STOP SLAVE too). Removing unused variables.
So, apparently LOCK_active_mi was unused at that time and Guilhem took it to serialize replication administrative commands. Now this lock is used. The fix, I suppose, should be to create a new lock and use it here. Something like LOCK_serialize_replication_admin_commands.
Hm. So I went through all of the code that references LOCK_active_mi to try and understand this. It seems there are two main uses: 1. As you say, to serialise admin commands. I think the list is: end_slave() (server shutdown); CHANGE MASTER; START/STOP SLAVE; START/STOP ALL SLAVES; RESET SLAVE; FLUSH SLAVE. 2. To protect access to master_info_index, eg. to prevent a Master_info from disappearing while it is accessed. There is a comment in rpl_rli.h that LOCK_active_mi protects Relay_log_info::inited, but I did not understand it - maybe it is wrong? So the idea would be to make a new lock for (1), and keep LOCK_active_mi for (2). Actually, using a lock for (1) seems a bad idea - STOP SLAVE can take a long time to run, and a mutex makes it unkillable. A condition variable might be better (to make STOP SLAVE killable). But I guess that is a separate issue, it would not affect the possibility of deadlocks. Both LOCK_active_mi and the new LOCK_serialize_replication_admin_commands must by themselves prevent master_info_index from having elements added or removed. This is needed for start_all_slaves() and stop_all_slaves() to work correctly, I think. Something will need to be done for remove_master_info(). Currently, it also tries to stop slave threads (from within free_key_master_info()) while holding LOCK_active_mi. It seems that threads should be stopped before attempting to remove their mi from master_info_index, probably? Actually, there would still be the deadlock problem after introducing the new lock, because it is possible for a slave thread to run CHANGE MASTER! (and I think START SLAVE / RESET SLAVE as well). But that is probably a bug, does not seem good that this is possible. I guess these need a check to throw an ER_LOCK_OR_ACTIVE_TRANSACTION error, like STOP SLAVE. I think I also found another lock problem while reading the code, for MASTER_POS_WAIT() og SHOW BINLOG EVENTS. They both grab a Master_info under LOCK_active_mi, but then proceed to access it after releasing the lock. I do not see anything that prevents the Master_info to disappear under their feet if RESET SLAVE ALL is run in another thread? But again, that's a separate issue, I suppose. Probably there are some other tricky deadlock issues lurking here. I don't know. It seems splitting into a new LOCK_serialize_replication_admin_commands could solve the deadlock issue, maybe, if the other problems mentioned above are addressed. It seems very hard to convince oneself that this would not introduce new problems somewhere, the locking in replication is really complex and feels very fragile :-/ It doesn't really feel like something one would want to do in 10.1 (certainly not 10.0), but maybe 10.2? It's kind of a very edge-case problem (and seems to have been there for a _long_ time). Still, hanging the mysqld process hard, requiring kill -9, is also not nice... Thanks, - Kristian.