Re: [Maria-developers] 9083627c50f: Make rename atomic/repeatable in MyISAM and Aria
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Michael! On May 04, Michael Widenius wrote:
revision-id: 9083627c50f (mariadb-10.5.2-562-g9083627c50f) parent(s): 105cda3b1cb author: Michael Widenius
committer: Michael Widenius timestamp: 2021-03-25 12:06:34 +0200 message: Make rename atomic/repeatable in MyISAM and Aria
This is required to make Atomic RENAME TABLE work for these engines
The requirement is that if we have a server crash in the middle of a storage engine rename call, the upcoming ddl log recovery should be able to finalize it by re-execute the rename.
diff --git a/storage/maria/ma_rename.c b/storage/maria/ma_rename.c index a4388596f6b..64dac32a51c 100644 --- a/storage/maria/ma_rename.c +++ b/storage/maria/ma_rename.c @@ -106,28 +106,37 @@ int maria_rename(const char *old_name, const char *new_name) _ma_reset_state(info); maria_close(info);
+ /* + This code is written so that it should be possible to re-run a + failed rename (even if there is a server crash in between the + renames) and complete it. + */ fn_format(from,old_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); if (mysql_file_rename_with_symlink(key_file_kfile, from, to, MYF(MY_WME | sync_dir))) - DBUG_RETURN(my_errno); + index_file_rename_error= my_errno;
Shouldn't you continue only if index_file_rename_error == ENOENT ? also, I only very quickly looked at mysql_file_rename_with_symlink, but it didn't look atomic to me when symlinks are present.
fn_format(from,old_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); ... diff --git a/storage/myisam/mi_rename.c b/storage/myisam/mi_rename.c index 19df2e54213..13cf8c60897 100644 --- a/storage/myisam/mi_rename.c +++ b/storage/myisam/mi_rename.c @@ -22,6 +22,7 @@ int mi_rename(const char *old_name, const char *new_name) { char from[FN_REFLEN],to[FN_REFLEN]; + int save_errno= 0; DBUG_ENTER("mi_rename");
#ifdef EXTRA_DEBUG @@ -32,10 +33,13 @@ int mi_rename(const char *old_name, const char *new_name) fn_format(from,old_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); if (mysql_file_rename_with_symlink(mi_key_file_kfile, from, to, MYF(MY_WME))) - DBUG_RETURN(my_errno); + save_errno= my_errno; fn_format(from,old_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); - DBUG_RETURN(mysql_file_rename_with_symlink(mi_key_file_dfile, - from, to, - MYF(MY_WME)) ? my_errno : 0); + if (mysql_file_rename_with_symlink(mi_key_file_dfile, + from, to, + MYF(MY_WME))) + if (save_errno) + save_errno= my_errno; + DBUG_RETURN(save_errno);
Why mi_rename isn't trying to undo a half-failed rename like ma_rename does? Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
![](https://secure.gravatar.com/avatar/68cce0b6e372c2c66f4d66c6356102af.jpg?s=120&d=mm&r=g)
Hi!
+ /* + This code is written so that it should be possible to re-run a + failed rename (even if there is a server crash in between the + renames) and complete it. + */ fn_format(from,old_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MARIA_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); if (mysql_file_rename_with_symlink(key_file_kfile, from, to, MYF(MY_WME | sync_dir))) - DBUG_RETURN(my_errno); + index_file_rename_error= my_errno;
Shouldn't you continue only if index_file_rename_error == ENOENT ?
I thought about that, but in the end thought that there in almost all real world cases not be any harm in trying to rename the other file too. If there would be some kind of privilege error, we would probably get it for the other file too.
also, I only very quickly looked at mysql_file_rename_with_symlink, but it didn't look atomic to me when symlinks are present.
Yes, very likely. I did however not had time to look at symlinks as I with the big hard disks of today I don't think many are using symlinks. Should be checked at some point.
fn_format(from,old_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MARIA_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); ... diff --git a/storage/myisam/mi_rename.c b/storage/myisam/mi_rename.c index 19df2e54213..13cf8c60897 100644 --- a/storage/myisam/mi_rename.c +++ b/storage/myisam/mi_rename.c @@ -22,6 +22,7 @@ int mi_rename(const char *old_name, const char *new_name) { char from[FN_REFLEN],to[FN_REFLEN]; + int save_errno= 0; DBUG_ENTER("mi_rename");
#ifdef EXTRA_DEBUG @@ -32,10 +33,13 @@ int mi_rename(const char *old_name, const char *new_name) fn_format(from,old_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MI_NAME_IEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); if (mysql_file_rename_with_symlink(mi_key_file_kfile, from, to, MYF(MY_WME))) - DBUG_RETURN(my_errno); + save_errno= my_errno; fn_format(from,old_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); fn_format(to,new_name,"",MI_NAME_DEXT,MY_UNPACK_FILENAME|MY_APPEND_EXT); - DBUG_RETURN(mysql_file_rename_with_symlink(mi_key_file_dfile, - from, to, - MYF(MY_WME)) ? my_errno : 0); + if (mysql_file_rename_with_symlink(mi_key_file_dfile, + from, to, + MYF(MY_WME))) + if (save_errno) + save_errno= my_errno; + DBUG_RETURN(save_errno);
Why mi_rename isn't trying to undo a half-failed rename like ma_rename does?
Older code and also a very unlikely case. When thinking about it, the code in Aria is probably wrong and should be fixed. We have already written a log entry that we are going to rename the tables. If we revert the table name change, the aria log entry will on recovery force the table to be renamed anyway, which is not good. Should be fixed at some point. Regards, Monty
participants (2)
-
Michael Widenius
-
Sergei Golubchik