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 <[email protected]>: > Hi! > > On Wed, Nov 23, 2016 at 4:31 PM, kentoku <[email protected]> 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
