Re: new mysqldump features (patch)

[Sergei Golubchik]

Hi!

On Aug 20, John David Duncan wrote:
> Hi,

Thanks for the patch!
The features look useful indeed!
See my comments below...
 
> In the attached patch I have implemented two new options in mysqldump.
> The -R (--rename-tables)  option loads all tables using temporary names
> and then renames them while holding a lock.  This can be useful when it is
> necessary to minimize lock time.  The output looks like this:
> 
> sh% mysqldump --rename-tables mydb zip city
> 
>   CREATE TABLE _new_zip ...
>   CREATE TABLE _new_city ...
>   INSERT ...
> 
>   LOCK TABLES zip city WRITE;
>   DROP TABLE zip;
>   ALTER TABLE _new_zip RENAME zip;
>   DROP TABLE city;
>   ALTER TABLE _new_city RENAME city;
>   UNLOCK TABLES;

I would agree with Tim here - it would be faster, simpler, and "more
atomic" to use RENAME TABLE here.

In your example it would be

  RENAME TABLE zip TO _old_zip, _new_zip TO zip,
               city TO _old_city, _new_city TO city;

Also, I would agree that it's better to rename the option, as Tim
suggested. And to add --atomic-prefix (or whatever the name is).

> The -U (--with-use-db) option assures that "USE db_name;" is always in the
> output; this is otherwise not true unless --databases or --all-databases
> is used.

Why do you think this option is necessary ?
I'm sure you need it, so you have a working scenario where this option
helps. Can you describe it briefly ?

> @@ -579,6 +606,9 @@
>  
>    sprintf(insert_pat,"SET OPTION SQL_QUOTE_SHOW_CREATE=%d", (opt_quoted ||
> opt_keywords));
>    table_name=quote_name(table,table_buff);
> +  if(opt_rename_tables) 
> +    target_table=rename_and_quote(table,target_buff);
> +  else target_table=table_name;
>    if (!mysql_query(sock,insert_pat))
>    {
>      /* using SHOW CREATE statement */
> @@ -610,14 +640,26 @@
>          write_header(sql_file, db);
>        }
>        if (!opt_xml)
> -        fprintf(sql_file, "\n--\n-- Table structure for table '%s'\n--\n\n",
> -                table);
> +        fprintf(sql_file, "\n--\n-- Table structure for table '%s'\n--\n\n",table);
>        if (opt_drop)
> -        fprintf(sql_file, "DROP TABLE IF EXISTS %s;\n",table_name);
> +        fprintf(sql_file, "DROP TABLE IF EXISTS %s;\n",target_table);
>  
>        tableRes=mysql_store_result(sock);
>        row=mysql_fetch_row(tableRes);
> -      if (!opt_xml)
> +      if(opt_rename_tables) {
> +        char *s = row[1];
> +        if(!(bcmp(row[1],"CREATE TABLE ",13))) {

Pardon me ?
When this condition may ever fail ?

> +          fprintf(sql_file,"CREATE TABLE %s ",target_table);
> +          for(s+=13 ; *s++ != ' ' ; );

This is wrong, you cannot assume that table name won't contain spaces.

> +          fprintf(sql_file, "%s;\n", s);
> +        }
> +        else { 
> +          fprintf(sql_file,"-- Failed to use renamed table\n\n");
> +          target_table=table_name;
> +          opt_rename_tables=0;
> +        }
> +      }  
> +      else if (!opt_xml)
>          fprintf(sql_file, "%s;\n", row[1]);
>        mysql_free_result(tableRes);
>      }
> @@ -690,14 +732,14 @@
>          fprintf(sql_file, "\n--\n-- Table structure for table '%s'\n--\n\n",
>                  table);
>        if (opt_drop)
> -        fprintf(sql_file, "DROP TABLE IF EXISTS %s;\n",table_name);
> -      fprintf(sql_file, "CREATE TABLE %s (\n", table_name);
> +        fprintf(sql_file, "DROP TABLE IF EXISTS %s;\n",target_table);
> +      fprintf(sql_file, "CREATE TABLE %s (\n", target_table);

this is questionable.
I don't really know what to do when opt_drop is used together with your
opt_rename_tables.

Should it (opt_drop) apply to target_table or original table_name ?

>      }
>      if (cFlag)
> -      sprintf(insert_pat, "INSERT %sINTO %s (", delayed, table_name);
> +      sprintf(insert_pat, "INSERT %sINTO %s (", delayed, target_table);
>      else
>      {
> -      sprintf(insert_pat, "INSERT %sINTO %s VALUES ", delayed, table_name);
> +      sprintf(insert_pat, "INSERT %sINTO %s VALUES ", delayed, target_table);
>        if (!extended_insert)
>          strcat(insert_pat,"(");
>      }
>  static void dumpTable(uint numFields, char *table)
>  {
> -  char query[QUERY_LENGTH], *end, buff[256],table_buff[NAME_LEN+3];
> +  char query[QUERY_LENGTH], *end, *target_table;
> +  char buff[256],table_buff[NAME_LEN+3],target_buff[NAME_LEN+3];  
>    MYSQL_RES     *res;
>    MYSQL_FIELD   *field;
>    MYSQL_ROW     row;
> @@ -909,6 +952,9 @@
>  
>    if (verbose)
>      fprintf(stderr, "-- Sending SELECT query...\n");
> +  if(opt_rename_tables) 
> +    target_table=rename_and_quote(table,target_buff);
> +  else target_table=quote_name(table,table_buff);

Why do it several times ? Isn't it better to pass it as a parameter ?

>    if (path)
>    {
>      char filename[FN_REFLEN], tmp_path[FN_REFLEN];
> @@ -1299,7 +1345,7 @@
>        DBerror(sock, "when doing refresh");
>             /* We shall continue here, if --force was given */
>    }
> -  while ((table = getTableName(0)))
> +  while ((table = getTableName(1)))

This way you removed mysql_free_result() call.
Better to do things clean.
That is to have  getTableName(opt_rename_tables) here
and getTableName(0) in rename_all_in_db().

>    {
>      numrows = getTableStructure(table, database);
>      if (!dFlag && numrows > 0)
> @@ -1309,6 +1355,8 @@
>      fprintf(md_result_file, "</database>\n");
>    if (lock_tables)
>      mysql_query(sock,"UNLOCK_TABLES");
> +  if (opt_rename_tables) 
> +    rename_all_in_db(database);
>    return 0;
>  } /* dump_all_tables_in_db */

Besides these minor comments, the patch is very good!

Regards,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  MySQL AB, Senior Software Developer
/_/  /_/\_, /___/\___\_\___/  Osnabrueck, Germany
       <___/  www.mysql.com