maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On Jan 21, Oleksandr Byelkin wrote:
> revision-id: 3e01da668d1 (mariadb-10.3.26-64-g3e01da668d1)
> parent(s): 59998d3480f
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2021-01-14 10:45:51 +0100
> message:
> 
> MDEV-21785: sequences used as default by other table not dumped in right order by mysqldump
> 
> Dump sequences first.
> 
> This atch made to keep it small and
> to keep number of queries to the server the same.
> 
> Order of tables in a dump can not be changed
> (except sequences first) because mysql_list_tables
> uses SHOW TABLES and I used SHOW FULL TABLES.

it looks like a rather overengineered implementation. I'd suggest to
simplify it as:

> diff --git a/client/mysqldump.c b/client/mysqldump.c
> index a964f96437d..17837713c01 100644
> --- a/client/mysqldump.c
> +++ b/client/mysqldump.c
> @@ -42,6 +42,11 @@
>  /* on merge conflict, bump to a higher version again */
>  #define DUMP_VERSION "10.19"
>  
> +/**
> +  First mysql version supporting sequences.
> +*/
> +#define FIRST_SEQUENCE_VERSION 100300

seems unnecessary, you implemented SHOW FULL TABLES
in 5.0.2-alpha. Oct 2004.

You can safely use SHOW FULL TABLES unconditionally.

> +
>  #include <my_global.h>
>  #include <my_sys.h>
>  #include <my_user.h>
> @@ -92,6 +97,13 @@
>  /* Max length GTID position that we will output. */
>  #define MAX_GTID_LENGTH 1024
>  
> +
> +/* Dump sequence/tables control */
> +#define DUMP_TABLE_TABLE 1
> +#define DUMP_TABLE_SEQUENCE 2
> +#define DUMP_TABLE_ALL (DUMP_TABLE_TABLE | DUMP_TABLE_SEQUENCE)
> +
> +
>  static my_bool ignore_table_data(const uchar *hash_key, size_t len);
>  static void add_load_option(DYNAMIC_STRING *str, const char *option,
>                               const char *option_value);
> @@ -171,6 +183,7 @@ static DYNAMIC_STRING dynamic_where;
>  static MYSQL_RES *get_table_name_result= NULL;
>  static MEM_ROOT glob_root;
>  static MYSQL_RES *routine_res, *routine_list_res;
> +static int get_table_name_result_short= 1;

that'll be unnecessary

