Re: [Maria-developers] 3e01da668d1: MDEV-21785: sequences used as default by other table not dumped in right order by mysqldump

Hi, Oleksandr! On Jan 21, Oleksandr Byelkin wrote:

revision-id: 3e01da668d1 (mariadb-10.3.26-64-g3e01da668d1) parent(s): 59998d3480f author: Oleksandr Byelkin committer: Oleksandr Byelkin 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 #include #include @@ -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 @@ -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@mariadb.org