Hi Serg, On Tue, May 17, 2016 at 10:26 AM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nirbhay!
.. cut ..
diff --git a/mysql-test/r/temp_table.result b/mysql-test/r/temp_table.result index ee0b3ab..94b02a6 100644 --- a/mysql-test/r/temp_table.result +++ b/mysql-test/r/temp_table.result @@ -308,3 +308,185 @@ show status like 'com_drop%table'; +DROP TABLE temp_t1; +# Cleanup +DROP DATABASE temp_db; +# MDEV-5535: End of tests
What's that? You have lots of tests for temporary tables - not that I mind the number, but they are mostly unrelated to MDEV-5535. Only the one last test is about the new feature. I'd expect it to be the other way around - many tests for the new functionality and few, if at all - for the unmodified behavior.
These generic tests were added to cover more possible use cases as the patch modified a good fraction of existing code. I have now moved these unrelated tests to a separate commit and added some more tests specific to MDEV-5535 in reopen_temp_table.test.
Good idea. I hope your separate commit with unrelated tests is *before* the MDEV-5535 commit, so that one could checkout before MDEV-5535, see unrelated tests pass, then checkout MDEV-5535 and see how unrelated tests still pass.
Yes & yes.
diff --git a/mysql-test/suite/handler/handler.inc b/mysql-test/suite/handler/handler.inc index c71dc53..ca67b62 100644 --- a/mysql-test/suite/handler/handler.inc +++ b/mysql-test/suite/handler/handler.inc @@ -489,7 +489,7 @@ handler t1 open as a1; handler a1 read a=(1); handler a1 read a next; handler a1 read a next; ---error ER_CANT_REOPEN_TABLE +#--error ER_CANT_REOPEN_TABLE
remove it, don't comment. And, please, fix the comment before this test.
Removed. What comment are you referring to?
This test in the handler.inc that you've edited. It has a comment before the test starts:
# # Bug#30882 Dropping a temporary table inside a stored function may # cause a server crash # # Test HANDLER statements in conjunction with temporary tables. While the temporary table # is open by a HANDLER, no other statement can access it. #
You've fixed temporary tables, so this comment is no longer true.
I wasn't able to find it as it was already removed. :)
select a,b from t1; handler a1 read a prev; handler a1 read a prev; diff --git a/sql/temporary_tables.h b/sql/temporary_tables.h new file mode 100644 index 0000000..c345bea --- /dev/null +++ b/sql/temporary_tables.h ... +class Temporary_tables { ... +private: + THD *m_thd;
Hmm, is that necessary? Temporary_tables class is instantiated in only one place - inside the THD. So Temporary_tables always belongs to its thd, and storing a point to the parent THD in the Temporary_tables you basically add another void* to already big THD class, while thid pointer stores no useful information at all.
A hackish way to fix is is to replace m_thd with
(((char*)this) - offsetof(THD, temporary_tables))
I'm not suggesting this hack, it is just to prove that m_thd is redundant. I hope you'll find a nicer and safer solution :)
How about current_thd in place of m_thd?
Better not. Svoj and Monty has recently done a lot of work removing current_thd from many places, no need to introduce new ones.
Ok
Besides, this simply reading THD pointer from somewhere, while, in fact, it's unambigously defined by 'this'.
Can you use TABLE::in_use pointer?
I am not sure about it. There are some methods that use THD without directly accessing TABLE. Or, may be, inherit THD from
Temporary_tables, and then thd==this :)
Temporary_table methods also access some of the THD data members like rgi_slave, variables, etc. Or that pointer arithmetics, even (as above).
I turned that into a macro, but its gives the following warning : warning: invalid access to non-static data member ‘Open_tables_state::temporary_tables’ of NULL object [-Winvalid-offsetof] #define CURRENT_THD ((THD *)(((char*)this) - offsetof(THD, temporary_tables))) I believe offsetof is not considered safe for non-PODs? OTOH, I can change the class methods to accept THD * as a parameter. and that way we can get rid of caching this this redundant pointer. A bit ugly but works. Best, Nirbhay
Regards, Sergei Chief Architect MariaDB and security@mariadb.org