maria-developers team mailing list archive

[Hakan Kuecuekyilmaz <hakan@xxxxxxxxxxxx>]

Hello Sanja,

On 15.09.2009, at 13:21, Oleksandr Byelkin wrote:

> Hi!
>
> This patch is big progress over previous one. Thank you! But still  
> there are things which should be fixed. See my comments in the code.
>
> What I have not found in the patch is correct description of the  
> option (in --help text) as we discussed in on IRC.

I lost that part, I will update the --help text now.

> 15 сент. 2009, в 08:03, Hakan Kuecuekyilmaz написал(а):
>
> [skip]
> > === modified file 'client/mysqlslap.c'
> > --- a/client/mysqlslap.c	2009-04-25 10:05:32 +0000
> > +++ b/client/mysqlslap.c	2009-09-15 05:00:24 +0000
> > @@ -423,6 +423,10 @@
> >  stats *sptr;
> >  conclusions conclusion;
> >  unsigned long long client_limit;
> > +  DYNAMIC_STRING table_string;
> > +  statement *ddl_with_engine;
> > +
> > +  init_dynamic_string(&table_string, "", 1024, 1024);
>
> In general any non-literal constant like above (1024) is bad idea.
>
> But you do not need this code at all. Engine should be added in:
> if (engine_stmt && engine_stmt->option && ptr->type ==  
> CREATE_TABLE_TYPE)
> ...
>
> So the question is why it is not work or why you need this code at  
> all?

It is needed for the --create --engine case, when the user supplies a
DDL with --create and the engines he wants to test with --engine.  I
moved down the init_dynamic_string() to the if () where it is actually
used

> >  head_sptr= (stats *)my_malloc(sizeof(stats) * iterations,
> >                                MYF(MY_ZEROFILL|MY_FAE|MY_WME));
> > @@ -448,8 +452,39 @@
> >
> >    /* First we create */
> >    if (create_statements)
> > -      create_schema(mysql, create_schema_string,  
> > create_statements, eptr);
> > -
> > +    {
> > +      /*
> > +        In case of user supplied DDL and engine, we add the engine
> > +        type to the DDL here.
> > +       */
> > +      if (create_string && eptr)
> > +      {
> > +        dynstr_append(&table_string, create_statements->string);
> > +        dynstr_append(&table_string, " Engine = ");
> > +        dynstr_append(&table_string, eptr->string);
> > +
> > +        if (eptr->option)
> > +        {
> > +          dynstr_append(&table_string, " ");
> > +          dynstr_append(&table_string, eptr->option);
> > +        }
> > +
> > +        ddl_with_engine= (statement *) my_malloc(sizeof(statement),
> > +                                                 MYF(MY_ZEROFILL| 
> > MY_FAE|MY_WME));
> > +        ddl_with_engine->string= (char *)  
> > my_malloc(table_string.length + 1,
> > +                                                     
> > MYF(MY_ZEROFILL|MY_FAE|MY_WME));
> > +        ddl_with_engine->length= table_string.length + 1;
> > +        ddl_with_engine->type= create_statements->type;
> > +        strmov(ddl_with_engine->string, table_string.str);
> > +        ddl_with_engine->next= create_statements->next;
> > +
> > +        dynstr_free(&table_string);
> > +
> > +        create_schema(mysql, create_schema_string,  
> > ddl_with_engine, eptr);
> > +      }
> > +      else
> > +        create_schema(mysql, create_schema_string,  
> > create_statements, eptr);
> > +    }
> >    /*
> >      If we generated GUID we need to build a list of them from  
> > creation that
> >      we can later use.
> > @@ -773,7 +808,7 @@
> > static statement *
> > build_table_string(void)
> > {
> > -  char       buf[HUGE_STRING_LENGTH];
> > +  char       buf[HUGE_STRING_LENGTH]= "";
>
> Why you need this initialization everywhere if you overwrite it late  
> in snprintf & Co ?

I don't really need them. They are removed now.

[cut]

> > -  if (engine_stmt)
> > -  {
> > -    len= snprintf(query, HUGE_STRING_LENGTH, "set storage_engine=` 
> > %s`",
> > -                  engine_stmt->string);
> > -    if (run_query(mysql, query, len))
> > -    {
> > -      fprintf(stderr,"%s: Cannot set default engine: %s\n",  
> > my_progname,
> > -              mysql_error(mysql));
> > -      exit(1);
> > -    }
> > -  }
> > -
>
> it would better to rollback previous patch or merge this one with  
> previous (we do not need incorrect patches pile up one over another,  
> I mean clear history)

I am creating a new patch after each review. Is that what you mean and  
want me
to do?

[cut]

Thanks,

Hakan

--
Hakan Küçükyılmaz, QA/Benchmark Engineer, Stuttgart/Germany
Monty Program Ab, http://askmonty.org/
Skype: hakank_  Phone: +49 171 1919839