Re: bzr commit into mysql-6.0-backup branch (jorgen.loland:2746) Bug#38426

[Øystein Grøvlen]

STATUS
======

Looks good, but I have request for an answer to an issue.

REQUESTS
========

1. It seems to me that the checking of underlying storage engines for
    partitions is something that would belong to the SI objects
    interface.  Any good reasons it is not a functionality of that
    interface?

COMMENTARY
==========

  - I would think a more general solutions would be to request
    references to the table_ids for the partitions and tell the drivers
    to backup them like ordinary tables.  Has that been considered?

  - There is a partly disable partition test in backup_default.test.
    Would this be an opportunity to clean this up?  I also fear that
    this patch causes that test to no longer be a regression test for
    the original problem since I guess that may be related to using the
    default engine. (In addition, the test case does not belong in
    backup_default anymore if it does not use the default engine.)

SUGGESTIONS
===========

1. Remove partition test from backup_default.  It seems to be covered
    by MyIsam version of new test.

2. Add partition definition to create statement [1]

3. Drop get_hton_se(). If not, drop one empty line after func [2]

4. Consider rephrasing comment [3]


See comments inline:


Jorgen Loland wrote:

...

 > +CREATE TABLE backup_part.t1 (
 > +  t1_autoinc INTEGER NOT NULL AUTO_INCREMENT,
 > +  t1_uuid CHAR(36),
 > +  PRIMARY KEY (t1_autoinc)
 > +);
 > +ALTER TABLE backup_part.t1 PARTITION BY HASH(t1_autoinc);

[1] You do not need a separate ALTER TABLE to define partitions.  This
can be part of CREATE TABLE (see backup_default.test for an example).

...

 >  inline
 > +storage_engine_ref get_hton_se(const handlerton *hton)
 > +{
 > +  DBUG_ASSERT(hton);
 > +#ifdef DBUG_OFF
 > +  return plugin_ref_to_se_ref(hton2plugin[hton->slot]);
 > +#else
 > +  return plugin_ref_to_se_ref(&hton2plugin[hton->slot]);
 > +#endif
 > +}
 > +
 > +

[2] I do not understand the purpose of this function. If !DBUG_OFF,
you add a reference for plugin_ref_to_se_ref to peal off.  Why not access
hton2plugin directly?

For some stupid reason, our code standard require only one empty lines
between functions.  I know two lines look better, but I guess we need
to obey (or alternatively protest by writing a public blog about it
;-) )

...

 > +              /*
 > +                 Only snapshot driver supports partitioning. Use
 > +                 default driver for all non-MVCC storage engines.
 > +               */

[3] I think this comment is a bit inaccurate.  It is not only
the snapshot driver that supports partitioning.  The default driver
does, too, and so would any other driver that is based on the server's
SQL interface.  The point is that storage engines know
nothing about partitions.  Hence, one can not expect native drivers
would be able to deal with a partitioned table.  In other words,
instead of handling the consistent_snapshot driver as a special case,
one could have solved this by handling native drivers as a special
case.

-- 
Øystein