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
