← Back to team overview

maria-developers team mailing list archive

Re: Working on spider patches, MDEV-7698

 

Hi!

> Better to instead test MYSQL versions than have a define that is
> always declared.
> With current code, the last #else in spd_param.cc will never be used:

You right and I'll add code of testing MYSQL versions in Spider code later.

Thanks,
Kentoku


2016-11-24 23:07 GMT+09:00 Michael Widenius <michael.widenius@xxxxxxxxx>:
> Hi!
>
> On Wed, Nov 23, 2016 at 4:31 PM, kentoku <kentokushiba@xxxxxxxxx> wrote:
>> Hi Monty!
>>
>> Please see comment at 2016-05-07 19:15 for patches for MariaDB 10.2.
>>
>>> I don't think this it will work removing the usage of p_elem->connect_string
>>>
>>> This is because each partition may have a different connect string.
>>
>> I understand this behavior. But  I don't think this overwriting is a
>> good idea.
>
> The problem is that your patch causes a usage case of partitioning and
> federated_x to fail, which is proven by the usage case:
>
> fedarated_partion.test
>
> We can't add a patch that will cause current correct usage of
> partition and federated_x
> to fail.
>
>> Because when an user writes both table's connect string and
>> partition's connect string, table's connect string is lost.
>
> In which case is it lost ?
> Can you add a test to federated_partition.test that shows in which case the
> connection information is lost?
>
>> This behavior causes a confusion of an user.
>
> I don't see how you can use federated_x and partition without having a different
> connect string for each partition.  I also don't see how this is
> confusing for the end user.
>
> One of the original ideas with the partition engine is that each
> partition can have options
> that are different from the other partitions. For example, one
> partition could be with InnoDB, another with MyISAM.  For this to
> work, there needs to exist a mechanism for specifing options per
> partition, which the federated engine is indirectly using.
>
>> In the other hand, Spider can read both table's connect string and
>> partition's connect string without overwriting.
>
> Where should the connection strings for each partition be stored?
> this needs to work both with Spider but also with other usage of the
> partitioning engine.
>
>> Spider uses table's
>> connect string for common settings and partition's connect string for
>> partition specific settings.
>> This is why this patch removes overwriting logic of connect string.
>
> As far as I saw, the patch did remove all storing and reading of the
> connect strings for the partitions which causes the test to fail.
>
> Where does spider store the tables connect strings ?
> How do you suggest this should work when spider is not used?
> Can you create a patch that will not cause federated_partiton.test to fail?
>
>>> +#define PLUGIN_VAR_CAN_MEMALLOC
>>> +
>>>  #ifndef MYSQL_CLIENT
>>>
>>> The above is not needed, as all code that is testing this is doing:
>>
>> Please move it under "#define SPIDER_HEX_VERSION" in
>> "storage/spider/spd_include.h" for now. This is checked in Spider
>> code.
>>
>>> #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 100000
>>>
>>> Which is always true in MariaDB 10.x
>>
>> Is this in Spider code, right?
> Yes
>
>> This is needed for supporting other versions. Spider still supports MySQL 5.5.
>
> So you mean that PLUGIN_VAR_CAN_MEMALLOC is only needed for MySQL?
>
> Looking at the code in spd_param.cc, there is no need to have
> PLUGIN_VAR_CAN_MEMALLOC
>
> Better to instead test MYSQL versions than have a define that is
> always declared.
> With current code, the last #else in spd_param.cc will never be used:
>
> #ifdef PLUGIN_VAR_CAN_MEMALLOC
> static MYSQL_SYSVAR_STR(
>   remote_access_charset,
>   spider_remote_access_charset,
>   PLUGIN_VAR_MEMALLOC |
>   PLUGIN_VAR_RQCMDARG,
>   "Set remote access charset at connecting for improvement performance
> of connection if you know",
>   NULL,
>   NULL,
>   NULL
> );
> #else
>
> dead code
>
> Regards,
> Monty


References