maria-developers team mailing list archive

[Sergei Petrunia <psergey@xxxxxxxxxxxx>]

Hi Sergei,

For the second review, I have either addressed the feedback, or wrote below
why I have not addressed it. My comments are marked with 'psergey:'

On Fri, Jul 13, 2012 at 09:12:16PM +0200, Sergei Golubchik wrote:
> On Jul 11, Sergei Petrunia wrote:
> > 
> > Please find attached the updated combined SHOW EXPLAIN patch. I believe all
> > of feedback from the previous review has been addressed, except for:
> 
> almost. you forgot a couple of issues, but it's in the review below, no
> problem.
> 
> > - Request to use of dbug sync facility for testing: this is waiting
> > for you to come up with idea about how this can be done,
> 
> Yes. I've not commented on that in this second review.
> 
> > - Error messages: you've indicated you were not happy with the current
> > ones but I don't have any clue what set of errors would be better.
> 
> There are few suggestions below.
> 
> See the full review below. I didn't have any major comments.
> There're mostly questions, and few suggestions and requests to move
> pieces of code around.
> 
> > === added file 'mysql-test/r/show_explain.result'
> > --- mysql-test/r/show_explain.result	1970-01-01 00:00:00 +0000
> > +++ mysql-test/r/show_explain.result	2012-07-10 17:23:00 +0000
> > @@ -0,0 +1,823 @@
> ...
> > +set debug_dbug='';
> > +#
> > +# Unfortunately, our test setup doesn't allow to check that test2
> > +# can do SHOW EXPLAIN on his own queries. This is because SET debug_dbug
> > +# requires SUPER privilege. Giving SUPER to test2 will make the test
> > +# meaningless
> 
> Why? In the code you check only for PROCESS_ACL, not for SUPER_ACL
> (which is correct). Why SUPER makes the test meaningless?
> 
psergey:
This was an error, I assumed that granting SUPER will also grant PROCESS. 
Apparently this is not the case, and I've now added the test.

