MDEV-7611 mysqldump --dump-slave always starts stopped slave #3780

ParadoxV5 · 2025-01-21T05:18:15Z

The Jira issue number for this PR is: MDEV-7611
- This bug’s from 10 years ago!? 😲

Description

When pausing replicas for mariadb-dump --dump-slave, store the names of replicas paused in a dynamically-malloced buffer;
use that (instead of a new SHOW SLAVE STATUS) to resume replicas when the program ends.

What problem is the patch trying to solve?

When making a dump with --dump-slave option, upon completion mysqldump starts slave threads even if they were not stopped by mysqldump itself. This behavior breaks delayed/manually synchronized slaves which have not to be running all the time.
⸺ MDEV-7611

Release Notes

mariadb-dump --dump-slave now only restarts replicas paused by it.

How can this PR be tested?

The new multi_source.mariadb-dump_slave tests this issue for both single- and multi-source replication.
Edge cases and changed side-effects remain testable by existing methods such as main.mysqldump.

Basing the PR against the correct MariaDB version

This is a new feature or a refactoring, and the PR is based against the main branch.
This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Create a Multi-Source Replication test for these `mariadb-dump --dump-slave` bugs: * MDEV-7611 (not fixed as of this commit) * MDEV-5624 (fixed a long time ago)

ParadoxV5 · 2025-01-21T05:21:57Z

client/mysqldump.c

 static int do_start_slave_sql(MYSQL *mysql_con)
 {
-  MYSQL_RES *slave;
-  MYSQL_ROW row;
  int error= 0;
  DBUG_ENTER("do_start_slave_sql");
-
-  /* We need to check if the slave sql is stopped in the first place */
-  if (mysql_query_with_error_report(mysql_con, &slave,
-                                    multi_source ?
-                                    "SHOW ALL SLAVES STATUS" :
-                                    "SHOW SLAVE STATUS"))
-    DBUG_RETURN(1);
-
-  while ((row= mysql_fetch_row(slave)))
+  for (; n_stopped_replicas--;)


To use the new list instead of SHOW SLAVE STATUS, I essentially rewrote do_start_slave_sql…

ParadoxV5 · 2025-01-21T05:30:00Z

client/mysqldump.c

+  stopped_replicas= my_malloc(PSI_NOT_INSTRUMENTED,
+                              slave->row_count*NAME_CHAR_LEN + 1, MYF(MY_WME));


I feel that it’s wrong wasting memory on short names and already-stopped replicas, but I feel that it’s more wrong spending extra effort reallocing unpredictably.

I’d like to iterate the MYSQL_RES an additional time before the following loop to sum up name sizes of (only) active replications.
Question: How to rewind it from mysql_fetch_row()s?

Note I didn’t check size overflow here because of the Broken Windows Theory:

server/client/mysqldump.c

Line 1986 in 6aa47fa

if (!(tmp=(char*) my_malloc(PSI_NOT_INSTRUMENTED, length*2+1, MYF(MY_WME))))

server/client/mysqldump.c

Line 6620 in 6aa47fa

result= my_malloc(PSI_NOT_INSTRUMENTED, result_length + 10, MYF(MY_WME));

ParadoxV5 · 2025-01-21T05:34:11Z

client/mysqldump.c

@@ -6123,6 +6135,8 @@ static int do_stop_slave_sql(MYSQL *mysql_con)
          mysql_free_result(slave);
          return 1;
        }
+        strmov(stopped_replicas[n_stopped_replicas++],
+               multi_source ? row[0] : "");


pedantically supporting no-multi-source on a best effort basis
(But those pre-10.0 versions were long-EOL… 😕)

server/client/mysqldump.c

Lines 7010 to 7015 in 6aa47fa

/* Check if the server support multi source */

if (mysql_get_server_version(mysql) >= 100000)

{

multi_source= 2;

have_mariadb_gtid= 1;

}

ParadoxV5 · 2025-01-21T05:37:19Z

client/mysqldump.c

