From: Dmitry Lenev
Date: January 14 2011 10:43am
Subject: Re: bzr commit into mysql-trunk-bugfixing branch
 (alexander.nozdrin:3471) Bug#27480
List-Archive: http://lists.mysql.com/commits/128718
Message-Id: <20110114104338.GD1912@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hello Alik!

Here are my comments about this patch:

* Alexander Nozdrin <alexander.nozdrin@stripped> [10/12/23 19:25]:
> #At file:///home/alik/MySQL/bzr/00/bug27480/mysql-trunk-bugfixing/ based on revid:tor.didriksen@stripped
> 
>  3471 Alexander Nozdrin	2010-12-23
>       Prerequisite patch for Bug#27480 (Extend CREATE TEMPORARY TABLES privilege
>       to allow temp table operations):
> 

I think it is better to add to this comment a rationale explaining
why we are doing this change.

Something like:

"Move opening of temporary tables (in normal case) to the stage of
 statement execution which precedes privilege checking. In the
 upcoming patches we will use this fact during privilege checks
 to easily and cheaply determine if table is temporary."

will suffice IMO.

...

>         - move opening of temporary tables out of open_table();
>       
>         - make open_table() to work with base tables and views only.
>           It will be renamed to open_base_table_or_view()
>           in a follow-up patch.
>       
>         - introduce open_temporary_table() to open temporary tables
>           (similar to open_table());
>       
>         - introduce open_temporary_tables() to open temporary tables
>           corresponding to table list elements;
>       
>         - introduce a new "command flag" (CF_PREOPEN_TMP_TABLES) to mark
>           statements that work with temporary tables, thus temporary tables
>           should be opened for those statements;
>       
>         - open temporary tables in a unified way in the beginning of
>           the statements marked with CF_PREOPEN_TMP_TABLES flag;
>       
>         - introduce a new "command flag" (CF_HA_CLOSE) to mark statements
>           for which open handlers (by HANDLER OPEN) should be closed;
>       
>         - close open handlers in a unified way in the beginning of
>           the statements marked with CF_HA_CLOSE flag.

...

> === modified file 'mysql-test/suite/innodb/r/innodb_mysql.result'
> --- a/mysql-test/suite/innodb/r/innodb_mysql.result	2010-12-20 13:24:37 +0000
> +++ b/mysql-test/suite/innodb/r/innodb_mysql.result	2010-12-23 16:18:19 +0000
> @@ -2830,4 +2830,17 @@ PACK_KEYS=0;
>  CREATE INDEX a ON t1 (a);
>  CREATE INDEX c on t1 (c);
>  DROP TABLE t1;
> +#
> +# Additional coverage for refactoring which is made as part
> +# of fix for bug #27480 "Extend CREATE TEMPORARY TABLES privilege
> +# to allow temp table operations".
> +#
> +# Check that OPTIMIZE table works for temporary InnoDB tables.
> +DROP TABLE IF EXISTS t1;
> +CREATE TEMPORARY TABLE t1 (a INT) ENGINE=InnoDB;
> +OPTIMIZE TABLE t1;
> +Table	Op	Msg_type	Msg_text
> +test.t1	optimize	note	Table does not support optimize, doing recreate + analyze instead
> +test.t1	optimize	status	OK
> +DROP TABLE t1;
>  End of 6.0 tests
> 
> === modified file 'mysql-test/suite/innodb/t/innodb_mysql.test'
> --- a/mysql-test/suite/innodb/t/innodb_mysql.test	2010-12-20 12:43:48 +0000
> +++ b/mysql-test/suite/innodb/t/innodb_mysql.test	2010-12-23 16:18:19 +0000
> @@ -992,4 +992,19 @@ CREATE INDEX c on t1 (c);
>  
>  DROP TABLE t1;
>  
> +
> +--echo #
> +--echo # Additional coverage for refactoring which is made as part
> +--echo # of fix for bug #27480 "Extend CREATE TEMPORARY TABLES privilege
> +--echo # to allow temp table operations".
> +--echo #
> +--echo # Check that OPTIMIZE table works for temporary InnoDB tables.
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
> +CREATE TEMPORARY TABLE t1 (a INT) ENGINE=InnoDB;
> +OPTIMIZE TABLE t1;
> +DROP TABLE t1;
> +
> +
>  --echo End of 6.0 tests

It turns out that we still need to notify InnoDB developers about changes
to tests to innodb suite. Could you please drop them a note that this
change will be eventually pushed to main tree?

I think it is OK to push this patch into separate "feature" tree
to enable QA and further work on it.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification