maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergei" == Sergei Golubchik <serg@xxxxxxxxxxxx> 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(&param, file);
>> +  old_proc_info= thd_proc_info(thd, "Checking table");
>> +  thd_progress_init(thd, 3);
>> (void) maria_chk_status(&param, file);                // Not fatal
>> error= maria_chk_size(&param, file);
>> if (!error)
>> error|= maria_chk_del(&param, file, param.testflag);
>> +  thd_progress_next_stage(thd);
>> if (!error)
>> error= maria_chk_key(&param, 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