List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:April 27 2011 7:47pm
Subject:Re: bzr commit into mysql-5.5 branch (alexander.nozdrin:3458) Bug#12374486
View as plain text  
Hi Alik,

On 4/25/11 11:12 AM, Alexander Nozdrin wrote:
> #At file:///home/alik/MySQL/bzr/00/bug12374486/mysql-5.5-bug12374486/ based on
> revid:jon.hauglid@stripped
>
>   3458 Alexander Nozdrin	2011-04-25
>        Patch for Bug#12374486 - SEVERE MEMORY LEAK IN PREPARED STATEMENTS
>        THAT CALL STORED PROCEDURES.
>
>        The bug was introduced by WL#4435. The problem was that if a stored
>        procedure generated a few result sets with different set of columns,
>        a new memory would be allocated after every EXECUTE for every
>        result set.
>
>        The fix is to introduce a new memory root in scope of MYSQL_STMT,
>        and to store result-set metadata in that memory root.
>

Looks good, some comments below.

>   {
>
> === modified file 'libmysql/libmysql.c'
> --- a/libmysql/libmysql.c	2011-03-08 17:39:25 +0000
> +++ b/libmysql/libmysql.c	2011-04-25 14:12:29 +0000
> @@ -94,6 +94,18 @@ sig_handler my_pipe_sig_handler(int sig)
>   static my_bool mysql_client_init= 0;
>   static my_bool org_my_init_done= 0;
>
> +typedef struct st_mysql_stmt_extension
> +{
> +  my_bool   fields_mem_root_initialized;

Is this "initialized" field really necessary? It seems to be possible to 
check the MEM_ROOT itself -- see alloc_root_inited().

> +  MEM_ROOT  fields_mem_root;
> +} MYSQL_STMT_EXT;
> +
> +
> +static inline MYSQL_STMT_EXT *get_stmt_exension(MYSQL_STMT *stmt)
> +{
> +  return (MYSQL_STMT_EXT *) stmt->extension;

Cast not needed.

> +}

Actually, I would be nice if you could just drop this function.

>
>   /*
>     Initialize the MySQL client library
> @@ -1478,6 +1490,7 @@ MYSQL_STMT * STDCALL
>   mysql_stmt_init(MYSQL *mysql)
>   {
>     MYSQL_STMT *stmt;
> +  MYSQL_STMT_EXT *stmt_ext;
>     DBUG_ENTER("mysql_stmt_init");
>
>     if (!(stmt= (MYSQL_STMT *) my_malloc(sizeof(MYSQL_STMT),
> @@ -1497,6 +1510,11 @@ mysql_stmt_init(MYSQL *mysql)
>     stmt->read_row_func= stmt_read_row_no_result_set;
>     stmt->prefetch_rows= DEFAULT_PREFETCH_ROWS;
>     strmov(stmt->sqlstate, not_error_sqlstate);
> +
> +  stmt_ext= alloc_root(&stmt->mem_root, sizeof (MYSQL_STMT_EXT));

Check for failure to allocate.

> +  stmt_ext->fields_mem_root_initialized= FALSE;
> +
> +  stmt->extension= stmt_ext;
>     /* The rest of statement members was bzeroed inside malloc */
>
>     DBUG_RETURN(stmt);
> @@ -1541,6 +1559,8 @@ int STDCALL
>   mysql_stmt_prepare(MYSQL_STMT *stmt, const char *query, ulong length)
>   {
>     MYSQL *mysql= stmt->mysql;
> +  MYSQL_STMT_EXT *stmt_ext= get_stmt_exension(stmt);
> +
>     DBUG_ENTER("mysql_stmt_prepare");
>
>     if (!mysql)
> @@ -1571,6 +1591,11 @@ mysql_stmt_prepare(MYSQL_STMT *stmt, con
>       stmt->bind_param_done= stmt->bind_result_done= FALSE;
>       stmt->param_count= stmt->field_count= 0;
>       free_root(&stmt->mem_root, MYF(MY_KEEP_PREALLOC));
> +    if (stmt_ext->fields_mem_root_initialized)
> +    {
> +      free_root(&stmt_ext->fields_mem_root, MYF(MY_KEEP_PREALLOC));
> +      stmt_ext->fields_mem_root_initialized= FALSE;
> +    }
>
>       int4store(buff, stmt->stmt_id);
>
> @@ -1631,12 +1656,17 @@ mysql_stmt_prepare(MYSQL_STMT *stmt, con
>   static void alloc_stmt_fields(MYSQL_STMT *stmt)
>   {
>     MYSQL_FIELD *fields, *field, *end;
> -  MEM_ROOT *alloc=&stmt->mem_root;
> +  MYSQL_STMT_EXT *stmt_ext= get_stmt_exension(stmt);
> +  MEM_ROOT *alloc=&stmt_ext->fields_mem_root;
>     MYSQL *mysql= stmt->mysql;
>
> -  DBUG_ASSERT(mysql->field_count);
> +  DBUG_ASSERT(stmt->field_count);
>
> -  stmt->field_count= mysql->field_count;
> +  if (stmt_ext->fields_mem_root_initialized)
> +    free_root(&stmt_ext->fields_mem_root, MYF(0));
> +
> +  init_alloc_root(&stmt_ext->fields_mem_root, 2048, 2048);
> +  stmt_ext->fields_mem_root_initialized= TRUE;

We could just drop the pre-allocation and initialize the mem_root when 
the prepared statement is initialized. This should make things simpler.

Regards,

Davi
Thread
bzr commit into mysql-5.5 branch (alexander.nozdrin:3458) Bug#12374486Alexander Nozdrin25 Apr
  • Re: bzr commit into mysql-5.5 branch (alexander.nozdrin:3458) Bug#12374486Davi Arnaut27 Apr