MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:antony Date:June 8 2007 10:19pm
Subject:bk commit into 5.1 tree (acurtis:1.2548) BUG#25513
View as plain text  
Below is the list of changes that have just been committed into a local
5.1 repository of antony. When antony does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet@stripped, 2007-06-08 15:19:28-07:00, acurtis@stripped +6 -0
  Bug#25513
    "Federated Transactions Failure"
    Bug occurs when AUTOCOMMIT=TRUE and where the user performs an operation
    which inserts more than one row into the federated table and the federated
    table references a remote table stored within a transactional storage
    engine. When the insert operation for any one row in the statement fails
    due to constraint violation, the federated engine is unable to perform
    statement rollback and so the remote table contains a partial commit.
    This bug was fixed by implementing  bulk-insert handling into the
    federated storage engine. This will relieve the bug for most common
    situations by enabling the generation of a multi-row insert into the
    remote table and thus permitting the remote table to perform statement
    rollback when neccessary.
    The multi-row insert is limited to the maximum packet size between servers
    and should the size overflow, more than one insert statement will be sent
    and this bug will reappear.
    The bulk-insert handling will offer a significant performance boost when
    inserting a large number of small rows.
    A new session variable 'federated_bulk_insert' permits disabling the new feature.
  
  Implemented support for: INSERT IGNORE/REPLACE/UPDATE IGNORE
  New comments in Doxygen format.

  mysql-test/r/federated_transactions.result@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +40 -0
    bug25513
      test for bug
      test for bulk-insert feature

  mysql-test/r/insert_select.result@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +1 -1
    alter like clause so that the federated_bulk_insert variable does not match.

  mysql-test/t/federated_transactions.test@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +46 -0
    bug25513
      test for bug
      test for bulk-insert feature

  mysql-test/t/insert_select.test@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +1 -1
    alter like clause so that the federated_bulk_insert variable does not match.

  storage/federated/ha_federated.cc@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +238 -56
    bug25513
      declare new session variable: federated_bulk_insert.
      implement support for bulk insert.
      handle IGNORE properly for INSERT/UPDATE

  storage/federated/ha_federated.h@stripped, 2007-06-08 15:19:23-07:00, acurtis@stripped +14 -0
    bug25513
      declare new members for bulk-insert support

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	acurtis
# Host:	ltamd64.xiphis.org
# Root:	/home/antony/work2/p2-bug25513.6

--- 1.4/mysql-test/r/federated_transactions.result	2007-06-08 15:19:41 -07:00
+++ 1.5/mysql-test/r/federated_transactions.result	2007-06-08 15:19:41 -07:00
@@ -44,6 +44,46 @@
 6	fig
 DELETE FROM federated.t1;
 DROP TABLE IF EXISTS federated.t1;
+CREATE TABLE federated.t1 (s1 int, n1 varchar(16), UNIQUE (s1))
+ENGINE=MyISAM;
+DROP TABLE IF EXISTS federated.t1;
+CREATE TABLE federated.t1 (s1 int, n1 varchar(16), UNIQUE (s1)) 
+ENGINE=FEDERATED
+CONNECTION='mysql://root@stripped:SLAVE_PORT/federated/t1';
+SET autocommit=1;
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+ERROR HY000: Got error 10000 'Error on remote system: 1582: Duplicate entry '1' for key 's1'' from FEDERATED
+SELECT * FROM federated.t1;
+s1	n1
+1	Moe
+2	Larry
+ALTER TABLE federated.t1 ENGINE=InnoDB;
+TRUNCATE federated.t1;
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+ERROR HY000: Got error 10000 'Error on remote system: 1582: Duplicate entry '1' for key 's1'' from FEDERATED
+SELECT * FROM federated.t1;
+s1	n1
+INSERT IGNORE INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+s1	n1
+1	Moe
+2	Larry
+TRUNCATE federated.t1;
+SET SESSION federated_bulk_insert=0;
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+ERROR HY000: Got error 10000 'Error on remote system: 1582: Duplicate entry '1' for key 's1'' from FEDERATED
+SELECT * FROM federated.t1;
+s1	n1
+1	Moe
+2	Larry
+TRUNCATE federated.t1;
+SET SESSION federated_bulk_insert=1;
+REPLACE INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+s1	n1
+1	Curly
+2	Larry
+DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;

