maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Oleksandr!

On May 23, Oleksandr Byelkin wrote:
> 22.05.2013 20:27, Sergei Golubchik пишет:
> > On May 22, sanja@xxxxxxxxxxxxxxxx wrote:
> >> ------------------------------------------------------------
> >> revno: 3769
> >> revision-id: sanja@xxxxxxxxxxxxxxxx-20130522125526-aw778iepj1yceskd
> >> parent: holyfoot@xxxxxxxxxxxx-20130514213637-3uvcda98gemgiquf
> >> committer: sanja@xxxxxxxxxxxxxxxx
> >> 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