Hi, Nirbhay! On May 16, Nirbhay Choubey wrote:
Hi Serg,
On Wed, Mar 30, 2016 at 7:12 AM, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nirbhay!
Here's a review of MDEV-5535. Sorry, it took a while - this was a big patch.
The approach is fine, my comments are mostly about details.
Thanks for the review and suggestions. I have tried to address them all, and while doing so, the original patch has changed quite a bit.
Sorry :)
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.
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.
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. Besides, this simply reading THD pointer from somewhere, while, in fact, it's unambigously defined by 'this'. Can you use TABLE::in_use pointer? Or, may be, inherit THD from Temporary_tables, and then thd==this :) Or that pointer arithmetics, even (as above). Regards, Sergei Chief Architect MariaDB and security@mariadb.org