Skip to content

MDEV-39880 Reïmplement MDEV-37146 to include MDEV-39519 #5458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ParadoxV5 wants to merge 1 commit into
base: 13.0
Choose a base branch
from
+68 −37
Open

MDEV-39880 Reïmplement MDEV-37146 to include MDEV-39519 #5458

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 68 additions & 37 deletions client/mysqldump.cc
View file
Open in desktop

@ParadoxV5 ParadoxV5 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not add --apply-slave-statements support for MySQL.
But I doubt that mariadb-dump should support outputting for MySQL.

STOP/START REPLICA also means STOP/START ALL SLAVES on MySQL, but not on MariaDB, so this is a (if not the only) detail where an output intended for MySQL would have an incomplete effect on MariaDB.

@ParadoxV5 ParadoxV5 Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

… which reminds me that I must use STOP/START REPLICA SQL_THREAD FOR CHANNEL '…', which is compatible with both MariaDB 10.7 and MySQL.
fixing now done

[P.S.] 10.6 isn’t dead dead tho…
[P.P.S] … eh screw it for today; ES and other extended supporters can add their own code branch or backport FOR CHANNEL to their 10.6.

So should the MDEV-39519 base…
But the old code design can only send the base STOP/START REPLICAL SQL_THREAD to MySQL, which works, just inconvenient.
Oh well.

Original file line number Diff line number Diff line change
Expand Up @@ -176,17 +176,20 @@ static uint opt_use_gtid;
static uint my_end_arg;
static char * opt_mysql_unix_port=0;
static int first_error=0;
/**
`true` for 11.6 or above servers, `false` otherwise.
@deprecated This and its `false` branches are to be removed after 11.4's EOL.
*/
static bool have_info_schema_slave_status;
static DYNAMIC_STRING extended_row;
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, *slave_status_res= NULL;

static struct
{
ptrdiff_t connection_name, slave_sql_running,
master_host, master_port,
relay_master_log_file, exec_master_log_pos;
} slave_status_indices;

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat pattern here, I like this



#include <sslopt-vars.h>
FILE *md_result_file= 0;
Expand Down Expand Up @@ -6499,23 +6502,48 @@ static int do_stop_slave_sql(MYSQL *mysql_con)
// do_stop_slave_sql() should only be called once
DBUG_ASSERT(!slave_status_res);

if (mysql_query_with_error_report(mysql_con, &slave_status_res,
have_info_schema_slave_status ?
// 11.6 and above
switch (mysql_query_with_error_report(mysql_con, &slave_status_res,

@ParadoxV5 ParadoxV5 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m tempted to revive my old draft implementation based on virtual methods.
But the old argument holds that we will eventually deprecate 11.4 & before.
It’s also really MySQL’s problem that their SQL version of SHOW REPLICA STATUS is still stuck behind Perf. Schema.

I did not extend mysql_query_with_retry() because this code design requires an overly detailed flexibility.
(Although it’d be cleaner with that virtual methods code design.)

Currently, the only other uses of mysql_query_with_retry() are in --master-data ’s and --delete-master-logs’s code, but they both query SHOW MASTER STATUS first and then SHOW BINARY LOG STATUS.

It’s not my current focus to get them to share their results, though (but it’s viable because --delete-master-logs includes --master-data).

Side note

While I removed --dump-slave & --apply-slave-statements support for pre-10.0 in MDEV-37146, I did not touch --master-data.

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mysql_query_with_error_report returns -1 only on ER_PARSE_ERROR. I don't think it will fire when missing the information_schema.slave_status. You likely have to extend mysql_query_with_error_report() to return -1 also on missing slave_status.

"SELECT Connection_name FROM information_schema.SLAVE_STATUS"
// If the slave's SQL thread is not running, we don't stop (or start) it.
" WHERE Slave_SQL_Running <> 'No'" :
"SHOW ALL SLAVES STATUS"
))
return(1);
" WHERE Slave_SQL_Running <> 'No'", true
)) {
default:

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default should conventionally be last in a switch-case. Though I suppose you did it here to keep case -1 last to make it easier to read this series of switch/if nests. I think that's fine, though any time you break convention, please add a comment to justify why. Don't assume it will be obvious for others :)

