Hi, Oleksandr! On May 23, Oleksandr Byelkin wrote:
22.05.2013 20:27, Sergei Golubchik пишет:
On May 22, sanja@montyprogram.com wrote:
------------------------------------------------------------ revno: 3769 revision-id: sanja@montyprogram.com-20130522125526-aw778iepj1yceskd parent: holyfoot@askmonty.org-20130514213637-3uvcda98gemgiquf committer: sanja@montyprogram.com branch nick: work-maria-5.5-MDEV-4520 timestamp: Wed 2013-05-22 15:55:26 +0300 message: MDEV-4520 fix. First, please, fix the changeset comment. The first line should be MDEV-<number><space><complete issue summary>
For example:
MDEV-4520 Assertion `0' fails in Query_cache::end_of_result on concurrent drop event and event execution
Then, after an empty line, you add whatever explanations you think are necessary. Like the one below: The only question is how happened that I did not know standard of commit comment (where that standard is/was described)?
This is a de facto standard. It wasn't formally stated as a rule to follow, but everyone is doing it anyway, and it's really convenient to have changeset comments structured this way. But now I don't mind making it a written rule - because it practically won't change anything :) Where do you suggest it should be written?
If there is no net.vio then query cache cant't get data via net_real_write() so it is better just do not try to cache such query. Also, it'd be good to record the fixed bug in the changeset metadata. This can be done either with "bzr gcommit" and my patches to it (see the article in KB), or from the command line with "bzr commit --fixes".
Again previous question...
It's not at all a standard. It's nice to have, but very few are doing it. Feel free not to.
=== modified file 'sql/sql_cache.cc' --- a/sql/sql_cache.cc 2013-05-07 11:05:09 +0000 +++ b/sql/sql_cache.cc 2013-05-22 12:55:26 +0000 @@ -3977,7 +3977,8 @@ Query_cache::is_cacheable(THD *thd, LEX if (thd->lex->safe_to_cache_query && (thd->variables.query_cache_type == 1 || (thd->variables.query_cache_type == 2 && (lex->select_lex.options & - OPTION_TO_QUERY_CACHE)))) + OPTION_TO_QUERY_CACHE))) && + thd->net.vio) { DBUG_PRINT("qcache", ("options: %lx %lx type: %u", (long) OPTION_TO_QUERY_CACHE, The fix itself looks ok. But please add a test case.
And think if there can be more cases (besides events) where SELECT is executed with thd->net.vio == 0. E.g. init-file? or init-connect? Or INSERT (in the replication thread) that invokes a stored function or a trigger? Anything you can think of. I spent more then half a day with this and here is result:
- vio is definitely unset for replication and/or events (I have no idea how that tests work, probably it will require several days to figure out)
Right, that's why I suggested a test with events. And a test with a SELECT in the replication thread.
- one bug found in mysql-test-run.pl and options files (MDEV-4567) (the bug really slows down any investigation about any test with options).
Sanja, if you want your submitted bug to have any chance of ever being fixed, please at least specify the affected version and the fix version.
- putting SELECT statement in --init_connect does not work.
What does it mean? How exactly it "does not work" ?
I was trying to catch it with putting assertion in my_net_write. there is no great success so far.
Should I continue or left it as is taking into account that bug was reported against our standard test suite?
But that's the point - it's not against our standard test suite. It's a test from the test suite run with the non-default command line options. Nobody runs the test suite this way, so if this will ever be broken again, it won't be noticed, no test will fail. That's why we need a test for this bug. Regards, Sergei