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

[Jørgen Løland]

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

I see your point. However, I think this suggestion is best thought of as 
another alternative to the mentioned WL. I.e., the result of the WL will 
be one of these alternatives (unless there are even more suggestions):

1. Modify the existing MyISAM native driver to handle partitioned tables
2. Add a new 'Partitioned MyISAM' driver. The new partitioned driver 
would lend a lot of functionality from the existing native driver
3. Add a generic partitioned driver. It will be used for any partitioned 
table and will make use of the most appropriate driver for each of the 
partitions.


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

You are absolutely right. I'll remove the method.

-- 
Jørgen Løland