+    char* stopped_replica= stopped_replicas[n_stopped_replicas];
+    char query[sizeof("START SLAVE ''") + NAME_CHAR_LEN];
+    DBUG_PRINT("info", ("Connection: '%.*s'", NAME_CHAR_LEN, stopped_replica));
+    // if SLAVE SQL is running, start it anyway to warn unexpected state change


The program runs SHOW SLAVE STATUS multiple times thoughout the various step functions:

do_show_slave_status

do_stop_slave_sql

do_start_slave_sql (before this PR)

Perhaps the intent is in case replication configs change in the middle of mariadb-dump?

ParadoxV5 · 2025-01-21T05:44:17Z

mysql-test/suite/multi_source/mariadb-dump_slave.test

@@ -0,0 +1,42 @@
+# Interactions with --dump-slave; based on `main.rpl_mysqldump_slave`


and multi_source.gtid_slave_pos for the setup

When pausing replicas for `mariadb-dump --dump-slave`, store the names of replicas paused in a dynamically-`malloc`ed buffer; use that (instead of a new SHOW _SLAVE_ STATUS) to resume replicas when the program ends. When making a dump with --dump-slave option, upon completion mysqldump starts slave threads even if they were not stopped by mysqldump itself. This behavior breaks delayed/manually synchronized slaves which have not to be running all the time. ⸺ MDEV-7611 Co-authored-by: Max Samoilenko <[email protected]>

ParadoxV5 · 2025-01-21T05:53:50Z

mysql-test/suite/rpl/t/rpl_mysqldump_gtid_slave_pos.test

 --exec $MYSQL_DUMP_SLAVE --dump-slave  --gtid mysql gtid_slave_pos > $MYSQLTEST_VARDIR/tmp/dump_2.sql
--source include/stop_slave.inc


This test was aware 😄.

MDEV-7611: create multi_source.mariadb-dump_slave

0b1b622

Create a Multi-Source Replication test for these `mariadb-dump --dump-slave` bugs: * MDEV-7611 (not fixed as of this commit) * MDEV-5624 (fixed a long time ago)

ParadoxV5 requested a review from bnestere January 21, 2025 05:18

ParadoxV5 added the MariaDB Corporation label Jan 21, 2025

ParadoxV5 commented Jan 21, 2025

View reviewed changes

ParadoxV5 force-pushed the mdev-7611 branch from fbffda8 to d6c19df Compare January 21, 2025 05:51

ParadoxV5 commented Jan 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-7611 mysqldump --dump-slave always starts stopped slave #3780

MDEV-7611 mysqldump --dump-slave always starts stopped slave #3780

ParadoxV5 commented Jan 21, 2025

ParadoxV5 Jan 21, 2025

ParadoxV5 Jan 21, 2025

ParadoxV5 Jan 21, 2025

ParadoxV5 Jan 21, 2025

ParadoxV5 Jan 21, 2025

ParadoxV5 Jan 21, 2025

		stopped_replicas= my_malloc(PSI_NOT_INSTRUMENTED,
		slave->row_count*NAME_CHAR_LEN + 1, MYF(MY_WME));

	/* Check if the server support multi source */
	if (mysql_get_server_version(mysql) >= 100000)
	{
	multi_source= 2;
	have_mariadb_gtid= 1;
	}

		@@ -0,0 +1,42 @@
		# Interactions with --dump-slave; based on `main.rpl_mysqldump_slave`

		--exec $MYSQL_DUMP_SLAVE --dump-slave --gtid mysql gtid_slave_pos > $MYSQLTEST_VARDIR/tmp/dump_2.sql
		--source include/stop_slave.inc

MDEV-7611 mysqldump --dump-slave always starts stopped slave #3780

Are you sure you want to change the base?

MDEV-7611 mysqldump --dump-slave always starts stopped slave #3780

Conversation

ParadoxV5 commented Jan 21, 2025

Description

What problem is the patch trying to solve?

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment

ParadoxV5 Jan 21, 2025

Choose a reason for hiding this comment