--- 1.7/mysql-test/t/federated_transactions.test	2007-06-08 15:19:41 -07:00
+++ 1.8/mysql-test/t/federated_transactions.test	2007-06-08 15:19:41 -07:00
@@ -37,4 +37,50 @@
 SELECT * FROM federated.t1;
 DELETE FROM federated.t1;
 
+#
+# Bug#25513 - Federated transaction failure on insert
+#
+connection slave;
+DROP TABLE IF EXISTS federated.t1;
+CREATE TABLE federated.t1 (s1 int, n1 varchar(16), UNIQUE (s1))
+  ENGINE=MyISAM;
+connection master;
+DROP TABLE IF EXISTS federated.t1;
+# # correct connection, same named tables
+--replace_result $SLAVE_MYPORT SLAVE_PORT
+eval CREATE TABLE federated.t1 (s1 int, n1 varchar(16), UNIQUE (s1)) 
+  ENGINE=FEDERATED
+  CONNECTION='mysql://root@stripped:$SLAVE_MYPORT/federated/t1';
+SET autocommit=1;
+--error ER_GET_ERRMSG
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+
+connection slave;
+ALTER TABLE federated.t1 ENGINE=InnoDB;
+TRUNCATE federated.t1;
+connection master;
+
+--error ER_GET_ERRMSG
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+
+# Test INSERT IGNORE
+INSERT IGNORE INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+TRUNCATE federated.t1;
+
+# turning off the bulk insert gets the old behaviour until
+# Federated learns to handle statement transactions properly.
+SET SESSION federated_bulk_insert=0;
+--error ER_GET_ERRMSG
+INSERT INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+TRUNCATE federated.t1;
+
+# Test REPLACE
+SET SESSION federated_bulk_insert=1;
+REPLACE INTO federated.t1 VALUES (1,"Moe"),(2,"Larry"),(1,"Curly");
+SELECT * FROM federated.t1;
+
 source include/federated_cleanup.inc;

--- 1.50/mysql-test/r/insert_select.result	2007-06-08 15:19:41 -07:00
+++ 1.51/mysql-test/r/insert_select.result	2007-06-08 15:19:41 -07:00
@@ -58,7 +58,7 @@
 INSERT INTO t1 (numeropost,icone,contenu,pseudo,date,signature,ip)
 SELECT 1618,icone,contenu,pseudo,date,signature,ip FROM t2
 WHERE numeropost=9 ORDER BY numreponse ASC;
-show variables like '%bulk%';
+show variables like 'bulk%';
 Variable_name	Value
 bulk_insert_buffer_size	8388608
 INSERT INTO t1 (numeropost,icone,contenu,pseudo,date,signature,ip)

--- 1.40/mysql-test/t/insert_select.test	2007-06-08 15:19:41 -07:00
+++ 1.41/mysql-test/t/insert_select.test	2007-06-08 15:19:41 -07:00
@@ -63,7 +63,7 @@
 SELECT 1618,icone,contenu,pseudo,date,signature,ip FROM t2
 WHERE numeropost=9 ORDER BY numreponse ASC;
 
-show variables like '%bulk%';
+show variables like 'bulk%';
 
 INSERT INTO t1 (numeropost,icone,contenu,pseudo,date,signature,ip)
 SELECT 1718,icone,contenu,pseudo,date,signature,ip FROM t2