> > +#
> > +#
> > === modified file 'sql/filesort.cc'
> > --- sql/filesort.cc	2012-05-21 13:30:25 +0000
> > +++ sql/filesort.cc	2012-06-25 14:39:26 +0000
> > @@ -1231,18 +1235,13 @@ int merge_buffers(SORTPARAM *param, IO_C
> >    void *first_cmp_arg;
> >    element_count dupl_count= 0;
> >    uchar *src;
> > -  killed_state not_killable;
> >    uchar *unique_buff= param->unique_buff;
> > -  volatile killed_state *killed= &current_thd->killed;
> > +  const bool killable= !param->not_killable;
> 
> did you ask Monty about this not_killable thing?
psergey:
No, I haven't yet. As I mentioned, my goal is to produce a SHOW EXPLAIN patch,
not a SHOW EXPLAIN plus some random refactorings patch.

> 
> > +  THD* const thd=current_thd;
> >    DBUG_ENTER("merge_buffers");
> >  
> > -  status_var_increment(current_thd->status_var.filesort_merge_passes);
> > -  current_thd->query_plan_fsort_passes++;
> > -  if (param->not_killable)
> > -  {
> > -    killed= &not_killable;
> > -    not_killable= NOT_KILLED;
> > -  }
> > +  status_var_increment(thd->status_var.filesort_merge_passes);
> > +  thd->query_plan_fsort_passes++;
> >  
> >    error=0;
> >    rec_length= param->rec_length;
> > === modified file 'sql/mysqld.cc'
> > --- sql/mysqld.cc	2012-06-08 09:18:56 +0000
> > +++ sql/mysqld.cc	2012-07-07 04:47:41 +0000
> > @@ -3908,6 +3909,7 @@ static int init_thread_environment()
> >    sp_cache_init();
> >  #ifdef HAVE_EVENT_SCHEDULER
> >    Events::init_mutexes();
> > +  init_show_explain_psi_keys();
> 
> Eh? Inside #ifdef HAVE_EVENT_SCHEDULER ?
> 
psergey:
it was my mistake, moved it outside.

> >  #endif
> >    /* Parameter for threads created for connections */
> >    (void) pthread_attr_init(&connection_attrib);
> > === modified file 'sql/sql_class.cc'
> > --- sql/sql_class.cc	2012-05-21 13:30:25 +0000
> > +++ sql/sql_class.cc	2012-07-11 09:39:56 +0000
> > @@ -3198,6 +3233,43 @@ void THD::restore_active_arena(Query_are
> >    DBUG_VOID_RETURN;
> >  }
> >  
> > +
> > +/*
> > +  Produce EXPLAIN data.
> > +
> > +  This function is APC-scheduled to be run in the context of the thread that
> > +  we're producing EXPLAIN for.
> > +*/
> > +
> > +void Show_explain_request::call_in_target_thread()
> > +{
> > +  Query_arena backup_arena;
> > +  bool printed_anything= FALSE;
> > +
> > +  /* 
> > +    Change the arena because JOIN::print_explain and co. are going to allocate
> > +    items. Let them allocate them on our arena.
> > +  */
> > +  target_thd->set_n_backup_active_arena((Query_arena*)request_thd,
> > +                                        &backup_arena);
> > +  
> > +  query_str.copy(target_thd->query(), 
> > +                 target_thd->query_length(),
> > +                 &my_charset_bin);
> 
> Is that right? my_charset_bin?
>
psergey:
No, it wasn't. I've added a conversion (see the second commit).

> > +
> > +  if (target_thd->lex->unit.print_explain(explain_buf, 0 /* explain flags*/,
> > +                                          &printed_anything))
> > +  {
> > +    failed_to_produce= TRUE;
> > +  }
> > +
> > +  if (!printed_anything)
> > +    failed_to_produce= TRUE;
> > +
> > +  target_thd->restore_active_arena((Query_arena*)request_thd, &backup_arena);
> > +}
> 
> wouldn't it be more appropriate to have this in sql_show.cc, not here?
> (and select_result_explain_buffer too)
>
psergey:
I don't think there is any difference. Moved the function to sql_show.cc

> > +
> > +
> >  Statement::~Statement()
> >  {
> >  }
> > === modified file 'sql/sql_class.h'
> > --- sql/sql_class.h	2012-05-21 13:30:25 +0000
> > +++ sql/sql_class.h	2012-07-11 09:39:56 +0000
> > @@ -1520,6 +1520,43 @@ class Global_read_lock
> >  
> >  extern "C" void my_message_sql(uint error, const char *str, myf MyFlags);
> >  
> > +class select_result_explain_buffer;
> > +
> > +
> > +/*
> > +  SHOW EXPLAIN request object. 
> > +  
> > +  The thread that runs SHOW EXPLAIN statement creates a Show_explain_request
> > +  object R, and then schedules APC call of
> > +  Show_explain_request::call((void*)&R).
> > +
> > +*/
> > +
> > +class Show_explain_request : public Apc_target::Apc_call
> 
> I think it'd be more logical to have this in sql_show.h
> 
psergey: Moved to sql_show.h 

> > +{
> > +public:
> > +  THD *target_thd;  /* thd that we're running SHOW EXPLAIN for */
> > +  THD *request_thd; /* thd that run SHOW EXPLAIN command */
> > +  
> > +  /* If true, there was some error when producing EXPLAIN output. */
> > +  bool failed_to_produce;
> > +   
> > +  /* SHOW EXPLAIN will be stored here */
> > +  select_result_explain_buffer *explain_buf;
> > +  
> > +  /* Query that we've got SHOW EXPLAIN for */
> > +  String query_str;
> > +  
> > +  /* Overloaded virtual function */
> > +  void call_in_target_thread();
> > +};
> > +
> > +class THD;
> > +void mysqld_show_explain(THD *thd, const char *calling_user, ulong thread_id);
> > +#ifndef DBUG_OFF
> > +void dbug_serve_apcs(THD *thd, int n_calls);
> > +#endif 
> > +
> >  /**
> >    @class THD
> >    For each client connection we create a separate thread with THD serving as
> > @@ -2185,6 +2222,15 @@ class THD :public Statement,
> >    */
> >    killed_state volatile killed;
> >  
> > +  inline bool check_killed()
> > +  {
> > +    if (killed)
> > +      return TRUE;
> > +    if (apc_target.have_apc_requests())
> > +      apc_target.process_apc_requests(); 
> > +    return FALSE;
> > +  }
> > +
> 
> You forgot to fix thd_killed() function, didn't you?
> 
psergey: Added the check in that function, too.

> >    /* scramble - random string sent to client on handshake */
> >    char	     scramble[SCRAMBLE_LENGTH+1];
> >  
> > @@ -2383,10 +2429,20 @@ class THD :public Statement,
> >    void close_active_vio();
> >  #endif
> >    void awake(killed_state state_to_set);
> > -
> > + 
> >    /** Disconnect the associated communication endpoint. */
> >    void disconnect();
> >  
> > +
> > +  /*
> > +    Allows this thread to serve as a target for others to schedule Async 
> > +    Procedure Calls on.
> > +
> > +    It's possible to schedule arbitrary C++ function calls. Currently, only
> 
> Fix the comment please. Something like
> 
>  It's possible to schedule any code to be executed this way, by
>  inheriting from the Apc_call object. Currently, only
>  Show_explain_request uses this.
> 
psergey:
Changed to the provided text

> > +    Show_explain_request uses this.
> > +  */
> > +  Apc_target apc_target;
> > +
> >  #ifndef MYSQL_CLIENT
> >    enum enum_binlog_query_type {
> >      /* The query can be logged in row format or in statement format. */
> > === modified file 'sql/sql_join_cache.cc'
> > --- sql/sql_join_cache.cc	2012-03-24 17:21:22 +0000
> > +++ sql/sql_join_cache.cc	2012-05-16 20:59:03 +0000
> > @@ -2236,7 +2236,7 @@ enum_nested_loop_state JOIN_CACHE::join_
> >    
> >    while (!(error= join_tab_scan->next()))   
> >    {
> > -    if (join->thd->killed)
> > +    if (join->thd->check_killed())
> 
> I would recommend to rename thd->killed (for example, to thd->killed_flag)
> just to make sure no code uses thd->killed the old way,
> and - more importantly - that we won't get new thd->killed uses in
> merges from MySQL.  

psergey:
I've tried renaming. This required to make changes in 160 places, and was a
1600-line patch:
http://s.petrunia.net/scratch/psergey-rename-killed-to-killed_flag.diff

Did you really intend to make such a big change?

> Actually, it'd be enough to make thd->killed
> private, no need to rename it
> 
- doesn't change the fact that one needs to change the code in 160 places,
- a lot of places that assign thd->killed or do things like "return
  thd->killed"  will need to access killed through function or be made THD's
  friends.

> >      {
> >        /* The user has aborted the execution of the query */
> >        join->thd->send_kill_message();
> > === modified file 'sql/sql_lex.h'
> > --- sql/sql_lex.h	2012-05-21 13:30:25 +0000
> > +++ sql/sql_lex.h	2012-07-10 17:23:00 +0000
> > @@ -2344,7 +2357,8 @@ struct LEX: public Query_tables_list
> >    char *backup_dir;				/* For RESTORE/BACKUP */
> >    char* to_log;                                 /* For PURGE MASTER LOGS TO */
> >    char* x509_subject,*x509_issuer,*ssl_cipher;
> > -  String *wild;
> > +  String *wild; /* Wildcard in SHOW {something} LIKE 'wild'*/ 
> > +  Item *show_explain_for_thread; /* id in SHOW EXPLAIN FOR id */
> 
> Why do extend LEX even more?
> KILL, for example, uses LEX::value_list
> 
psergey:
KILL does NOT use INFORMATION_SCHEMA. This is why it is able to use
LEX::value_list.

SHOW ... LIKE commands that do use information schema, use the "String *wild"
member above. I am following the way SHOW commands work.

> >    sql_exchange *exchange;
> >    select_result *result;
> >    Item *default_value, *on_update_value;
> > === modified file 'sql/sql_parse.cc'
> > --- sql/sql_parse.cc	2012-05-21 18:54:41 +0000
> > +++ sql/sql_parse.cc	2012-07-10 17:23:00 +0000
> > @@ -2143,6 +2144,32 @@ mysql_execute_command(THD *thd)
> >      execute_show_status(thd, all_tables);
> >      break;
> >    }
> > +  case SQLCOM_SHOW_EXPLAIN:
> > +  {
> > +    if (!thd->security_ctx->priv_user[0] &&
> > +        check_global_access(thd,PROCESS_ACL))
> > +      break;
> > +
> > +    /*
> > +      The select should use only one table, it's the SHOW EXPLAIN pseudo-table
> > +    */
> > +    if (lex->sroutines.records || lex->query_tables->next_global)
> > +    {
> > +      my_error(ER_NOT_SUPPORTED_YET, MYF(0), "Usage of subqueries or stored "
> > +               "function calls as part of this statement");
> 
> Ok. Here I'd suggest to create a new error message
> 
> ER_MSG_SUBQUERIES_OR_ROUTINES
>   "Usage of subqueries or stored function calls as part of this statement"
> 
> and use it as
> 
>    my_error(ER_NOT_SUPPORTED_YET, MYF(0), ER(ER_MSG_SUBQUERIES_OR_ROUTINES));
> 
> also in SQLCOM_KILL and SQLCOM_CREATE_EVENT cases.
> 
psergey:
I've changed SHOW EXPLAIN code not to use this error (it uses
ER_SET_CONSTANTS_ONLY now). Is that ok?

> > +      break;
> > +    }
> > +
> > +    Item **it= &(lex->show_explain_for_thread);
> > +    if ((!(*it)->fixed && (*it)->fix_fields(lex->thd, it)) || 
> > +        (*it)->check_cols(1))
> > +    {
> > +      my_message(ER_SET_CONSTANTS_ONLY, ER(ER_SET_CONSTANTS_ONLY),
> > +		 MYF(0));
> 
> I'd suggest to rephrase the error message a little bit:
> 
> -  eng "You may only use constant expressions with SET"
> -  ger "Bei SET dürfen nur konstante Ausdrücke verwendet werden"
> -  rus "Вы можете использовать в SET только константные выражения"
> +  eng "You may only use constant expressions in this statement"
> +  ger "Bei diesem Befehl dürfen nur konstante Ausdrücke verwendet werden"
> +  rus "С этой командой вы можете использовать только константные выражения"
> 
> it'll work better for KILL too. You can keep other translations of this
> error message unchanged.
> 
psergey:
Done.

> > +      goto error;
> > +    }
> > +    /* no break; fall through */
> > +  }
> >    case SQLCOM_SHOW_DATABASES:
> >    case SQLCOM_SHOW_TABLES:
> >    case SQLCOM_SHOW_TRIGGERS:
> > === modified file 'sql/sql_select.cc'
> > --- sql/sql_select.cc	2012-06-04 15:26:11 +0000
> > +++ sql/sql_select.cc	2012-07-05 20:28:30 +0000
> > @@ -928,6 +979,13 @@ bool JOIN::prepare_stage2()
> >  }
> >  
> >  
> > +int JOIN::optimize()
> > +{
> > +  int res= optimize_inner();
> > +  if (!res)
> > +    have_query_plan= QEP_AVAILABLE;
> 
> Why wouldn't you simply set have_query_plan from the old JOIN::optimize,
> I mean, why did you have to move it to optimize_inner() ?
> 
> I thought it's because in some cases you need to invoke optimize_inner()
> directly, without setting have_query_plan. But you don't seem to do it
> anywhere.
> 
psergey:
There are multiple exits from JOIN::optimize, and I wanted to cover them all
with one statement.
> > +  return res;
> > +}
> >  /**
> >    global select optimisation.
> >  
> > @@ -2142,6 +2200,32 @@ JOIN::save_join_tab()
> >  }
> >  
> >  
> > +void JOIN::exec()
> > +{
> > +  /*
> > +    Enable SHOW EXPLAIN only if we're in the top-level query.
> > +  */
> > +  thd->apc_target.enable();
> > +  DBUG_EXECUTE_IF("show_explain_probe_join_exec_start", 
> > +                  if (dbug_user_var_equals_int(thd, 
> > +                                               "show_explain_probe_select_id", 
> > +                                               select_lex->select_number))
> > +                        dbug_serve_apcs(thd, 1);
> > +                 );
> > +
> > +  exec_inner();
> 
> Same question as above, about optimize_inner().

psergey:
The answer is similar: there are too many exits from JOIN::exec, and I didn't want to
have the show_explain_probe_join_exec_end "probe" all over the place.

> And also - you didn't remove apc_target.enable/disable, as we've discussed.
> 
psergey:
Answered in (why-apc-enable-disable) below.

> > +
> > +  DBUG_EXECUTE_IF("show_explain_probe_join_exec_end", 
> > +                  if (dbug_user_var_equals_int(thd, 
> > +                                               "show_explain_probe_select_id", 
> > +                                               select_lex->select_number))
> > +                        dbug_serve_apcs(thd, 1);
> > +                 );
> > +
> > +  thd->apc_target.disable();
> > +}
> > +
> > +
> >  /**
> >    Exec select.
> >  
> > @@ -2458,6 +2542,10 @@ JOIN::exec()
> >        DBUG_PRINT("info",("Creating group table"));
> >        
> >        /* Free first data from old join */
> > +      
> > +      /*
> > +        psergey-todo: this is the place of pre-mature JOIN::free call.
> > +      */
> 
> forgotten comment?
> 
psergey: No, it should be there as it relates to MDEV-325, BUG#992942.

