maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nirbhay!

On May 16, Nirbhay Choubey wrote:
> Hi Serg,
> 
> On Wed, Mar 30, 2016 at 7:12 AM, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx