MDEV-36923 : Add warning messages for applier FK failures #4342
Conversation
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 jan.lindstrom@galeracluster.com
| SET GLOBAL DEBUG_DBUG = ""; | ||
| SET DEBUG_SYNC = 'RESET'; | ||
| connection node_1; | ||
| call mtr.add_suppression("WSREP: Foreign key check table:.*"); |
There was a problem hiding this comment.
.* at the end of the regular expression is redundant, because there is no implied $ anchor. Shouldn’t there also be a search_pattern_in_file.inc for ensuring that the message is present?
| fputs("\nBut in child table ", ef); | ||
| ut_print_name(ef, trx, foreign->foreign_table_name); | ||
| fputs(fk_table_str.c_str(), ef); | ||
| fprintf(ef, ", in index %s", foreign->foreign_index->name()); |
There was a problem hiding this comment.
Please, let’s avoid using any std::string and use a single my_printf_error or similar for this. See how the function dict_table_open_failed() is displaying table names.
There was a problem hiding this comment.
I do not see reason for this because function already used std::string see line 771 i.e. fk_str = dict_print_info_on_foreign_key_in_create_format(...)
There was a problem hiding this comment.
Yes, it was already suboptimal, and since the time the code was originally written, we are aware of a more efficient way of displaying InnoDB table names. One thing that I overlooked yesterday is the conversion from my_charset_filename to system_charset_info (UTF-8). A test case for this must involve table names that include some non-alphanumeric characters, such as - or $. The test case also needs to search for these messages in the SHOW ENGINE INNODB STATUS output.
There was a problem hiding this comment.
I will revert changes to here as they are not needed, I rather change only wsrep code and change InnoDB as less as possible.
| void wsrep_fk_warn(const std::string warn_msg, | ||
| const trx_t* trx, | ||
| const dict_table_t *table, | ||
| const dict_index_t *index, | ||
| const dberr_t err) | ||
| { | ||
| std::string fk_warn= "Foreign key check table: %s index: %s failed for "; | ||
| fk_warn+= warn_msg; | ||
| fk_warn+=" err: %s"; |
There was a problem hiding this comment.
Please avoid the use of std::string. It can lead to significant memory heap fragmentation.
The message is very confusing, because CHECK TABLE looks like a SQL statement. Why is the name of the failing constraint not being displayed? That together with the referencing table name should be sufficient diagnostics.
There was a problem hiding this comment.
Maybe it causes heap fragmentation, but std::string is used also in InnoDB codebase already, this function is called only when some foreign key action fails in applier, this means that foreign key action was successful on originating node in cluster and now write set of this action was successfully certified but execution failed for some reason when write set is applied in other node. In some cases this means that cluster nodes are inconsistent state leading to inconsistency voting (i.e. what was majority of nodes decision on this write set). Meaning that this error printing should not be any regular or frequent thing. In case there is lock wait as this is applier it will kill conflicting local transactions, thus there is possibility that two conflicting foreign key actions arrive from different nodes in cluster in same time and they are executed in order of their sequence number, if there still is lock wait or deadlock error write set action is replayed.
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 jan.lindstrom@galeracluster.com
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
mainbranch.PR quality check