MWL#182: Explain running statements

- Code cleanup

6 files changed, 96 insertions, 42 deletions

diff --git a/sql/my_apc.cc b/sql/my_apc.cc
index 91559483b1f..58290ece56b 100644
--- a/sql/my_apc.cc
+++ b/sql/my_apc.cc
@@ -24,6 +24,14 @@
 */
 
 
+/* 
+  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()
 {
   // todo: should use my_pthread_... functions instead?
@@ -36,6 +44,9 @@ void Apc_target::init()
 }
 
 
+/* 
+  Destroy the target. The target must be disabled when this call is made.
+*/
 void Apc_target::destroy()
 {
   DBUG_ASSERT(!enabled);
@@ -43,6 +54,9 @@ void Apc_target::destroy()
 }
 
 
+/* 
+  Enter ther state where the target is available for serving APC requests
+*/
 void Apc_target::enable()
 {
   pthread_mutex_lock(&LOCK_apc_queue);
@@ -51,6 +65,13 @@ void Apc_target::enable()
 }
 
 
+/* 
+  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;
@@ -62,6 +83,9 @@ void Apc_target::disable()
     process_apc_requests();
 }
 
+
+/* (internal) Put request into the request list */
+
 void Apc_target::enqueue_request(Call_request *qe)
 {
   //call_queue_size++;
@@ -81,6 +105,13 @@ void Apc_target::enqueue_request(Call_request *qe)
   }
 }
 
