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