maria-developers team mailing list archive

[Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>]

Hi Monty,

While fixing test failures in the MariaDB 5.5 tree, I encountered problems in
sql_string, related to changes you did there in 5.2. I managed to come up with
some solution, but please see below patch and see if you can suggest something
better.

Two issues:


1. You added realloc_with_extra() / realloc_with_extra_if_needed(), and
used them to replace calls to realloc(). But realloc() as part of its
operation puts a terminating '\0' at the end of the allocation; with your
change this is now at a different (and not very useful) position in the
buffer. My change tries to put it in the original place, but it is not
terribly elegant, and perhaps your original change needs to be rethought.


2. I found a terrible mess (IMHO): We have two distict, subtly differing,
sql_string.{h,cc}, one in /sql/ and one in /client/. We even seem to have a
requirement for ABI compatibility between the two, since mysql_embedded uses
client/sql_string.h with sql/sql_string.cc :-( And in fact your change seems
to break such ABI compatibility, so I suspect mysql_embedded might be subtly
broken in 5.2.


But why do we need to have two sql_string in the first place? Especially given
that they are apparently sufficiently close to be ABI compatible ... can't we
eliminate on of them to get at least a little sanity into the sql_string
story?

 - Kristian.
knielsen@xxxxxxxxxxxxxxx writes:

> At http://bazaar.launchpad.net/~maria-captains/maria/5.5
>
> ------------------------------------------------------------
> revno: 3170
> revision-id: knielsen@xxxxxxxxxxxxxxx-20111209144027-yf5as4v9lb6jt3s7
> parent: knielsen@xxxxxxxxxxxxxxx-20111208170848-pijq4i8ceqwcl19q
> committer: knielsen@xxxxxxxxxxxxxxx
> branch nick: mariadb-5.5
> timestamp: Fri 2011-12-09 15:40:27 +0100
> message:
>   After-merge fixes for 5.5 merge.
>   
>   Fix typo causing too low timeout value for wait_for_slave_param.inc.
>   Fix wrong/missing '\0' termination in String:realloc() after Monty's
>   optimisations to reduce reallocation calls.
> === modified file 'mysql-test/include/wait_for_slave_param.inc'
> --- a/mysql-test/include/wait_for_slave_param.inc	2010-12-20 14:15:01 +0000
> +++ b/mysql-test/include/wait_for_slave_param.inc	2011-12-09 14:40:27 +0000
> @@ -79,7 +79,7 @@ if ($_slave_check_configured == 'No such
>  
>  # mysqltest doesn't provide any better way to multiply by 10
>  --let $_wait_for_slave_param_zero= 0
> ---let $_slave_timeout_counter= $_slave_timeout$zero
> +--let $_slave_timeout_counter= $_slave_timeout$_wait_for_slave_param_zero
>  --let $_slave_continue= 1
>  while ($_slave_continue)
>  {
>
> === modified file 'sql/rpl_handler.cc'
> --- a/sql/rpl_handler.cc	2011-11-03 18:17:05 +0000
> +++ b/sql/rpl_handler.cc	2011-12-09 14:40:27 +0000
> @@ -397,7 +397,7 @@ int Binlog_transmit_delegate::before_sen
>  
>    int ret= 0;
>    FOREACH_OBSERVER(ret, before_send_event, thd,
> -                   (&param, (uchar *)packet->c_ptr(),
> +                   (&param, (uchar *)packet->c_ptr_safe(),
>                      packet->length(),
>                      log_file+dirname_length(log_file), log_pos));
>    return ret;
> @@ -411,7 +411,7 @@ int Binlog_transmit_delegate::after_send
>  
>    int ret= 0;
>    FOREACH_OBSERVER(ret, after_send_event, thd,
> -                   (&param, packet->c_ptr(), packet->length()));
> +                   (&param, packet->c_ptr_safe(), packet->length()));
>    return ret;
>  }
>  
>
> === modified file 'sql/sql_string.cc'
> --- a/sql/sql_string.cc	2011-11-03 18:17:05 +0000
> +++ b/sql/sql_string.cc	2011-12-09 14:40:27 +0000
> @@ -80,8 +80,12 @@ bool String::real_alloc(uint32 length)
>     no allocation occured.
>  
>     @retval true An error occured when attempting to allocate memory.
> +
> +   The _internal() version is just to share code between realloc() and
> +   realloc_with_extra(), so that we can put the NULL termination at the right
> +   position.
>  */
> -bool String::realloc(uint32 alloc_length)
> +bool String::realloc_internal(uint32 alloc_length)
>  {
>    if (Alloced_length <= alloc_length)
>    {
> @@ -109,6 +113,13 @@ bool String::realloc(uint32 alloc_length
>      Ptr= new_ptr;
>      Alloced_length= len;
>    }
> +  return FALSE;
> +}
> +
> +bool String::realloc(uint32 alloc_length)
> +{
> +  if (realloc_internal(alloc_length))
> +    return TRUE;
>    Ptr[alloc_length]=0;                  // This make other funcs shorter
>    return FALSE;
>  }
>
> === modified file 'sql/sql_string.h'
> --- a/sql/sql_string.h	2011-11-03 18:17:05 +0000
> +++ b/sql/sql_string.h	2011-12-09 14:40:27 +0000
> @@ -255,12 +255,18 @@ public:
>      return real_alloc(arg_length);
>    }
>    bool real_alloc(uint32 arg_length);                   // Empties old string
> +private:
> +  bool realloc_internal(uint32 arg_length);
> +public:
>    bool realloc(uint32 arg_length);
>    bool realloc_with_extra(uint32 arg_length)
>    {
>      if (extra_alloc < 4096)
>        extra_alloc= extra_alloc*2+128;
> -    return realloc(arg_length + extra_alloc);
> +    if (realloc_internal(arg_length + extra_alloc))
> +      return TRUE;
> +    Ptr[arg_length]= 0;
> +    return FALSE;
>    }
>    bool realloc_with_extra_if_needed(uint32 arg_length)
>    {
>
> _______________________________________________
> commits mailing list
> commits@xxxxxxxxxxx
> https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits