Re: bk commit into 5.1 tree (ramil:1.2576) BUG#35732

[Sergei Golubchik]

Hi!

On Apr 07, ramil@stripped wrote:
> ChangeSet@stripped, 2008-04-07 17:31:20+05:00, ramil@stripped +3 -0
>   Fix for bug #35732: read-only blocks SELECT statements in InnoDB
>   
>   Problem: SELECTs prohibited for a transactional SE in autocommit mode
>   if read_only is set.
>   
>   Fix: allow them.

basically ok.
See comments below, and make sure that global read lock is not affected
by this bug (I suspect it is).

Ok to push after that.
 
> diff -Nrup a/mysql-test/t/read_only_innodb.test b/mysql-test/t/read_only_innodb.test
> --- a/mysql-test/t/read_only_innodb.test	2006-11-21 07:40:31 +04:00
> +++ b/mysql-test/t/read_only_innodb.test	2008-04-07 17:31:18 +05:00
> @@ -36,8 +36,30 @@ select * from table_11733 ;
>  -- error ER_OPTION_PREVENTS_STATEMENT
>  COMMIT;
>  
> +disconnect con1;
> +
>  connection default;
>  set global read_only=0;
>  drop table table_11733 ;
>  drop user test@localhost;
>  
> +#
> +# Bug #35732: read-only blocks SELECT statements in InnoDB
> +#
> +GRANT CREATE, SELECT, DROP ON *.* TO test@localhost;
> +CREATE TABLE t1(a INT) ENGINE=INNODB;
> +INSERT INTO t1 VALUES (0), (1);
> +SET GLOBAL read_only=1;
> +
> +connect(con1, localhost, test, , test);
> +connection con1;
> +SELECT * FROM t1;
> +
> +disconnect con1;
> +
> +connection default;
> +SET GLOBAL read_only=0;
> +DROP TABLE t1;
> +DROP USER test@localhost;
> +
> +--echo echo End of 5.1 tests 

Add a not-autocommit test, please:

BEGIN;
SELECT * FROM t1;
COMMIT;

> diff -Nrup a/sql/handler.cc b/sql/handler.cc
> --- a/sql/handler.cc	2008-03-12 12:13:19 +04:00
> +++ b/sql/handler.cc	2008-04-07 17:31:18 +05:00
> @@ -954,16 +954,19 @@ int ha_prepare(THD *thd)
>    A helper function to evaluate if two-phase commit is mandatory.
>    As a side effect, propagates the read-only/read-write flags
>    of the statement transaction to its enclosing normal transaction.
> -
> -  @retval TRUE   we must run a two-phase commit. Returned
> -                 if we have at least two engines with read-write changes.
> -  @retval FALSE  Don't need two-phase commit. Even if we have two
> -                 transactional engines, we can run two independent
> -                 commits if changes in one of the engines are read-only.
> +  
> +  If we have at least two engines with read-write changes we must
> +  run a two-phase commit. Otherwise we can run several independent
> +  commits as the only transactional engine has read-write changes
> +  and others are read-only.
> +
> +  @retval   0   All engines are read-only.
> +  @retval   1   We have the only engine with read-write changes.
> +  @retval  >1   More than one engine have read-write changes.

Emphasize here that return value is NOT the exact number of engines with
read-write changes, as you see the function returns prematurely as soon
as rw_ha_count > 1. That is, you don't say it, and strictly speaking
your comment is correct, but one can think that return value is the
exact number of engines with read-write changes, so add a couple of
sentences to clarify that it is not.

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer/Server Architect
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140