MDEV-36923 : Add warning messages for applier FK failures #4342

janlindstrom · 2025-10-06T12:06:21Z

The Jira issue number for this PR is: MDEV-36923

Description

Added warning messages for applier FK failures. Warnings are printed by default and can be disabled using
set global wsrep_mode=APPLIER_DISABLE_FK_WARNINGS;

Ported to MariaDB [email protected]

Release Notes

TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on "Writing good test cases".

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

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.
[ x] 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

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

mysql-test/suite/galera/r/MW-369.result

storage/innobase/row/row0ins.cc

mysql-test/suite/galera/t/galera_fk_selfreferential.test

sql/wsrep_mysqld.h

storage/innobase/row/row0ins.cc

sql/wsrep_mysqld.h

storage/innobase/row/row0ins.cc

dr-m · 2025-10-20T10:57:05Z

mysql-test/suite/galera/r/galera_var_wsrep_mode.result

-SET GLOBAL wsrep_mode=128;
-ERROR 42000: Variable 'wsrep_mode' can't be set to the value of '128'
+SET GLOBAL wsrep_mode=65536;
+ERROR 42000: Variable 'wsrep_mode' can't be set to the value of '65536'


Is this a tight limit? As far as I understand, the Sys_var_set Sys_wsrep_mode was extended with only one member. Hence I’d expect 256 to be a disallowed value.

dr-m · 2025-10-20T10:58:50Z

mysql-test/suite/galera/t/MDEV-36923.test

+--let $assert_count = 1
+--let $assert_file = $MYSQLTEST_VARDIR/log/mysqld.2.err
+--let $assert_text = Foreign key warning in table
+--let $assert_select = Foreign key warning in table
+--source include/assert_grep.inc
+--let $assert_text = adding an index entry to a child table failed
+--let $assert_select = adding an index entry to a child table failed
+--source include/assert_grep.inc


The table name and the constraint name are being omitted here, although a comment right before the statement INSERT INTO grandchild claims to know them.

Added full text.

dr-m · 2025-10-20T12:25:10Z

storage/innobase/row/row0ins.cc

+#ifndef UNIV_DEBUG
+  /* In release builds do not output lock wait warnings to avoid
+     flooding error log. */
+  if (err == DB_LOCK_WAIT)
+    return;
+#endif


Do we really need this even in debug instrumented builds? Would a Galera specific DBUG_EXECUTE_IF in row_mysql_handle_errors() serve a similar purpose?

We really do not want to log lock waits in release builds, they are not errors this change is intended. Case where (err != DB_SUCCESS && err != DB_LOCK_WAIT) is the only interesting case I just do not know test case to produce it.

dr-m · 2025-10-20T12:26:10Z

storage/innobase/row/row0ins.cc

+  const char* end;
+  end= innobase_convert_name(db_table, sizeof db_table,
+			     foreign->foreign_table_name,
+			     strlen(foreign->foreign_table_name),
+			     trx->mysql_thd);


TAB should not be for formatting new functions. Why are the declaration and initialization split?

dr-m · 2025-10-20T12:30:12Z

storage/innobase/row/row0ins.cc

+  db_table[end - db_table] = '\0';
+
+  const char *fk_id= strchr(foreign->id, '/');


end may post one char past the end of db_table. If that is the case, this assignment would constitute a buffer overflow and fail to ensure that the db_table is NUL terminated. In my previous review I suggested the use of "%.*s" and specifying the length explicitly. The buffer overflow would have been avoided by that.

The formatting around the assignment operator = is inconsistent.

I do not think so, see ut0ut.cc funtion ut_get_name, db_table size is bigger than actual table name can be even in multibyte names.

I will make buffer slightly longer to avoid overflow.

dr-m · 2025-10-20T12:32:07Z

storage/innobase/row/row0ins.cc

+  if (fk_id)
+    fk_id++;
+  else
+    fk_id= foreign->id; /* a constraint name from before MySQL 4.0.18 */


Not implementing dict_foreign_t::sql_id() will cause a run-time merge conflict with #3960. If the tests are not revised to actually cover the constraint names that the messages are expected to output, then such conflicts will remain undetected.

storage/innobase/row/row0ins.cc

dr-m · 2025-10-20T12:37:58Z

storage/innobase/row/row0ins.cc

 	if (err != DB_SUCCESS) {
-
 		goto nonstandard_exit_func;
 	}


Heikki Tuuri insisted that there be an extra blank line before any goto statement. I disagree with that, but then again I think that we should avoid any changes to formatting unless some code nearby is being changed. This is not the only occurence of a blank line before a goto statement in this function.

dr-m · 2025-10-20T12:39:12Z

storage/innobase/row/row0ins.cc

