Bug #25034 MySQL does not correctly handle errors on statement commit
Submitted: 13 Dec 2006 10:54 Modified: 18 Jun 2008 11:20
Reporter: Jan Lindström Email Updates:
Status: Won't fix Impact on me:
None 
Category:MySQL Server: General Severity:S2 (Serious)
Version:5.0 & 5.1 OS:Any (Any)
Assigned to: Konstantin Osipov CPU Architecture:Any
Tags: Contribution

[13 Dec 2006 10:54] Jan Lindström
Description:
If a storage engine returns duplicate key or HA_ERR_NO_REFERENCED_ROW error on statement commit not in ::write_row() i.e. is using a deferred foreign key checks. MySQL does not correctly handle error code returned from the storage engine.

mysql> CREATE TABLE `test1` (
    ->   `a` int(11) NOT NULL auto_increment,
    ->   PRIMARY KEY  (`a`)
    -> ) ENGINE=solidDB;
Query OK, 0 rows affected (0.10 sec)

mysql> CREATE TABLE `test2` (
    ->   `a` int(11) NOT NULL auto_increment,
    ->   `ref_test1_a` int(11) default NULL,
    ->   PRIMARY KEY  (`a`)
    -> ) ENGINE=solidDB;
Query OK, 0 rows affected (0.11 sec)

mysql> insert into test2 values(1,1);
Query OK, 1 row affected (0.02 sec)

mysql> alter table test2 add constraint fk1 foreign key (ref_test1_a)
references test1(a);
Query OK, 1 row affected, 1 warning (0.01 sec)
Records: 1  Duplicates: 0  Warnings: 0

mysql> show warnings;
+-------+------+------------------------------------------------------------------+
| Level | Code | Message                                                       
  |
+-------+------+------------------------------------------------------------------+
| Error | 1216 | Cannot add or update a child row: a foreign key constraint
fails |
+-------+------+------------------------------------------------------------------+
1 row in set (0.00 sec)

How to repeat:
CREATE TABLE `test1` (
  `a` int(11) NOT NULL auto_increment,
  PRIMARY KEY  (`a`)
) ENGINE=solidDB;

CREATE TABLE `test2` (
  `a` int(11) NOT NULL auto_increment,
  `ref_test1_a` int(11) default NULL,
  PRIMARY KEY  (`a`)
) ENGINE=solidDB;

insert into test2 values(1,1);

alter table test2 add constraint fk1 foreign key (ref_test1_a)
references test1(a);

Suggested fix:
Add error handling to both commit and rollback on handler level.
[30 Dec 2006 11:08] Valeriy Kravchuk
Thank you for a problem report. Please, send the results of:

SHOW VARIABLES LIKE 'sql_mode'\G

MySQL should return error (nor warning) in cases like that if(!) strict mode (STRICT_TRANS_TABLES) is set.
[31 Jan 2007 0:00] Bugs System
No feedback was provided for this bug for over a month, so it is
being suspended automatically. If you are able to provide the
information that was originally requested, please do so and change
the status of the bug back to "Open".
[31 Jan 2007 7:50] Jan Lindström
I use defaults.

mysql> SHOW VARIABLES LIKE 'sql_mode'\G
*************************** 1. row ***************************
Variable_name: sql_mode
        Value: 
1 row in set (0.01 sec)

Note that there are imilar problems with truncate table, drop table, alter table, rename table, create index commands. Storage engine can return error code from all of these commands and MySQL should print exactly that error code not some other error code.
[6 Feb 2007 7:02] Jan Lindström
Suggested fix:

Introduce a new function on handlerton for retrieving error message from the handler if commit fails and add error handling to handler commit.

--- 1.292/sql/handler.cc	2007-01-16 10:34:40 +02:00
+++ 1.293/sql/handler.cc	2007-01-16 10:34:40 +02:00
@@ -745,6 +745,19 @@
   DBUG_RETURN(error);
 }
 
+/* Return key if error because of duplicated keys */
+
+uint handler::get_dup_key(int error)
+{
+  DBUG_ENTER("handler::get_dup_key");
+  table->file->errkey  = (uint) -1;
+  if (error == HA_ERR_FOUND_DUPP_KEY || error == HA_ERR_FOREIGN_DUPLICATE_KEY ||
+      error == HA_ERR_FOUND_DUPP_UNIQUE || error == HA_ERR_NULL_IN_SPATIAL ||
+      error == HA_ERR_DROP_INDEX_FK)
+    info(HA_STATUS_ERRKEY | HA_STATUS_NO_LOCK);
+  DBUG_RETURN(table->file->errkey);
+}
+
 /*
   NOTE - this function does not care about global read lock.
   A caller should.
@@ -764,8 +777,91 @@
       int err;
       if ((err= (*(*ht)->commit)(*ht, thd, all)))
       {
+          int errcode;
+
+          /* Note that errors are possible on commit phase on some
+             storage engines */
+          
+          switch (err) {
+              case HA_ERR_ROW_IS_REFERENCED:
+              {
+                  String str;
+                  TABLE *table=NULL;
+                  
+                  int key_nr = (*(*ht)->get_error_message)(*ht, thd, &table, err, &str);
+                  my_error(ER_ROW_IS_REFERENCED_2, MYF(0), str.c_ptr_safe());
+                  error = 1;
+                  break;
+              }
+              case HA_ERR_NO_REFERENCED_ROW:
+              {
+                  String str;
+                  TABLE *table=NULL;
+                  
+                  int key_nr = (*(*ht)->get_error_message)(*ht, thd, &table, err, &str);
+                  my_error(ER_NO_REFERENCED_ROW_2, MYF(0), str.c_ptr_safe());
+                  error = 1;
+                  break;
+              }
+              case HA_ERR_WRONG_COMMAND:
+                  errcode = ER_ILLEGAL_HA;
+                  break;
+              case HA_ERR_FOUND_DUPP_KEY:
+              {
+                  char key[MAX_KEY_LENGTH];
+                  String str(key,sizeof(key),system_charset_info);
+                  TABLE *table=NULL;
+
+                  int key_nr = (*(*ht)->get_error_message)(*ht, thd, &table, err, &str);
+
+                  if (key_nr == MAX_KEY)
+                  {
+                      /* Key is unknown */
+                      str.copy("", 0, system_charset_info);
+                      my_printf_error(ER_DUP_ENTRY, ER(ER_DUP_ENTRY),
+                                      MYF(0), str.c_ptr(), "*UNKNOWN*");
+                  }
+                  else
+                  {
+                      /* Table is opened and defined at this point */
+                      key_unpack(&str,table,(uint) key_nr);
+                      uint max_length=MYSQL_ERRMSG_SIZE-(uint) strlen(ER(ER_DUP_ENTRY));
+                      if (str.length() >= max_length)
+                      {
+                          str.length(max_length-4);
+                          str.append(STRING_WITH_LEN("..."));
+                      }
+                      my_printf_error(ER_DUP_ENTRY, ER(ER_DUP_ENTRY),
+                                      MYF(0), str.c_ptr(), table->key_info[key_nr].name);
+                  }
+                  
+                  error=1;
+                  break;
+              }
+              case HA_ERR_LOCK_WAIT_TIMEOUT:
+                  errcode = ER_LOCK_WAIT_TIMEOUT;
+                  break;
+              case HA_ERR_LOCK_DEADLOCK:
+                  errcode = ER_LOCK_DEADLOCK;
+                  break;
+              case HA_ERR_READ_ONLY_TRANSACTION:
+                  errcode = ER_READ_ONLY_TRANSACTION;
+                  break;
+              default:
+                  errcode = ER_ERROR_DURING_COMMIT;
+                  break;
+          }
+
+          if (!error) {    
+              my_error(errcode, MYF(0), err);
+          }
+          
+        error=1;

--- 1.252/sql/handler.h	2007-01-16 10:34:40 +02:00
+++ 1.253/sql/handler.h	2007-01-16 10:34:40 +02:00
@@ -397,6 +397,16 @@
 #define MAX_XID_LIST_SIZE  (1024*128)
 #endif
 
+/* The handler for a table type.  Will be included in the TABLE structure */
+
+struct st_table;
+typedef struct st_table TABLE;
+struct st_foreign_key_info;
+typedef struct st_foreign_key_info FOREIGN_KEY_INFO;
+
+typedef struct st_savepoint SAVEPOINT;
+extern ulong savepoint_alloc_size;
+
 /*
   These structures are used to pass information from a set of SQL commands
   on add/drop/change tablespace definitions to the proper hton.
@@ -682,6 +692,8 @@
                      const char *wild, bool dir, List<char> *files);
    int (*table_exists_in_engine)(handlerton *hton, THD* thd, const char *db,
                                  const char *name);
+   int (*get_error_message)(handlerton *hton, THD *thd, TABLE**,int, String* buf);
+        
    uint32 license; /* Flag for Engine License */
    void *data; /* Location for engines to keep personal structures */
 };
[18 Jun 2008 11:20] Konstantin Osipov
I believe this is not relevant any more. We can't test it in the server, so should not fix until the server ships with an engine with deferred foreign key checks.