Re: [Maria-developers] [Commits] Rev 3067: Added progress reporting for alter table, LOAD DATA INFILE and for aria tables: check table, repair table, analyze table. in lp:maria/5.3
Hi, Michael! On Jun 30, Michael Widenius wrote:
message: Added progress reporting for alter table, LOAD DATA INFILE and for aria tables: check table, repair table, analyze table. - The client gets a progress report message that triggers a callback function if requested with mysql_options(MYSQL_PROGRESS_CALLBACK, function) - Added Progress field last to 'show processlist' - Stage, Max_stage and Progress field added to information_schema.progresslist - The 'mysql' client by defaults enables progress reports when the output is a tty. - Added progress_report_time time variable to configure how often progress reports is sent to client Added read only system variable 'in_transaction' which is 1 if we have executed a BEGIN statement.
=== modified file 'include/mysql/plugin.h' --- a/include/mysql/plugin.h 2010-10-25 13:21:16 +0000 +++ b/include/mysql/plugin.h 2011-06-30 12:23:49 +0000 @@ -783,6 +784,21 @@ int thd_killed(const MYSQL_THD thd);
/** + Report progress for long running operations (for information schema) + + @param thd User thread connection handle + @param progress Where we are now + @param max_progress Progress will continue up to this +*/ + +void thd_progress_init(MYSQL_THD thd, unsigned int max_stage); +void thd_progress_report(MYSQL_THD thd, + unsigned long long progress, + unsigned long long max_progress); +void thd_progress_next_stage(MYSQL_THD thd); +void thd_progress_end(MYSQL_THD thd);
This should be implemented as a service. See libservices/HOWTO
+ +/** Return the thread id of a user thread
@param thd user thread connection handle
=== modified file 'mysql-test/r/create.result' --- a/mysql-test/r/create.result 2011-05-16 11:05:45 +0000 +++ b/mysql-test/r/create.result 2011-06-30 12:23:49 +0000 @@ -1760,7 +1760,10 @@ t1 CREATE TABLE `t1` ( `TIME` int(7) NOT NULL DEFAULT '0', `STATE` varchar(64) DEFAULT NULL, `INFO` longtext, - `TIME_MS` decimal(22,3) NOT NULL DEFAULT '0.000' + `TIME_MS` decimal(22,3) NOT NULL DEFAULT '0.000', + `STAGE` tinyint(2) NOT NULL DEFAULT '0', + `MAX_STAGE` tinyint(2) NOT NULL DEFAULT '0', + `PROGRESS_DONE` decimal(7,3) NOT NULL DEFAULT '0.000'
please run ./mtr --suite funcs_1 --ps-protocol and update test results accordingly.
) DEFAULT CHARSET=utf8 drop table t1; create temporary table t1 like information_schema.processlist; === modified file 'scripts/mytop.sh' --- a/scripts/mytop.sh 2011-06-27 16:30:05 +0000 +++ b/scripts/mytop.sh 2011-06-30 12:23:49 +0000 @@ -17,6 +17,7 @@ use strict; use DBI; use Getopt::Long; use Socket; +use List::Util qw[min max];
conventionally one uses qw/.../ or qw(...)
$main::VERSION = "1.9a";
@@ -1075,21 +1076,22 @@ sub GetData() ## Threads ##
- #my $sz = $width - 52; - my @sz = (9, 9, 15, 10, 10, 6, 8); + my @sz = (9, 8, 15, 9, 6, 5, 6, 8);
do I understand correctly, that you've also committed other mytop bugfixes in this changeset?
my $used = scalar(@sz) + Sum(@sz); - my $free = $width - $used; - + my $state= $width <= 80 ? 6 : int(min(6+($width-80)/3, 15)); + my $free = $width - $used - ($state - 6); + my $format= "%9s %8s %15s %9s %6s %5s %6s %${state}s %-.${free}s\n"; + my $format2= "%9d %8.8s %15.15s %9.9s %6d %5.1f %6.6s %${state}.${state}s %-${free}.${free}s\n"; print BOLD() if ($HAS_COLOR);
- printf "%9s %9s %15s %10s %10s %6s %8s %-${free}s\n", - 'Id','User','Host/IP','DB','Time', 'Cmd', 'State', 'Query'; + printf $format, + 'Id','User','Host/IP','DB','Time', '%', 'Cmd', 'State', 'Query';
print RESET() if ($HAS_COLOR);
## Id User Host DB - printf "%9s %9s %15s %10s %10s %6s %8s %-.${free}s\n", - '--','----','-------','--','----', '---', '-----', '----------'; + printf $format, + '--','----','-------','--','----', '-', '---', '-----', '----------';
$lines_left -= 2;
=== modified file 'sql-common/client.c' --- a/sql-common/client.c 2011-03-18 15:03:43 +0000 +++ b/sql-common/client.c 2011-06-30 12:23:49 +0000 @@ -875,6 +886,29 @@ static void cli_flush_use_result(MYSQL * }
+static void cli_report_progress(MYSQL *mysql, uchar *packet, uint length) +{ + if (mysql->options.extension && mysql->options.extension->report_progress && + length > 5)
you basically ignore the packet if length <= 5. Better report an error here.
+ { + uint stage, max_stage, proc_length; + double progress; + uchar *start= packet; + + packet++; /* Ignore number of strings */ + stage= (uint) *packet++; + max_stage= (uint) *packet++; + progress= uint3korr(packet)/1000.0; + packet+= 3; + proc_length= net_field_length(&packet); + if (packet + proc_length > start + length) + return; /* Error packet */
same here. I am not sure I like the idea of silently ignoring bad packets.
+ (*mysql->options.extension->report_progress)(mysql, stage, max_stage, + progress, (char*) packet, + proc_length); + } +} + #ifdef __WIN__ static my_bool is_NT(void) { @@ -883,57 +917,6 @@ static my_bool is_NT(void) } #endif
- -#ifdef CHECK_LICENSE -/** - Check server side variable 'license'. - - If the variable does not exist or does not contain 'Commercial', - we're talking to non-commercial server from commercial client. - - @retval 0 success - @retval !0 network error or the server is not commercial. - Error code is saved in mysql->net.last_errno. -*/ - -static int check_license(MYSQL *mysql) -{ - MYSQL_ROW row; - MYSQL_RES *res; - NET *net= &mysql->net; - static const char query[]= "SELECT @@license"; - static const char required_license[]= STRINGIFY_ARG(LICENSE); - - if (mysql_real_query(mysql, query, sizeof(query)-1)) - { - if (net->last_errno == ER_UNKNOWN_SYSTEM_VARIABLE) - { - set_mysql_extended_error(mysql, CR_WRONG_LICENSE, unknown_sqlstate, - ER(CR_WRONG_LICENSE), required_license); - } - return 1; - } - if (!(res= mysql_use_result(mysql))) - return 1; - row= mysql_fetch_row(res); - /* - If no rows in result set, or column value is NULL (none of these - two is ever true for server variables now), or column value - mismatch, set wrong license error. - */ - if (!net->last_errno && - (!row || !row[0] || - strncmp(row[0], required_license, sizeof(required_license)))) - { - set_mysql_extended_error(mysql, CR_WRONG_LICENSE, unknown_sqlstate, - ER(CR_WRONG_LICENSE), required_license); - } - mysql_free_result(res); - return net->last_errno; -} -#endif /* CHECK_LICENSE */
should be a separate changeset
@@ -3737,6 +3715,15 @@ mysql_options(MYSQL *mysql,enum mysql_op case MYSQL_DEFAULT_AUTH: extension_set_string(&mysql->options, default_auth, arg); break; + case MYSQL_PROGRESS_CALLBACK: + if (!mysql->options.extension) + mysql->options.extension= (struct st_mysql_options_extention *) + my_malloc(sizeof(struct st_mysql_options_extention), + MYF(MY_WME | MY_ZEROFILL)); + if (mysql->options.extension) + mysql->options.extension->report_progress= + (void (*)(const MYSQL *, uint, uint, double, const char *, uint)) arg; + break;
please create a macro extension_set(), and rewrite extension_set_string() to use extension_set(). Something like #define extension_set(OPTS, X, VAL) \ if (!(OPTS)->extension) \ (OPTS)->extension= (struct st_mysql_options_extention *) \ my_malloc(sizeof(struct st_mysql_options_extention), \ MYF(MY_WME | MY_ZEROFILL)); \ (OPTS)->extension->X= VAL; #define extension_set_string(OPTS, X, STR) \ if ((OPTS)->extension) \ my_free((OPTS)->extension->X, MYF(MY_ALLOW_ZERO_PTR)); \ extension_set(OPTS, X, my_strdup((STR), MYF(MY_WME)));
default: DBUG_RETURN(1); }
=== modified file 'sql/protocol.cc' --- a/sql/protocol.cc 2011-05-28 02:11:32 +0000 +++ b/sql/protocol.cc 2011-06-30 12:23:49 +0000 @@ -515,6 +515,52 @@ void net_end_statement(THD *thd) }
+/** + Send a progress report to the client + + What we send is: + header (255,255,255,1) + stage, max_stage as on byte integers + % withing the stage as %*100000 as a 3 byte integer
write "percentage" not "%". It only takes few more key presses to type the complete word, and is much more clear than printf-like %*100000 besides, in the packet you put percentage*1000 or ratio*100000, but not percentage*100000.
+ proc_info as a string +*/ + +const uchar progress_header[2]= {(uchar) 255, (uchar) 255 }; + +void net_send_progress_packet(THD *thd) +{ + uchar buff[200], *pos; + const char *proc_info= thd->proc_info ? thd->proc_info : ""; + uint length= strlen(proc_info); + ulonglong progress; + + if (unlikely(!thd->net.vio)) + return; // Socket is closed + + pos= buff; + /* + Store number of strings first. This allows us to later expand the + progress indicator if needed. + */ + *pos++= (uchar) 1; // Number of strings + *pos++= (uchar) thd->progress.stage + 1; + /* + We have the max() here to avoid problems if max_stage is not set, + which may happen during automatic repair of table + */ + *pos++= (uchar) max(thd->progress.max_stage, thd->progress.stage + 1); + progress= 0; + if (thd->progress.max_counter) + progress= 100000ULL * thd->progress.counter / thd->progress.max_counter; + int3store(pos, progress); // Between 0 & 100000 + pos+= 3; + pos= net_store_data(pos, (const uchar*) proc_info, + min(length, sizeof(buff)-7)); + net_write_command(&thd->net, (uchar) 255, progress_header, 2, (uchar*) buff,
better sizeof(progress_header) instead of a literal 2.
+ (uint) (pos - buff)); +} + + /**************************************************************************** Functions used by the protocol functions (like net_send_ok) to store strings and numbers in the header result packet.
=== modified file 'sql/set_var.cc' --- a/sql/set_var.cc 2011-06-11 09:04:42 +0000 +++ b/sql/set_var.cc 2011-06-30 12:23:49 +0000 @@ -527,6 +528,10 @@ static sys_var_thd_ulong sys_opti static sys_var_thd_optimizer_switch sys_optimizer_switch(&vars, "optimizer_switch", &SV::optimizer_switch);
+static sys_var_long_ptr sys_progress_report_time(&vars, + "progress_report_time", + &opt_progress_report_time);
this should be session local variable, not global. (as discussed on the phone)
@@ -997,6 +1002,12 @@ static sys_var_enum_const sys_plugin &plugin_maturity, &plugin_maturity_values);
+static sys_var_readonly sys_in_transaction(&vars, "in_transaction", + OPT_SESSION, SHOW_BOOL, + in_transaction); + + +
this belongs to a separate changeset
bool sys_var::check(THD *thd, set_var *var) { var->save_result.ulonglong_value= var->value->val_int(); @@ -3399,6 +3410,13 @@ static uchar *get_myisam_mmap_size(THD * return (uchar *)&myisam_mmap_size; }
+static uchar *in_transaction(THD *thd) +{ + long tmp; // Not active + tmp= test(thd->options & OPTION_BEGIN);
better tmp= test(thd->server_status & SERVER_STATUS_IN_TRANS);
+ thd->sys_var_tmp.my_bool_value= tmp; + return (uchar*) &thd->sys_var_tmp.my_bool_value; +}
/**************************************************************************** Main handling of variables:
=== modified file 'sql/sql_acl.cc' --- a/sql/sql_acl.cc 2011-05-20 19:47:39 +0000 +++ b/sql/sql_acl.cc 2011-06-30 12:23:49 +0000 @@ -7478,7 +7478,7 @@ static ulong parse_client_handshake_pack THD *thd= mpvio->thd; NET *net= &thd->net; char *end; - + ulonglong client_capabilities;
why? client_capabilities are 32-bit, not 64.
DBUG_ASSERT(mpvio->status == MPVIO_EXT::FAILURE);
if (pkt_len < MIN_HANDSHAKE_SIZE) === modified file 'sql/sql_class.h' --- a/sql/sql_class.h 2011-06-27 16:07:24 +0000 +++ b/sql/sql_class.h 2011-06-30 12:23:49 +0000 @@ -1551,6 +1551,17 @@ class THD :public Statement, ulonglong prior_thr_create_utime, thr_create_utime; ulonglong start_utime, utime_after_lock;
+ // Process indicator + struct { + /* Set to 1 if command should report progress to client for the command */ + my_bool report_to_client; + /* Internal: If we should report progress to client */ + my_bool report;
these comments are confusing. I could not understand from them what is the difference between report_to_client and report - both specify "if we should report progress to client". and why they are my_bool, not bool?
+ uint stage, max_stage; + ulonglong counter, max_counter; + ulonglong last_report_time; + } progress; + thr_lock_type update_lock_default; Delayed_insert *di;
=== modified file 'sql/sql_table.cc' --- a/sql/sql_table.cc 2011-06-27 16:07:24 +0000 +++ b/sql/sql_table.cc 2011-06-30 12:23:49 +0000 @@ -8015,8 +8018,10 @@ copy_data_between_tables(TABLE *from,TAB HA_POS_ERROR) goto err; } - }; + thd_progress_next_stage(thd); + }
+ thd_proc_info(thd, "copy to tmp table");
you need to document explicitly that stage name (that is sent over wire) may change any time, without changing the number of a stage. Which, I'd say, is not a very logical requirement, and it may be better not to change stage names in the middle of a stage.
/* Tell handler that we have values for all columns in the to table */ to->use_all_columns(); to->mark_virtual_columns_for_write(TRUE); === modified file 'storage/maria/ha_maria.cc' --- a/storage/maria/ha_maria.cc 2011-06-27 16:07:24 +0000 +++ b/storage/maria/ha_maria.cc 2011-06-30 12:23:49 +0000 @@ -711,6 +711,15 @@ int _ma_killed_ptr(HA_CHECK *param) }
+void _ma_report_progress(HA_CHECK *param, ulonglong progress, + ulonglong max_progress) +{ + thd_progress_report((THD*)param->thd, + progress + max_progress * param->stage, + max_progress * param->max_stage);
why? I'd expect simple thd_progress_report((THD*)param->thd, progress, max_progress) explained on the phone. but this absolutely needs a comment here.
+} + + void _ma_check_print_error(HA_CHECK *param, const char *fmt, ...) { va_list args; @@ -1132,12 +1141,16 @@ int ha_maria::check(THD * thd, HA_CHECK_ return HA_ADMIN_ALREADY_DONE;
maria_chk_init_for_check(¶m, file); + old_proc_info= thd_proc_info(thd, "Checking table"); + thd_progress_init(thd, 3); (void) maria_chk_status(¶m, file); // Not fatal error= maria_chk_size(¶m, file); if (!error) error|= maria_chk_del(¶m, file, param.testflag); + thd_progress_next_stage(thd); if (!error) error= maria_chk_key(¶m, file);
you forgot to set proc_info for a new stage (here, and above)
+ thd_progress_next_stage(thd); if (!error) { if ((!(param.testflag & T_QUICK) && === modified file 'storage/maria/ma_sort.c' --- a/storage/maria/ma_sort.c 2011-01-11 13:36:41 +0000 +++ b/storage/maria/ma_sort.c 2011-06-30 12:23:49 +0000 @@ -1066,6 +1087,8 @@ merge_index(MARIA_SORT_PARAM *info, uint if (merge_buffers(info,keys,tempfile,(IO_CACHE*) 0,sort_keys,buffpek,buffpek, buffpek+maxbuffer)) DBUG_RETURN(1); /* purecov: inspected */ + if (info->sort_info->param->max_stage != 1) /* If not parallel */ + _ma_report_progress(info->sort_info->param, 1, 1); DBUG_RETURN(0); } /* merge_index */
would be nice if you'd copied these changes into MyISAM too.
=== modified file 'tests/mysql_client_test.c' --- a/tests/mysql_client_test.c 2011-06-07 16:13:02 +0000 +++ b/tests/mysql_client_test.c 2011-06-30 12:23:49 +0000 @@ -309,7 +309,8 @@ mysql_simple_prepare(MYSQL *mysql_arg, c
@return pointer to initialized and connected MYSQL object */ -static MYSQL* client_connect(ulong flag, uint protocol, my_bool auto_reconnect) +static MYSQL* client_connect(ulonglong flag, uint protocol,
flags fit in 32 bits, no need to change to ulonglong
+ my_bool auto_reconnect) { MYSQL* mysql; int rc;
Regards, Sergei
Hi!
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
Sergei> Hi, Michael! Sergei> On Jun 30, Michael Widenius wrote:
message: Added progress reporting for alter table, LOAD DATA INFILE and for aria tables: check table, repair table, analyze table.
=== modified file 'include/mysql/plugin.h'
<cut>
+void thd_progress_init(MYSQL_THD thd, unsigned int max_stage); +void thd_progress_report(MYSQL_THD thd, + unsigned long long progress, + unsigned long long max_progress); +void thd_progress_next_stage(MYSQL_THD thd); +void thd_progress_end(MYSQL_THD thd);
Sergei> This should be implemented as a service. Sergei> See libservices/HOWTO ok. (We agreed to also move thd_proc_info() to libservices)
+ +/** Return the thread id of a user thread
@param thd user thread connection handle
=== modified file 'mysql-test/r/create.result' --- a/mysql-test/r/create.result 2011-05-16 11:05:45 +0000 +++ b/mysql-test/r/create.result 2011-06-30 12:23:49 +0000 @@ -1760,7 +1760,10 @@ t1 CREATE TABLE `t1` ( `TIME` int(7) NOT NULL DEFAULT '0', `STATE` varchar(64) DEFAULT NULL, `INFO` longtext, - `TIME_MS` decimal(22,3) NOT NULL DEFAULT '0.000' + `TIME_MS` decimal(22,3) NOT NULL DEFAULT '0.000', + `STAGE` tinyint(2) NOT NULL DEFAULT '0', + `MAX_STAGE` tinyint(2) NOT NULL DEFAULT '0', + `PROGRESS_DONE` decimal(7,3) NOT NULL DEFAULT '0.000'
Sergei> please run Sergei> ./mtr --suite funcs_1 --ps-protocol Sergei> and update test results accordingly. Will do. I also changed PROGRESS_DONE to PROGRESS
) DEFAULT CHARSET=utf8 drop table t1; create temporary table t1 like information_schema.processlist; === modified file 'scripts/mytop.sh' --- a/scripts/mytop.sh 2011-06-27 16:30:05 +0000 +++ b/scripts/mytop.sh 2011-06-30 12:23:49 +0000 @@ -17,6 +17,7 @@ use strict; use DBI; use Getopt::Long; use Socket; +use List::Util qw[min max];
Sergei> conventionally one uses qw/.../ or qw(...)
$main::VERSION = "1.9a";
@@ -1075,21 +1076,22 @@ sub GetData() ## Threads ##
- #my $sz = $width - 52; - my @sz = (9, 9, 15, 10, 10, 6, 8); + my @sz = (9, 8, 15, 9, 6, 5, 6, 8);
Sergei> do I understand correctly, that you've also committed other Sergei> mytop bugfixes in this changeset? No, this is all to get place for displaying '%' properly.
=== modified file 'sql-common/client.c' --- a/sql-common/client.c 2011-03-18 15:03:43 +0000 +++ b/sql-common/client.c 2011-06-30 12:23:49 +0000 @@ -875,6 +886,29 @@ static void cli_flush_use_result(MYSQL * }
+static void cli_report_progress(MYSQL *mysql, uchar *packet, uint length) +{ + if (mysql->options.extension && mysql->options.extension->report_progress && + length > 5)
Sergei> you basically ignore the packet if length <= 5. Sergei> Better report an error here. I have now added error reporting to cli_report_progress()
@@ -3737,6 +3715,15 @@ mysql_options(MYSQL *mysql,enum mysql_op case MYSQL_DEFAULT_AUTH: extension_set_string(&mysql->options, default_auth, arg); break; + case MYSQL_PROGRESS_CALLBACK: + if (!mysql->options.extension) + mysql->options.extension= (struct st_mysql_options_extention *) + my_malloc(sizeof(struct st_mysql_options_extention), + MYF(MY_WME | MY_ZEROFILL)); + if (mysql->options.extension) + mysql->options.extension->report_progress= + (void (*)(const MYSQL *, uint, uint, double, const char *, uint)) arg; + break;
Sergei> please create a macro extension_set(), and rewrite Sergei> extension_set_string() to use extension_set(). Something like Sergei> #define extension_set(OPTS, X, VAL) \ Sergei> if (!(OPTS)->extension) \ Sergei> (OPTS)->extension= (struct st_mysql_options_extention *) \ Sergei> my_malloc(sizeof(struct st_mysql_options_extention), \ Sergei> MYF(MY_WME | MY_ZEROFILL)); \ Sergei> (OPTS)->extension->X= VAL; Sergei> #define extension_set_string(OPTS, X, STR) \ Sergei> if ((OPTS)->extension) \ Sergei> my_free((OPTS)->extension->X, MYF(MY_ALLOW_ZERO_PTR)); \ Sergei> extension_set(OPTS, X, my_strdup((STR), MYF(MY_WME))); Didn't think it was needed for only one case. If there is a single more case than sure.
=== modified file 'sql/protocol.cc' --- a/sql/protocol.cc 2011-05-28 02:11:32 +0000 +++ b/sql/protocol.cc 2011-06-30 12:23:49 +0000 @@ -515,6 +515,52 @@ void net_end_statement(THD *thd) }
+/** + Send a progress report to the client + + What we send is: + header (255,255,255,1) + stage, max_stage as on byte integers + % withing the stage as %*100000 as a 3 byte integer
Sergei> write "percentage" not "%". It only takes few more key presses to type Sergei> the complete word, and is much more clear than printf-like %*100000 Sergei> besides, in the packet you put percentage*1000 or ratio*100000, but Sergei> not percentage*100000. ok <cut>
=== modified file 'sql/set_var.cc' --- a/sql/set_var.cc 2011-06-11 09:04:42 +0000 +++ b/sql/set_var.cc 2011-06-30 12:23:49 +0000 @@ -527,6 +528,10 @@ static sys_var_thd_ulong sys_opti static sys_var_thd_optimizer_switch sys_optimizer_switch(&vars, "optimizer_switch", &SV::optimizer_switch);
+static sys_var_long_ptr sys_progress_report_time(&vars, + "progress_report_time", + &opt_progress_report_time);
Sergei> this should be session local variable, not global. Sergei> (as discussed on the phone) Done.
@@ -997,6 +1002,12 @@ static sys_var_enum_const sys_plugin &plugin_maturity, &plugin_maturity_values);
+static sys_var_readonly sys_in_transaction(&vars, "in_transaction", + OPT_SESSION, SHOW_BOOL, + in_transaction); + + +
Sergei> this belongs to a separate changeset ok. <cut>
=== modified file 'sql/sql_acl.cc' --- a/sql/sql_acl.cc 2011-05-20 19:47:39 +0000 +++ b/sql/sql_acl.cc 2011-06-30 12:23:49 +0000 @@ -7478,7 +7478,7 @@ static ulong parse_client_handshake_pack THD *thd= mpvio->thd; NET *net= &thd->net; char *end; - + ulonglong client_capabilities;
Sergei> why? client_capabilities are 32-bit, not 64. My mistake; Had a bug that I thought was caused by wrong type and did a test here but forgot to remove.
DBUG_ASSERT(mpvio->status == MPVIO_EXT::FAILURE);
if (pkt_len < MIN_HANDSHAKE_SIZE) === modified file 'sql/sql_class.h' --- a/sql/sql_class.h 2011-06-27 16:07:24 +0000 +++ b/sql/sql_class.h 2011-06-30 12:23:49 +0000 @@ -1551,6 +1551,17 @@ class THD :public Statement, ulonglong prior_thr_create_utime, thr_create_utime; ulonglong start_utime, utime_after_lock;
+ // Process indicator + struct { + /* Set to 1 if command should report progress to client for the command */ + my_bool report_to_client; + /* Internal: If we should report progress to client */ + my_bool report;
Sergei> these comments are confusing. I could not understand from them what is Sergei> the difference between report_to_client and report - both specify "if we Sergei> should report progress to client". Sergei> and why they are my_bool, not bool? should be bool. Typo.
=== modified file 'sql/sql_table.cc' --- a/sql/sql_table.cc 2011-06-27 16:07:24 +0000 +++ b/sql/sql_table.cc 2011-06-30 12:23:49 +0000 @@ -8015,8 +8018,10 @@ copy_data_between_tables(TABLE *from,TAB HA_POS_ERROR) goto err; } - }; + thd_progress_next_stage(thd); + }
+ thd_proc_info(thd, "copy to tmp table");
Sergei> you need to document explicitly that stage name (that is sent over wire) Sergei> may change any time, without changing the number of a stage. I have now done that in the kb. Sergei> Which, I'd say, is not a very logical requirement, and it may be better Sergei> not to change stage names in the middle of a stage. We do call thd_proc_info() a lot in the server to give more information in process_list what the server is doing. (For example, waiting on a mutex). Stage is something bigger than a proc_info() change. A stage is for example in alter table: 1) sort the table (if requested) 2) copy data 3) create keys.
+++ b/storage/maria/ha_maria.cc 2011-06-30 12:23:49 +0000 @@ -711,6 +711,15 @@ int _ma_killed_ptr(HA_CHECK *param) }
+void _ma_report_progress(HA_CHECK *param, ulonglong progress, + ulonglong max_progress) +{ + thd_progress_report((THD*)param->thd, + progress + max_progress * param->stage, + max_progress * param->max_stage);
Sergei> why? I'd expect simple Sergei> thd_progress_report((THD*)param->thd, progress, max_progress) Sergei> explained on the phone. Sergei> but this absolutely needs a comment here. I added the following: /* Report progress to mysqld This is a bit more complex than what a normal progress report function normally is. The reason is that this is called by enable_index/repair which is one stage in ALTER TABLE and we can't use the external stage/max_stage for this. thd_progress_init/thd_progress_next_stage is to be called by high level commands like CHECK TABLE or REPAIR TABLE, not by sub commands like enable_index(). In ma_check.c it's easier to work with stages than with a total progress, so we use internal stage/max_stage here to keep the code simple. */
maria_chk_init_for_check(¶m, file); + old_proc_info= thd_proc_info(thd, "Checking table"); + thd_progress_init(thd, 3); (void) maria_chk_status(¶m, file); // Not fatal error= maria_chk_size(¶m, file); if (!error) error|= maria_chk_del(¶m, file, param.testflag); + thd_progress_next_stage(thd); if (!error) error= maria_chk_key(¶m, file);
Sergei> you forgot to set proc_info for a new stage (here, and above) I didn't think it was needed to add one extra status information here. I have now change this to use: Checking status, Checking keys & Checking data.
+ thd_progress_next_stage(thd); if (!error) { if ((!(param.testflag & T_QUICK) && === modified file 'storage/maria/ma_sort.c' --- a/storage/maria/ma_sort.c 2011-01-11 13:36:41 +0000 +++ b/storage/maria/ma_sort.c 2011-06-30 12:23:49 +0000 @@ -1066,6 +1087,8 @@ merge_index(MARIA_SORT_PARAM *info, uint if (merge_buffers(info,keys,tempfile,(IO_CACHE*) 0,sort_keys,buffpek,buffpek, buffpek+maxbuffer)) DBUG_RETURN(1); /* purecov: inspected */ + if (info->sort_info->param->max_stage != 1) /* If not parallel */ + _ma_report_progress(info->sort_info->param, 1, 1); DBUG_RETURN(0); } /* merge_index */
Sergei> would be nice if you'd copied these changes into MyISAM too. I plan to do this later. I wanted Aria as a test case for how to do it. When we are completely satisified with this, we can copy this to MyISAM.
=== modified file 'tests/mysql_client_test.c' --- a/tests/mysql_client_test.c 2011-06-07 16:13:02 +0000 +++ b/tests/mysql_client_test.c 2011-06-30 12:23:49 +0000 @@ -309,7 +309,8 @@ mysql_simple_prepare(MYSQL *mysql_arg, c
@return pointer to initialized and connected MYSQL object */ -static MYSQL* client_connect(ulong flag, uint protocol, my_bool auto_reconnect) +static MYSQL* client_connect(ulonglong flag, uint protocol,
Sergei> flags fit in 32 bits, no need to change to ulonglong ok. Regards, Monty
Michael Widenius <monty@askmonty.org> writes:
"Sergei" == Sergei Golubchik <serg@askmonty.org> writes:
Sergei> please create a macro extension_set(), and rewrite Sergei> extension_set_string() to use extension_set(). Something like
Sergei> #define extension_set(OPTS, X, VAL) \ Sergei> if (!(OPTS)->extension) \ Sergei> (OPTS)->extension= (struct st_mysql_options_extention *) \ Sergei> my_malloc(sizeof(struct st_mysql_options_extention), \ Sergei> MYF(MY_WME | MY_ZEROFILL)); \ Sergei> (OPTS)->extension->X= VAL;
Sergei> #define extension_set_string(OPTS, X, STR) \ Sergei> if ((OPTS)->extension) \ Sergei> my_free((OPTS)->extension->X, MYF(MY_ALLOW_ZERO_PTR)); \ Sergei> extension_set(OPTS, X, my_strdup((STR), MYF(MY_WME)));
Didn't think it was needed for only one case. If there is a single more case than sure.
I will need this for MWL#192 I think (to remember the state when returning in the middle of a non-blocking call waiting for more data on socket). - Kristian.
participants (3)
-
Kristian Nielsen
-
Michael Widenius
-
Sergei Golubchik