+				if (trx->is_wsrep()) {
+					wsrep_applier_log_fk(trx, foreign,
+						      "Setting shared record lock "
+						      "to matching record in the "
+						      "child table failed err: ",
+						      err);


In my previous review, I pointed out that we have some duplicated err: err: strings in the output. This still seems to be the case at least for this message.

Add warning messages if applier write set execution fails because of foreign key constraint error. Warnings are printed by default and can be disabled using SET GLOBAL wsrep_mode=APPLIER_DISABLE_FK_WARNINGS.

dr-m · 2025-10-23T08:01:13Z

mysql-test/suite/galera/r/MDEV-36923.result

+include/assert_grep.inc [Foreign key warning in table: \`test\`\.\`grandchild\` constraint \`fk_2\` failed err: adding an index entry to a child table failed.]
+connection node_1;
+drop table grandchild;
+drop table child;
+drop table parent;
+SET GLOBAL wsrep_provider_options = 'pc.ignore_sb=false';
+SET GLOBAL wsrep_provider_options = 'pc.weight=1';
+connection node_2;
+call mtr.add_suppression("WSREP: Foreign key warning in table");


There is some a duplicated word failed in the message as well as some confusion between warning and err. Why is the mtr.add_suppression() less specific? Do we really want to ignore any errors that could be issued for other table or constraint names?

dr-m · 2025-10-23T08:04:42Z

mysql-test/suite/galera/r/galera_FK_duplicate_client_insert,no_warnings.rdiff

+--- /home/jan/work/mariadb/10.11/mysql-test/suite/galera/r/galera_FK_duplicate_client_insert.result
+++ /home/jan/work/mariadb/10.11/mysql-test/suite/galera/r/galera_FK_duplicate_client_insert.reject


Absolute path names do not add any useful information. Some other redundant information had been removed in 88d9348.

dr-m · 2025-10-23T08:11:58Z

mysql-test/suite/galera/t/MW-369.test

+--let $assert_text = Foreign key warning in table: \`test\`\.\`cg\` constraint \`fk_1\` failed err: Setting shared record lock to matching record in the child table failed \(Lock wait\).
+--let $assert_select = Foreign key warning in table: \`test\`\.\`cg\` constraint \`fk_1\` failed err: Setting shared record lock to matching record in the child table failed \(Lock wait\).


There is no table named cg or constraint fk_1 in this test. The error message is misleading or incorrect.

Also, the "child table" appears to be incorrect as well. The SQL terminology would be "referencing table". But, the referencing table is cg-, and there is no need to set any shared lock on it. INSERT would typically acquire an implicit exclusive lock. I believe that a shared lock would be set on a record in the referenced table, or "parent table".

Furthermore, I think that it is a dangerous path to issue "fake" errors in a debug instrumented executable. Why do we need to be so specific? Why can’t we just report "deadlock", "lock wait timeout" or "interrupted" without specifying which lock wait was aborted by that condition? If we need to be specific, then the exact message would have to be output somewhere in row_mysql_handle_errors() and in my opinion it should not be specific to Galera replication.

Table cg was created on line 322 as well as fk_1, I will move this little bit earlier.

I'm sorry but I do not understand your second paragraph.

dr-m · 2025-10-23T08:17:46Z

storage/innobase/include/dict0mem.h

+    Starting with MariaDB 12, constraint names are prepended with the
+    dict_table_t::name and the invalid UTF-8 sequence 0xff. */
+    const char *s;
+    return ((s= strchr(id, '\377')) || (s= strchr(id, '/'))) ? ++s : id;


This is not what I requested earlier. We must not create an impression that a downgrade would work if it has not been tested. The check for '\377' must not be added.

janlindstrom self-assigned this Oct 6, 2025

janlindstrom requested a review from dr-m October 6, 2025 12:06

svoj added the Codership Codership Galera label Oct 6, 2025

dr-m mentioned this pull request Oct 7, 2025

MDEV-36923 : Add warning messages for applier FK failures #4152

Closed

2 tasks

dr-m requested changes Oct 7, 2025

View reviewed changes

mysql-test/suite/galera/r/MW-369.result Outdated Show resolved Hide resolved

storage/innobase/row/row0ins.cc Show resolved Hide resolved

storage/innobase/row/row0ins.cc Outdated Show resolved Hide resolved

janlindstrom force-pushed the 10.11-MDEV-36923 branch 2 times, most recently from e80adde to 04491fc Compare October 9, 2025 07:28

dr-m reviewed Oct 10, 2025

View reviewed changes

janlindstrom force-pushed the 10.11-MDEV-36923 branch 2 times, most recently from 3595516 to ada1189 Compare October 15, 2025 14:27

dr-m reviewed Oct 16, 2025

View reviewed changes

janlindstrom force-pushed the 10.11-MDEV-36923 branch 5 times, most recently from 22e5686 to a40d8be Compare October 20, 2025 12:36

dr-m reviewed Oct 20, 2025

View reviewed changes

MDEV-36923 : Add warning messages for applier FK failures

f915b1c

Add warning messages if applier write set execution fails because of foreign key constraint error. Warnings are printed by default and can be disabled using SET GLOBAL wsrep_mode=APPLIER_DISABLE_FK_WARNINGS.

janlindstrom force-pushed the 10.11-MDEV-36923 branch from a40d8be to f915b1c Compare October 21, 2025 06:30

dr-m reviewed Oct 23, 2025

View reviewed changes

		db_table[end - db_table] = '\0';

		const char *fk_id= strchr(foreign->id, '/');

		--- /home/jan/work/mariadb/10.11/mysql-test/suite/galera/r/galera_FK_duplicate_client_insert.result
		+++ /home/jan/work/mariadb/10.11/mysql-test/suite/galera/r/galera_FK_duplicate_client_insert.reject

		--let $assert_text = Foreign key warning in table: \`test\`\.\`cg\` constraint \`fk_1\` failed err: Setting shared record lock to matching record in the child table failed \(Lock wait\).
		--let $assert_select = Foreign key warning in table: \`test\`\.\`cg\` constraint \`fk_1\` failed err: Setting shared record lock to matching record in the child table failed \(Lock wait\).

Uh oh!

MDEV-36923 : Add warning messages for applier FK failures #4342

Are you sure you want to change the base?

MDEV-36923 : Add warning messages for applier FK failures #4342

Uh oh!

Conversation

janlindstrom commented Oct 6, 2025

Description

Release Notes

How can this PR be tested?

Basing the PR against the correct MariaDB version

PR quality check

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants