Re: bk commit into 5.0 tree (cmiller:1.2572) BUG#34192

[Alexander Nozdrin]

Hi Chad,

thanks for your patch. I think, it is generally Okay, but I would suggest some 
minor changes.

On 10 March 2008 18:44:35 Chad MILLER wrote:
> ChangeSet@stripped, 2008-03-10 11:44:31-04:00, cmiller@stripped +1
> -0 Bug#34192: mysqldump from mysql 5.0.51 silently fails on dumping \
> databases from 4.0 server
>
>   Contribution from Arkadiusz Miskiewicz.  No CLA required for this size.

Err... The patch from Arkadiusz Miskiewicz was actually wrong. Not sure, it is 
a "contribution".

> diff -Nrup a/client/mysqldump.c b/client/mysqldump.c
> --- a/client/mysqldump.c	2007-12-04 22:07:00 -05:00
> +++ b/client/mysqldump.c	2008-03-10 11:44:29 -04:00
> @@ -109,6 +109,7 @@ static char  *opt_password=0,*current_us
>               *log_error_file= NULL;
>  static char **defaults_argv= 0;
>  static char compatible_mode_normal_str[255];
> +static my_bool server_supports_switching_charsets= TRUE;

Could you please add a comment explaining the intention (the purpose) of this 
variable? Something like: this variables if the server supports 
character_set_results session variable.

>  static ulong opt_compatible_mode= 0;
>  #define MYSQL_OPT_MASTER_DATA_EFFECTIVE_SQL 1
>  #define MYSQL_OPT_MASTER_DATA_COMMENTED_SQL 2
> @@ -1111,11 +1112,13 @@ static int connect_to_db(char *host, cha
>      DB_error(&mysql_connection, "when trying to connect");
>      DBUG_RETURN(1);
>    }
> -  /*
> -    Don't dump SET NAMES with a pre-4.1 server (bug#7997).
> -  */
>    if (mysql_get_server_version(&mysql_connection) < 40100)
> +  {
> +    /* Don't dump SET NAMES with a pre-4.1 server (bug#7997).  */
>      opt_set_charset= 0;
> +

Could you please add a comment here -- something like: pre-4.1 server does not 
support character_set_results session variable.

> +    server_supports_switching_charsets= FALSE;
> +  }
>    /*
>      As we're going to set SQL_MODE, it would be lost on reconnect, so we
>      cannot reconnect.
> @@ -1705,10 +1708,20 @@ static uint get_table_structure(char *ta
>
>        my_snprintf(buff, sizeof(buff), "show create table %s",
> result_table);
>
> -      if (switch_character_set_results(mysql, "binary") ||
> -          mysql_query_with_error_report(mysql, &result, buff) ||
> -          switch_character_set_results(mysql, default_charset))
> +      if (server_supports_switching_charsets)
> +      {
> +        if (switch_character_set_results(mysql, "binary") != 0)

The thing is that switch_character_set_results() is used not only in this 
context, but for dumping views, stored routines and events. Although these 
objects don't exists in pre-4.1 server, I would put this check in 
switch_character_set_results() function. Just for the sake of consistency and 
readability. Something like this:

static int switch_character_set_results(MYSQL *mysql, const char *cs_name)
{
  if (server_supports_switching_charsets)
  {
    /*
      The server does not support character_set_results variable.
      Nothing should be done.
    */

    return FALSE;
  }

  ...
}

mysqldump is already very cluttered application so it would be good to
1) document even "obvious" things; 2) not hide server-version knowledge
in the code (like the knowledge of which objects are available in which
versions).

I'm ready to approve the patch if you're agree with these notes.
Otherwise feel free to contact me and we'll talk them over.

Thanks again!

-- 
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com