Avoid not needed renames in ALTER TABLE

Removed not needed table renames when doing ALTER TABLE when engine
changes and both of the following is true:
- Either new or old engine does not store the table in files
- Neither old or new engine uses files from another engine

We also skip renames when ALTER TABLE does an explicit rename

This improves performance, especially for engines where rename is
a slow operation (like the upcoming S3 engine)

sql/ha_partition.cc

@@ -72,7 +72,8 @@
 #define PAR_ENGINES_OFFSET 12
 #define PARTITION_ENABLED_TABLE_FLAGS (HA_FILE_BASED | \
                                        HA_REC_NOT_IN_SEQ | \
-                                       HA_CAN_REPAIR)
+                                       HA_CAN_REPAIR | \
+                                       HA_REUSES_FILE_NAMES)
 #define PARTITION_DISABLED_TABLE_FLAGS (HA_CAN_GEOMETRY | \
                                         HA_DUPLICATE_POS | \
                                         HA_CAN_INSERT_DELAYED | \

sql/ha_sequence.cc

@@ -101,7 +101,7 @@ int ha_sequence::open(const char *name, int mode, uint flags)
       ha_open() sets the following for us. We have to set this for the
       underlying handler
     */
-    file->cached_table_flags= file->table_flags();
+    file->cached_table_flags= (file->table_flags() | HA_REUSES_FILE_NAMES);
 
     file->reset_statistics();
     internal_tmp_table= file->internal_tmp_table=

sql/handler.h

