Hi Sergei, I've made notes from review phone call, as well as replied to some comments below. (Comments that have been addressed were silently deletedx , comments that are without reply were not adressed are still on my todo).

...

=== 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-06-19 09:51:33 +0000 @@ -0,0 +1,698 @@ +max(c) +9 +# We can catch EXPLAIN, too. +set @show_expl_tmp= @@optimizer_switch; +set optimizer_switch='index_condition_pushdown=on,mrr=on,mrr_sort_keys=on'; +explain select max(c) from t1 where a < 10; +show explain for $thr2; +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t1 range a a 5 NULL 10 Using index condition; Rowid-ordered scan +Warnings: +Note 1003 explain select max(c) from t1 where a < 10 +id select_type table type possible_keys key key_len ref rows Extra +1 SIMPLE t1 range a a 5 NULL 10 Using index condition; Rowid-ordered scan +set optimizer_switch= @show_expl_tmp; +# UNION, first branch +set @show_explain_probe_select_id=1; +set debug_dbug='d,show_explain_probe_join_exec_start';

generally, we have debug_sync facility for synchronizing threads. it should be much easier and less error prone to use, than coding around dbug.

would debug_sync work here?

As agreed on the phone: I was unable to make use of debug sync for this purpose, SergeiG will investigate.

...

+explain select a from t0 A union select a+1 from t0 B; +show explain for $thr2; +id select_type table type possible_keys key key_len ref rows Extra +1 PRIMARY A ALL NULL NULL NULL NULL 10 +2 UNION B ALL NULL NULL NULL NULL 10 +NULL UNION RESULT ALL NULL NULL NULL NULL NULL +Warnings: +Note 1003 explain select a from t0 A union select a+1 from t0 B === modified file 'sql/filesort.cc' --- sql/filesort.cc 2012-05-21 13:30:25 +0000 +++ sql/filesort.cc 2012-06-12 19:04:11 +0000 @@ -1231,18 +1235,18 @@ 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= ¤t_thd->killed; + const bool killable= !param->not_killable; + THD* const thd=current_thd;

This "not_killable" thing is only used once, in uniques.cc. Added in 2001. Can you ask Monty why filesort shouldn't be killed if invoked from uniques.cc? I'd rather prefer to remove this code, than to let it drag along for another eleven years

Ok, I can ask Monty and remove not_killable after the SHOW EXPLAIN work is done. I do not want to mix SHOW EXPLAIN patch with random changes in filesort.

...

=== modified file 'sql/sql_select.cc' --- sql/sql_select.cc 2012-06-04 15:26:11 +0000 +++ sql/sql_select.cc 2012-06-12 19:05:07 +0000 @@ -272,6 +272,53 @@ Item_equal *find_item_equal(COND_EQUAL * JOIN_TAB *first_depth_first_tab(JOIN* join); JOIN_TAB *next_depth_first_tab(JOIN* join, JOIN_TAB* tab);

+#ifndef DBUG_OFF +// psergey: +void dbug_serve_apcs(THD *thd, int n_calls) +{ + // TODO how do we signal that we're SHOW-EXPLAIN-READY? + const char *save_proc_info= thd->proc_info; + thd_proc_info(thd, "show_explain_trap"); + + int n_apcs= thd->apc_target.n_calls_processed + n_calls; + while (thd->apc_target.n_calls_processed < n_apcs) + { + my_sleep(300);

Argh. Sleeps in the test. debug_sync was created to allow tests with reliable syncronization and without sleeps.

This seems to be the only way with current synchonization scheme. If we use dbug sync this may change. Currently show_explain.test takes 3.5 sec on the laptop, for 76 SHOW EXPLAIN commands.

...

@@ -7861,6 +7961,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? + */

Are you going to do something about it, or just leave the comment around?

I'll just leave a comment, for now. I don't want SHOW EXPLAIN patch to include a rewrite of parts of the optimizer. I think, eventually we will figure out the reason for the difference and remove dependency on thd->lex->describe.

...

=== modified file 'sql/sql_class.h' --- sql/sql_class.h 2012-05-21 13:30:25 +0000 +++ sql/sql_class.h 2012-06-19 14:08:12 +0000 @@ -3243,6 +3318,37 @@ public:

/* + A select result sink that collects the sent data and then can flush it to + network when requested. + + This class is targeted at collecting EXPLAIN output: + - Unoptimized data storage (can't handle big datasets) + - Unlike select_result class, we don't assume that the sent data is an + output of a SELECT_LEX_UNIT (and so we dont apply "LIMIT x,y" from the + unit) +*/ + +class select_result_explain_buffer : public select_result_sink

Eh... Why haven't you done it as an I_S table? It doesn't have to be available via SELECT * FROM I_S.xxx but it should still use the same interface.

Why did you prefer to code a completely new way of implementing SHOW commands?

(As discussed on the phone) I'll look at using I_S tables and SHOW command handling for storing EXPLAIN output. The rationale for this is "we've had switch other SHOW commands to using I_S".

...

+{ +public: + THD *thd; + Protocol *protocol; + select_result_explain_buffer(){}; + + /* The following is called in the child thread: */ + int send_data(List<Item> &items); + + /* this will be called in the parent thread: */ + void flush_data(); + + void discard_data(); + + List<String> data_rows; +}; + + + +/* Base class for select_result descendands which intercept and transform result set rows. As the rows are not sent to the client, sending of result set metadata should be suppressed as well. === modified file 'sql/protocol.h' --- sql/protocol.h 2012-01-13 14:50:02 +0000 +++ sql/protocol.h 2012-06-12 19:04:11 +0000 @@ -73,6 +78,20 @@ public: virtual bool send_result_set_metadata(List<Item> *list, uint flags); bool send_result_set_row(List<Item> *row_items);

+ void get_packet(const char **start, size_t *length) + { + *start= packet->ptr(); + *length= packet->length(); + } + void set_packet(const char *start, size_t len) + { + packet->length(0); + packet->append(start, len); +#ifndef DBUG_OFF + field_pos= field_count - 1; +#endif + }

This is part of your select_result_explain_buffer code.

I don't understand the question? Anyway, this part is likely to be gone after SHOW EXPLAIN is made to use I_S tables.

...

+ bool store(I_List *str_list); bool store(const char *from, CHARSET_INFO *cs); String *storage_packet() { return packet; } === modified file 'sql/sql_class.cc' --- sql/sql_class.cc 2012-05-21 13:30:25 +0000 +++ sql/sql_class.cc 2012-06-19 14:02:19 +0000 @@ -2151,6 +2166,21 @@ void THD::rollback_item_tree_changes() }

+/* + Check if the thread has been killed, and also process "APC requests" + + @retval true The thread is killed, execution should be interrupted + @retval false Not killed, continue execution +*/ + +bool THD::check_killed() +{ + if (killed) + return TRUE; + apc_target.process_apc_requests(); + return FALSE; +}

I imagined it slightly differently. Without special code for killing, but simply

bool THD::check_killed() { return apc_target.process_apc_requests(); }

where process_apc_requests() returns the return value of request->func(request->func_arg)

This way the function can decide whether to abort the statement processing (and how) or not. And to kill the thread one would need to do not

thd->killed=1;

but

thd->killed=apc_kill_thread_func;

Independently from the above, process_apc_requests() should do as little as possible in the normal case, when there are no apc requests. It used to be one pointer dereference for killed, now it's a function calls.

Suggestion, move the first if(get_first_in_queue()) to a separate wrapper function, which is declared inline, in the my_apc.h and call the real process_apc_requests from this wrapper function. Like

class Apc_target { void process_apc_requests() { if (get_first_in_queue()) really_process_apc_requests(); } ...

This way the compiler will inline process_apc_requests and get_first_in_queue, making the most common code path into one if (apc_calls)

And check_killed should be an inline function too, in the sql_class.h

Also, you forgot to change thd_killed() function to use thd->check_killed().

...

+ /***************************************************************************** ** Functions to provide a interface to select results *****************************************************************************/ @@ -3198,6 +3322,42 @@ void THD::restore_active_arena(Query_are DBUG_VOID_RETURN; } ... + + Statement::~Statement() { } === modified file 'sql/sql_parse.cc' --- sql/sql_parse.cc 2012-05-21 18:54:41 +0000 +++ sql/sql_parse.cc 2012-06-12 19:04:11 +0000 @@ -3127,6 +3128,32 @@ end_with_restore_list: thd->security_ctx->priv_user), lex->verbose); break; + case SQLCOM_SHOW_EXPLAIN: + { + /* Same security as SHOW PROCESSLIST (TODO check this) */

please add a test for it

Ok, will do.

...

+ if (!thd->security_ctx->priv_user[0] && + check_global_access(thd,PROCESS_ACL)) + break; + + Item *it= (Item *)lex->value_list.head(); + + if (lex->table_or_sp_used()) + { + my_error(ER_NOT_SUPPORTED_YET, MYF(0), "Usage of subqueries or stored " + "function calls as part of this statement");

This doesn't allow to localize the error message. generally, hard-coding of error messages should be avoided, whenever possible

I'd simply made it a syntax error in the parser.

On the other hand... why not to allow it, if KILL does.

KILL doesn't allow this, grep for 'case SQLCOM_KILL' in sql_parse.cc

...

+ break; + } + + if ((!it->fixed && it->fix_fields(lex->thd, &it)) || it->check_cols(1)) + { + my_message(ER_SET_CONSTANTS_ONLY, ER(ER_SET_CONSTANTS_ONLY),

What does it have to do with SET? "You may only use constant expressions with SET"

This check was copied from KILL statement handling.

...

+ MYF(0)); + goto error; + } + + mysqld_show_explain(thd, (ulong)it->val_int()); + break; + } case SQLCOM_SHOW_AUTHORS: res= mysqld_show_authors(thd); break; === modified file 'sql/sql_show.cc' --- sql/sql_show.cc 2012-05-21 18:54:41 +0000 +++ sql/sql_show.cc 2012-06-12 19:04:11 +0000 @@ -1998,6 +1998,116 @@ void mysqld_list_processes(THD *thd,cons DBUG_VOID_RETURN; }

