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