[Maria-developers] Rev 2731: Fix for in file:///Users/hakan/work/monty_program/maria-debug/
At file:///Users/hakan/work/monty_program/maria-debug/ ------------------------------------------------------------ revno: 2731 revision-id: hakan@askmonty.org-20090915050024-ltsa4r1n23kr7qm5 parent: knielsen@knielsen-hq.org-20090907131358-6bq1e3qfcgi30hu8 committer: Hakan Kuecuekyilmaz <hakan@askmonty.org> branch nick: maria-debug timestamp: Tue 2009-09-15 07:00:24 +0200 message: Fix for mysqlslap: setting --engine does not get replicated http://bugs.mysql.com/bug.php?id=46967 and mysqlslap: specifying --engine and --create does not work with --engine=<storage_engine>:<option> https://bugs.launchpad.net/maria/+bug/429773 Problems were that an --engine=<storage_engine> was translated to a "set storage_engine = <storage_engine>", wich is _not_ replicated. A --engine=<storage_engine>:<option> was not always properly parsed and in some cases crashed. Fixed by eliminating "set storage_engine = ..." and adding proper DDL generation. Initialized an unitialized buffer to prevent crashes and fixed to use proper pointer for in case of --engine=<storage_engine>:<option> being the last element in list of --engines. Also cleaned up code for better readability. Q: Should MySQL's replication actually replicate a "set storage_engine = ..." command or not? A: No it should not. It is documented here: http://dev.mysql.com/doc/refman/5.1/en/replication-features-variables.html ... "The storage_engine system variable is not replicated, which is a good thing for replication between different storage engines." ... Before the patch, mysqlslap was behaving this way: +-------------------------------+--------+-------------+ | | single | replication | +-------------------------------+--------+-------------+ | Before patch | +-------------------------------+--------+-------------+ | --engine[1] | +-----+-------------------------+--------+-------------+ | 1.1 | eng1 | OK | Not OK | | 1.2 | eng1,eng2 | OK | Not OK | | 1.3 | eng1,eng2,eng3 | OK | Not OK | | 1.4 | memory:option | OK | Not OK | | 1.5 | memory:option,eng1 | OK | Not OK | | 1.6 | eng1,memory:option | Not OK | Not OK | | 1.7 | memory:option,eng1,eng2 | Crash | Not OK | | 1.8 | eng1,memory:option,eng2 | OK | Not OK | | 1.9 | eng1,eng2,memory:option | Not OK | Not OK | +-----+-------------------------+--------+-------------+ +-------------------------------+--------+-------------+ | --create --engine[2] | +-----+-------------------------+--------+-------------+ | 2.1 | eng1 | OK | Not OK | | 2.2 | eng1,eng2 | OK | Not OK | | 2.3 | eng1,eng2,eng3 | OK | Not OK | | 2.4 | memory:option | Not OK | Not OK | | 2.5 | memory:option,eng1 | Not OK | Not OK | | 2.6 | eng1,memory:option | Not OK | Not OK | | 2.7 | memory:option,eng1,eng2 | Crash | Not OK | | 2.8 | eng1,memory:option,eng2 | Not OK | Not OK | | 2.9 | eng1,eng2,memory:option | Not OK | Not OK | +-----+-------------------------+--------+-------------+ After my final patch, mysqlslap now runs like this: +-------------------------------+--------+-------------+ | | single | replication | +-------------------------------+--------+-------------+ | After third patch | +-------------------------------+--------+-------------+ | --engine[1] | +-----+-------------------------+--------+-------------+ | 1.1 | eng1 | OK | OK | | 1.2 | eng1,eng2 | OK | OK | | 1.3 | eng1,eng2,eng3 | OK | OK | | 1.4 | memory:option | OK | OK | | 1.5 | memory:option,eng1 | OK | OK | | 1.6 | eng1,memory:option | OK | OK | | 1.7 | memory:option,eng1,eng2 | OK | OK | | 1.8 | eng1,memory:option,eng2 | OK | OK | | 1.9 | eng1,eng2,memory:option | OK | OK | +-----+-------------------------+--------+-------------+ +-------------------------------+--------+-------------+ | --create --engine[2] | +-----+-------------------------+--------+-------------+ | 2.1 | eng1 | OK | OK | | 2.2 | eng1,eng2 | OK | OK | | 2.3 | eng1,eng2,eng3 | OK | OK | | 2.4 | memory:option | OK | OK | | 2.5 | memory:option,eng1 | OK | OK | | 2.6 | eng1,memory:option | OK | OK | | 2.7 | memory:option,eng1,eng2 | OK | OK | | 2.8 | eng1,memory:option,eng2 | OK | OK | | 2.9 | eng1,eng2,memory:option | OK | OK | +-----+-------------------------+--------+-------------+ === 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); 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]= ""; unsigned int col_count; statement *ptr; DYNAMIC_STRING table_string; @@ -900,7 +935,7 @@ static statement * build_update_string(void) { - char buf[HUGE_STRING_LENGTH]; + char buf[HUGE_STRING_LENGTH]= ""; unsigned int col_count; statement *ptr; DYNAMIC_STRING update_string; @@ -958,6 +993,7 @@ ptr->type= UPDATE_TYPE_REQUIRES_PREFIX ; else ptr->type= UPDATE_TYPE; + strmov(ptr->string, update_string.str); dynstr_free(&update_string); DBUG_RETURN(ptr); @@ -973,8 +1009,8 @@ static statement * build_insert_string(void) { - char buf[HUGE_STRING_LENGTH]; - unsigned int col_count; + char buf[HUGE_STRING_LENGTH]= ""; + unsigned int col_count; statement *ptr; DYNAMIC_STRING insert_string; DBUG_ENTER("build_insert_string"); @@ -1064,8 +1100,8 @@ static statement * build_select_string(my_bool key) { - char buf[HUGE_STRING_LENGTH]; - unsigned int col_count; + char buf[HUGE_STRING_LENGTH]= ""; + unsigned int col_count; statement *ptr; static DYNAMIC_STRING query_string; DBUG_ENTER("build_select_string"); @@ -1117,6 +1153,7 @@ ptr->type= SELECT_TYPE_REQUIRES_PREFIX; else ptr->type= SELECT_TYPE; + strmov(ptr->string, query_string.str); dynstr_free(&query_string); DBUG_RETURN(ptr); @@ -1175,8 +1212,6 @@ exit(1); } - - if (auto_generate_sql && num_of_query && auto_actual_queries) { fprintf(stderr, @@ -1217,6 +1252,7 @@ num_int_cols= atoi(str->string); if (str->option) num_int_cols_index= atoi(str->option); + option_cleanup(str); } @@ -1229,6 +1265,7 @@ num_char_cols_index= atoi(str->option); else num_char_cols_index= 0; + option_cleanup(str); } @@ -1463,6 +1500,7 @@ if (tty_password) opt_password= get_tty_password(NullS); + DBUG_RETURN(0); } @@ -1477,6 +1515,7 @@ if (verbose >= 3) printf("%.*s;\n", len, query); + return mysql_real_query(mysql, query, len); } @@ -1556,7 +1595,7 @@ create_schema(MYSQL *mysql, const char *db, statement *stmt, option_string *engine_stmt) { - char query[HUGE_STRING_LENGTH]; + char query[HUGE_STRING_LENGTH]= ""; statement *ptr; statement *after_create; int len; @@ -1592,18 +1631,6 @@ } } - 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); - } - } - count= 0; after_create= stmt; @@ -1615,10 +1642,23 @@ if (engine_stmt && engine_stmt->option && ptr->type == CREATE_TABLE_TYPE) { - char buffer[HUGE_STRING_LENGTH]; - - snprintf(buffer, HUGE_STRING_LENGTH, "%s %s", ptr->string, - engine_stmt->option); + char buffer[HUGE_STRING_LENGTH]= ""; + + snprintf(buffer, HUGE_STRING_LENGTH, "%s Engine = %s %s", + ptr->string, engine_stmt->string, engine_stmt->option); + if (run_query(mysql, buffer, strlen(buffer))) + { + fprintf(stderr,"%s: Cannot run query %.*s ERROR : %s\n", + my_progname, (uint)ptr->length, ptr->string, mysql_error(mysql)); + exit(1); + } + } + else if (engine_stmt && engine_stmt->string && ptr->type == CREATE_TABLE_TYPE) + { + char buffer[HUGE_STRING_LENGTH]= ""; + + snprintf(buffer, HUGE_STRING_LENGTH, "%s Engine = %s", + ptr->string, engine_stmt->string); if (run_query(mysql, buffer, strlen(buffer))) { fprintf(stderr,"%s: Cannot run query %.*s ERROR : %s\n", @@ -1650,8 +1690,9 @@ static int drop_schema(MYSQL *mysql, const char *db) { - char query[HUGE_STRING_LENGTH]; + char query[HUGE_STRING_LENGTH]= ""; int len; + DBUG_ENTER("drop_schema"); len= snprintf(query, HUGE_STRING_LENGTH, "DROP SCHEMA IF EXISTS `%s`", db); @@ -1842,7 +1883,7 @@ int length; unsigned int key_val; char *key; - char buffer[HUGE_STRING_LENGTH]; + char buffer[HUGE_STRING_LENGTH]= ""; /* This should only happen if some sort of new engine was @@ -1940,17 +1981,22 @@ uint count= 0; /* We know that there is always one */ for (tmp= *sptr= (option_string *)my_malloc(sizeof(option_string), - MYF(MY_ZEROFILL|MY_FAE|MY_WME)); + MYF(MY_ZEROFILL|MY_FAE|MY_WME)); (retstr= strchr(ptr, delm)); tmp->next= (option_string *)my_malloc(sizeof(option_string), - MYF(MY_ZEROFILL|MY_FAE|MY_WME)), + MYF(MY_ZEROFILL|MY_FAE|MY_WME)), tmp= tmp->next) { - char buffer[HUGE_STRING_LENGTH]; + char buffer[HUGE_STRING_LENGTH]= ""; char *buffer_ptr; count++; strncpy(buffer, ptr, (size_t)(retstr - ptr)); + /* + Handle --engine=memory:max_row=200 cases, or more general speaking + --engine=<storage_engine>:<options>. --engine=<storage_engine:option + will be translated to Engine = storage_engine option + */ if ((buffer_ptr= strchr(buffer, ':'))) { char *option_ptr; @@ -1971,13 +2017,15 @@ tmp->length= (size_t)(retstr - ptr); } + /* Skip delimiter delm */ ptr+= retstr - ptr + 1; if (isspace(*ptr)) ptr++; + count++; } - if (ptr != origin+length) + if (ptr != origin + length) { char *origin_ptr; @@ -1986,7 +2034,8 @@ char *option_ptr; tmp->length= (size_t)(origin_ptr - ptr); - tmp->string= my_strndup(origin, tmp->length, MYF(MY_FAE)); + // tmp->string= my_strndup(origin, tmp->length, MYF(MY_FAE)); + tmp->string= my_strndup(ptr, tmp->length, MYF(MY_FAE)); option_ptr= (char *)ptr + 1 + tmp->length; @@ -2036,7 +2085,7 @@ if (ptr != script+length) { tmp->string= my_strndup(ptr, (uint)((script + length) - ptr), - MYF(MY_FAE)); + MYF(MY_FAE)); tmp->length= (size_t)((script + length) - ptr); count++; } @@ -2092,8 +2141,9 @@ void print_conclusions_csv(conclusions *con) { - char buffer[HUGE_STRING_LENGTH]; + char buffer[HUGE_STRING_LENGTH]= ""; const char *ptr= auto_generate_sql_type ? auto_generate_sql_type : "query"; + snprintf(buffer, HUGE_STRING_LENGTH, "%s,%s,%ld.%03ld,%ld.%03ld,%ld.%03ld,%d,%llu\n", con->engine ? con->engine : "", /* Storage engine we ran against */
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.
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?
>
> 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 ?
> unsigned int col_count;
> statement *ptr;
> DYNAMIC_STRING table_string;
> @@ -900,7 +935,7 @@
> static statement *
> build_update_string(void)
> {
> - char buf[HUGE_STRING_LENGTH];
> + char buf[HUGE_STRING_LENGTH]= "";
> unsigned int col_count;
> statement *ptr;
> DYNAMIC_STRING update_string;
> @@ -958,6 +993,7 @@
> ptr->type= UPDATE_TYPE_REQUIRES_PREFIX ;
> else
> ptr->type= UPDATE_TYPE;
> +
> strmov(ptr->string, update_string.str);
> dynstr_free(&update_string);
> DBUG_RETURN(ptr);
> @@ -973,8 +1009,8 @@
> static statement *
> build_insert_string(void)
> {
> - char buf[HUGE_STRING_LENGTH];
> - unsigned int col_count;
> + char buf[HUGE_STRING_LENGTH]= "";
> + unsigned int col_count;
> statement *ptr;
> DYNAMIC_STRING insert_string;
> DBUG_ENTER("build_insert_string");
> @@ -1064,8 +1100,8 @@
> static statement *
> build_select_string(my_bool key)
> {
> - char buf[HUGE_STRING_LENGTH];
> - unsigned int col_count;
> + char buf[HUGE_STRING_LENGTH]= "";
> + unsigned int col_count;
> statement *ptr;
> static DYNAMIC_STRING query_string;
> DBUG_ENTER("build_select_string");
> @@ -1117,6 +1153,7 @@
> ptr->type= SELECT_TYPE_REQUIRES_PREFIX;
> else
> ptr->type= SELECT_TYPE;
> +
> strmov(ptr->string, query_string.str);
> dynstr_free(&query_string);
> DBUG_RETURN(ptr);
> @@ -1175,8 +1212,6 @@
> exit(1);
> }
>
> -
> -
> if (auto_generate_sql && num_of_query && auto_actual_queries)
> {
> fprintf(stderr,
> @@ -1217,6 +1252,7 @@
> num_int_cols= atoi(str->string);
> if (str->option)
> num_int_cols_index= atoi(str->option);
> +
> option_cleanup(str);
> }
>
> @@ -1229,6 +1265,7 @@
> num_char_cols_index= atoi(str->option);
> else
> num_char_cols_index= 0;
> +
> option_cleanup(str);
> }
>
> @@ -1463,6 +1500,7 @@
>
> if (tty_password)
> opt_password= get_tty_password(NullS);
> +
> DBUG_RETURN(0);
> }
>
> @@ -1477,6 +1515,7 @@
>
> if (verbose >= 3)
> printf("%.*s;\n", len, query);
> +
> return mysql_real_query(mysql, query, len);
> }
>
> @@ -1556,7 +1595,7 @@
> create_schema(MYSQL *mysql, const char *db, statement *stmt,
> option_string *engine_stmt)
> {
> - char query[HUGE_STRING_LENGTH];
> + char query[HUGE_STRING_LENGTH]= "";
> statement *ptr;
> statement *after_create;
> int len;
> @@ -1592,18 +1631,6 @@
> }
> }
>
> - 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)
> count= 0;
> after_create= stmt;
>
> @@ -1615,10 +1642,23 @@
>
> if (engine_stmt && engine_stmt->option && ptr->type ==
> CREATE_TABLE_TYPE)
> {
> - char buffer[HUGE_STRING_LENGTH];
> -
> - snprintf(buffer, HUGE_STRING_LENGTH, "%s %s", ptr->string,
> - engine_stmt->option);
> + char buffer[HUGE_STRING_LENGTH]= "";
> +
> + snprintf(buffer, HUGE_STRING_LENGTH, "%s Engine = %s %s",
> + ptr->string, engine_stmt->string, engine_stmt-
> >option);
> + if (run_query(mysql, buffer, strlen(buffer)))
> + {
> + fprintf(stderr,"%s: Cannot run query %.*s ERROR : %s\n",
> + my_progname, (uint)ptr->length, ptr->string,
> mysql_error(mysql));
> + exit(1);
> + }
> + }
> + else if (engine_stmt && engine_stmt->string && ptr->type ==
> CREATE_TABLE_TYPE)
> + {
> + char buffer[HUGE_STRING_LENGTH]= "";
> +
> + snprintf(buffer, HUGE_STRING_LENGTH, "%s Engine = %s",
> + ptr->string, engine_stmt->string);
> if (run_query(mysql, buffer, strlen(buffer)))
> {
> fprintf(stderr,"%s: Cannot run query %.*s ERROR : %s\n",
> @@ -1650,8 +1690,9 @@
> static int
> drop_schema(MYSQL *mysql, const char *db)
> {
> - char query[HUGE_STRING_LENGTH];
> + char query[HUGE_STRING_LENGTH]= "";
> int len;
> +
> DBUG_ENTER("drop_schema");
> len= snprintf(query, HUGE_STRING_LENGTH, "DROP SCHEMA IF EXISTS `
> %s`", db);
>
> @@ -1842,7 +1883,7 @@
> int length;
> unsigned int key_val;
> char *key;
> - char buffer[HUGE_STRING_LENGTH];
> + char buffer[HUGE_STRING_LENGTH]= "";
>
> /*
> This should only happen if some sort of new engine was
> @@ -1940,17 +1981,22 @@
> uint count= 0; /* We know that there is always one */
>
> for (tmp= *sptr= (option_string *)my_malloc(sizeof(option_string),
> - MYF(MY_ZEROFILL|MY_FAE|
> MY_WME));
> + MYF(MY_ZEROFILL|
> MY_FAE|MY_WME));
> (retstr= strchr(ptr, delm));
> tmp->next= (option_string *)my_malloc(sizeof(option_string),
> - MYF(MY_ZEROFILL|MY_FAE|
> MY_WME)),
> + MYF(MY_ZEROFILL|
> MY_FAE|MY_WME)),
> tmp= tmp->next)
> {
> - char buffer[HUGE_STRING_LENGTH];
> + char buffer[HUGE_STRING_LENGTH]= "";
> char *buffer_ptr;
>
> count++;
> strncpy(buffer, ptr, (size_t)(retstr - ptr));
> + /*
> + Handle --engine=memory:max_row=200 cases, or more general
> speaking
> + --engine=<storage_engine>:<options>. --
> engine=<storage_engine:option
> + will be translated to Engine = storage_engine option
> + */
> if ((buffer_ptr= strchr(buffer, ':')))
> {
> char *option_ptr;
> @@ -1971,13 +2017,15 @@
> tmp->length= (size_t)(retstr - ptr);
> }
>
> + /* Skip delimiter delm */
> ptr+= retstr - ptr + 1;
> if (isspace(*ptr))
> ptr++;
> +
> count++;
> }
>
> - if (ptr != origin+length)
> + if (ptr != origin + length)
> {
> char *origin_ptr;
>
> @@ -1986,7 +2034,8 @@
> char *option_ptr;
>
> tmp->length= (size_t)(origin_ptr - ptr);
> - tmp->string= my_strndup(origin, tmp->length, MYF(MY_FAE));
> + // tmp->string= my_strndup(origin, tmp->length, MYF(MY_FAE));
> + tmp->string= my_strndup(ptr, tmp->length, MYF(MY_FAE));
>
> option_ptr= (char *)ptr + 1 + tmp->length;
>
> @@ -2036,7 +2085,7 @@
> if (ptr != script+length)
> {
> tmp->string= my_strndup(ptr, (uint)((script + length) - ptr),
> - MYF(MY_FAE));
> + MYF(MY_FAE));
> tmp->length= (size_t)((script + length) - ptr);
> count++;
> }
> @@ -2092,8 +2141,9 @@
> void
> print_conclusions_csv(conclusions *con)
> {
> - char buffer[HUGE_STRING_LENGTH];
> + char buffer[HUGE_STRING_LENGTH]= "";
> const char *ptr= auto_generate_sql_type ? auto_generate_sql_type :
> "query";
> +
> snprintf(buffer, HUGE_STRING_LENGTH,
> "%s,%s,%ld.%03ld,%ld.%03ld,%ld.%03ld,%d,%llu\n",
> con->engine ? con->engine : "", /* Storage engine we ran
> against */
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
participants (2)
-
Hakan Kuecuekyilmaz -
Oleksandr Byelkin