+ +/* + SHOW EXPLAIN FOR command handler + + @param thd Current thread's thd + @param thread_id Thread whose explain we need + + @notes + - Attempt to do "SHOW EXPLAIN FOR <myself>" will properly produce "target not + running EXPLAINable command". + - todo: check how all this can/will work when using thread pools

Done?

There is no testcase for it, but I've had a discussion with Wlad and we came to conclusion that SHOW EXPLAIN should not have any issues with thread pools. I've removed the todo:

...

+*/ + +void mysqld_show_explain(THD *thd, ulong thread_id) +{ + THD *tmp; + Protocol *protocol= thd->protocol; + List<Item> field_list; + DBUG_ENTER("mysqld_show_explain"); +

You don't seem to check the privileges here.

I check them inside mysql_execute_command(), like KILL does.

...

+ thd->make_explain_field_list(field_list); + if (protocol->send_result_set_metadata(&field_list, Protocol::SEND_NUM_ROWS | + Protocol::SEND_EOF)) + DBUG_VOID_RETURN; + + /* + Find the thread we need EXPLAIN for. Thread search code was copied from + kill_one_thread() + */ + 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) + { + 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 an EXPLAIN request. + todo: where to get timeout from? + */ + 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; + explain_buf->thd=thd; + explain_buf->protocol= thd->protocol; + + 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(Show_explain_request::get_explain_data, + (void*)&explain_req, + timeout_sec, &timed_out); + + if (bres || explain_req.failed_to_produce) + { + /* TODO not enabled or time out */ + if (timed_out) + { + my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), + "SHOW EXPLAIN", + "Timeout"); + } + else + { + my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0), + "SHOW EXPLAIN", + "Target is not running EXPLAINable command");