+
+/* 
+  (internal) Remove given request from the request queue. 
+  
+  The request is not necessarily first in the queue.
+*/
+
 void Apc_target::dequeue_request(Call_request *qe)
 {
   //call_queue_size--;
@@ -99,8 +130,10 @@ void Apc_target::dequeue_request(Call_request *qe)
 
 
 /*
-  Make an apc call in another thread. The caller is responsible so 
-  that we're not calling to ourselves.
+  Make an APC (Async Procedure Call) in another thread. 
+
+  The caller is responsible for making sure he's not calling an Apc_target 
+  that is serviced by the same thread it is called from.
   
   psergey-todo: Should waits here be KILLable? (it seems one needs 
   to use thd->enter_cond() calls to be killable)
@@ -119,7 +152,7 @@ bool Apc_target::make_apc_call(apc_func_t func, void *func_arg,
     Call_request apc_request;
     apc_request.func= func;
     apc_request.func_arg= func_arg;
-    apc_request.done= FALSE;
+    apc_request.processed= FALSE;
     (void)pthread_cond_init(&apc_request.COND_request, NULL);
     (void)pthread_mutex_init(&apc_request.LOCK_request, MY_MUTEX_INIT_SLOW);
     pthread_mutex_lock(&apc_request.LOCK_request);
@@ -133,19 +166,19 @@ bool Apc_target::make_apc_call(apc_func_t func, void *func_arg,
     
     int wait_res= 0;
     /* todo: how about processing other errors here? */
-    while (!apc_request.done && (wait_res != ETIMEDOUT))
+    while (!apc_request.processed && (wait_res != ETIMEDOUT))
     {
       wait_res= pthread_cond_timedwait(&apc_request.COND_request,
                                        &apc_request.LOCK_request, &abstime);
     }
 
-    if (!apc_request.done)
+    if (!apc_request.processed)
     {
-      /* We timed out */
-      apc_request.done= TRUE;
+      /* The wait has timed out. Remove the request from the queue */
+      apc_request.processed= TRUE;
       *timed_out= TRUE;
       pthread_mutex_unlock(&apc_request.LOCK_request);
-
+      //psergey-todo: "Whoa rare event" refers to this part, right? put a comment.
       pthread_mutex_lock(&LOCK_apc_queue);
       dequeue_request(&apc_request);
       pthread_mutex_unlock(&LOCK_apc_queue);
@@ -171,7 +204,8 @@ bool Apc_target::make_apc_call(apc_func_t func, void *func_arg,
 
 
 /*
-  Process all APC requests
+  Process all APC requests.
+  This should be called periodically by the APC target thread.
 */
 
 void Apc_target::process_apc_requests()
@@ -190,7 +224,7 @@ void Apc_target::process_apc_requests()
     request->what="seen by process_apc_requests";
     pthread_mutex_lock(&request->LOCK_request);
 
-    if (request->done)
+    if (request->processed)
     {
       /*
         We can get here when
@@ -214,7 +248,7 @@ void Apc_target::process_apc_requests()
     */
     request->what="dequeued by process_apc_requests";
     dequeue_request(request);
-    request->done= TRUE;
+    request->processed= TRUE;
 
     pthread_mutex_unlock(&LOCK_apc_queue);
 
diff --git a/sql/my_apc.h b/sql/my_apc.h
index 3906aa24408..0698703ad40 100644
--- a/sql/my_apc.h
+++ b/sql/my_apc.h
@@ -3,9 +3,18 @@
 */
 
 /*
-  Design
-  - Mutex-guarded request queue (it belongs to the target), which can be enabled/
-    disabled (when empty).
+  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
@@ -21,31 +30,11 @@ public:
   Apc_target() : enabled(0), apc_calls(NULL) /*, call_queue_size(0)*/ {} 
   ~Apc_target() { DBUG_ASSERT(!enabled && !apc_calls);}
 
-  /* 
-    Initialize the target. This must be called before anything else. Right
-    after initialization, the target is disabled.
-  */
   void init();
-
-  /* 
-    Destroy the target. The target must be disabled when this call is made.
-  */
   void destroy();
-  
-  /* 
-    Enter into state where this target will be serving APC requests
-  */
   void enable();
-
-  /* 
-    Leave the state where we could serve APC requests (will serve all already 
-    enqueued requests)
-  */
   void disable();
   
-  /*
-    This should be called periodically to serve observation requests.
-  */
   void process_apc_requests();
 
   typedef void (*apc_func_t)(void *arg);
@@ -68,18 +57,32 @@ public:
 #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 
+     - a thread that has posted a request may time out (or be KILLed) and 
+       cancel the request, which means we'll need to remove its request at 
+       arbitrary point in time.
+  */
   Call_request *apc_calls;
-  pthread_mutex_t LOCK_apc_queue;
 
+  pthread_mutex_t LOCK_apc_queue;
 
   class Call_request
   {
   public:
-    apc_func_t func;
-    void *func_arg;
-    bool done;
+    apc_func_t func; /* Function to call */
+    void *func_arg;  /* Argument to pass it */
+    bool processed;
 
     pthread_mutex_t LOCK_request;
     pthread_cond_t COND_request;
@@ -87,13 +90,15 @@ private:
     Call_request *next;
     Call_request *prev;
     
-    const char *what;
+    const char *what; /* 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;
   }
 };
diff --git a/sql/sql_class.cc b/sql/sql_class.cc
index d6f976cb505..545c9aff5e9 100644
--- a/sql/sql_class.cc
+++ b/sql/sql_class.cc
@@ -3033,7 +3033,8 @@ void Show_explain_request::get_explain_data(void *arg)
   req->target_thd->set_n_backup_active_arena((Query_arena*)req->request_thd,
                                              &backup_arena);
 
-  req->target_thd->lex->unit.print_explain(req->explain_buf);
+  if (req->target_thd->lex->unit.print_explain(req->explain_buf))
+    req->failed_to_produce= TRUE;
 
   req->target_thd->restore_active_arena((Query_arena*)req->request_thd, 
                                         &backup_arena);
diff --git a/sql/sql_class.h b/sql/sql_class.h
index 85f5d35f3d2..de7c8de6c26 100644
--- a/sql/sql_class.h
+++ b/sql/sql_class.h
@@ -1467,6 +1467,8 @@ public:
   THD *target_thd;
   THD *request_thd;
   
+  bool failed_to_produce;
+  
   select_result_explain_buffer *explain_buf;
 
   static void get_explain_data(void *arg);
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
index 544246a1a4d..661ca2b4383 100644
--- a/sql/sql_lex.cc
+++ b/sql/sql_lex.cc
@@ -3792,6 +3792,15 @@ int st_select_lex_unit::print_explain(select_result_sink *output)
 {
   int res= 0;
   SELECT_LEX *first= first_select();
+  
+  if (first && !first->next_select() && !first->join)
+  {
+    /*
+      If there is only one child, 'first', and it has join==NULL, emit "not in
+      EXPLAIN state" error.
+    */
+    return 1;
+  }
 
   for (SELECT_LEX *sl= first; sl; sl= sl->next_select())
   {
diff --git a/sql/sql_show.cc b/sql/sql_show.cc
index c30a8aad1eb..dd1b749e8bd 100644
--- a/sql/sql_show.cc
+++ b/sql/sql_show.cc
@@ -2112,16 +2112,19 @@ void mysqld_show_explain(THD *thd, ulong thread_id)
     explain_req.explain_buf= explain_buf;
     explain_req.target_thd= tmp;
     explain_req.request_thd= thd;
+    explain_req.failed_to_produce= FALSE;
 
     bres= tmp->apc_target.make_apc_call(Show_explain_request::get_explain_data,
                                         (void*)&explain_req,
                                         timeout_sec, &timed_out);
-    if (bres)
+
+    if (bres || explain_req.failed_to_produce)
     {
       /* TODO not enabled or time out */
       my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), 
                "SHOW EXPLAIN",
                "Target is not running EXPLAINable command");
+      bres= TRUE;
     }
     pthread_mutex_unlock(&tmp->LOCK_thd_data);
     if (!bres)