Hi Monty,
From: Michael Widenius <monty@mariadb.com>
commit 7abf83f86829ed94de618af4749eb96d5379bd90 (origin/bb-10.6-monty) Author: Michael Widenius <monty@mariadb.com> Date: Wed Nov 8 16:57:58 2023 -0700
MDEV-32551: "Read semi-sync reply magic number error" warnings on master
I read through the patch, looks good. A few comments below, mostly minor things and typos that you can consider as you like. - Kristian.
- We should in 11.3 changed the default value for rpl-semi-sync-master-wait-no-slave from TRUE to FALSE as the TRUE
Sys_semisync_master_wait_no_slave( "rpl_semi_sync_master_wait_no_slave", - "Wait until timeout when no semi-synchronous replication slave " - "available (enabled by default).", + "Wait until timeout when no semi-synchronous replication slave is " + "available.", GLOBAL_VAR(rpl_semi_sync_master_wait_no_slave), CMD_LINE(OPT_ARG), DEFAULT(TRUE), - NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0), - ON_UPDATE(fix_rpl_semi_sync_master_wait_no_slave)); + NO_MUTEX_GUARD, NOT_IN_BINLOG, ON_CHECK(0));
-my_bool rpl_semi_sync_master_wait_no_slave = 1; +my_bool rpl_semi_sync_master_wait_no_slave = 0;
There seems to be som inconsistency here: - The commit is based on 10.6, but the commit comment says 11.3 - The option help text is changed in the commit - The "DEFAULT(TRUE)" in the sysvar definition is _not_ changed in the commit - The C initialization values of the my_bool rpl_semi_sync_master_wait_no_slave _is_ changed in the commit. Should it be DEFAULT(FALSE) also in this patch, and the commit message say "10.6"? Or should these changes be removed from the 10.6 patch and put in 11.3 instead? At least this looks inconsistent.
diff --git a/mysql-test/include/wait_for_pattern_in_file.inc b/mysql-test/include/wait_for_pattern_in_file.inc new file mode 100644 index 00000000000..d088cb125c2 --- /dev/null +++ b/mysql-test/include/wait_for_pattern_in_file.inc @@ -0,0 +1,54 @@ +# ==== Purpose ==== +# +# Waits until pattern comes into log file or until a timeout is reached.
Nice addition, this will be useful for other things!
+--let $keep_include_silent= 1
Why do you set this in the .inc? (Normally this is set in the .test files that want to skip the printout). +let SEARCH_SILENT=1; Maybe the .inc should restore the old value at the end (in case a .test will at some point set it globally for its own calls to include/search_pattern_in_file.inc)
+--let $keep_include_silent= 0
Here, better restore the original value (in case some .test has set it to 1 for itself).
+# Test is binlog format indepdendent, so save resources
(Typo s/indepdendent/independent/)
+# The followowing command is likely to cause the slave master is not yet setup +# for semi-sync
(Typo s/followowing/following/)
- strcpy(m_reply_file_name, ""); - strcpy(m_wait_file_name, ""); + m_reply_file_name[0]= m_wait_file_name[0]= 0;
Interestingly, GCC actually creates identical code for these (it knows to optimize strcpy()). (I'm just mentioning this as I know we both are interested in low-level code details, not asking you to revert the change). Example code: ----------------------------------------------------------------------- #include <string.h> struct X { char m_a[100]; char m_b[10]; void init1(); void init2(); }; void X::init1() { strcpy(m_a, ""); strcpy(m_b, ""); } void X::init2() { m_a[0]= m_b[0]= 0; } ----------------------------------------------------------------------- Result from gcc -O3: ----------------------------------------------------------------------- .type _ZN1X5init1Ev, @function _ZN1X5init1Ev: movb $0, (%rdi) movb $0, 100(%rdi) ret .type _ZN1X5init2Ev, @function _ZN1X5init2Ev: movb $0, 100(%rdi) movb $0, (%rdi) ret ----------------------------------------------------------------------- Identical, except that we now initialize in the opposite order ;-)
+#ifndef DBUG_OFF +void dbug_verify_no_duplicate_slaves(Slave_ilist *m_slaves, THD *thd) +{
You could make this (debug-only) function "static".
+ Master is not responding (gone away?) or it has turned semi sync + off. Turning off semi-sync repsonces as there is no point in sending
(Typo s/repsonces/responses/)