maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Holyfoot!

On May 09, holyfoot@xxxxxxxxxxxx wrote:
> ------------------------------------------------------------
> revno: 3518
> revision-id: holyfoot@xxxxxxxxxxxx-20120509185945-i1yx6d38phgoqzes
> parent: sanja@xxxxxxxxxxxxxxxx-20120507181437-1zl1v1uxje7v6994
> committer: Alexey Botchkov <holyfoot@xxxxxxxxxxxx>
> branch nick: mdev-136
> timestamp: Wed 2012-05-09 23:59:45 +0500
> message:
>   MDEV-136 Non-blocking "set read_only".
>      Handle the 'set read_only=1' in lighter way, than the FLUSH TABLES READ LOCK;
>      For the transactional engines we don't wait for operations on that tables to finish.

See my comments below.

> === modified file 'mysql-test/r/read_only.result'
> --- a/mysql-test/r/read_only.result	2009-03-06 14:56:17 +0000
> +++ b/mysql-test/r/read_only.result	2012-05-09 18:59:45 +0000
> @@ -59,7 +59,7 @@
>  connection con1;
>  select @@global.read_only;
>  @@global.read_only
> -0
> +1

This is prone to race conditions.
Please, fix the test to remove "send" here and below.
(assuming the new result is correct)

But is it correct ?
Why set read_only is not blocked by a write locked myisam table?

>  unlock tables ;
>  select @@global.read_only;
>  @@global.read_only
> @@ -80,7 +80,7 @@
>  connection con1;
>  select @@global.read_only;
>  @@global.read_only
> -0
> +1
>  unlock tables ;
>  select @@global.read_only;
>  @@global.read_only

Where are the tests for the changed functionality?

> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2012-04-19 06:16:30 +0000
> +++ b/sql/sql_base.cc	2012-05-09 18:59:45 +0000
> @@ -933,7 +938,8 @@
>        for (uint idx=0 ; idx < open_cache.records ; idx++)
>        {
>          TABLE *table=(TABLE*) hash_element(&open_cache,idx);
> -        if (table->in_use)
> +        if (table->in_use &&
> +            (!set_readonly_mode || !table->file->has_transactions()))

I wonder how this could work. The line below sets a flag *on a thread*.
The task description tells "not wait for transactional tables", while your
change means "not set a flag if all tables used in a thread are
transactional". That is, if a thread uses both transactional and
non-transactional tables, your change does nothing.

Back to my first question - where are the tests for this new task?

>            table->in_use->some_tables_deleted= 1;
>        }
>      }

Regards,
Sergei