maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Igor!

On Jun 09, Igor Babaev wrote:
> revno: 4234
> revision-id: igor@xxxxxxxxxxxx-20140610050716-0x8xaquf3itvofjc
> parent: bar@xxxxxxxxxxx-20140606062952-x2jwt4mgrgr6tpw6
> committer: Igor Babaev <igor@xxxxxxxxxxxx>
> branch nick: maria-10.0-bugs
> timestamp: Mon 2014-06-09 22:07:16 -0700
> message:
>   Fixed bug mdev-6071.
>   The method JOIN_CACHE::init may fail (return 1) if some conditions on the
>   used join buffer is not satisfied. For example it fails if join_buffer_size
>   is greater than join_buffer_space_limit. The conditions should be checked
>   when running the EXPLAIN command for the query. That's why the method
>   JOIN_CACHE::init has to be called for EXPLAIN commands as well.

It's an improvement over the old behavior - as EXPLAIN shows correctly
whether BKA is used.

But I think there's still not enough diagnostics. In the bug report
Sergey wrote that
"
 I think it's a big gotcha that @join_buffer_size must be lower than
 @@join_buffer_space_limit when optimizer_space_limit optimization is
 off.
"
So, I'd believe users will be very confused that in some cases BKA
is suddenly turned off, when they expect it to be used.

Of course, without this bugfix they would've not even noticed that,
which is even worse. So, this bugfix is good.

But it would be nice to have some kind of an explanation of why BKA is
not used. At the very least in the manual. Better - here as a warning,
like

  if (for_explain)
    push_warning("BKA was not used because ...";
  goto fail;

> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2014-06-05 22:07:27 +0000
> +++ b/sql/sql_select.cc	2014-06-10 05:07:16 +0000
> @@ -10541,7 +10541,7 @@
>      if (cache_level == 1)
>        prev_cache= 0;
>      if ((tab->cache= new JOIN_CACHE_BNL(join, tab, prev_cache)) &&
> -        ((options & SELECT_DESCRIBE) || !tab->cache->init()))
> +         !tab->cache->init(MY_TEST(options & SELECT_DESCRIBE)))

MY_TEST here (and in other similar places) is redundant,
you could've simply written

            !tab->cache->init(options & SELECT_DESCRIBE))

>      {
>        tab->icp_other_tables_ok= FALSE;
>        return (2 - MY_TEST(!prev_cache));

and while it wasn't part of your bug fix, I cannot help but noticing
that MY_TEST() is redundant here too. !prev_cache is boolean, it's
always 0 or 1.

Regards,
Sergei