MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:antony Date:June 11 2007 3:29pm
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-11 08:29:42-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.

  mysql-test/r/federated_transactions.result@stripped, 2007-06-11 08:29:35-07:00, acurtis@stripped +27 -0
    bug25513
      test for bug
      test for bulk-insert feature

  mysql-test/r/insert_select.result@stripped, 2007-06-11 08:29:35-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-11 08:29:35-07:00, acurtis@stripped +35 -0
    bug25513
      test for bug
      test for bulk-insert feature

  mysql-test/t/insert_select.test@stripped, 2007-06-11 08:29:35-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-11 08:29:35-07:00, acurtis@stripped +185 -55
    bug25513
      declare new session variable: federated_bulk_insert.
      implement support for bulk insert.

  storage/federated/ha_federated.h@stripped, 2007-06-11 08:29:35-07:00, acurtis@stripped +11 -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-11 08:29:56 -07:00
+++ 1.5/mysql-test/r/federated_transactions.result	2007-06-11 08:29:56 -07:00
@@ -44,6 +44,33 @@
 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
+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
+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-11 08:29:56 -07:00
+++ 1.8/mysql-test/t/federated_transactions.test	2007-06-11 08:29:56 -07:00
@@ -37,4 +37,39 @@
 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;
+
+# 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;
+
 source include/federated_cleanup.inc;

--- 1.50/mysql-test/r/insert_select.result	2007-06-11 08:29:56 -07:00
+++ 1.51/mysql-test/r/insert_select.result	2007-06-11 08:29:56 -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-11 08:29:56 -07:00
+++ 1.41/mysql-test/t/insert_select.test	2007-06-11 08:29:56 -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-11 08:29:56 -07:00
+++ 1.102/storage/federated/ha_federated.cc	2007-06-11 08:29:56 -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,101 @@
 }
 
 
+/** 
+  @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_start_bulk_insert()
+*/
+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.
+
+  @details
+      This method packs many rows into a single bulk-insert statement and
+      transmits it to the remote server.
+
+  @return
+    @retval 0       No error
+	@retval HA_FEDERATED_ERROR_WITH_REMOTE_SYSTEM A remote error occured
+*/
+int ha_federated::send_bulk_insert()
+{
+  int rc= 0;
+  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
+	@retval HA_FEDERATED_ERROR_WITH_REMOTE_SYSTEM A remote error occured
+*/
+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 ? (my_errno= rc) : 0);
+}
+
+
 /*
   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 +1868,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];
@@ -1841,11 +1920,7 @@
   */
   insert_string.append(STRING_WITH_LEN("INSERT 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 +1930,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 +1953,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 +1964,84 @@
     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())
   {
+    /*
+	  sizeof_trailing_comma == 2, which is able to remove the comma and
+	  backtick as usied in insert_string, and the comma and space as used
+	  in values_string
+	*/
     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() + 2 > max_query_size())
+      rc= send_bulk_insert();	
 
-  if (mysql_real_query(mysql, insert_string.ptr(), insert_string.length()))
+    /*
+	  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 occurs 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.
+		We cannot easily recover from this error due to out of memory,
+		so all we can do is ensure that the bulk_insert string is
+		as it was before this call.
+	   */
+      bulk_insert.length= old_length;
+      rc= HA_ERR_OUT_OF_MEM;
+    }
+  }
+  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);
 }
 
 /*
@@ -2898,7 +3019,7 @@
 
 int ha_federated::stash_remote_error()
 {
-  DBUG_ENTER("ha_federated::stash_remote_error()");
+  DBUG_ENTER("ha_federated::stash_remote_error");
   remote_error_number= mysql_errno(mysql);
   strmake(remote_error_buf, mysql_error(mysql), sizeof(remote_error_buf)-1);
   DBUG_RETURN(HA_FEDERATED_ERROR_WITH_REMOTE_SYSTEM);
@@ -3074,6 +3195,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 +3216,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-11 08:29:56 -07:00
+++ 1.47/storage/federated/ha_federated.h	2007-06-11 08:29:56 -07:00
@@ -102,6 +102,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() {}
Thread
bk commit into 5.1 tree (acurtis:1.2548) BUG#25513antony11 Jun