This doesn't allow to localize the error message. generally, hard-coding of error messages should be avoided, whenever possible

(as discussed on the phone) Will look into adding an error message that will be a template for all EXPLAIN errors.

...

=== added file 'sql/my_apc.h' --- sql/my_apc.h 1970-01-01 00:00:00 +0000 +++ sql/my_apc.h 2012-06-19 13:59:00 +0000 ... + + @retval FALSE - Ok, the call has been made + @retval TRUE - Call wasnt made (either the target is in disabled state or + timeout occured) + */ + bool make_apc_call(apc_func_t func, void *func_arg, + int timeout_sec, bool *timed_out);

Hmm. Either you make a C++ interface or a C interface. If it's C++, then you pass an object instance, with a execute (for example) method, and this object has all the data. If you pass a function and a void pointer - it's C-style interface, but then why did you write the rest of apc code in C++?

(agreed to make a C++ interface with function-class)

...

+ +#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 (i.e. they are removed from the front) + - 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.

This explains why you have a list, and why it's doubly-linked. But why is it circular?

...

+ */ + Call_request *apc_calls; + + class Call_request + { + public: + apc_func_t func; /* Function to call */ + void *func_arg; /* Argument to pass it */ + + /* 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; + } +}; + +/////////////////////////////////////////////////////////////////////// + === added file 'sql/my_apc.cc' --- sql/my_apc.cc 1970-01-01 00:00:00 +0000 +++ sql/my_apc.cc 2012-06-12 19:04:11 +0000 @@ -0,0 +1,377 @@ +/* + TODO: MP AB Copyright

Right. Please do.

...

+*/ + + +#ifdef MY_APC_STANDALONE + +#include +#include +#include + +#include "my_apc.h" + +#else + +#include "sql_priv.h" +#include "sql_class.h" + +#endif + + +/* + Standalone testing: + g++ -c -DMY_APC_STANDALONE -g -I.. -I../include -o my_apc.o my_apc.cc + g++ -L../mysys -L../dbug -L../strings my_apc.o -lmysys -ldbug -lmystrings -lpthread -lrt

Eh, please no. Nobody will be doing that. Add unit tests properly, into unittest/

...

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

This doesn't seem to make a lot of sense. Apc_target is inside the THD, and it needs to store a pointer to the THD's mutex?

I understand that originally, it has its own mutex and then you simply changed it to use the THD's mutex. But the result isn't particularly logical.

On the other hand, you don't remember the THD itself, and the apc function will always need to use current_thd.

It doesn't use it now.

I'm not sure what a good solution could be. For example, it apc could be more tightly bound to a THD, by inheriting THD class from Apc_target.

What exactly is wrong with the current solution? Apc_target is not integrated with THD, there is only one point where they interact, and it's the mutex. Maybe this question/comment will become irrelevant once Apc_target is changed to be the universal APC delivery mechanism (see next comment).

Or less bound, and providing a C interface, by putting it in mysys_var.

But I think that making it very tightly THD-dependent is the right approach. Otherwise, it is assumed that it can be used for non-THD threads, and it puts unnecessary limitations on what a callback function can do.

...

+#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: */

1. Why? 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.

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

...

+ enabled++; +} + + +/* + 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) +{ ... +/* + Make an APC (Async Procedure Call) to another thread. + + - The caller is responsible for making sure he's not calling to the same + thread. + + - The caller should have locked target_thread_mutex. + + + psergey-todo: Should waits here be KILLable? (it seems one needs + to use thd->enter_cond() calls to be killable)

I think yes, with a timeout of 30 seconds - killable.

...

+*/ + + if (enabled) + { + /* Create and post the request */ + Call_request apc_request; + apc_request.func= func; + apc_request.func_arg= func_arg; + apc_request.processed= FALSE; + mysql_cond_init(0 /* do not track in PS */, &apc_request.COND_request, NULL);

Why not?

...

+ enqueue_request(&apc_request); + apc_request.what="enqueued by make_apc_call"; + + struct timespec abstime; + const int timeout= timeout_sec; + set_timespec(abstime, timeout); ... + +class Apc_order +{ +public: + int value; // The value + int *where_to; // Where to write it + Apc_order(int a, int *b) : value(a), where_to(b) {} +}; + +void test_apc_func(void *arg) +{ + Apc_order *order=(Apc_order*)arg; + usleep(int_rand(1000)); + *(order->where_to) = order->value; + __sync_fetch_and_add(&apcs_served, 1);

It's gcc-only. Use my_atomic instead. See other concurrent tests in unittest/mysys

...

+} +

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

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?

...

+# +# === 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= ¤t_thd->killed; + const bool killable= !param->not_killable;

did you ask Monty about this not_killable thing?

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. 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= ¬_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?

...

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

...

+ + 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: Removed it. 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 +#include +#include +#include + +#include + +/* + 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

Hi Sergei, On Tue, Aug 07, 2012 at 01:37:33PM +0200, Sergei Golubchik wrote:

On Jul 17, Sergei Petrunia wrote:

...

...

...

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

Yes. Looking at this big patch I see that we have lots of places where thd->killed is checked directly, instead of using thd->check_killed(), that is, there can be long delays where APC requests won't be served (despite the fact that KILL works there).

But if you feel uneasy doing this, let's create a separate jira task for "refactor KILL to work via APC" and I will do it.

Filed https://mariadb.atlassian.net/browse/MDEV-442

...

...

...

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

Could you please explain, why using INFORMATION_SCHEMA prevents you from using LEX::value_list ?

Ok, we've figured there's no valid reason. Started to use LEX::value_list.

...

...

...

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

Yes, thanks.

...

...

...

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

I wouldn't do it that way, but it's your task. Okay.

...

...

...

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

I've just tried it in your tree (5.5-show-explain). 26 lines added, 34 lines removed. It adds less code than it removes :) But more importantly, if something will need to be changed in this loop, only one place will need to be changed.

Here's the function that I've used (copied from kill_one_thread):

THD *find_thread_by_id(ulong id) { THD *tmp; 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 == id) { mysql_mutex_lock(&tmp->LOCK_thd_data); // Lock from delete break; } } mysql_mutex_unlock(&LOCK_thread_count); return tmp; }

Ok, thanks. I have put the function in.

...

...

...

+ */ + mysql_mutex_lock(&LOCK_thread_count); // For unlink from list + I_List_iterator<THD> it(threads); + while ((tmp=it++)) + { === 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 ... +/* + 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? ... Hence, we have decided not to implement always-wake-up system.

My recollection is that we agreed to ignore this limitation for now. That is, in case of a THD waiting for network and a thread-pool enabled, SHOW EXPLAIN will time out, but as SHOW EXPLAIN is useless anyway for connections in the "idle" state, it's not a big deal. When we'll have more APC users we'll get back to it.

But again, if you'd like, let's keep it your way, with enabled/disabled. And move this change to the "refactor KILL to work via APC" task, that I mentioned above.

Done, added to task.

...

Your review is trying to pull stuff in different directions. On one hand, INFORMATION_SCHEMA support is added for 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?

But you didn't create the I_S table, right? I mean, it's marked as hidden and can not be selected from, only SHOW command works?

Yes. The table is currently marked as hidden.

I was contemplating that when we need to send APC requests to many threads at once, we could do it in parallel - post all requests, then wait for all of them to complete. But there's no need to do it now - it's a future extension, we'll consider it when we'll have a use case.

...

...

...

=== 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 @@ ... 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?

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

I'm just trying to understand your logic, when it is not obvious from the code. If I'd wanted you to change something, I'd wrote "please change this". But here I'm merely asking, I'm not trying to imply that there are defects of that some other way of doing this piece of code would be better. No hidden meaning :)

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