Hi, Oleksandr!
On Jan 21, Oleksandr Byelkin wrote:
> revision-id: 3e01da668d1 (mariadb-10.3.26-64-g3e01da668d1)
> parent(s): 59998d3480f
> author: Oleksandr Byelkin <sa...@mariadb.com>
> committer: Oleksandr Byelkin <sa...@mariadb.com>
> 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 secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
