List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:January 18 2012 1:03pm
Subject:bzr push into mysql-trunk branch (tor.didriksen:3745 to 3746) Bug#13580775
View as plain text  
 3746 Tor Didriksen	2012-01-18
      Bug#13580775 ASSERTION FAILED: RECORD_LENGTH == M_RECORD_LENGTH
      
      filesort tries to re-use the sort buffer between invocations in order to save
      malloc/free overhead.
      The fix for Bug 11748783 - 37359: FILESORT CAN BE MORE EFFICIENT.
      added an assert that buffer properties (num_records, record_length) are
      consistent between invocations. Indeed, they are not necessarily consistent.
      
      Fix: re-allocate the sort buffer if properties change.
     @ mysql-test/r/partition.result
        New test case.
     @ mysql-test/t/partition.test
        New test case.
     @ sql/filesort.cc
        If we already have allocated a sort buffer, then verify that the same properties
        (keys, rec_length) that we need for this invocation.
     @ sql/filesort_utils.h
        Add sort_buffer_properties
     @ sql/table.h
        Add sort_buffer_properties
     @ unittest/gunit/filesort_buffer-t.cc
        Test new member funciton.

    modified:
      mysql-test/r/partition.result
      mysql-test/t/partition.test
      sql/filesort.cc
      sql/filesort_utils.h
      sql/table.h
      unittest/gunit/filesort_buffer-t.cc
 3745 Praveenkumar Hulakund	2012-01-18
      BUG#11764724 - 57586: UNKNOWN TABLE WHEN TRYING TO DROP A TABLE AND NO AVAILABLE UNDO 
      SLOTS LEFT
      
      Modified changes made as part of fix for this bug to  fix Perf regression.
      FIX for perf regression:
      -------------------------------
      Current fix had a if condition to check whether return value of function "ha_delete_table"
      is "HA_ERR_TOO_MANY_CONCURRENT_TRXS". If yes then I used to "goto err" and in "err:" check
      whether error is "HA_ERR_TOO_MANY_CONCURRENT_TRXS" and print error message. So there were 
      TWO if conditions added to check whether error is "HA_ERR_TOO_MANY_CONCURRENT_TRXS" or not.
      These conditions were getting evaluated every time, this caused perf regression.
      
      As a fix, removed both the if conditions and added only one if condition in "if (error)" block.
      This condition is added to check whether error is "HA_ERR_TOO_MANY_CONCURRENT_TRXS". If yes 
      then print error and then "goto err". Now, this condition is checked only if there is any error.
      Otherwise it will not be evaluated and so performance should not be affected.
      
      Actual Issue Description:
      -------------------------------
      If you attempt to drop an existing InnoDB table, but you do not have any 
      available undo slots open, then you will receive an "unknown table" error. 
      
      mysql> DROP TABLE test.innodb_table_monitor; 
      ERROR 1051 (42S02): Unknown table 'innodb_table_monitor' 
      
      Actual issue Analysis:
      -------------------------------
      Here, max number of concurrent transactions/connections are started and then the
      next transaction to drop a table is initiated. Since, system has already max number
      of transactions running, next transaction to "drop table" was not started and innodb
      returned error "DB_TOO_MANY_CONCURRENT_TRXS". But this error was not handled properly 
      in sql layer because of which "drop table" operation was throwing wrong error message. 
      
      Fix:
      -------------------------------
      As a fix, I have added check in sql (in function "mysql_rm_table_part2") to handle error 
      code "HA_ERR_TOO_MANY_CONCURRENT_TRXS" and throw proper error message.
      
      Ouput of drop command after fix (with max number of concurrent transaction running)
      
      mysql> drop table test;
      ERROR 177 (HY000): Too many active concurrent transactions

    modified:
      sql/sql_table.cc
=== modified file 'mysql-test/r/partition.result'
--- a/mysql-test/r/partition.result	2011-10-06 09:50:40 +0000
+++ b/mysql-test/r/partition.result	2012-01-18 13:03:10 +0000
@@ -2429,3 +2429,25 @@ SELECT * FROM vtmp;
 1
 DROP VIEW vtmp;
 DROP TABLE t1;
+#
+# Bug#13580775 ASSERTION FAILED: RECORD_LENGTH == M_RECORD_LENGTH,
+# FILE FILESORT_UTILS.CC
+#
+CREATE TABLE t1 (
+a INT PRIMARY KEY,
+b INT,
+c CHAR(1),
+d INT,
+KEY (c,d)
+) PARTITION BY KEY () PARTITIONS 1;
+INSERT INTO t1 VALUES (1,1,'a',1), (2,2,'a',1);
+SELECT 1 FROM t1 WHERE 1 IN
+(SELECT  group_concat(b)
+FROM t1
+WHERE c > geomfromtext('point(1 1)')
+GROUP BY b
+);
+1
+1
+1
+DROP TABLE t1;

=== modified file 'mysql-test/t/partition.test'
--- a/mysql-test/t/partition.test	2011-10-06 09:50:40 +0000
+++ b/mysql-test/t/partition.test	2012-01-18 13:03:10 +0000
@@ -2433,3 +2433,27 @@ SELECT 1 FROM t1 AS t1_0 JOIN t1 ON t1_0
 SELECT * FROM vtmp;
 DROP VIEW vtmp;
 DROP TABLE t1;
