maria-developers team mailing list archive

[Sergey Petrunia <sergey@xxxxxxxxxxx>]

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