>  
>  
>  #include <sslopt-vars.h>
> @@ -3853,13 +3866,16 @@ static char *alloc_query_str(size_t size)
>    ARGS
>     table - table name
>     db    - db name
> +   type_ctrl - dump table or sequences or both
>  
>     RETURNS
>      void
>  */
>  
>  
> -static void dump_table(const char *table, const char *db, const uchar *hash_key, size_t len)
> +static void dump_table(const char *table, const char *db,
> +                       const uchar *hash_key, size_t len,
> +                       const uint type_ctrl)
>  {
>    char ignore_flag;
>    char buf[200], table_buff[NAME_LEN+3];
> @@ -3881,9 +3897,11 @@ static void dump_table(const char *table, const char *db, const uchar *hash_key,
>    */
>    if (check_if_ignore_table(table, table_type) & IGNORE_SEQUENCE_TABLE)
>    {
> -    get_sequence_structure(table, db);
> +    if (type_ctrl & DUMP_TABLE_SEQUENCE)
> +      get_sequence_structure(table, db);
> +    DBUG_VOID_RETURN;
> +  } else if (!(type_ctrl & DUMP_TABLE_TABLE))
>      DBUG_VOID_RETURN;
> -  }

here I'd remove the whole if(). Let's say the convention is that
dump_table() should never be invoked for sequences, it's the caller's
job to ensure it.

>    /*
>      Make sure you get the create table info before the following check for
>      --no-data flag below. Otherwise, the create table info won't be printed.
> @@ -4368,18 +4386,49 @@ static void dump_table(const char *table, const char *db, const uchar *hash_key,
>  } /* dump_table */
>  
>  
> -static char *getTableName(int reset)
> +static char *getTableName(int reset, uint table_control)
>  {
>    MYSQL_ROW row;
>  
>    if (!get_table_name_result)
>    {
> -    if (!(get_table_name_result= mysql_list_tables(mysql,NullS)))
> -      return(NULL);
> +    if (mysql_get_server_version(mysql) >= FIRST_SEQUENCE_VERSION)
> +    {
> +      const char *query=
> +        "SHOW FULL TABLES";
> +
> +      if (mysql_query_with_error_report(mysql, 0, query))
> +        return (NULL);
> +
> +      if (!(get_table_name_result= mysql_store_result(mysql)))
> +        return (NULL);
> +
> +      get_table_name_result_short= 0;
> +    }
> +    else
> +    {
> +      if (!(get_table_name_result= mysql_list_tables(mysql,NullS)))
> +        return(NULL);
> +      get_table_name_result_short= 1;
> +    }

this will be twice as small, if you won't try to support before-5.0
servers.

>    }
>    if ((row= mysql_fetch_row(get_table_name_result)))
> -    return((char*) row[0]);
> -
> +  {
> +    int sequence;
> +    if (get_table_name_result_short ||
> +        table_control == DUMP_TABLE_ALL)
> +      return((char*) row[0]);
> +    while (row &&
> +             (((sequence= (strcmp(row[1], "SEQUENCE") == 0)) &&
> +               (table_control & DUMP_TABLE_TABLE)) ||
> +              ((!sequence) &&
> +               (table_control & DUMP_TABLE_SEQUENCE))))

I'd rather just do

   static char *getTableName(int reset, uint want_sequence)

and

   while (row && strcmp(row[1], "SEQUENCE") == want_sequence)
     row= mysql_fetch_row(get_table_name_result);

> +    {
> +      row= mysql_fetch_row(get_table_name_result);
> +    }
> +    if (row)
> +      return((char*) row[0]);
> +  }
>    if (reset)
>      mysql_data_seek(get_table_name_result,0);      /* We want to read again */
>    else
> @@ -4785,17 +4834,17 @@ static int dump_all_stats()
>    /* EITS added in 10.0.1 */
>    if (mysql_get_server_version(mysql) >= 100001)
>    {
> -    dump_table("column_stats", "mysql", NULL, 0);
> -    dump_table("index_stats", "mysql", NULL, 0);
> -    dump_table("table_stats", "mysql", NULL, 0);
> +    dump_table("column_stats", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +    dump_table("index_stats", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +    dump_table("table_stats", "mysql", NULL, 0, DUMP_TABLE_TABLE);

this wouldn't be needed if the caller will only use dump_table for
tables

>    }
>    /* Innodb may be disabled */
>    if (!mysql_query(mysql, "show fields from innodb_index_stats"))
>    {
>      MYSQL_RES *tableres= mysql_store_result(mysql);
>      mysql_free_result(tableres);
> -    dump_table("innodb_index_stats", "mysql", NULL, 0);
> -    dump_table("innodb_table_stats", "mysql", NULL, 0);
> +    dump_table("innodb_index_stats", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +    dump_table("innodb_table_stats", "mysql", NULL, 0, DUMP_TABLE_TABLE);
>    }
>    opt_no_create_info= prev_no_create_info;
>    return 0;
> @@ -4817,11 +4866,11 @@ static int dump_all_timezones()
>    opt_prev_no_create_info= opt_no_create_info;
>    opt_no_create_info= 1;
>    fprintf(md_result_file,"\nUSE mysql;\n");
> -  dump_table("time_zone", "mysql", NULL, 0);
> -  dump_table("time_zone_name", "mysql", NULL, 0);
> -  dump_table("time_zone_leap_second", "mysql", NULL, 0);
> -  dump_table("time_zone_transition", "mysql", NULL, 0);
> -  dump_table("time_zone_transition_type", "mysql", NULL, 0);
> +  dump_table("time_zone", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +  dump_table("time_zone_name", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +  dump_table("time_zone_leap_second", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +  dump_table("time_zone_transition", "mysql", NULL, 0, DUMP_TABLE_TABLE);
> +  dump_table("time_zone_transition_type", "mysql", NULL, 0, DUMP_TABLE_TABLE);
>    opt_no_create_info= opt_prev_no_create_info;
>    return 0;
>  }
> @@ -5346,12 +5396,29 @@ static int dump_all_tables_in_db(char *database)
>        DBUG_RETURN(1);
>      }
>    }
> -  while ((table= getTableName(0)))
> +
> +  if (mysql_get_server_version(mysql) >= FIRST_SEQUENCE_VERSION &&
> +      !opt_no_create_info)
> +  {
> +    // First process sequences
> +    while ((table= getTableName(1, DUMP_TABLE_SEQUENCE)))
> +    {
> +      char *end= strmov(afterdot, table);
> +      if (include_table((uchar*) hash_key, end - hash_key))
> +      {
> +        dump_table(table, database, (uchar*) hash_key, end - hash_key,
> +                   DUMP_TABLE_SEQUENCE);
> +      }
> +    }
> +    table_ctrl= DUMP_TABLE_TABLE; // next pass
> +  }

and instead of all the above, you could do just

    for (int table_ctrl= 1; table_ctrl >= 0; table_ctrl--)

> +  while ((table= getTableName(0, table_ctrl)))
>    {
>      char *end= strmov(afterdot, table);
>      if (include_table((uchar*) hash_key, end - hash_key))
>      {
> -      dump_table(table, database, (uchar*) hash_key, end - hash_key);
> +      dump_table(table, database, (uchar*) hash_key, end - hash_key,
> +                 table_ctrl);
>        my_free(order_by);
>        order_by= 0;
>        if (opt_dump_triggers && mysql_get_server_version(mysql) >= 50009)
> @@ -5745,11 +5813,24 @@ static int dump_selected_tables(char *db, char **table_names, int tables)
>        DBUG_RETURN(1);
>      }
>    }
> +
> +  if (mysql_get_server_version(mysql) >= FIRST_SEQUENCE_VERSION &&
> +      !opt_no_create_info)
> +  {
> +    /* Dump Sequence first */
> +    for (pos= dump_tables; pos < end; pos++)
> +    {
> +      DBUG_PRINT("info",("Dumping sequence(?) %s", *pos));
> +      dump_table(*pos, db, NULL, 0, DUMP_TABLE_SEQUENCE);
> +    }
> +  }
> +  else
> +    table_ctrl|= DUMP_TABLE_SEQUENCE; // dump all on next pass

and here, in dump_selected_tables(), you'd need to do
check_if_ignore_table() == IGNORE_SEQUENCE_TABLE.

>    /* Dump each selected table */
>    for (pos= dump_tables; pos < end; pos++)
>    {
>      DBUG_PRINT("info",("Dumping table %s", *pos));
> -    dump_table(*pos, db, NULL, 0);
> +    dump_table(*pos, db, NULL, 0, table_ctrl);
>      if (opt_dump_triggers &&
>          mysql_get_server_version(mysql) >= 50009)
>      {
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx