Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-24830

Write a warning to error log if Galera replicates InnoDB table with no primary key

Details

    Description

      If wsrep_certify_nonPK is enabled, then Galera will certify transactions for InnoDB tables with no primary key. wsrep_certify_nonPK is enabled by default, so this is actually the default behavior:

      https://mariadb.com/docs/reference/mdb/system-variables/wsrep_certify_nonpk/#mdb-system-variables-wsrep-certify-nonpk

      With Galera, it is not necessarily safe to define tables that have no primary key. Perhaps if wsrep_certify_nonPK is enabled and if log_warnings is set to >=2, then MariaDB should write a warning to the error log if it certifies a transactions for an InnoDB table that has no primary key. e.g. maybe it could look like this:

      [Warning] wsrep_certify_nonPK is enabled, so Galera certified transaction for table db1.tab, which has no primary key. For optimal performance and consistency, it is recommended to define a primary key for all tables that Galera writes to.

      If we add a warning like this, then we should probably limit the number of times that it gets printed, so that it doesn't flood the log.

      Attachments

        Issue Links

          Activity

            GeoffMontee Default value for log_warnings is 2 so this could have too big effect on normal cluster. Can I use value >2 instead ?

            jplindst Jan Lindström (Inactive) added a comment - GeoffMontee Default value for log_warnings is 2 so this could have too big effect on normal cluster. Can I use value >2 instead ?

            Hi jplindst.

            In my opinion, not having a PK on a table does deserve a warning by default with Galera, so I think it makes the most sense to log a warning when log_warnings=2.

            Are you concerned that the log could be flooded with the message? If so, then you could probably avoid flooding the log by doing a check similar to the check for ER_BINLOG_UNSAFE_STATEMENT. e.g.:

            /**
              Auxiliary function to check if the warning for unsafe repliction statements
              should be thrown or suppressed.
              Logic is:
              - If we get more than LIMIT_UNSAFE_WARNING_ACTIVATION_THRESHOLD_COUNT errors
                of one type, that type of errors will be suppressed for
                LIMIT_UNSAFE_WARNING_ACTIVATION_TIMEOUT.
              - When the time limit has been reached, all suppression is reset.
              This means that if one gets many different types of errors, some of them
              may be reset less than LIMIT_UNSAFE_WARNING_ACTIVATION_TIMEOUT. However at
              least one error is disable for this time.
              SYNOPSIS:
              @params
               unsafe_type - The type of unsafety.
              RETURN:
                0   0k to log
                1   Message suppressed
            */
             
            static bool protect_against_unsafe_warning_flood(int unsafe_type)
            

            https://github.com/MariaDB/server/blob/mariadb-10.5.8/sql/sql_class.cc#L7170

            GeoffMontee Geoff Montee (Inactive) added a comment - Hi jplindst . In my opinion, not having a PK on a table does deserve a warning by default with Galera, so I think it makes the most sense to log a warning when log_warnings=2. Are you concerned that the log could be flooded with the message? If so, then you could probably avoid flooding the log by doing a check similar to the check for ER_BINLOG_UNSAFE_STATEMENT . e.g.: /** Auxiliary function to check if the warning for unsafe repliction statements should be thrown or suppressed. Logic is: - If we get more than LIMIT_UNSAFE_WARNING_ACTIVATION_THRESHOLD_COUNT errors of one type, that type of errors will be suppressed for LIMIT_UNSAFE_WARNING_ACTIVATION_TIMEOUT. - When the time limit has been reached, all suppression is reset. This means that if one gets many different types of errors, some of them may be reset less than LIMIT_UNSAFE_WARNING_ACTIVATION_TIMEOUT. However at least one error is disable for this time. SYNOPSIS: @params unsafe_type - The type of unsafety. RETURN: 0 0k to log 1 Message suppressed */   static bool protect_against_unsafe_warning_flood( int unsafe_type) https://github.com/MariaDB/server/blob/mariadb-10.5.8/sql/sql_class.cc#L7170

            GeoffMontee Thanks for the feedback. I have another question. Should I print warning to error log only or should every DML that causes unsafe Galera operation also print warning? At the moment it seems based on mtr-tests we have: Completed: Failed 181/1243 tests, 85.44% were successful. I should somehow modify that many tests using current implementation and above will not help on that. Here is example of current output:

             INSERT INTO t2 VALUES ('abc');
            +Warnings:
            +Warning	1105	wsrep_certify_nonPK is enabled, so Galera certified DML command on a table (test.t2) without an explicit primary key. For optimal performance and consistency, it is recommended to define a primary key for all tables that Galera writes to.
            

            jplindst Jan Lindström (Inactive) added a comment - GeoffMontee Thanks for the feedback. I have another question. Should I print warning to error log only or should every DML that causes unsafe Galera operation also print warning? At the moment it seems based on mtr-tests we have: Completed: Failed 181/1243 tests, 85.44% were successful. I should somehow modify that many tests using current implementation and above will not help on that. Here is example of current output: INSERT INTO t2 VALUES ('abc'); +Warnings: +Warning 1105 wsrep_certify_nonPK is enabled, so Galera certified DML command on a table (test.t2) without an explicit primary key. For optimal performance and consistency, it is recommended to define a primary key for all tables that Galera writes to.

            Hi jplindst,

            Should I print warning to error log only or should every DML that causes unsafe Galera operation also print warning?

            Good point. It would probably make sense to log a warning for all unsafe operations. I believe that some other unsafe operations might be:

            • A session has binlog_format set to STATEMENT, and the session writes to an InnoDB table. The associated statement will be logged in statement-based format, which is unsupported by Galera,
            • A session has binlog_format set to MIXED, and the session writes to an InnoDB table. The associated statement could be logged in statement-based format, which is unsupported by Galera.
            • A node has wsrep_replicate_myisam set to OFF, and a session on the node writes to a MyISAM table, so the write will not be replicated.
            • A node has wsrep_replicate_aria set to OFF, and a session on the node writes to an Aria table, so the write will not be replicated.
            • A session writes to any other non-InnoDB storage engine , so the write will not be replicated.
            • A session has wsrep_on set to OFF, and the session writes to an InnoDB table, so the write will not be replicated.

            What do you think about those?

            Thanks!

            GeoffMontee Geoff Montee (Inactive) added a comment - Hi jplindst , Should I print warning to error log only or should every DML that causes unsafe Galera operation also print warning? Good point. It would probably make sense to log a warning for all unsafe operations. I believe that some other unsafe operations might be: A session has binlog_format set to STATEMENT , and the session writes to an InnoDB table. The associated statement will be logged in statement-based format, which is unsupported by Galera, A session has binlog_format set to MIXED , and the session writes to an InnoDB table. The associated statement could be logged in statement-based format, which is unsupported by Galera. A node has wsrep_replicate_myisam set to OFF , and a session on the node writes to a MyISAM table, so the write will not be replicated. A node has wsrep_replicate_aria set to OFF , and a session on the node writes to an Aria table, so the write will not be replicated. A session writes to any other non-InnoDB storage engine , so the write will not be replicated. A session has wsrep_on set to OFF , and the session writes to an InnoDB table, so the write will not be replicated. What do you think about those? Thanks!

            GeoffMontee In this ticket I will handle only primary key issue. Rest of the ideas will be considered on MDEV-20008.

            jplindst Jan Lindström (Inactive) added a comment - GeoffMontee In this ticket I will handle only primary key issue. Rest of the ideas will be considered on MDEV-20008 .
            jplindst Jan Lindström (Inactive) added a comment - https://github.com/MariaDB/server/commit/22fe31fec7648ac340690aefbab6053bbf5d02f6

            People

              jplindst Jan Lindström (Inactive)
              GeoffMontee Geoff Montee (Inactive)
              Votes:
              2 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.