--- 1.101/storage/federated/ha_federated.cc	2007-06-08 15:19:41 -07:00
+++ 1.102/storage/federated/ha_federated.cc	2007-06-08 15:19:41 -07:00
@@ -389,6 +389,10 @@
 static HASH federated_open_tables;              // To track open tables
 pthread_mutex_t federated_mutex;                // To init the hash
 
+static MYSQL_THDVAR_BOOL(bulk_insert, PLUGIN_VAR_OPCMDARG,
+  "Enable Federated bulk-inserts",
+  NULL, NULL, TRUE);
+
 /* Variables used when chopping off trailing characters */
 static const uint sizeof_trailing_comma= sizeof(", ") - 1;
 static const uint sizeof_trailing_closeparen= sizeof(") ") - 1;
@@ -907,6 +911,8 @@
   mysql(0), stored_result(0)
 {
   trx_next= 0;
+  
+  bzero(&bulk_insert, sizeof(bulk_insert));
 }
 
 
@@ -1741,6 +1747,109 @@
 }
 
 
+/** 
+  @brief Prepares the storage engine for bulk-inserts. 
+
+  @param rows   estimated number of rows in bulk insert or 0 if unknown.
+
+  @details Initializes memory structures required for bulk-inserts.
+  Invoked by ha_sart_bulk_insert()
+  
+  @return
+    None
+*/
+void ha_federated::start_bulk_insert(ha_rows rows)
+{
+  uint page_size;
+  DBUG_ENTER("ha_federated::start_bulk_insert");
+
+  /**
+    We don't bother with bulk-insert semantics when the estimated rows==1.
+    The rows value will be 0 if the server does not know how many rows
+    would be inserted. This can occur when performing INSERT-SELECT.
+  */
+  
+  if (rows == 1 || !THDVAR(ha_thd(), bulk_insert))
+    DBUG_VOID_RETURN;
+
+  page_size= my_getpagesize();
+  init_dynamic_string(&bulk_insert, NULL, page_size, page_size);
+  bulk_inserted= 0;
+
+  DBUG_VOID_RETURN;
+}
+
+
+/**
+  @brief Sends a single multi-row insert statement to the remote server.
+
+  SYNOPSIS
+    send_bulk_insert()
+
+  @details
+      This method packs many rows into a single bulk-insert statement and
+      transmits it to the remote server.
+
+  @return
+  @retval 0     No error
+*/
+int ha_federated::send_bulk_insert()
+{
+  char insert_buffer[FEDERATED_QUERY_BUFFER_SIZE];
+  String insert_string(insert_buffer, sizeof(insert_buffer), &my_charset_bin);
+  uint max_packet_size= mysql->net.max_packet_size;
+  int rc= 0;
+  uint idx;
+  DBUG_ENTER("ha_federated::end_bulk_insert");
+
+  DBUG_ASSERT(bulk_insert.str && bulk_insert.length > 1);
+  
+  /* subtract 1 from the length to remove the trailing comma */
+  if (mysql_real_query(mysql, bulk_insert.str, bulk_insert.length - 1))
+    rc= stash_remote_error();
+  else
+  /*
+    If the table we've just written a record to contains an auto_increment
+    field, then store the last_insert_id() value from the foreign server
+  */
+  if (table->next_number_field)
+    update_auto_increment();
+
+  /* reset query string */
+  bulk_insert.length= 0;
+
+  DBUG_RETURN(rc);
+}
+
+
+/**
+  @brief End bulk inserts
+
+  @details This method will repeatedly call send_bulk_insert() to send 
+  all rows to the remote server. Finally, it will deinitialize the 
+  bulk-insert structures.
+  Invoked by ha_end_bulk_insert()
+
+  @return
+  @retval   0    No error
+*/
+int ha_federated::end_bulk_insert()
+{
+  int rc= 0;
+  DBUG_ENTER("ha_federated::end_bulk_insert()");
+
+  if (bulk_insert.str)
+  {
+    if (bulk_insert.length)
+      rc= send_bulk_insert();
+
+    dynstr_free(&bulk_insert);
+  }
+  
+  DBUG_RETURN(rc);
+}
+
+
 /*
   write_row() inserts a row. No extra() hint is given currently if a bulk load
   is happeneding. buf() is a byte array of data. You can use the field
@@ -1767,52 +1876,30 @@
     in two strings
       "INSERT INTO t1 ("
       and
-      " VALUES ("
+      ""
 
     If there are fields with values, they get appended, with commas, and 
     the last loop, a trailing comma is there
 
     "INSERT INTO t1 ( col1, col2, colN, "
 
-    " VALUES ( 'val1', 'val2', 'valN', "
+    "'val1', 'val2', 'valN', "
 
     Then, if there are fields, it should decrement the string by ", " length.
 
     "INSERT INTO t1 ( col1, col2, colN"
-    " VALUES ( 'val1', 'val2', 'valN'"
+    "'val1', 'val2', 'valN'"
 
-    Then it adds a close paren to both - if there are fields
+    Then it adds a close paren to the first string - if there are fields
 
     "INSERT INTO t1 ( col1, col2, colN)"
-    " VALUES ( 'val1', 'val2', 'valN')"
+    "'val1', 'val2', 'valN'"
 
     Then appends both together
     "INSERT INTO t1 ( col1, col2, colN) VALUES ( 'val1', 'val2', 'valN')"
 
-    So... the problem, is if you have the original statement:
-
-    "INSERT INTO t1 VALUES ()"
-
-    Which is legitimate, but if the code thinks there are fields
-
-    "INSERT INTO t1 ("
-    " VALUES ( "
-
-    If the field flag is set, but there are no commas, reduces the 
-    string by strlen(", ")
-
-    "INSERT INTO t1 "
-    " VALUES "
-
-    Then adds the close parenthesis
-
-    "INSERT INTO t1  )"
-    " VALUES  )"
-
-    So, I have to use a bool as before, set in the loop where fields and commas
-    are appended to the string
   */
