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

[Jørgen Løland]

Øystein,

Thanks for good feedback. See my comments inline

Øystein Grøvlen wrote:
> 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?

Calling h->start_consistent_snapshot to check if the engine is
Falcon/InnoDB is consistent with how it's done in be_snapshot.h. It may 
be that this (and other functionality) would belong in a new version of 
si_objects, but I think we need to think the design through before 
modifying it at this stage.

Sticking to 'consistent with existing code' of OK with you.

> 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?

Yes, a WL has been created for this. See
https://intranet.mysql.com/worklog/Backup-BackLog/index.pl?tid=4651

>  - 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.

It is not covered by MyISAM version because this bug was specific to 
default driver for partitioned Falcon. However, for now it is not 
possible to select default driver for partitioned falcon, so the test 
will run with snapshot no matter what I do. Deleting test-stub from 
backup_default as suggested.

> 2. Add partition definition to create statement [1]

Agree.

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

Because of the strange difference between plugin_ref type in 
debug/non-debug builds, I'll stick to the method. However, I agree with 
you that this behavior is strange at best...

> 4. Consider rephrasing comment [3]

Will do

> 
> 
> 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.
> 


-- 
Jørgen Løland