List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:January 4 2012 9:52am
Subject:bzr push into mysql-trunk branch (jorgen.loland:3505 to 3506) Bug#13350136
View as plain text  
 3506 Jorgen Loland	2012-01-04
      BUG#13350136 - TEMPORARY MYISAM TABLES DO NOT PACK LONG 
                     VARCHAR COLUMNS
      
      MyISAM has three record formats (fixed, packed and compressed) 
      out of which fixed and packed are applicable for temporary 
      tables. The difference is that with fixed length record format,
      VARCHAR trailing bytes are written whereas for packed record 
      format the trailing bytes are not written. The packed record 
      therefore saves space at the cost of some additional memcopies.
      
      The packed record format is currently not in use by temporary
      tables, although old commit messages suggests that this has not
      always been the case, e.g:
      
         revision: sp1r-monty@stripped
         Date: 2004-12-06
         "Internal temporary files can now use fixed length tables 
          if the used VARCHAR columns are short"
      
      This CS enables packed record format for temporary tables.
      
      The CS also includes a fix for a MyISAM bug that is only a
      problem when the packed format and a special unique constraint
      (that checksums wide columns) is used. This unique constraint
      only applies to internal temporary tables when uniqueness is
      required on columns with a total size bigger than normal 
      indexes allow (1000 bytes). The problem:
      
      A column's value in MyISAM can be read from or written to the 
      row buffer using MI_COLUMNDEF::offset ("normal" column 
      definition) or HA_KEYSEG::start (key definition). The former 
      is calculated in mi_open() and is the sum of the length of all
      preceding columns. The latter is calculated in mi_create(). 
      The bug was that in mi_create(), HA_KEYSEG::start was set to 
      the offset of the column for the on-disk format (reclength) 
      whereas it should have been the offset for the in-memory format.
      
      Due to this bug, unique constraint values were written to one
      offset (HA_KEYSEG::start in mi_check_unique()) and read from
      another offset (MI_COLUMNDEF::offset in _mi_pack_record et al),
      resulting in duplicate rows in the result set because equally
      valued rows got seemingly different checksums.
     @ mysql-test/suite/opt_trace/r/temp_table.result
        Trace changes because packed record format is used for tmp table
     @ sql/sql_tmp_table.cc
        Enable packed record format for MyISAM temporary tables
     @ storage/myisam/mi_create.c
        HA_KEYSEG::start is supposed to point to the same offset as
        MI_COLUMNDEF::offset, which is the offset of a column when in
        memory. Instead it pointed to the offset for the on-disk 
        format. Set in mi_create()

    modified:
      mysql-test/suite/opt_trace/r/temp_table.result
      sql/sql_tmp_table.cc
      storage/myisam/mi_create.c
 3505 Guilhem Bichot	2012-01-03
      Fix for
      Bug#11766384 - 59487: WRONG RESULT WITH STRAIGHT_JOIN AND RIGHT JOIN
      BUG#11752239 - 43368: STRAIGHT_JOIN DOESN'T WORK FOR NESTED JOINS 
      BUG#11766858 - 60080: STRAIGHT_JOIN IS NOT ENFORCED WHEN RIGHT-HAND TABLE IS ITSELF A JOIN 
      dependencies were not propagated when a nested join was flattened.
     @ mysql-test/r/innodb_icp.result
        This is the testcase for
        BUG 11766858 - 60080: STRAIGHT_JOIN IS NOT ENFORCED WHEN RIGHT-HAND TABLE IS ITSELF A JOIN
        Query has
        EXPLAIN SELECT table1.col_int_nokey
        FROM t1 AS table1 STRAIGHT_JOIN (t1 AS table2 INNER JOIN t1 AS table3 etc)
        and so table1 should be first.
     @ mysql-test/suite/opt_trace/include/general2.inc
        bug is fixed
     @ mysql-test/suite/opt_trace/r/general2_no_prot.result
        Query is:
        SELECT t1.f2 FROM t1 STRAIGHT_JOIN (t2 JOIN t3 etc)
        so t2 and t3 should be marked as dependent from t1 (due to straight_join);
        they weren't. Now we see they are (depends_on_map_bits says "0").
        t3 used to be recognized as constant ("system" table);
        now that it is dependent on other tables, it's not
        (see the test for !s->dependent on which I added a comment
        in sql_select.cc: it makes the table not be recognized
        as constant). So t3 now participates in plan search
        (not necessarily a good thing, except that we can
        say that this is better observing STRAIGHT_JOIN: marking
        a table as constant moves it to the plan's front,
        producing an order which is not the one requested
        by the user...).
        So we explore plan t1-t3-t2 and t1-t2, which all respect
        STRAIGHT_JOIN. We don't anymore explore the plan starting with t2,
        as now-correct t2->dep_tables make it impossible (t2 depends on t1).
        The fact that t3 is not constant is also seen in WHERE
        conditions (no more constant propagation).
     @ mysql-test/t/join.test
        test for BUG#11752239
     @ mysql-test/t/join_outer.test
        test for Bug#11766384
     @ sql/sql_optimizer.cc
        Consider this scenario: t1 STRAIGHT_JOIN (t2 JOIN t3):
        - a nested join may depend on other tables (here (t2 JOIN t3) depends
        on t1, due to STRAIGHT_JOIN), this is recorded in the nested join's
        dep_tables variable
        - when flattening this nested join (eliminating it, eliminating parenthesis),
        the nested join goes away, so its dep_tables is lost:
        the query becomes t1 JOIN t2 JOIN t3. So STRAIGHT_JOIN is not honoured.
        The fix is: tables of the nested join must inherit dependencies of
        the to-be-deleted nested join.
        
        We even got an assert sometimes (testcase in join_outer.test),
        consider SELECT STRAIGHT_JOIN t2 RIGHT JOIN (t1 JOIN t3).
        The parser produces this list of TABLE_LIST objects:
        t2 - NJ(t1 - t3). The JOIN's join_list is reversed so that
        right join appears as left join: NJ(t1 - t3) - t2.
        The initial order of JOIN_TABs is always that of
        TABLE_LISTs: t2 - t1 - t3 (and this is why the assert
        happened only with a RIGHT JOIN, btw see WL 6059
        "Make RIGHT JOIN be fully converted to LEFT JOIN").
        In the testcase of join_outer.test, the initial order of
        JOIN_TABs had t2 before t1,t4,t5. Because dep_tables was wrong
        (too weak) for t2, join_tab_cmp_straight() didn't move t2 to the plan's tail;
        it moved some other tables to the head because they had
        a correct dep_tables; t2 ended in the middle. Then
        check_interleaving_with_nj() correctly recognized that this
        plan cannot be executed, because t2 was between t4 and
        t3 i.e. in the middle of (t3 JOIN t4) - you can't
        have a table in the middle of a nest which doesn't own it,
        this causes problems for NULL-complementing.

    modified:
      mysql-test/r/innodb_icp.result
      mysql-test/r/innodb_icp_all.result
      mysql-test/r/innodb_icp_none.result
      mysql-test/r/join.result
      mysql-test/r/join_outer.result
      mysql-test/r/join_outer_bka.result
      mysql-test/r/join_outer_bka_nixbnl.result
      mysql-test/r/myisam_icp.result
      mysql-test/r/myisam_icp_all.result
      mysql-test/r/myisam_icp_none.result
      mysql-test/suite/opt_trace/include/general2.inc
      mysql-test/suite/opt_trace/r/general2_no_prot.result
      mysql-test/suite/opt_trace/r/general2_ps_prot.result
      mysql-test/t/join.test
      mysql-test/t/join_outer.test
      sql/sql_optimizer.cc
