List:Commits« Previous MessageNext Message »
From:Sergei Golubchik Date:August 1 2007 6:37pm
Subject:Re: bk commit into 5.1 tree (anozdrin:1.2573) BUG#30123
View as plain text  
Hi!

On Jul 31, Alexander Nozdrin wrote:
> ChangeSet@stripped, 2007-07-31 20:02:39+04:00, anozdrin@ibm. +1 -0
>   Fix for BUG#30123: mysqldump is unable to work with old servers.
>   
>   New server (as of 5.1.21) provides new features:
>     - SHOW CREATE TRIGGER;
>     - character set information for SHOW TRIGGERS and SHOW CREATE
>       EVENT | FUNCTION | PROCEDURE statements.
>   Mysqldump uses these features to generate proper dump.
>   
>   The bug happened when new mysqldump was used to dump older servers.
>   The problem was that 5.1.21 new features are not available, so
>   mysqldump exited with error code or just crashed.
>   
>   The fix is to detect if mysqldump has ben run against older server
>   and don't use new 5.1.21 functionality in this case. Certainly,
>   the dump generated for the older server suffers from the character
>   set problems fixed by BUG#16291 and the like.
> 
>   client/mysqldump.c@stripped, 2007-07-31 20:02:28+04:00, anozdrin@ibm. +374 -177
>     Don't use new server features if they are not available.

Thanks.
Please, see my comments below
 
> diff -Nrup a/client/mysqldump.c b/client/mysqldump.c
> --- a/client/mysqldump.c	2007-07-27 22:20:27 +04:00
> +++ b/client/mysqldump.c	2007-07-31 20:02:28 +04:00
> @@ -1227,6 +1227,126 @@ static int switch_character_set_results(
>    return mysql_real_query(mysql, query_buffer, query_length);
>  }
>  
> +/**
> +  Cover DEFINER-clause for CREATE TRIGGER statement in version-specific
> +  comments.

Not a very good comment. What does it mean "cover definer clause" ?
I don't understand what this function is suppose to do.

> +  TODO: this is definitely a BAD IDEA to parse SHOW CREATE output.
> +  However, we can not use INFORMATION_SCHEMA instead:
> +    1. INFORMATION_SCHEMA provides data in UTF8, but here we need data in
> +       the original character set;
> +    2. INFORMATION_SCHEMA does not provide information about routine
> +       parameters now.
> +
> +  @param[in] trigger_def_str    CREATE TRIGGER statement string.
> +  @param[in] trigger_def_length length of the trigger_def_str.
> +
> +  @return pointer to the new allocated query string.
> +*/
> +
> +static char *cover_definer_clause_in_trigger(const char *trigger_def_str,
> +                                             uint trigger_def_length)
...
> @@ -1766,16 +1886,36 @@ static uint dump_events_for_db(char *db)
...
> +          }
> +            else
> +            {
> +              /*
> +                mysqldump is being run against the server, that does not
> +                provide character set information in SHOW CREATE
> +                statements.
> +
> +                NOTE: the dump may be incorrect, since character set
> +                information is required in order to restore event properly.
> +              */
> +
> +              fprintf(sql_file,
> +                      "--\n"
> +                      "-- WARNING: old server version. "
> +                        "The following dump may be incomplete.\n"
> +                      "--\n");

Are you sure it's a good idea to add this warning before every
procedure, every function, every trigger, and every event ?

> +            }
>  
>            switch_sql_mode(sql_file, delimiter, row[1]);
>  
> @@ -2660,18 +2778,109 @@ continue_xml:
...
> +static int dump_triggers_for_table(char *table_name, char *db_name)
> +{
> +  FILE       *sql_file= md_result_file;
> +  char       name_buff[NAME_LEN*4+3];
> +  char       query_buff[QUERY_LENGTH];
> +  uint       old_opt_compatible_mode= opt_compatible_mode;
> +  MYSQL_RES  *show_triggers_rs;
> +  MYSQL_ROW  row;
> +
> +  char       db_cl_name[MY_CS_NAME_SIZE];
> +
> +  DBUG_ENTER("dump_triggers_for_table");
> +  DBUG_PRINT("enter", ("db: %s, table_name: %s", db_name, table_name));
> +
> +  /* Do not use ANSI_QUOTES on triggers in dump */
> +  opt_compatible_mode&= ~MASK_ANSI_QUOTES;
> +
> +  /* Get database collation. */
> +
> +  if (fetch_db_collation(db_name, db_cl_name, sizeof (db_cl_name)))
> +    DBUG_RETURN(TRUE);
> +
> +  if (switch_character_set_results(mysql, "binary"))
> +    DBUG_RETURN(TRUE);

In the old code I don't see switch to binary charset.
Why it's here ? It doesn't look like this change is related to "old
mysqld servers".

> +  /* Get list of triggers. */
> +
> +  my_snprintf(query_buff, sizeof(query_buff),
> +              "SHOW TRIGGERS LIKE %s",
> +              quote_for_like(table_name, name_buff));
> +
> +  if (mysql_query_with_error_report(mysql, &show_triggers_rs, query_buff))
> +    DBUG_RETURN(TRUE);
> +
> +  if (mysql_num_rows(show_triggers_rs))
> +    fprintf(sql_file, "\n");
> +
> +  /* Dump triggers. */

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Radlkoferstr. 2, D-81373 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140
Thread
bk commit into 5.1 tree (anozdrin:1.2573) BUG#30123Alexander Nozdrin31 Jul
  • Re: bk commit into 5.1 tree (anozdrin:1.2573) BUG#30123Sergei Golubchik1 Aug