-  my_bool commas_added= FALSE;
+  int rc= 0;
   char insert_buffer[FEDERATED_QUERY_BUFFER_SIZE];
   char values_buffer[FEDERATED_QUERY_BUFFER_SIZE];
   char insert_field_value_buffer[STRING_BUFFER_USUAL_SIZE];
@@ -1839,13 +1926,16 @@
   /*
     start both our field and field values strings
   */
-  insert_string.append(STRING_WITH_LEN("INSERT INTO `"));
+  if (!ignore_duplicates)
+    insert_string.append(STRING_WITH_LEN("INSERT INTO `"));
+  else
+  if (replace_duplicates)
+    insert_string.append(STRING_WITH_LEN("REPLACE INTO `"));
+  else
+    insert_string.append(STRING_WITH_LEN("INSERT IGNORE INTO `"));
+  
   insert_string.append(share->table_name, share->table_name_length);
-  insert_string.append('`');
-  insert_string.append(STRING_WITH_LEN(" ("));
-
-  values_string.append(STRING_WITH_LEN(" VALUES "));
-  values_string.append(STRING_WITH_LEN(" ("));
+  insert_string.append(STRING_WITH_LEN("` (`"));
 
   /*
     loop through the field pointer array, add any fields to both the values
@@ -1855,7 +1945,6 @@
   {
     if (bitmap_is_set(table->write_set, (*field)->field_index))
     {
-      commas_added= TRUE;
       if ((*field)->is_null())
         values_string.append(STRING_WITH_LEN(" NULL "));
       else
@@ -1879,7 +1968,7 @@
         make the following appends conditional as we don't know if the
         next field is in the write set
       */
-      insert_string.append(STRING_WITH_LEN(", "));
+      insert_string.append(STRING_WITH_LEN("`,`"));
       values_string.append(STRING_WITH_LEN(", "));
     }
   }