=== modified file 'mysql-test/suite/opt_trace/r/temp_table.result'
--- a/mysql-test/suite/opt_trace/r/temp_table.result	2011-11-21 08:51:57 +0000
+++ b/mysql-test/suite/opt_trace/r/temp_table.result	2012-01-04 09:51:39 +0000
@@ -133,7 +133,7 @@ SELECT uniq, col1 FROM t1 GROUP BY col2,
                 "key_length": 1040,
                 "unique_constraint": true,
                 "location": "disk (MyISAM)",
-                "record_format": "fixed"
+                "record_format": "packed"
               } /* tmp_table_info */
             } /* creating_tmp_table */
           },
@@ -153,7 +153,7 @@ SELECT uniq, col1 FROM t1 GROUP BY col2,
             "filesort_priority_queue_optimization": {
               "limit": 3,
               "rows_estimate": 18,
-              "row_size": 1043,
+              "row_size": 1042,
               "memory_available": 262144,
               "chosen": true
             } /* filesort_priority_queue_optimization */,
@@ -163,7 +163,7 @@ SELECT uniq, col1 FROM t1 GROUP BY col2,
               "rows": 4,
               "examined_rows": 8,
               "number_of_tmp_files": 0,
-              "sort_buffer_size": 4204,
+              "sort_buffer_size": 4200,
               "sort_mode": "<sort_key, rowid>"
             } /* filesort_summary */
           }
@@ -484,7 +484,7 @@ SELECT uniq, col1, col2 FROM t1 GROUP BY
                 "key_length": 13,
                 "unique_constraint": false,
                 "location": "disk (MyISAM)",
-                "record_format": "fixed"
+                "record_format": "packed"
               } /* tmp_table_info */
             } /* converting_tmp_table_to_myisam */
           },
@@ -506,7 +506,7 @@ SELECT uniq, col1, col2 FROM t1 GROUP BY
               "rows": 16,
               "examined_rows": 16,
               "number_of_tmp_files": 0,
-              "sort_buffer_size": 676,
+              "sort_buffer_size": 650,
               "sort_mode": "<sort_key, rowid>"
             } /* filesort_summary */
           }

