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

[Øystein Grøvlen]

Jørgen Løland wrote:
> Ø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.

Fair enough.  I will approve the patch.

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

I do not think the WL cover my suggested approach.  It seems to suggest 
specific handling of partitioned MyISAM tables.  My suggestion is to 
handle all partitioned tables regardless of storage engine the same way.

...

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

It seems to me that you are creating the difference yourself here.  If 
you look at the definition of plugin_ref_to_se_ref:

#ifdef DBUG_OFF
#define plugin_ref_to_se_ref(A) (A)
#define se_ref_to_plugin_ref(A) (A)
#else
#define plugin_ref_to_se_ref(A) ((A) ? *(A) : NULL)
#define se_ref_to_plugin_ref(A) &(A)
#endif

The only thing it does is to dereference a pointer in debug builds. 
However, when using plugin_ref_to_se_ref, you create this pointer yourself:

+#ifdef DBUG_OFF
+  return plugin_ref_to_se_ref(hton2plugin[hton->slot]);
+#else
+  return plugin_ref_to_se_ref(&hton2plugin[hton->slot]);
+#endif

What is the purpose of the that?  The end result seems to be
hton2plugin[hton->slot] in both cases.

--
Øystein