+
+--echo #
+--echo # Bug#13580775 ASSERTION FAILED: RECORD_LENGTH == M_RECORD_LENGTH,
+--echo # FILE FILESORT_UTILS.CC
+--echo #
+
+CREATE TABLE t1 (
+  a INT PRIMARY KEY,
+  b INT,
+  c CHAR(1),
+  d INT,
+  KEY (c,d)
+) PARTITION BY KEY () PARTITIONS 1;
+
+INSERT INTO t1 VALUES (1,1,'a',1), (2,2,'a',1);
+
+SELECT 1 FROM t1 WHERE 1 IN
+(SELECT  group_concat(b)
+ FROM t1
+ WHERE c > geomfromtext('point(1 1)')
+ GROUP BY b
+);
+
+DROP TABLE t1;

=== modified file 'sql/filesort.cc'
--- a/sql/filesort.cc	2012-01-12 14:53:51 +0000
+++ b/sql/filesort.cc	2012-01-18 13:03:10 +0000
@@ -38,6 +38,7 @@
 #include "opt_trace.h"
 
 #include <algorithm>
+#include <utility>
 using std::max;
 using std::min;
 
@@ -284,6 +285,19 @@ ha_rows filesort(THD *thd, TABLE *table,
     {
       ha_rows keys= memory_available / (param.rec_length + sizeof(char*));
       param.max_keys_per_buffer= (uint) min(num_rows, keys);
+      if (table_sort.get_sort_keys())
+      {
+        // If we have already allocated a buffer, it better have same size!
+        if (std::make_pair(param.max_keys_per_buffer, param.rec_length) !=
+            table_sort.sort_buffer_properties())
+        {
+          /*
+           table->sort will still have a pointer to the same buffer,
+           but that will be overwritten by the assignment below.
+          */
+          table_sort.free_sort_buffer();
+        }
+      }
       table_sort.alloc_sort_buffer(param.max_keys_per_buffer, param.rec_length);
       if (table_sort.get_sort_keys())
         break;
@@ -444,7 +458,10 @@ ha_rows filesort(THD *thd, TABLE *table,
 #ifdef SKIP_DBUG_IN_FILESORT
   DBUG_POP();			/* Ok to DBUG */
 #endif
+
+  // Assign the copy back!
   table->sort= table_sort;
+
   DBUG_PRINT("exit",
              ("num_rows: %ld examined_rows: %ld found_rows: %ld",
               (long) num_rows, (long) *examined_rows, (long) *found_rows));

=== modified file 'sql/filesort_utils.h'
--- a/sql/filesort_utils.h	2011-11-07 15:32:36 +0000
+++ b/sql/filesort_utils.h	2012-01-18 13:03:10 +0000
@@ -20,6 +20,8 @@
 #include "my_base.h"
 #include "sql_array.h"
 
+#include <utility>
+
 /*
   Calculate cost of merge sort
 
@@ -86,6 +88,10 @@ public:
   /// Allocates the buffer, but does *not* initialize pointers.
   uchar **alloc_sort_buffer(uint num_records, uint record_length);
 
+  /// What is the <num_records, record_length> for the buffer?
+  std::pair<uint, uint> sort_buffer_properties()
+  { return std::make_pair(m_idx_array.size(), m_record_length); }
+
   /// Frees the buffer.
   void free_sort_buffer();
 

=== modified file 'sql/table.h'
--- a/sql/table.h	2012-01-13 09:33:13 +0000
+++ b/sql/table.h	2012-01-18 13:03:10 +0000
@@ -337,6 +337,9 @@ public:
   uchar **alloc_sort_buffer(uint num_records, uint record_length)
   { return filesort_buffer.alloc_sort_buffer(num_records, record_length); }
 
+  std::pair<uint, uint> sort_buffer_properties()
+  { return filesort_buffer.sort_buffer_properties(); }
+
   void free_sort_buffer()
   { filesort_buffer.free_sort_buffer(); }
 

=== modified file 'unittest/gunit/filesort_buffer-t.cc'
--- a/unittest/gunit/filesort_buffer-t.cc	2011-12-20 09:51:05 +0000
+++ b/unittest/gunit/filesort_buffer-t.cc	2012-01-18 13:03:10 +0000
@@ -16,11 +16,11 @@
 // First include (the generated) my_config.h, to get correct platform defines.
 #include "my_config.h"
 #include <gtest/gtest.h>
+#include <utility>
 
 #include "filesort_utils.h"
 #include "table.h"
 
-
 namespace {
 
 class FileSortBufferTest : public ::testing::Test
@@ -38,7 +38,15 @@ protected:
 TEST_F(FileSortBufferTest, FileSortBuffer)
 {
   const char letters[10]= "abcdefghi";
+  std::pair<uint, uint> buffer_properties= fs_info.sort_buffer_properties();
+  EXPECT_EQ(0U, buffer_properties.first);
+  EXPECT_EQ(0U, buffer_properties.second);
+
   uchar **sort_keys= fs_info.alloc_sort_buffer(10, sizeof(char));
+  buffer_properties= fs_info.sort_buffer_properties();
+  EXPECT_EQ(10U, buffer_properties.first);
+  EXPECT_EQ(sizeof(char), buffer_properties.second);
+
   uchar **null_sort_keys= NULL;
   EXPECT_NE(null_sort_keys, sort_keys);
   for (uint ix= 0; ix < 10; ++ix)

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk branch (tor.didriksen:3745 to 3746) Bug#13580775Tor Didriksen20 Jan