@@ -1890,37 +1979,79 @@
     AND, we don't want to chop off the last char '('
     insert will be "INSERT INTO t1 VALUES ();"
   */
-  if (commas_added)
+  if (values_string.length())
   {
     insert_string.length(insert_string.length() - sizeof_trailing_comma);
-    /* chops off leading commas */
+    /* chops off trailing comma */
     values_string.length(values_string.length() - sizeof_trailing_comma);
     insert_string.append(STRING_WITH_LEN(") "));
   }
   else
   {
-    /* chops off trailing ) */
-    insert_string.length(insert_string.length() - sizeof_trailing_closeparen);
+    /* chops off trailing "(`" */
+    insert_string.length(insert_string.length() - 2);
   }
 
-  /* we always want to append this, even if there aren't any fields */
-  values_string.append(STRING_WITH_LEN(") "));
-
-  /* add the values */
-  insert_string.append(values_string);
+  /*
+    Check if bulk-inserts are in effect
+  */  
+  if (bulk_insert.str)
+  {
+    uint old_length;
+  
+    /* if the query size is going to overflow the packet, send the rows */
+    if (bulk_insert.length + values_string.length() > max_query_size())
+      rc= send_bulk_insert();	
+
+    /* if the bulk_insert string is empty, the INSERT statement part
+       needs to be appended. The row is then appended to the bulk_insert
+       string and if any error occurrs while appending to the string,
+       the string length is reset to its previous value */
+	if ((!(old_length= bulk_insert.length) &&
+         (dynstr_append_mem(&bulk_insert, insert_string.ptr(),
+                            insert_string.length()) ||
+          dynstr_append_mem(&bulk_insert, STRING_WITH_LEN(" VALUES ")))) ||
+        dynstr_append_mem(&bulk_insert, STRING_WITH_LEN("(")) ||
+        dynstr_append_mem(&bulk_insert, values_string.ptr(), 
+                          values_string.length()) ||
+        dynstr_append_mem(&bulk_insert, STRING_WITH_LEN("),")))
+    {
+      /* reset the length to be where it was before the append */
+      bulk_insert.length= old_length;
+      rc= HA_ERR_OUT_OF_MEM;
+    }
+    else
 
-  if (mysql_real_query(mysql, insert_string.ptr(), insert_string.length()))
+    /* if we are at the last row or the multi-row insert statement is
+       already over the limit or if the previous row added is larger
+       than half the packet size, send the statement to the remote server */
+    if ((estimation_rows_to_insert == ++bulk_inserted ||
+         bulk_insert.length >= max_query_size() ||
+         values_string.length() > max_query_size()/2) && !rc)
+      rc= send_bulk_insert();
+  }
+  else
   {
-    DBUG_RETURN(stash_remote_error());
+    insert_string.append(STRING_WITH_LEN(" VALUES ("));
+
+    /* add the values */
+    insert_string.append(values_string);
+
+    /* we always want to append this, even if there aren't any fields */
+    insert_string.append(')');
+
+    if (mysql_real_query(mysql, insert_string.ptr(), insert_string.length()))
+      rc= stash_remote_error();
+    else
+    /*
+  If the table we've just written a record to contains an auto_increment
+  field, then store the last_insert_id() value from the foreign server
+    */
+    if (table->next_number_field)
+      update_auto_increment();
   }
-  /*
-    If the table we've just written a record to contains an auto_increment
-    field, then store the last_insert_id() value from the foreign server
-  */
-  if (table->next_number_field)
-    update_auto_increment();
 
-  DBUG_RETURN(0);
+  DBUG_RETURN(rc);
 }
 
 /*
@@ -2053,7 +2184,10 @@
   update_string.length(0);
   where_string.length(0);
 
-  update_string.append(STRING_WITH_LEN("UPDATE `"));
+  if (!ignore_duplicates)
+    update_string.append(STRING_WITH_LEN("UPDATE `"));
+  else
+    update_string.append(STRING_WITH_LEN("UPDATE IGNORE `"));
   update_string.append(share->table_name);
   update_string.append(STRING_WITH_LEN("` SET "));
 
@@ -2770,6 +2904,45 @@
 }
 
 
+/**
+  @brief Handles extra signals from MySQL server
+  @param operation Signal
+ */
+int ha_federated::extra(ha_extra_function operation)
+{
+  DBUG_ENTER("ha_federated::extra");
+  switch (operation) {
+  case HA_EXTRA_IGNORE_DUP_KEY:
+    ignore_duplicates= TRUE;
+	break;
+  case HA_EXTRA_WRITE_CAN_REPLACE:
+    replace_duplicates= TRUE;
+	break;
+  case HA_EXTRA_WRITE_CANNOT_REPLACE:
+    replace_duplicates= FALSE;
+    break;
+  case HA_EXTRA_NO_IGNORE_DUP_KEY:
+    reset();
+    break;
+  default:
+    /* do nothing */
+    DBUG_PRINT("info",("unhandled operation: %d", (uint) operation));
+  }
+  DBUG_RETURN(0);
+}
+
+
+/**
+  @brief Resets the storage engine to a 'newly opened' state.
+ */
+int ha_federated::reset()
+{
+  ignore_duplicates= FALSE;
+  replace_duplicates= FALSE;
+  return 0;
+}
+
+
 /*
   Used to delete all rows in a table. Both for cases of truncate and
   for cases where the optimizer realizes that all rows will be
@@ -3074,6 +3247,15 @@
 struct st_mysql_storage_engine federated_storage_engine=
 { MYSQL_HANDLERTON_INTERFACE_VERSION };
 
+
+/** Declaration for Federated server variables */
+static struct st_mysql_sys_var* federated_system_variables[]=
+{
+  MYSQL_SYSVAR(bulk_insert),
+  NULL
+};
+
+
 mysql_declare_plugin(federated)
 {
   MYSQL_STORAGE_ENGINE_PLUGIN,
@@ -3086,7 +3268,7 @@
   federated_done, /* Plugin Deinit */
   0x0100 /* 1.0 */,
   NULL,                       /* status variables                */
-  NULL,                       /* system variables                */
+  federated_system_variables, /* system variables                */
   NULL                        /* config options                  */
 }
 mysql_declare_plugin_end;

