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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
