maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #04229
Re: [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
Follow ups