@@ -292,6 +292,9 @@ enum enum_alter_inplace_result {
 
 #define HA_PERSISTENT_TABLE              (1ULL << 48)
 
+/* If storage engine uses another engine as a base */
+#define HA_REUSES_FILE_NAMES             (1ULL << 49)
+
 /*
   Set of all binlog flags. Currently only contain the capabilities
   flags.
@@ -510,6 +513,7 @@ enum legacy_db_type
   DB_TYPE_BINLOG=21,
   DB_TYPE_PBXT=23,
   DB_TYPE_PERFORMANCE_SCHEMA=28,
+  DB_TYPE_S3=41,
   DB_TYPE_ARIA=42,
   DB_TYPE_TOKUDB=43,
   DB_TYPE_SEQUENCE=44,

sql/sql_table.cc

@@ -2734,7 +2734,8 @@ bool log_drop_table(THD *thd, const LEX_CSTRING *db_name,
 */
 
 bool quick_rm_table(THD *thd, handlerton *base, const LEX_CSTRING *db,
-                    const LEX_CSTRING *table_name, uint flags, const char *table_path)
+                    const LEX_CSTRING *table_name, uint flags,
+                    const char *table_path)
 {
   char path[FN_REFLEN + 1];
   int error= 0;
@@ -2742,11 +2743,13 @@ bool quick_rm_table(THD *thd, handlerton *base, const LEX_CSTRING *db,
 
   size_t path_length= table_path ?
     (strxnmov(path, sizeof(path) - 1, table_path, reg_ext, NullS) - path) :
-    build_table_filename(path, sizeof(path)-1, db->str, table_name->str, reg_ext, flags);
-  if (mysql_file_delete(key_file_frm, path, MYF(0)))
-    error= 1; /* purecov: inspected */
+    build_table_filename(path, sizeof(path)-1, db->str, table_name->str,
+                         reg_ext, flags);
+  if (!(flags & NO_FRM_RENAME))
+    if (mysql_file_delete(key_file_frm, path, MYF(0)))
+      error= 1; /* purecov: inspected */
   path[path_length - reg_ext_length]= '\0'; // Remove reg_ext
-  if (flags & NO_HA_TABLE)
+  if ((flags & (NO_HA_TABLE | NO_PAR_TABLE)) == NO_HA_TABLE)
   {
     handler *file= get_new_handler((TABLE_SHARE*) 0, thd->mem_root, base);
     if (!file)
@@ -5509,7 +5512,8 @@ mysql_rename_table(handlerton *base, const LEX_CSTRING *old_db,
   {
     if (rename_file_ext(from,to,reg_ext))
       error= my_errno;
-    (void) file->ha_create_partitioning_metadata(to, from, CHF_RENAME_FLAG);
+    if (!(flags & NO_PAR_TABLE))
+      (void) file->ha_create_partitioning_metadata(to, from, CHF_RENAME_FLAG);
   }
   else if (!file || likely(!(error=file->ha_rename_table(from_base, to_base))))
   {
@@ -9251,6 +9255,7 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
                        Alter_info *alter_info,
                        uint order_num, ORDER *order, bool ignore)
 {
+  bool engine_changed;
   DBUG_ENTER("mysql_alter_table");
 
   /*
@@ -10177,6 +10182,23 @@ do_continue:;
     goto end_temporary;
   }
 
+  /*
+    Check if file names for the engine are unique.  If we change engine
+    and file names are unique then we don't need to rename the original
+    table to a temporary name during the rename phase
+
+    File names are unique if engine changed and
+    - Either new or old engine does not store the table in files
+    - Neither old or new engine uses files from another engine
+      The above is mainly true for the sequence and the partition engine.
+  */
+  engine_changed= ((new_table->file->ht != table->file->ht) &&
+                   (((!(new_table->file->ha_table_flags() & HA_FILE_BASED) ||
+                      !(table->file->ha_table_flags() & HA_FILE_BASED))) ||
+                    (!(table->file->ha_table_flags() & HA_REUSES_FILE_NAMES) &&
+                     !(new_table->file->ha_table_flags() &
+                       HA_REUSES_FILE_NAMES))));
+
   /*
     Close the intermediate table that will be the new table, but do
     not delete it! Even though MERGE tables do not have their children
@@ -10220,23 +10242,39 @@ do_continue:;
   /*
     Rename the old table to temporary name to have a backup in case
     anything goes wrong while renaming the new table.
+    We only have to do this if name of the table is not changed.
+    If we are changing to use another table handler, we don't
+    have to do the rename as the table names will not interfer.
   */
   char backup_name_buff[FN_LEN];
   LEX_CSTRING backup_name;
   backup_name.str= backup_name_buff;
 
-  backup_name.length= my_snprintf(backup_name_buff, sizeof(backup_name_buff),
-                                  "%s2-%lx-%lx", tmp_file_prefix,
-                                  current_pid, (long) thd->thread_id);
-  if (lower_case_table_names)
-    my_casedn_str(files_charset_info, backup_name_buff);
-  if (mysql_rename_table(old_db_type, &alter_ctx.db, &alter_ctx.table_name,
-                         &alter_ctx.db, &backup_name, FN_TO_IS_TMP))
+  DBUG_PRINT("info", ("is_table_renamed: %d  engine_changed: %d",
+                      alter_ctx.is_table_renamed(), engine_changed));
+
+  if (!alter_ctx.is_table_renamed())
   {
-    // Rename to temporary name failed, delete the new table, abort ALTER.
-    (void) quick_rm_table(thd, new_db_type, &alter_ctx.new_db,
-                          &alter_ctx.tmp_name, FN_IS_TMP);
-    goto err_with_mdl;
+    backup_name.length= my_snprintf(backup_name_buff, sizeof(backup_name_buff),
+                                    "%s2-%lx-%lx", tmp_file_prefix,
+                                    current_pid, (long) thd->thread_id);
+    if (lower_case_table_names)
+      my_casedn_str(files_charset_info, backup_name_buff);
+    if (mysql_rename_table(old_db_type, &alter_ctx.db, &alter_ctx.table_name,
+                           &alter_ctx.db, &backup_name,
+                           FN_TO_IS_TMP |
+                           (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE : 0)))
+    {
+      // Rename to temporary name failed, delete the new table, abort ALTER.
+      (void) quick_rm_table(thd, new_db_type, &alter_ctx.new_db,
+                            &alter_ctx.tmp_name, FN_IS_TMP);
+      goto err_with_mdl;
+    }
+  }
+  else
+  {
+    /* The original table is the backup */
+    backup_name= alter_ctx.table_name;
   }
 
   // Rename the new table to the correct name.
@@ -10248,10 +10286,15 @@ do_continue:;
     (void) quick_rm_table(thd, new_db_type, &alter_ctx.new_db,
                           &alter_ctx.tmp_name, FN_IS_TMP);
 
-    // Restore the backup of the original table to the old name.
-    (void) mysql_rename_table(old_db_type, &alter_ctx.db, &backup_name,
-                              &alter_ctx.db, &alter_ctx.alias,
-                              FN_FROM_IS_TMP | NO_FK_CHECKS);
+    if (!alter_ctx.is_table_renamed())
+    {
+      // Restore the backup of the original table to the old name.
+      (void) mysql_rename_table(old_db_type, &alter_ctx.db, &backup_name,
+                                &alter_ctx.db, &alter_ctx.alias,
+                                FN_FROM_IS_TMP | NO_FK_CHECKS |
+                                (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE :
+                                 0));
+    }
     goto err_with_mdl;
   }
 
@@ -10271,7 +10314,9 @@ do_continue:;
       // Restore the backup of the original table to the old name.
       (void) mysql_rename_table(old_db_type, &alter_ctx.db, &backup_name,
                                 &alter_ctx.db, &alter_ctx.alias,
-                                FN_FROM_IS_TMP | NO_FK_CHECKS);
+                                FN_FROM_IS_TMP | NO_FK_CHECKS |
+                                (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE :
+                                 0));
       goto err_with_mdl;
     }
     rename_table_in_stat_tables(thd, &alter_ctx.db, &alter_ctx.alias,
@@ -10279,7 +10324,19 @@ do_continue:;
   }
 
   // ALTER TABLE succeeded, delete the backup of the old table.
-  if (quick_rm_table(thd, old_db_type, &alter_ctx.db, &backup_name, FN_IS_TMP))
+  error= quick_rm_table(thd, old_db_type, &alter_ctx.db, &backup_name,
+                        FN_IS_TMP |
+                        (engine_changed ? NO_HA_TABLE | NO_PAR_TABLE: 0));
+  if (engine_changed)
+  {
+    /* the .frm file was removed but not the original table */
+    error|= quick_rm_table(thd, old_db_type, &alter_ctx.db,
+                           &alter_ctx.table_name,
+                           NO_FRM_RENAME |
+                           (engine_changed ? 0 : FN_IS_TMP));
+  }
+
+  if (error)
   {
     /*
       The fact that deletion of the backup failed is not critical
@@ -10325,6 +10382,7 @@ do_continue:;
   DBUG_RETURN(false);
 
 err_new_table_cleanup:
+  DBUG_PRINT("error", ("err_new_table_cleanup"));
   my_free(const_cast<uchar*>(frm.str));
   /*
     No default value was provided for a DATE/DATETIME field, the
@@ -10374,11 +10432,16 @@ do_continue:;
   DBUG_RETURN(true);
 
 err_with_mdl_after_alter:
+  DBUG_PRINT("error", ("err_with_mdl_after_alter"));
   /* the table was altered. binlog the operation */
   DBUG_ASSERT(!(mysql_bin_log.is_open() &&
                 thd->is_current_stmt_binlog_format_row() &&
                 (create_info->tmp_table())));
-  write_bin_log(thd, true, thd->query(), thd->query_length());
+  /*
+    We can't reset error as we will return 'true' below and the server
+    expects that error is set
+  */
+  write_bin_log(thd, FALSE, thd->query(), thd->query_length());
 
 err_with_mdl:
   /*

sql/sql_table.h

@@ -140,6 +140,8 @@ static const uint NO_HA_TABLE=     1 << 4;
 static const uint SKIP_SYMDIR_ACCESS= 1 << 5;
 /** Don't check foreign key constraints while renaming table */
 static const uint NO_FK_CHECKS=    1 << 6;
+/* Don't delete .par table in quick_rm_table() */
+static const uint NO_PAR_TABLE=   1 << 7;
 
 uint filename_to_tablename(const char *from, char *to, size_t to_length,
                            bool stay_quiet = false);