=== modified file 'sql/sql_tmp_table.cc'
--- a/sql/sql_tmp_table.cc	2011-12-15 15:15:37 +0000
+++ b/sql/sql_tmp_table.cc	2012-01-04 09:51:39 +0000
@@ -464,8 +464,8 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
   uint  temp_pool_slot=MY_BIT_NONE;
   uint fieldnr= 0;
   ulong reclength, string_total_length;
-  bool  using_unique_constraint= 0;
-  bool  use_packed_rows= 0;
+  bool  using_unique_constraint= false;
+  bool  use_packed_rows= false;
   bool  not_all_columns= !(select_options & TMP_TABLE_ALL_COLUMNS);
   char  *tmpname,path[FN_REFLEN];
   uchar	*pos, *group_buff, *bitmaps;
@@ -527,10 +527,10 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
       */
       (*tmp->item)->marker= 4;
       if ((*tmp->item)->max_length >= CONVERT_IF_BIGGER_TO_BLOB)
-	using_unique_constraint=1;
+        using_unique_constraint= true;
     }
     if (param->group_length >= MAX_BLOB_WIDTH)
-      using_unique_constraint=1;
+      using_unique_constraint= true;
     if (group)
       distinct=0;				// Can't use distinct
   }
@@ -750,6 +750,14 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
         *blob_field++= fieldnr;
 	blob_count++;
       }
+
+      if (new_field->real_type() == MYSQL_TYPE_STRING ||
+          new_field->real_type() == MYSQL_TYPE_VARCHAR)
+      {
+        string_count++;
+        string_total_length+= new_field->pack_length();
+      }
+
       if (item->marker == 4 && item->maybe_null)
       {
 	group_null_items++;
@@ -800,7 +808,7 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
     if (group &&
 	(param->group_parts > table->file->max_key_parts() ||
 	 param->group_length > table->file->max_key_length()))
-      using_unique_constraint=1;
+      using_unique_constraint= true;
   }
   else
   {
@@ -836,7 +844,7 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
       (string_total_length >= STRING_TOTAL_LENGTH_TO_PACK_ROWS &&
       (reclength / string_total_length <= RATIO_TO_PACK_ROWS ||
        string_total_length / string_count >= AVG_STRING_LENGTH_TO_PACK_ROWS)))
-    use_packed_rows= 1;
+    use_packed_rows= true;
 
   if (!use_packed_rows)
     share->db_create_options&= ~HA_OPTION_PACK_RECORD;
@@ -956,6 +964,10 @@ create_tmp_table(THD *thd,TMP_TABLE_PARA
              field->real_type() == MYSQL_TYPE_STRING &&
 	     length >= MIN_STRING_LENGTH_TO_PACK_ROWS)
       recinfo->type=FIELD_SKIP_ENDSPACE;
+    else if (use_packed_rows && 
+             field->real_type() == MYSQL_TYPE_VARCHAR &&
+             length >= MIN_STRING_LENGTH_TO_PACK_ROWS)
+      recinfo->type= FIELD_VARCHAR;
     else
       recinfo->type=FIELD_NORMAL;
     if (!--hidden_field_count)
@@ -1732,7 +1744,10 @@ bool create_myisam_tmp_table(TABLE *tabl
                        start_recinfo,
                        share->uniques, &uniquedef,
                        &create_info,
-                       HA_CREATE_TMP_TABLE)))
+                       HA_CREATE_TMP_TABLE | 
+                       ((share->db_create_options & HA_OPTION_PACK_RECORD) ?
+                        HA_PACK_RECORD : 0)
+                       )))
   {
     table->file->print_error(error,MYF(0));	/* purecov: inspected */
     table->db_stat=0;

=== modified file 'storage/myisam/mi_create.c'
--- a/storage/myisam/mi_create.c	2011-08-29 12:08:58 +0000
+++ b/storage/myisam/mi_create.c	2012-01-04 09:51:39 +0000
@@ -462,7 +462,6 @@ int mi_create(const char *name,uint keys
     key_del[i]=HA_OFFSET_ERROR;
 
   unique_key_parts=0;
-  offset=reclength-uniques*MI_UNIQUE_HASH_LENGTH;
   for (i=0, uniquedef=uniquedefs ; i < uniques ; i++ , uniquedef++)
   {
     uniquedef->key=keys+i;
@@ -727,7 +726,7 @@ int mi_create(const char *name,uint keys
 #endif
   }
   /* Create extra keys for unique definitions */
-  offset=reclength-uniques*MI_UNIQUE_HASH_LENGTH;
+  offset= real_reclength - uniques * MI_UNIQUE_HASH_LENGTH;
   memset(&tmp_keydef, 0, sizeof(tmp_keydef));
   memset(&tmp_keyseg, 0, sizeof(tmp_keyseg));
   for (i=0; i < uniques ; i++)

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-trunk branch (jorgen.loland:3505 to 3506) Bug#13350136Jorgen Loland9 Jan