[Maria-developers] Review request: SHOW EXPLAIN
Hi Sergei, Please find attached the combined SHOW EXPLAIN patch. It is ready for review of the concurrency part. BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
Hi, Sergei! Thanks! Please find my comments below.
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?
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
Remove the commented-out code please
I'm not a great fan of special user variable names. and for using them for debugging too. But ok, passing down values for debugging is not a new problem, let's keep it your way, until we have a better solution that works in all cases.
Ok, I'm not reviewing the optimizer and explain part, right?
How did you generate the diff? Better use my plugin from the last section in http://kb.askmonty.org/en/how-to-get-more-out-of-bzr-when-working-on-mariadb... it works automatically, in all diffs, and better detects function/class names (it is not confused by public:/private:)
Argh. Sleeps in the test. debug_sync was created to allow tests with reliable syncronization and without sleeps.
Are you going to do something about it, or just leave the comment around?
I suppose, before this task is considered complete and ready for push you'll look through the tree and remove all your todo comments, right?
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?
This is part of your select_result_explain_buffer code.
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().
obsolete comment?
please add a test for it
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.
What does it have to do with SET? "You may only use constant expressions with SET"
Done?
You don't seem to check the privileges here.
This doesn't allow to localize the error message. generally, hard-coding of error messages should be avoided, whenever possible
forgotten comment
commented-out code again
I don't understand "This function must not be called from a thread that's different from the target thread" Try to rephrase it please
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++?
This explains why you have a list, and why it's doubly-linked. But why is it circular?
Right. Please do.
Eh, please no. Nobody will be doing that. Add unit tests properly, into unittest/
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. 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. 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.
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.
add mysql_mutex_assert_owner(LOCK_thd_data_ptr);
add mysql_mutex_assert_owner(LOCK_thd_data_ptr);
I think yes, with a timeout of 30 seconds - killable.
add mysql_mutex_assert_owner(LOCK_thd_data_ptr);
Why not?
It's gcc-only. Use my_atomic instead. See other concurrent tests in unittest/mysys
Regards, Sergei
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).
As agreed on the phone: I was unable to make use of debug sync for this purpose, SergeiG will investigate.
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.
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.
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.
(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".
I don't understand the question? Anyway, this part is likely to be gone after SHOW EXPLAIN is made to use I_S tables.
Ok, will do.
KILL doesn't allow this, grep for 'case SQLCOM_KILL' in sql_parse.cc
This check was copied from KILL statement handling.
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:
I check them inside mysql_execute_command(), like KILL does.
(as discussed on the phone) Will look into adding an error message that will be a template for all EXPLAIN errors.
(agreed to make a C++ interface with function-class)
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).
(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.
BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
Hi, Sergei! A couple of replies: On Jun 26, Sergei Petrunia wrote:
That's ok. But then I'd suggest to create a task for it in Jira (and keep the comment too).
Exactly. That's what I meant by my comment.
SHOW PROCESSLIST verifies that either one has PROCESS_ACL or one can see only his own threads. I don't see where you have this check. The one in mysql_execute_command() doesn't do it, as far as I can see. May be I'm wrong here. But anyway, when you add tests for privileges, you'll see what works and what doesn't. Regards, Sergei
Hi, I'm not sure this use case is meant, but waking up an idle connection is a complex problem, if one wants to do it portably and reliably. Currently, it works ok for KILL <connection> in 5.5, socket is shut down and this wakes up a thread. However the problem is that after shutdown the connection is not usable anymore, thus shutdown() cannot be used apart from KILL <connection> context. On Unix but not on Windows, there is pthread_kill does not work if threadpool is in use (a connection is not stuck in read/recv in this case). On Windows but not on Unix, there is IO cancelation, which would work even with the thread pool, and could be used to wake up an idle connection by cancelling read io. But in case of threadpool+Unix I do not see any good solution for APC. Wlad
What else should it do? It cannot handle EINTR in a reasonable way - there is no fix thread-to-connection relation, - there are several threads that can wait, and there are 2 different "waits" (one on epoll or similar, another one on condition, and this wait is not interrupted with EINTR) - The thread that can be interrupted by epoll_wait is not predefined, and can change its role to be a "worker" thread, and then wait on condition.
Regards, Sergei
Hi, Vladislav!
Ok, to avoid spending any more time on that I suggest the following: APC code won't send APC requests to idle (COM_END) threads. SHOW EXPLAIN does not need it. thd->query_string does not need it. KILL QUERY does not need it. Any other KILL will shutdown the socket anyway. In the future, when we will need idle threads to receive APC requests, we'll have to find a solution that works with a threadpool. But for now we can survive without it. Regards, Sergei
Hi Sergei, Hi Sergei, Please find attached the updated combined SHOW EXPLAIN patch. I believe all of feedback from the previous review has been addressed, except for: - 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, - 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. BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
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.
Why? In the code you check only for PROCESS_ACL, not for SUPER_ACL (which is correct). Why SUPER makes the test meaningless?
did you ask Monty about this not_killable thing?
Eh? Inside #ifdef HAVE_EVENT_SCHEDULER ?
Is that right? my_charset_bin?
wouldn't it be more appropriate to have this in sql_show.cc, not here? (and select_result_explain_buffer too)
I think it'd be more logical to have this in sql_show.h
You forgot to fix thd_killed() function, didn't you?
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.
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
Why do extend LEX even more? KILL, for example, uses LEX::value_list
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.
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.
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.
Same question as above, about optimize_inner(). And also - you didn't remove apc_target.enable/disable, as we've discussed.
forgotten comment?
Did you already create a Jira task for this?
forgotten comment?
Perhaps, it'd make sense to extract this code to a separate function and reuse it?
s/PROCESSLIST/PROCESS/ the name of the privilege is "PROCESS"
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))
Here you could've used ER_LOCK_WAIT_TIMEOUT (esp. if we remove "transaction" from the error message text)
This certainly belongs to sql_show.cc
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.
perhaps all that (above) belongs to the Call_request constructor?
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.
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)
why fprintf() instead of diag() ?
Regards, Sergei
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:
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.
psergey: it was my mistake, moved it outside.
psergey: No, it wasn't. I've added a conversion (see the second commit).
psergey: I don't think there is any difference. Moved the function to sql_show.cc
psergey: Moved to sql_show.h
psergey: Added the check in that function, too.
psergey: Changed to the provided text
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.
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.
psergey: I've changed SHOW EXPLAIN code not to use this error (it uses ER_SET_CONSTANTS_ONLY now). Is that ok?
psergey: Done.
psergey: There are multiple exits from JOIN::optimize, and I wanted to cover them all with one statement.
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.
psergey: No, it should be there as it relates to MDEV-325, BUG#992942.
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.
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.
psergey: Changed.
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.
psergey: Changed to use ER_LOCK_WAIT_TIMEOUT. I didn't change the message text.
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.
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.
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.
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?
psergey: Changed to diag().
BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
Hi, Sergei! On Jul 17, Sergei Petrunia wrote:
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.
Could you please explain, why using INFORMATION_SCHEMA prevents you from using LEX::value_list ?
Yes, thanks.
I wouldn't do it that way, but it's your task. Okay.
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; }
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.
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? 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.
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 :) Regards, Sergei
Hi Sergei, On Tue, Aug 07, 2012 at 01:37:33PM +0200, Sergei Golubchik wrote:
Filed https://mariadb.atlassian.net/browse/MDEV-442
Ok, we've figured there's no valid reason. Started to use LEX::value_list.
Ok, thanks. I have put the function in.
Done, added to task.
Yes. The table is currently marked as hidden.
BR Sergei -- Sergei Petrunia, Software Developer Monty Program AB, http://askmonty.org Blog: http://s.petrunia.net/blog
participants (3)
-
Sergei Golubchik -
Sergei Petrunia -
Vladislav Vaintroub