return 1;
case 0:
have_info_schema_slave_status= true;
break;
case -1:
have_info_schema_slave_status= false;
// Between 10.0 and 11.5 inclusive
switch (mysql_query_with_error_report(mysql_con, &slave_status_res,
"SHOW ALL SLAVES STATUS", true

@ParadoxV5 ParadoxV5 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered using REPLICAS here as well, but I noticed that if MySQL suddenly adds SHOW ALL REPLICAS STATUS, but in MySQL’s column order rather than MariaDB’s, then these assumptions would break in the absence of version checks.

(It’s not MDEV-39880’s concern, but version ≥ 10.0 checks probably will also fall apart soon with the upcoming MySQL 26.7.
They have not released their last quarter’s commits, so the only way to verify is to download their entire pre-release, which I’m not bothering.)

On the other hand, I assume that both MariaDB deleting the SLAVES keyword and MySQL reviving it are very unlikely.

)) {
default:
return 1;
case 0:
slave_status_indices= {0, 13, 3, 5, 11, 23};
break;
case -1:
// MySQL 8.0.22 and above
if (mysql_query_with_error_report(mysql_con, &slave_status_res,
"SHOW REPLICA STATUS"
))
return 1;
slave_status_indices= {55, 11, 1, 3, 9, 21};
}
}

// Loop over all slaves
while ((row= mysql_fetch_row(slave_status_res)))
{
if ((have_info_schema_slave_status ||
strcmp(row[/* Slave_SQL_Running */ 13], "No")))
strcmp(row[slave_status_indices.slave_sql_running], "No")))
{
char query[25 + NAME_CHAR_LEN]; // sizeof(snprintf)
snprintf(query, sizeof(query), "STOP SLAVE '%.*s' SQL_THREAD",
NAME_CHAR_LEN, row[/* Connection_name */ 0]);
char query[39 + NAME_CHAR_LEN]; // sizeof(snprintf)
snprintf(query, sizeof(query),
"STOP REPLICA SQL_THREAD FOR CHANNEL '%.*s'", // 10.7+ or MySQL

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use FOR CHANNEL? Won't this break when running on MariaDB 10.6 (still supported)?

NAME_CHAR_LEN, row[slave_status_indices.connection_name]);

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

slave_status_indices.connection_name isn't initialized here when have_info_schema_slave_status. Though note this isn't true for the parent access to slave_status_indices:

    if ((have_info_schema_slave_status ||
        strcmp(row[slave_status_indices.slave_sql_running], "No")))

which first checks have_info_schema_slave_status.

I think the information_schema.slave_status column name strings & its slave_status_indices should be initialized together.

if (mysql_query_with_error_report(mysql_con, nullptr, query))
return 1;
}
Expand All @@ -6541,7 +6569,7 @@ static int add_slave_statements(void)
return(0);
}

