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

INSERT IGNORE on constraints result in ERROR rather than warning

Details

    • Bug
    • Status: Closed (View Workflow)
    • Major
    • Resolution: Fixed
    • 10.4.31, 10.11.4
    • 11.4.1
    • MySQL version: 10.11.4-MariaDB-1:10.11.4+maria~ubu2204 through PHP extension MySQLi

    Description

      The following INSERT IGNORE emits an error instead of a warning:

      DROP TABLE IF EXISTS `my_table`;

      CREATE TABLE `my_table` (
      `first_col` char(63),
      `second_col` char(63),
      constraint `my_const` check (`first_col` != `second_col`)
      );

      INSERT IGNORE INTO `my_table` VALUES ('a', 'a');

      Attachments

        Issue Links

          Activity

            danblack Daniel Black added a comment - - edited

            I checked some earlier versions https://dbfiddle.uk/H9aZj6Y9 and it seems it was always causing an error, so I don't think its a regression.

            It seems the KB document https://mariadb.com/kb/en/insert-ignore/ inconsistent as looking at documentation this only applies to this list of errors is ignored.

            error generated

            ERROR 4025 (23000): CONSTRAINT `my_const` failed for `test`.`my_table`
            

            It does seem rather inconsistent to not insert on a FK constraint, but error on a local constraint.

            I tested a fix:

            diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
            index d5f9ba68ccd..df319c4fb68 100644
            --- a/sql/sql_insert.cc
            +++ b/sql/sql_insert.cc
            @@ -1129,10 +1129,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list,
                     }
                   }
             
            -      if ((res= table_list->view_check_option(thd,
            -                                              (values_list.elements == 1 ?
            -                                               0 :
            -                                               ignore))) ==
            +      if ((res= table_list->view_check_option(thd, ignore)) ==
                       VIEW_CHECK_SKIP)
                     continue;
                   else if (res == VIEW_CHECK_ERROR)
            
            

            The reason for this check was added in 2aad30394d33eef23545d717f4b92eb280c2b19d around views with WITH CHECK OPTION.

            mysql_load and mysql_updates (from LOAD DATA and UPDATE) just pass with lex.ignore to view_check_option.

            Sanja, why is the insert behaviour different?

            Why does it error rather than warn if one value provided, by two will ignore?

            danblack Daniel Black added a comment - - edited I checked some earlier versions https://dbfiddle.uk/H9aZj6Y9 and it seems it was always causing an error, so I don't think its a regression. It seems the KB document https://mariadb.com/kb/en/insert-ignore/ inconsistent as looking at documentation this only applies to this list of errors is ignored. error generated ERROR 4025 (23000): CONSTRAINT `my_const` failed for `test`.`my_table` It does seem rather inconsistent to not insert on a FK constraint, but error on a local constraint. I tested a fix: diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc index d5f9ba68ccd..df319c4fb68 100644 --- a/sql/sql_insert.cc +++ b/sql/sql_insert.cc @@ -1129,10 +1129,7 @@ bool mysql_insert(THD *thd, TABLE_LIST *table_list, } } - if ((res= table_list->view_check_option(thd, - (values_list.elements == 1 ? - 0 : - ignore ))) == + if ((res= table_list->view_check_option(thd, ignore )) == VIEW_CHECK_SKIP) continue ; else if (res == VIEW_CHECK_ERROR) The reason for this check was added in 2aad30394d33eef23545d717f4b92eb280c2b19d around views with WITH CHECK OPTION . mysql_load and mysql_updates (from LOAD DATA and UPDATE) just pass with lex.ignore to view_check_option. Sanja, why is the insert behaviour different? Why does it error rather than warn if one value provided, by two will ignore?

            My understanding is that INSERT IGNORE would always enforce CONSTRAINT…FOREIGN KEY (unless they were separately disabled by SET foreign_key_checks=0). Based on this, it could have made sense to not ignore any CONSTRAINT…CHECK violations either.

            I suppose that you could implement ignorable CHECK constraints by rewriting them as virtual column expressions. If I remember correctly, INSERT IGNORE would not only ignore unique key violations (omit one of the duplicate rows) but also ignore errors in evaluating non-indexed virtual columns.

            We may have some similar confusion around ALTER IGNORE TABLE.

            marko Marko Mäkelä added a comment - My understanding is that INSERT IGNORE would always enforce CONSTRAINT…FOREIGN KEY (unless they were separately disabled by SET foreign_key_checks=0 ). Based on this, it could have made sense to not ignore any CONSTRAINT…CHECK violations either. I suppose that you could implement ignorable CHECK constraints by rewriting them as virtual column expressions. If I remember correctly, INSERT IGNORE would not only ignore unique key violations (omit one of the duplicate rows) but also ignore errors in evaluating non-indexed virtual columns. We may have some similar confusion around ALTER IGNORE TABLE .
            danblack Daniel Black added a comment -

            So the issue as to why its an error based on testing and its really obvious by the above code change, is that a single row of values will error, more than one row will warn.

            https://dbfiddle.uk/9BPpIaew

            I asked monty about this and this was because a non-transactional engine could abort before it did anything one row, but more than one it had to continue. Not the most convincing of arguments, but has been this way, and now that you've reported it could be fixed.

            This special rule for INSERT IGNORE VALUES doesn't apply to UPDATE IGNORE or LOAD DATA IGNORE.

            marko FK checks - https://dbfiddle.uk/LuMuGDEg are ok, as the IGNORE is preventing a FK violating row from being inserted which is different from the disable checks and just insert it. Interesting enough, this is a warning.

            So for consistency, lets fix the limited special case and just do warnings always.

            danblack Daniel Black added a comment - So the issue as to why its an error based on testing and its really obvious by the above code change, is that a single row of values will error, more than one row will warn. https://dbfiddle.uk/9BPpIaew I asked monty about this and this was because a non-transactional engine could abort before it did anything one row, but more than one it had to continue. Not the most convincing of arguments, but has been this way, and now that you've reported it could be fixed. This special rule for INSERT IGNORE VALUES doesn't apply to UPDATE IGNORE or LOAD DATA IGNORE . marko FK checks - https://dbfiddle.uk/LuMuGDEg are ok, as the IGNORE is preventing a FK violating row from being inserted which is different from the disable checks and just insert it. Interesting enough, this is a warning. So for consistency, lets fix the limited special case and just do warnings always.
            danblack Daniel Black added a comment - for review: https://github.com/MariaDB/server/pull/2783
            danblack Daniel Black added a comment -

            So while this was intended behaviour at some point:

            1. Generating an error violates what IGNORE is meant to do.
            2. People who use IGNORE don't expect it to error
            3. This behaviour isn't documented
            4. MySQL doesn't generate an error
            5. The error depends on the insert mechanism, so is rather an odd case.

            If there's a MyISAM/Aria case that needs to be fixed, lets do that in the actual engine rather than the SQL layer.

            danblack Daniel Black added a comment - So while this was intended behaviour at some point: Generating an error violates what IGNORE is meant to do. People who use IGNORE don't expect it to error This behaviour isn't documented MySQL doesn't generate an error The error depends on the insert mechanism, so is rather an odd case. If there's a MyISAM/Aria case that needs to be fixed, lets do that in the actual engine rather than the SQL layer.
            danblack Daniel Black added a comment -

            Review by monty said target for non-GA version.

            danblack Daniel Black added a comment - Review by monty said target for non-GA version.

            To be consistent, should MDEV-31985 be fixed as well (without introducing a performance regression like MDEV-31835)?

            marko Marko Mäkelä added a comment - To be consistent, should MDEV-31985 be fixed as well (without introducing a performance regression like MDEV-31835 )?

            People

              danblack Daniel Black
              jkoop Joe Koop
              Votes:
              0 Vote for this issue
              Watchers:
              5 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.