--- 1.46/storage/federated/ha_federated.h	2007-06-08 15:19:41 -07:00
+++ 1.47/storage/federated/ha_federated.h	2007-06-08 15:19:41 -07:00
@@ -88,6 +88,7 @@
   MYSQL_ROW_OFFSET current_position;  // Current position used by ::position()
   int remote_error_number;
   char remote_error_buf[FEDERATED_QUERY_BUFFER_SIZE];
+  bool ignore_duplicates, replace_duplicates;
 
 private:
   /*
@@ -102,6 +103,17 @@
                              bool records_in_range, bool eq_range);
   int stash_remote_error();
 
+  /* The following members are for bulk-insert support */
+  ha_rows bulk_inserted;
+  DYNAMIC_STRING bulk_insert;
+  uint max_query_size() const
+  { return mysql->net.max_packet_size - 64 /* packet overhead */; }
+  int send_bulk_insert();
+
+protected:
+  void start_bulk_insert(ha_rows rows);
+  int end_bulk_insert();
+
 public:
   ha_federated(handlerton *hton, TABLE_SHARE *table_arg);
   ~ha_federated() {}
@@ -216,6 +228,8 @@
   int rnd_pos(uchar *buf, uchar *pos);                            //required
   void position(const uchar *record);                            //required
   int info(uint);                                              //required
+  int extra(ha_extra_function operation);
+  int reset();
 
   void update_auto_increment(void);
   int repair(THD* thd, HA_CHECK_OPT* check_opt);
Thread
bk commit into 5.1 tree (acurtis:1.2548) BUG#25513antony8 Jun