Code review comment for lp:~ebergen/maria/mysqld_safe_fix

Revision history for this message
Michael Widenius (monty) wrote :

Hi!

>>>>> "Eric" == Eric Bergen <email address hidden> writes:

Eric> On Mon, Feb 25, 2013 at 5:40 AM, Michael Widenius <email address hidden> wrote:
>>
>> Hi!
>>
>>>>>>> "Eric" == Eric Bergen <email address hidden> writes:
>>
Eric> Eric Bergen has proposed merging lp:~ebergen/maria/mysqld_safe_fix into lp:maria.
Eric> Requested reviews:
Eric> Maria-captains (maria-captains)
>>
Eric> For more details, see:
Eric> https://code.launchpad.net/~ebergen/maria/mysqld_safe_fix/+merge/150210
>>
Eric> This contains two changes to mysqld_safe.sh
>>
Eric> The first adds an option --crash-script for a script to call when mysqld crashes. This can be useful for gathering extra information, notifying operators, or triggering a failover when using HA.
>>
>> This part is ok.
>>
Eric> The second removes a rm command that deleted the sock file. This could delete the sock file for another instance when MYSQL_UNIX_PORT environment variable is set.
>>
>> What you removed was:
>>
>> - rm -f $safe_mysql_unix_port "$pid_file" # Some extra safety
>>
>> You can't remove the delation of "$pid_file";
>>
>> Without that mysqld_safe.sh will not properly.

Eric> mysqld_safe does a proper pid file check earlier and fails with an
Eric> error message if the stale pid file can't be cleaned up. The server
Eric> also opens the pid file with O_TRUNC so it will also get cleaned up.

Yes, however the above 'rm -f' is in the loop that is executed on
restart. So the 'rm' needs to be there so that mysqld_safe can
know if the server did start and/or crash again.

>> There is one problem with not removing the socket port:
>> - If you don't do that, mysqld will not restart from a crash as the
>> old socket file will stop the server from starting.

Eric> The server does a blind unlink call on the socket file prior to
Eric> creating a new one so it won't fail on a stale socket file.

I have seen in the past InnoDB refusing to start because the socket
file existed from before.

This is probably with older InnoDB versions, as the newest one instead
uses a lock on the idbata file.

This means that it's indeed safe to remove the removal of the socket
file in mysqld_safe for newer servers.

Eric> This caused problems for me because I use a non-standard installation. Upgrading the MariaDB-Server rpm would delete the sock file from my relocated mysqld leaving it orphaned.
>>
>> If you specify "socket" in your my.cnf, this should never go wrong.
>>
>> If you don't specify "socket" and you have another MySQL server
>> running, your new server should not start as the socket file would be
>> conflicting.
>>
>> Can you please describe in litte more detail how your installation
>> looked like where you got conflicting socket names?

Eric> This happens when relocating a mysql instance from the default paths
Eric> provided in rpms. It is fairly common to relocate mysql for developers
Eric> to use a server with their own instances.

Eric> The problem showed up for me when a user was setting
Eric> MYSQL_UNIX_PORT=/home/user/path/to/relocated/mysql.sock and then
Eric> upgraded the MariaDB-Server package. The mysqld_safe executed during
Eric> the rpm installation deleted the relocated mysql_unix_port while
Eric> running the installation.

Wouldn't the same thing happen if mysqld tried to start with
MYSQL_UNIX_PORT set to the wrong path?

Eric> The real fix from my end is to manage MYSQL_UNIX_PORT better or to get
Eric> rid of it in favor of ~/.my.cnf. I found it odd that there was a line
Eric> with the comment "Some extra safety" implying that it didn't fix any
Eric> known issue. It was also doing cleanup that was harmful in my case and
Eric> not necessary.

Before it was necessary. Not with the latest InnoDB anymore.

I will apply your patch, but keep removal of the pid file, tomorrow.

Regards,
Monty

« Back to merge proposal