maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Sergei!

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?

> +#
> +#
> === 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?

> +  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 ?

>  #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?

> +
> +  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)

> +
> +
>  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

> +{
> +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?

>    /* 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.

> +    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.  Actually, it'd be enough to make thd->killed
private, no need to rename it

>      {
>        /* 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

>    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.

> +      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.

> +      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.

> +  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().
And also - you didn't remove apc_target.enable/disable, as we've discussed.

> +
> +  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?

>        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?

>        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?

>    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?

> +  */
> +  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"

> +      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?
If you wouldn't, you could've simply create ER_TARGET_NOT_EXPLAINABLE
and write my_error(ER_TARGET_NOT_EXPLAINABLE, MYF(0))

> +      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)

> +      }
> +      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

> +
> +
> +#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.

> +
> +
> +/* 
> +  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?

> + 
> +    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.

> +
> +  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?

(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() ?

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

Regards,
Sergei