static int do_show_slave_status(MYSQL *mysql_con,
static int do_show_slave_status(MYSQL *mysql_con, int have_mariadb_gtid,
int use_gtid, char *set_gtid_pos,
size_t set_gtid_pos_size)
{
Expand All @@ -6551,7 +6579,6 @@ static int do_show_slave_status(MYSQL *mysql_con,
(opt_slave_data == MYSQL_OPT_SLAVE_DATA_COMMENTED_SQL) ? "-- " : "";
const char *gtid_comment_prefix= (use_gtid ? comment_prefix : "-- ");
const char *nogtid_comment_prefix= (!use_gtid ? comment_prefix : "-- ");
char gtid_pos[MAX_GTID_LENGTH];

if (have_info_schema_slave_status)
{
Expand All @@ -6566,6 +6593,7 @@ static int do_show_slave_status(MYSQL *mysql_con,
mysql_free_result(slave);
return 1;
}
slave_status_indices= {0, /* unused */ 3, 1, 2, 3, 4};

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is slave_sql_running assigned to 3 if it is unused? I'd suggest making it an obviously wrong number (like max ptrdiff_t).

}
else
{
Expand All @@ -6574,13 +6602,18 @@ static int do_show_slave_status(MYSQL *mysql_con,
slave= slave_status_res;
}

if (get_gtid_pos(gtid_pos, false))
if (have_mariadb_gtid)

@bnestere bnestere Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix for MySQL? It looks like, before, against MySQL, we would error and return here. Does the rest of the dump actually work against MySQL? If not, it is probably better to error than to silently output commands which won't actually work.

Alternatively, you mention

For 10.11: Warn when using --gtid to dump MySQL or a pre-GTID MariaDB?

I'd say this is error-worthy rather than warn.

{
mysql_free_result(slave);
return 1;
char gtid_pos[MAX_GTID_LENGTH];
if (get_gtid_pos(gtid_pos, false))
{
if (have_info_schema_slave_status)
mysql_free_result(slave);
return 1;
}
snprintf(set_gtid_pos, set_gtid_pos_size,
fmt_gtid_pos, gtid_comment_prefix, gtid_pos);
}
snprintf(set_gtid_pos, set_gtid_pos_size,
fmt_gtid_pos, gtid_comment_prefix, gtid_pos);

print_comment(md_result_file, 0,
"\n-- The following is the replication SQL position "
Expand All @@ -6603,16 +6636,18 @@ static int do_show_slave_status(MYSQL *mysql_con,
if (use_gtid)

@ParadoxV5 ParadoxV5 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 10.11: Warn when using --gtid to dump MySQL or a pre-GTID MariaDB?

The behaviour before MDEV-37146 or MDEV-39519 appears to still dump no-MariaDB-GTID servers with the following snippet, just not followed by SET GLOBAL gtid_slave_pos='…';.

-- The deferred gtid setting for slave corresponding to the dump-slave CHANGE-MASTER follows
-- GTID position to start replication:

fprintf(md_result_file,
"%sCHANGE MASTER '%.*s' TO MASTER_USE_GTID=slave_pos;\n",
gtid_comment_prefix, NAME_CHAR_LEN, row[/* Connection_name */ 0]);
gtid_comment_prefix,
NAME_CHAR_LEN, row[slave_status_indices.connection_name]);
fprintf(md_result_file, "%sCHANGE MASTER '%.*s' TO ",
nogtid_comment_prefix, NAME_CHAR_LEN, row[/* Connection_name */ 0]);
nogtid_comment_prefix,
NAME_CHAR_LEN, row[slave_status_indices.connection_name]);
if (opt_include_master_host_port)
fprintf(md_result_file, "MASTER_HOST='%s', MASTER_PORT=%s, ",
row[have_info_schema_slave_status ? 1 : /* Master_Host */ 3],
row[have_info_schema_slave_status ? 2 : /* Master_Port */ 5]);
row[slave_status_indices.master_host],
row[slave_status_indices.master_port]);
fprintf(md_result_file, "MASTER_LOG_FILE='%s', MASTER_LOG_POS=%s;\n",
row[have_info_schema_slave_status ? 3 : /* Relay_Master_Log_File */ 11],
row[have_info_schema_slave_status ? 4 : /* Exec_Master_Log_Pos */ 23]);
row[slave_status_indices.relay_master_log_file],
row[slave_status_indices.exec_master_log_pos ]);
check_io(md_result_file);
}

Expand Down Expand Up @@ -6641,12 +6676,12 @@ static int do_start_slave_sql(MYSQL *mysql_con)
*/
while ((row= mysql_fetch_row(slave_status_res)))
{
char query[26 + NAME_CHAR_LEN]; // sizeof(snprintf)
char query[40 + NAME_CHAR_LEN]; // sizeof(snprintf)
snprintf(query, sizeof(query),
"START SLAVE '%.*s' SQL_THREAD",
NAME_CHAR_LEN, row[/* Connection_name */ 0]);
"START REPLICA SQL_THREAD FOR CHANNEL '%.*s'",
NAME_CHAR_LEN, row[slave_status_indices.connection_name]);
if ((have_info_schema_slave_status ||
strcmp(row[/* Slave_SQL_Running */ 13], "No")
strcmp(row[slave_status_indices.slave_sql_running], "No")
) && mysql_query_with_error_report(mysql_con, nullptr, query))
{
fprintf(stderr, "%s: Error: Unable to start slave '%s'\n",
Expand Down Expand Up @@ -7675,12 +7710,7 @@ int main(int argc, char **argv)
init_connection_pool(opt_parallel);

/* Check if the server support multi source */
unsigned long server_version= mysql_get_server_version(mysql);
if (server_version >= 100000)
{
have_info_schema_slave_status= server_version >= 110600;
have_mariadb_gtid= 1;
}
have_mariadb_gtid= mysql_get_server_version(mysql) >= 100000;

if (opt_slave_data && do_stop_slave_sql(mysql))
goto err;
Expand Down Expand Up @@ -7747,6 +7777,7 @@ int main(int argc, char **argv)
sizeof(master_set_gtid_pos)))
goto err;
if (opt_slave_data && do_show_slave_status(mysql,
have_mariadb_gtid,
opt_use_gtid,
slave_set_gtid_pos,
sizeof(slave_set_gtid_pos)))
Expand Down
Loading