> >        curr_join->join_free();
> >        if (curr_join->make_simple_join(this, curr_tmp_table))
> >  	DBUG_VOID_RETURN;
> > @@ -7861,6 +7965,13 @@ static bool create_ref_for_key(JOIN *joi
> >        if (keyuse->null_rejecting) 
> >          j->ref.null_rejecting |= 1 << i;
> >        keyuse_uses_no_tables= keyuse_uses_no_tables && !keyuse->used_tables;
> > +      /*
> > +        Todo: we should remove this check for thd->lex->describe on the next
> > +        line. With SHOW EXPLAIN code, EXPLAIN printout code no longer depends
> > +        on it. However, removing the check caused change in lots of query
> > +        plans! Does the optimizer depend on the contents of
> > +        table_ref->key_copy ? If yes, do we produce incorrect EXPLAINs? 
> > +      */
> 
> Did you already create a Jira task for this?
> 
psergey:
No, I haven't. I cannot imagine anybody starting to work on such Jira task.
There can't be any business case for it. It can only be done as a part of 
some other task.

> >        if (!keyuse->val->used_tables() && !thd->lex->describe)
> >        {					// Compare against constant
> >  	store_key_item tmp(thd, 
> > @@ -8015,6 +8127,7 @@ JOIN::make_simple_join(JOIN *parent, TAB
> >        !(parent->join_tab_reexec= (JOIN_TAB*) thd->alloc(sizeof(JOIN_TAB))))
> >      DBUG_RETURN(TRUE);                        /* purecov: inspected */
> >  
> > +  // psergey-todo: here, save the pointer for original join_tabs.
> 
> forgotten comment?
psergey: No, it should be there as it relates to MDEV-325, BUG#992942.

> 
> >    join_tab= parent->join_tab_reexec;
> >    table= &parent->table_reexec[0]; parent->table_reexec[0]= temp_table;
> >    table_count= top_join_tab_count= 1;
> > === modified file 'sql/sql_show.cc'
> > --- sql/sql_show.cc	2012-05-21 18:54:41 +0000
> > +++ sql/sql_show.cc	2012-07-10 17:23:00 +0000
> > @@ -1998,6 +1998,124 @@ void mysqld_list_processes(THD *thd,cons
> >    DBUG_VOID_RETURN;
> >  }
> >  
> > +
> > +static 
> > +const char *target_not_explainable_cmd="Target is not running EXPLAINable command";
> > +
> > +/*
> > +  Store the SHOW EXPLAIN output in the temporary table.
> > +*/
> > +
> > +int fill_show_explain(THD *thd, TABLE_LIST *table, COND *cond)
> > +{
> > +  const char *calling_user;
> > +  THD *tmp;
> > +  my_thread_id  thread_id;
> > +  DBUG_ENTER("fill_show_explain");
> > +
> > +  DBUG_ASSERT(cond==NULL);
> > +  thread_id= thd->lex->show_explain_for_thread->val_int();
> > +  calling_user= (thd->security_ctx->master_access & PROCESS_ACL) ?  NullS :
> > +                 thd->security_ctx->priv_user;
> > +  /* 
> > +    Find the thread we need EXPLAIN for. Thread search code was copied from
> > +    kill_one_thread()
> 
> Perhaps, it'd make sense to extract this code to a separate function and
> reuse it?
>
psergey:
It's a 13-line loop, I don't think it makes sense to extract it into a
separate function. The separate function will have a complex calling
convention (it returns with acquired mutex), so I will end up adding 
more code than I have removed.

> > +  */
> > +  mysql_mutex_lock(&LOCK_thread_count); // For unlink from list
> > +  I_List_iterator<THD> it(threads);
> > +  while ((tmp=it++))
> > +  {
> > +    if (tmp->command == COM_DAEMON)
> > +      continue;
> > +    if (tmp->thread_id == thread_id)
> > +    {
> > +      mysql_mutex_lock(&tmp->LOCK_thd_data);	// Lock from delete
> > +      break;
> > +    }
> > +  }
> > +  mysql_mutex_unlock(&LOCK_thread_count);
> > +
> > +  if (tmp)
> > +  {
> > +    Security_context *tmp_sctx= tmp->security_ctx;
> > +    /*
> > +      If calling_user==NULL, calling thread has SUPER or PROCESS
> > +      privilege, and so can do SHOW EXPLAIN on any user.
> > +      
> > +      if calling_user!=NULL, he's only allowed to view SHOW EXPLAIN on
> > +      his own threads.
> > +    */
> > +    if (calling_user && (!tmp_sctx->user || strcmp(calling_user, 
> > +                                                   tmp_sctx->user)))
> > +    {
> > +      my_error(ER_SPECIFIC_ACCESS_DENIED_ERROR, MYF(0), "PROCESSLIST");
> 
> s/PROCESSLIST/PROCESS/
> the name of the privilege is "PROCESS"
> 
psergey: Changed.

> > +      mysql_mutex_unlock(&tmp->LOCK_thd_data);
> > +      DBUG_RETURN(1);
> > +    }
> > +
> > +    if (tmp == thd)
> > +    {
> > +      mysql_mutex_unlock(&tmp->LOCK_thd_data);
> > +      my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), "SHOW EXPLAIN",
> > +               target_not_explainable_cmd);
> 
> Why ER_ERROR_WHEN_EXECUTING_COMMAND? I mean, generally when an error X
> happens, the error message says "X", not "Error when executing command: X".
> Like, "Unknown storage engine FOO", and not
> "Error when executing command CREATE TABLE: Unknown storage engine FOO"
> So, why do you prefer to use ER_ERROR_WHEN_EXECUTING_COMMAND here?

psergey:
"target not EXPLAINable" error is special because it can only occur when one
is running SHOW EXPLAIN.

> If you wouldn't, you could've simply create ER_TARGET_NOT_EXPLAINABLE
> and write my_error(ER_TARGET_NOT_EXPLAINABLE, MYF(0))
>
psergey:
Introduced ER_TARGET_NOT_EXPLAINABLE, changed the code to use this error.

> > +      DBUG_RETURN(1);
> > +    }
> > +
> > +    bool bres;
> > +    /* 
> > +      Ok we've found the thread of interest and it won't go away because 
> > +      we're holding its LOCK_thd data. Post it a SHOW EXPLAIN request.
> > +    */
> > +    bool timed_out;
> > +    int timeout_sec= 30;
> > +    Show_explain_request explain_req;
> > +    select_result_explain_buffer *explain_buf;
> > +    
> > +    explain_buf= new select_result_explain_buffer(thd, table->table);
> > +
> > +    explain_req.explain_buf= explain_buf;
> > +    explain_req.target_thd= tmp;
> > +    explain_req.request_thd= thd;
> > +    explain_req.failed_to_produce= FALSE;
> > +    
> > +    /* Ok, we have a lock on target->LOCK_thd_data, can call: */
> > +    bres= tmp->apc_target.make_apc_call(thd, &explain_req, timeout_sec, &timed_out);
> > +
> > +    if (bres || explain_req.failed_to_produce)
> > +    {
> > +      if (thd->killed)
> > +      {
> > +        thd->send_kill_message();
> > +      }
> > +      else 
> > +      if (timed_out)
> > +      {
> > +        my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), 
> > +                 "SHOW EXPLAIN",
> > +                 "Timeout");
> 
> Here you could've used ER_LOCK_WAIT_TIMEOUT
> (esp. if we remove "transaction" from the error message text)

psergey:
Changed to use ER_LOCK_WAIT_TIMEOUT. I didn't change the message text. 

> 
> > +      }
> > +      else
> > +      {
> > +        my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), 
> > +                 "SHOW EXPLAIN", target_not_explainable_cmd);
> > +      }
> > +      bres= TRUE;
> > +    }
> > +    else
> > +    {
> > +      push_warning(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
> > +                   ER_YES, explain_req.query_str.c_ptr_safe());
> > +    }
> > +    DBUG_RETURN(bres);
> > +  }
> > +  else
> > +  {
> > +    my_error(ER_NO_SUCH_THREAD, MYF(0), thread_id);
> > +    DBUG_RETURN(1);
> > +  }
> > +}
> > +
> > +
> >  int fill_schema_processlist(THD* thd, TABLE_LIST* tables, COND* cond)
> >  {
> >    TABLE *table= tables->table;
> > === added file 'sql/my_apc.cc'
> > --- sql/my_apc.cc	1970-01-01 00:00:00 +0000
> > +++ sql/my_apc.cc	2012-07-10 17:23:00 +0000
> > @@ -0,0 +1,302 @@
> > +/*
> > +   Copyright (c) 2011 - 2012, Monty Program Ab
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; version 2 of the License.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, write to the Free Software
> > +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
> > +
> > +
> > +#ifndef MY_APC_STANDALONE
> > +
> > +#include "sql_priv.h"
> > +#include "sql_class.h"
> > +
> > +#endif
> > +
> > +/* For standalone testing of APC system, see unittest/sql/my_apc-t.cc */
> > +
> > +#ifndef MY_APC_STANDALONE
> > +
> > +ST_FIELD_INFO show_explain_fields_info[]=
> > +{
> > +  /* field_name, length, type, value, field_flags, old_name*/
> > +  {"id", 3, MYSQL_TYPE_LONGLONG, 0 /*value*/, MY_I_S_MAYBE_NULL, "id", 
> > +    SKIP_OPEN_TABLE},
> > +  {"select_type", 19, MYSQL_TYPE_STRING, 0 /*value*/, 0, "select_type", 
> > +    SKIP_OPEN_TABLE},
> > +  {"table", NAME_CHAR_LEN, MYSQL_TYPE_STRING, 0 /*value*/, MY_I_S_MAYBE_NULL,
> > +   "table", SKIP_OPEN_TABLE},
> > +  {"type", 10, MYSQL_TYPE_STRING, 0, MY_I_S_MAYBE_NULL, "type", SKIP_OPEN_TABLE},
> > +  {"possible_keys", NAME_CHAR_LEN*MAX_KEY, MYSQL_TYPE_STRING, 0/*value*/,
> > +    MY_I_S_MAYBE_NULL, "possible_keys", SKIP_OPEN_TABLE},
> > +  {"key", NAME_CHAR_LEN, MYSQL_TYPE_STRING, 0/*value*/, MY_I_S_MAYBE_NULL, 
> > +   "key", SKIP_OPEN_TABLE},
> > +  {"key_len", NAME_CHAR_LEN*MAX_KEY, MYSQL_TYPE_STRING, 0/*value*/, 
> > +    MY_I_S_MAYBE_NULL, "key_len", SKIP_OPEN_TABLE},
> > +  {"ref", NAME_CHAR_LEN*MAX_REF_PARTS, MYSQL_TYPE_STRING, 0/*value*/,
> > +    MY_I_S_MAYBE_NULL, "ref", SKIP_OPEN_TABLE},
> > +  {"rows", 10, MYSQL_TYPE_LONGLONG, 0/*value*/, MY_I_S_MAYBE_NULL, "rows", 
> > +    SKIP_OPEN_TABLE},
> > +  {"Extra", 255, MYSQL_TYPE_STRING, 0/*value*/, 0 /*flags*/, "Extra", 
> > +    SKIP_OPEN_TABLE},
> > +  {0, 0, MYSQL_TYPE_STRING, 0, 0, 0, SKIP_OPEN_TABLE}
> > +};
> 
> This certainly belongs to sql_show.cc
> 
psergey:
I fail to understand the logic. Should all List<XXX> definitions be in
sql_list.cc, also?

But I have moved it to sql_show.cc.

> > +
> > +
> > +#endif
> > +/* 
> > +  Initialize the target. 
> > +   
> > +  @note 
> > +  Initialization must be done prior to enabling/disabling the target, or making
> > +  any call requests to it.
> > +  Initial state after initialization is 'disabled'.
> > +*/
> > +void Apc_target::init(mysql_mutex_t *target_mutex)
> > +{
> > +  DBUG_ASSERT(!enabled);
> > +  LOCK_thd_data_ptr= target_mutex;
> > +#ifndef DBUG_OFF
> > +  n_calls_processed= 0;
> > +#endif
> > +}
> > +
> > +
> > +/* 
> > +  Destroy the target. The target must be disabled when this call is made.
> > +*/
> > +void Apc_target::destroy()
> > +{
> > +  DBUG_ASSERT(!enabled);
> > +}
> > +
> > +
> > +/* 
> > +  Enter ther state where the target is available for serving APC requests
> > +*/
> > +void Apc_target::enable()
> > +{
> > +  /* Ok to do without getting/releasing the mutex: */
> > +  enabled++;
> > +}
> 
> My comment in the first review was
> 
>   2. I don't see why you need enable/disable calls at all.
>   You use that to disable SHOW EXPLAIN for certain parts of the code,
>   but this is wrong. Apc is not SHOW EXPLAIN, you should not disable the
>   whole apc mechanism only because SHOW EXPLAIN cannot be served.
>   Instead the show-explain-generating callback function should
>   detect when it can and cannot be used and react appropriately.
> 
> And your reply was
> 
>   (as discussed on the phone) will try to
>   - get the APC mechanism to work always, including ability to wake up
>   threads that are sleeping/waiting.
>   - EXPLAIN will check whether the APC is available in the context of the
>   target thread.

psergey:
But haven't we later come to conclusion that it is not currently possible to
implement? 

As we have discussed with Wlad before:
- wakeup is non-trivial with thread pool: since there is no one-to-one mapping
  between THDs and threads, you'll need to find out which thread to wake up, 
  then wake it up, switch it to the THD that you wanted to wake up, etc.

- wakeup of thread that's waiting on network IO under windows+thread pool is 
  possible, but not trivial,

- wakup of thread that's waiting on network IO under windows with no thread
  pool is not currently possible,

Hence, we have decided not to implement always-wake-up system.

Your review is trying to pull stuff in different directions. On one hand,
INFORMATION_SCHEMA support is addedfor querying EXPLAINs from many threads. 
On the other hand, you want APC system to do the basic check of whether EXPLAIN
can be produced in the context of the target thread. This way,
  
  SELECT * FROM information_schema.explain

will need to switch context to every thread on the server. Does this make any
sense?

^^ (why-apc-enable-disable) is described above.

> > +
> > +
> > +/* 
> > +  Make the target unavailable for serving APC requests. 
> > +  
> > +  @note
> > +    This call will serve all requests that were already enqueued
> > +*/
> > +
> > +void Apc_target::disable()
> > +{
> > +  bool process= FALSE;
> > +  mysql_mutex_lock(LOCK_thd_data_ptr);
> > +  if (!(--enabled))
> > +    process= TRUE;
> > +  mysql_mutex_unlock(LOCK_thd_data_ptr);
> > +  if (process)
> > +    process_apc_requests();
> > +}
> > +
> > +
> > +/* [internal] Put request qe into the request list */
> > +
> > +void Apc_target::enqueue_request(Call_request *qe)
> > +{
> > +  mysql_mutex_assert_owner(LOCK_thd_data_ptr);
> > +  if (apc_calls)
> > +  {
> > +    Call_request *after= apc_calls->prev;
> > +    qe->next= apc_calls;
> > +    apc_calls->prev= qe;
> > +     
> > +    qe->prev= after;
> > +    after->next= qe;
> > +  }
> > +  else
> > +  {
> > +    apc_calls= qe;
> > +    qe->next= qe->prev= qe;
> > +  }
> > +}
> > +
> > +
> > +/* 
> > +  [internal] Remove request qe from the request queue. 
> > +  
> > +  The request is not necessarily first in the queue.
> > +*/
> > +
> > +void Apc_target::dequeue_request(Call_request *qe)
> > +{
> > +  mysql_mutex_assert_owner(LOCK_thd_data_ptr);
> > +  if (apc_calls == qe)
> > +  {
> > +    if ((apc_calls= apc_calls->next) == qe)
> > +    {
> > +      apc_calls= NULL;
> > +    }
> > +  }
> > +
> > +  qe->prev->next= qe->next;
> > +  qe->next->prev= qe->prev;
> > +}
> > +
> > +#ifdef HAVE_PSI_INTERFACE
> > +
> > +/* One key for all conds */
> > +PSI_cond_key key_show_explain_request_COND;
> > +
> > +static PSI_cond_info show_explain_psi_conds[]=
> > +{
> > +  { &key_show_explain_request_COND, "show_explain", 0 /* not using PSI_FLAG_GLOBAL*/ }
> > +};
> > +
> > +void init_show_explain_psi_keys(void)
> > +{
> > +  if (PSI_server == NULL)
> > +    return;
> > +
> > +  PSI_server->register_cond("sql", show_explain_psi_conds, 
> > +                            array_elements(show_explain_psi_conds));
> > +}
> > +#endif
> > +
> > +
> > +/*
> > +  Make an APC (Async Procedure Call) to another thread. 
> > + 
> > +  @detail
> > +  Make an APC call: schedule it for execution and wait until the target
> > +  thread has executed it. 
> > +
> > +  - The caller is responsible for making sure he's not posting request
> > +    to the thread he's calling this function from.
> > +
> > +  - The caller must have locked target_mutex. The function will release it.
> > +
> > +  @retval FALSE - Ok, the call has been made
> > +  @retval TRUE  - Call wasnt made (either the target is in disabled state or
> > +                    timeout occured)
> > +*/
> > +
> > +bool Apc_target::make_apc_call(THD *caller_thd, Apc_call *call, 
> > +                               int timeout_sec, bool *timed_out)
> > +{
> > +  bool res= TRUE;
> > +  *timed_out= FALSE;
> > +
> > +  if (enabled)
> > +  {
> > +    /* Create and post the request */
> > +    Call_request apc_request;
> > +    apc_request.call= call;
> > +    apc_request.processed= FALSE;
> > +    mysql_cond_init(key_show_explain_request_COND, &apc_request.COND_request,
> > +                    NULL);
> > +    enqueue_request(&apc_request);
> > +    apc_request.what="enqueued by make_apc_call";
> 
> perhaps all that (above) belongs to the Call_request constructor?
> 
psergey:
Not really. Call_request has no methods. The code has exactly one place where
Call_request objects are constructed. I see no need for a constructor.

> > + 
> > +    struct timespec abstime;
> > +    const int timeout= timeout_sec;
> > +    set_timespec(abstime, timeout);
> > +
> > +    int wait_res= 0;
> > +    const char *old_msg;
> > +    old_msg= caller_thd->enter_cond(&apc_request.COND_request, 
> > +                                    LOCK_thd_data_ptr, "show_explain");
> > +    /* todo: how about processing other errors here? */
> > +    while (!apc_request.processed && (wait_res != ETIMEDOUT))
> > +    {
> > +      /* We own LOCK_thd_data_ptr */
> > +      wait_res= mysql_cond_timedwait(&apc_request.COND_request,
> > +                                     LOCK_thd_data_ptr, &abstime);
> > +                                      // &apc_request.LOCK_request, &abstime);
> > +      if (caller_thd->killed)
> > +        break;
> > +    }
> > +
> > +    if (!apc_request.processed)
> > +    {
> > +      /* 
> > +        The wait has timed out, or this thread was KILLed.
> > +        Remove the request from the queue (ok to do because we own
> > +        LOCK_thd_data_ptr)
> > +      */
> > +      apc_request.processed= TRUE;
> > +      dequeue_request(&apc_request);
> > +      *timed_out= TRUE;
> > +      res= TRUE;
> > +    }
> > +    else
> > +    {
> > +      /* Request was successfully executed and dequeued by the target thread */
> > +      res= FALSE;
> > +    }
> > +    /* 
> > +      exit_cond() will call mysql_mutex_unlock(LOCK_thd_data_ptr) for us:
> > +    */
> > +    caller_thd->exit_cond(old_msg);
> > +
> > +    /* Destroy all APC request data */
> > +    mysql_cond_destroy(&apc_request.COND_request);
> > +  }
> > +  else
> > +  {
> > +    mysql_mutex_unlock(LOCK_thd_data_ptr);
> > +  }
> > +  return res;
> > +}
> > +
> > +
> > +/*
> > +  Process all APC requests.
> > +  This should be called periodically by the APC target thread.
> > +*/
> > +
> > +void Apc_target::process_apc_requests()
> > +{
> > +  if (!get_first_in_queue())
> > +    return;
> 
> You don't need that anymore, because the caller does
> have_requests() first. So, process_apc_requests() is a slow
> function, which does not need shortcuts like that.
>
psergey: Removed it.

> > +
> > +  while (1)
> > +  {
> > +    Call_request *request;
> > + 
> > +    mysql_mutex_lock(LOCK_thd_data_ptr);
> > +    if (!(request= get_first_in_queue()))
> > +    {
> > +      /* No requests in the queue */
> > +      mysql_mutex_unlock(LOCK_thd_data_ptr);
> > +      break;
> > +    }
> > +
> > +    /* 
> > +      Remove the request from the queue (we're holding queue lock so we can be 
> > +      sure that request owner won't try to remove it)
> > +    */
> > +    request->what="dequeued by process_apc_requests";
> > +    dequeue_request(request);
> > +    request->processed= TRUE;
> > +
> > +    request->call->call_in_target_thread();
> > +    request->what="func called by process_apc_requests";
> > +
> > +#ifndef DBUG_OFF
> > +    n_calls_processed++;
> > +#endif
> > +    mysql_cond_signal(&request->COND_request);
> > +    mysql_mutex_unlock(LOCK_thd_data_ptr);
> > +  }
> > +}
> > +
> > === added file 'sql/my_apc.h'
> > --- sql/my_apc.h	1970-01-01 00:00:00 +0000
> > +++ sql/my_apc.h	2012-07-11 09:39:56 +0000
> > @@ -0,0 +1,134 @@
> > +/*
> > +   Copyright (c) 2011 - 2012, Monty Program Ab
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; version 2 of the License.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, write to the Free Software
> > +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
> > +
> > +/*
> > +  Interface
> > +  ~~~~~~~~~
> > +   (
> > +    - This is an APC request queue
> > +    - We assume there is a particular owner thread which periodically calls
> > +      process_apc_requests() to serve the call requests.
> > +    - Other threads can post call requests, and block until they are exectued.
> > +  )
> > +
> > +  Implementation
> > +  ~~~~~~~~~~~~~~
> > +  - The target has a mutex-guarded request queue.
> > +
> > +  - After the request has been put into queue, the requestor waits for request
> > +    to be satisfied. The worker satisifes the request and signals the
> > +    requestor.
> > +*/
> > +
> > +class THD;
> > +
> > +/*
> > +  Target for asynchronous procedure calls (APCs). 
> > +   - A target is running in some particular thread, 
> > +   - One can make calls to it from other threads.
> > +*/
> > +class Apc_target
> > +{
> > +  mysql_mutex_t *LOCK_thd_data_ptr;
> > +public:
> > +  Apc_target() : enabled(0), apc_calls(NULL) {} 
> > +  ~Apc_target() { DBUG_ASSERT(!enabled && !apc_calls);}
> > +
> > +  void init(mysql_mutex_t *target_mutex);
> > +  void destroy();
> > +  void enable();
> > +  void disable();
> > +  
> > +  void process_apc_requests();
> > +  /* 
> > +    A lightweight function, intended to be used in frequent checks like this:
> > +
> > +      if (apc_target.have_requests()) apc_target.process_apc_requests()
> > +  */
> > +  inline bool have_apc_requests()
> > +  {
> > +    return test(apc_calls);
> > +  }
> > +  
> > +  /* Functor class for calls you can schedule */
> > +  class Apc_call
> > +  {
> > +  public:
> > +    /* This function will be called in the target thread */
> > +    virtual void call_in_target_thread()= 0;
> > +    virtual ~Apc_call() {}
> > +  };
> > +  
> > +  /* Make a call in the target thread (see function definition for details) */
> > +  bool make_apc_call(THD *caller_thd, Apc_call *call, int timeout_sec, bool *timed_out);
> 
> Ok. strictly speaking, you could've put caller_thd, timeout_sec, and
> timed_out into the Apc_call class. But you did not. Why?
> 
> But you did not. Why?
psergey:
Short answer: because I don't care.

Long answer: 
What's the benefit of putting timeout_sec, caller_thd into Apc_call class? If
you remember, we wanted a Functor class, and Apc_call is exactly that. 
Other members are distinct parameters to make_apc_call() function.

I'm sorry, I do not understand the point of "Why did you not do X" questions. 
There are nearly infinitely many things I could have done, but I needed to 
produce exactly one patch, and that required me to stop at some variant. 
I stopped at a C-type function call. Per your request, I changed it to a C++ 
functor. What else do you need?

My idea of code reviews is that their goal is to point out *problems* in the
code.  There is no goal to perform an exhaustive consideration of all 
possible ways something could be coded.

Does having four parameters to make_apc_call() function cause any defect? If
yes, could you please explicitly mention it?


> (a possible answer could be "this makes Apc_call independent
> from the thread and allows to send the same Apc_call object
> with many make_apc_call() methods to different threads" -
> but I don't know if that's the case, the above is just an example
> of a possible reasoning)
> 
> > +
> > +#ifndef DBUG_OFF
> > +  int n_calls_processed; /* Number of calls served by this target */
> > +#endif
> > +private:
> > +  class Call_request;
> > +
> > +  /* 
> > +    Non-zero value means we're enabled. It's an int, not bool, because one can
> > +    call enable() N times (and then needs to call disable() N times before the 
> > +    target is really disabled)
> > +  */
> > +  int enabled;
> > +
> > +  /* 
> > +    Circular, double-linked list of all enqueued call requests. 
> > +    We use this structure, because we 
> > +     - process requests sequentially: requests are added at the end of the 
> > +       list and removed from the front. With circular list, we can keep one
> > +       pointer.
> > +     - a thread that has posted a request may time out (or be KILLed) and 
> > +       cancel the request, which means we need a fast request-removal
> > +       operation.
> > +  */
> > +  Call_request *apc_calls;
> > + 
> > +  class Call_request
> > +  {
> > +  public:
> > +    Apc_call *call; /* Functor to be called */
> > +
> > +    /* The caller will actually wait for "processed==TRUE" */
> > +    bool processed;
> > +
> > +    /* Condition that will be signalled when the request has been served */
> > +    mysql_cond_t COND_request;
> > +    
> > +    /* Double linked-list linkage */
> > +    Call_request *next;
> > +    Call_request *prev;
> > +    
> > +    const char *what; /* (debug) state of the request */
> > +  };
> > +
> > +  void enqueue_request(Call_request *qe);
> > +  void dequeue_request(Call_request *qe);
> > +
> > +  /* return the first call request in queue, or NULL if there are none enqueued */
> > +  Call_request *get_first_in_queue()
> > +  {
> > +    return apc_calls;
> > +  }
> > +};
> > +
> > +#ifdef HAVE_PSI_INTERFACE
> > +void init_show_explain_psi_keys(void);
> > +#endif
> > +
> > === added file 'unittest/sql/my_apc-t.cc'
> > --- unittest/sql/my_apc-t.cc	1970-01-01 00:00:00 +0000
> > +++ unittest/sql/my_apc-t.cc	2012-07-05 18:04:13 +0000
> > @@ -0,0 +1,227 @@
> > +/*
> > +   Copyright (c) 2012, Monty Program Ab
> > +
> > +   This program is free software; you can redistribute it and/or modify
> > +   it under the terms of the GNU General Public License as published by
> > +   the Free Software Foundation; version 2 of the License.
> > +
> > +   This program is distributed in the hope that it will be useful,
> > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +   GNU General Public License for more details.
> > +
> > +   You should have received a copy of the GNU General Public License
> > +   along with this program; if not, write to the Free Software
> > +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
> > +
> > +/*
> > +  This file does standalone APC system tests.
> > +*/
> > +#include <stdio.h>
> > +#include <my_global.h>
> > +#include <my_pthread.h>
> > +#include <my_sys.h>
> > +
> > +#include <tap.h>
> > +
> > +/*
> > +  A fake THD with enter_cond/exit_cond and some other members.
> > +*/
> > +class THD 
> > +{
> > +  mysql_mutex_t* thd_mutex; 
> > +public:
> > +  bool killed;
> > +
> > +  THD() : killed(FALSE) {}
> > +  inline const char* enter_cond(mysql_cond_t *cond, mysql_mutex_t* mutex,
> > +                                const char* msg)
> > +  {
> > +    mysql_mutex_assert_owner(mutex);
> > +    thd_mutex= mutex;
> > +    return NULL;
> > +  }
> > +  inline void exit_cond(const char* old_msg)
> > +  {
> > +    mysql_mutex_unlock(thd_mutex);
> > +  }
> > +};
> > +
> > +#include "../sql/my_apc.h"
> > +
> > +#define MY_APC_STANDALONE 1
> > +#include "../sql/my_apc.cc"
> > +
> > +volatile bool started= FALSE;
> > +volatile bool service_should_exit= FALSE;
> > +volatile bool requestors_should_exit=FALSE;
> > +
> > +/*  Counters for APC calls */
> > +int apcs_served= 0;
> > +int apcs_missed=0;
> > +int apcs_timed_out=0;
> > +mysql_mutex_t apc_counters_mutex;
> > +
> > +inline void increment_counter(int *var)
> > +{
> > +  mysql_mutex_lock(&apc_counters_mutex);
> > +  *var= *var+1;
> > +  mysql_mutex_unlock(&apc_counters_mutex);
> > +}
> > +
> > +volatile bool have_errors= false;
> > +
> > +Apc_target apc_target;
> > +mysql_mutex_t target_mutex;
> > +
> > +int int_rand(int size)
> > +{
> > +  return (int) (0.5 + ((double)rand() / RAND_MAX) * size);
> > +}
> > +
> > +/* 
> > +  APC target thread (the one that will serve the APC requests). We will have
> > +  one target.
> > +*/
> > +void *test_apc_service_thread(void *ptr)
> > +{
> > +  my_thread_init();
> > +  mysql_mutex_init(0, &target_mutex, MY_MUTEX_INIT_FAST);
> > +  apc_target.init(&target_mutex);
> > +  apc_target.enable();
> > +  started= TRUE;
> > +  fprintf(stderr, "# test_apc_service_thread started\n");
> > +  while (!service_should_exit)
> > +  {
> > +    //apc_target.disable();
> > +    my_sleep(10000);
> > +    //apc_target.enable();
> > +    for (int i = 0; i < 10 && !service_should_exit; i++)
> > +    {
> > +      apc_target.process_apc_requests();
> > +      my_sleep(int_rand(30));
> > +    }
> > +  }
> > +  apc_target.disable();
> > +  apc_target.destroy();
> > +  mysql_mutex_destroy(&target_mutex);
> > +  my_thread_end();
> > +  pthread_exit(0);
> > +  return NULL;
> > +}
> > +
> > +
> > +/*
> > +  One APC request (to write 'value' into *where_to)
> > +*/
> > +class Apc_order : public Apc_target::Apc_call
> > +{
> > +public:
> > +  int value;   // The value 
> > +  int *where_to;  // Where to write it
> > +  Apc_order(int a, int *b) : value(a), where_to(b) {}
> > +
> > +  void call_in_target_thread()
> > +  {
> > +    my_sleep(int_rand(1000));
> > +    *where_to = value;
> > +    increment_counter(&apcs_served);
> > +  }
> > +};
> > +
> > +
> > +/*
> > +  APC requestor thread. It makes APC requests, and checks if they were actually
> > +  executed.
> > +*/
> > +void *test_apc_requestor_thread(void *ptr)
> > +{
> > +  my_thread_init();
> > +  fprintf(stderr, "# test_apc_requestor_thread started\n");
> > +  THD my_thd;
> > +
> > +  while (!requestors_should_exit)
> > +  {
> > +    int dst_value= 0;
> > +    int src_value= int_rand(4*1000*100);
> > +    /* Create an APC to do "dst_value= src_value" assignment */
> > +    Apc_order apc_order(src_value, &dst_value);
> > +    bool timed_out;
> > +
> > +    mysql_mutex_lock(&target_mutex);
> > +    bool res= apc_target.make_apc_call(&my_thd, &apc_order, 60, &timed_out);
> > +    if (res)
> > +    {
> > +      if (timed_out)
> > +        increment_counter(&apcs_timed_out);
> > +      else
> > +        increment_counter(&apcs_missed);
> > +
> > +      if (dst_value != 0)
> > +      {
> > +        fprintf(stderr, "APC was done even though return value says it wasnt!\n");
> > +        have_errors= true;
> > +      }
> > +    }
> > +    else
> > +    {
> > +      if (dst_value != src_value)
> > +      {
> > +        fprintf(stderr, "APC was not done even though return value says it was!\n");
> > +        have_errors= true;
> > +      }
> > +    }
> > +    //my_sleep(300);
> > +  }
> > +  fprintf(stderr, "# test_apc_requestor_thread exiting\n");
> > +  my_thread_end();
> > +  return NULL;
> > +}
> > +
> > +/* Number of APC requestor threads */
> > +const int N_THREADS=23;
> > +
> > +
> > +int main(int args, char **argv)
> > +{
> > +  pthread_t service_thr;
> > +  pthread_t request_thr[N_THREADS];
> > +  int i;
> > +
> > +  my_thread_global_init();
> > +
> > +  mysql_mutex_init(0, &apc_counters_mutex, MY_MUTEX_INIT_FAST);
> > +
> > +  plan(1);
> > +  diag("Testing APC delivery and execution");
> > +
> > +  pthread_create(&service_thr, NULL, test_apc_service_thread, (void*)NULL);
> > +  while (!started)
> > +    my_sleep(1000);
> > +  for (i = 0; i < N_THREADS; i++)
> > +    pthread_create(&request_thr[i], NULL, test_apc_requestor_thread, (void*)NULL);
> > +  
> > +  for (i = 0; i < 15; i++)
> > +  {
> > +    my_sleep(500*1000);
> > +    fprintf(stderr, "# %d APCs served %d missed\n", apcs_served, apcs_missed);
> > +  }
> > +  fprintf(stderr, "# Shutting down requestors\n");
> > +  requestors_should_exit= TRUE;
> > +  for (i = 0; i < N_THREADS; i++)
> > +    pthread_join(request_thr[i], NULL);
> > +  
> > +  fprintf(stderr, "# Shutting down service\n");
> > +  service_should_exit= TRUE;
> > +  pthread_join(service_thr, NULL);
> > +
> > +  mysql_mutex_destroy(&apc_counters_mutex);
> > +
> > +  fprintf(stderr, "# Done.\n");
> 
> why fprintf() instead of diag() ?
> 
psergey: Changed to diag().

> > +  my_thread_end();
> > +  my_thread_global_end();
> > +
> > +  ok1(!have_errors);
> > +  return exit_status();
> > +}

BR
 Sergei
-- 
Sergei Petrunia, Software Developer
Monty Program AB, http://askmonty.org
Blog: http://s.petrunia.net/blog