Hi Varun, I have only cosmetic comments, please find them below. Ok to push after they are addressed.
The issue here was with the re-execution of a subquey, after few executions the subquery execution returned error that not enough space was provided inside the sort buffer. This is happening because with the introduction of packed sort keys, during the phase where sort length is calculated the parameters of the sort keys are not reset and we keep up adding the sort length to the previous calculated sort length for the earlier executions of the subquery. This happens because the sort keys structure is allocated only once and so if their is re-execution of a subquery involving sorting then we should be resetting the parameters of the sort keys.
The comment is hard to read. How about being more specific and using shorter sentences: The error was: filesort was done in a subquery which was executed multiple times. During each execution, sortlength() computed total sort key length in Sort_keys::sort_length, without resetting it first. Eventually Sort_keys::sort_length got larger than @@sort_buffer_size, which caused filesort() to be aborted with error. Fixed by making sortlength() to compute lengths only during the first invocation. Subsequent invocations return pre-computed values.
@@ -2518,7 +2526,7 @@ void Sort_param::try_to_pack_sortkeys() return;
const uint sz= Sort_keys::size_of_length_field; - uint sort_len= sort_keys->get_sort_length(); + uint sort_len= sort_keys->get_sort_length_with_original_values();
/* Heuristic introduced, skip packing sort keys if saving less than 128 bytes
diff --git a/sql/sql_sort.h b/sql/sql_sort.h index 40f0c5ede5f..6951d4d190a 100644 --- a/sql/sql_sort.h +++ b/sql/sql_sort.h @@ -258,7 +258,9 @@ class Sort_keys :public Sql_alloc, Sort_keys_array(arr, count), m_using_packed_sortkeys(false), size_of_packable_fields(0), - sort_length(0) + sort_length_with_original_values(0), + sort_length_with_memcmp_values(0), + first_execution(true)
This violates the coding style. Please ident the member initialization (see e.g. item.h for examples)
{ DBUG_ASSERT(!is_null()); }a ... @@ -307,9 +319,12 @@ class Sort_keys :public Sql_alloc,
void increment_original_sort_length(uint len) { - sort_length+= len; + sort_length_with_original_values+= len; }
+ bool is_first_execution() { return first_execution; } + void set_first_execution(bool val) { first_execution= val; }
A comment just about the naming: I would not drag "execution" into here. "Sort_keys" are not really executed. How about changing the functions to be bool is_parameters_computed(); bool set_parameters_computed(); and the variable to be "bool parameters_computed" ?
+ static const uint size_of_length_field= 4;
private:
BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog