maria-developers team mailing list archive

[Nirbhay Choubey <nirbhay@xxxxxxxxxxx>]

Hi Serg,

On Tue, May 17, 2016 at 10:26 AM, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx
>