Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

Hi!

> Is this because the table ta_l is connected to a remote table trough
> spider that already has the
> rows in it ?

Yes.

> If yes, then it would be good to have a comment about this is in the
> test and also insert
> data that is different from the original one to make this part clear.

This test is create table with insert ignore test. So I am expecting
warnings. Previous test is create table with insert test, so I don't
think the insert data should be changed. But is it looks unclear?
Should I add any comments?

Thanks,
Kentoku


2016-11-25 0:18 GMT+09:00 Michael Widenius :
> Hi!
>
> On Wed, Nov 23, 2016 at 3:38 PM, kentoku  wrote:
>> Hi Monty!
>>
>> Thank you for starting this task. I appreciate it.
>>
>>> Kentoku, do you have patches for the test files, or should I just take
>>> them from the above spider branch or from somewhere else ?
>>
>> I just attached test files into MDEV-7698. Please use it. And please
>> let me know if you get a error from test. Sometimes, test results are
>> changed by patches. In this case I should check it.
>
> I will start working on applying the test files to 10.2-spider now
> I was mostly confused by that in the current 10.2 the test and result
> files doesn't match.
>
>>> I can't figure out,why we get the above warnings.
>>> This is from a patch we discussed at booking.com one year ago.  Any
>>> explanation for the above warnings would be appreciated.
>>>
>>> You can branch 10.2-spider and check the current state.
>>
>> O.K. I'll check it.
>
> Please send an email when you have figured out why we get the new
> warnings. This looks very strange.
>
> Here is the executed code with warnings:
> mysql-test-run spider/handler.basic_sql
>
> CREATE TABLE ta_l (
> PRIMARY KEY(a)
> ) ENGINE=Spider DEFAULT CHARSET=utf8 COMMENT='database
> "auto_test_remote", table "ta_r"'
> CONNECTION='host "localhost", socket
> "/home/my/maria-10.2-spider/mysql-test/var/tmp/mysqld.2.1.sock", user
> "root",
> password ""'
> IGNORE SELECT a, b, c FROM tb_l;
> Warnings:
> Warning1062Duplicate entry '1' for key 'PRIMARY'
> Warning1062Duplicate entry '2' for key 'PRIMARY'
> Warning1062Duplicate entry '3' for key 'PRIMARY'
> Warning1062Duplicate entry '4' for key 'PRIMARY'
> Warning1062Duplicate entry '5' for key 'PRIMARY
>
> But when we do a select from the table, it looks like all rows are there.
>
> Is this because the table ta_l is connected to a remote table trough
> spider that already has the
> rows in it ?
> If yes, then it would be good to have a comment about this is in the
> test and also insert
> data that is different from the original one to make this part clear.
>
> Regards,
> Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

Hi!

> All spider tests now passes in 10.2

Great!

> This fails as the above variable doesn't yet exists.
> I will update the test results as soon the variable exists again.

ok.

Thanks,
Kentoku


2016-11-25 0:55 GMT+09:00 Michael Widenius :
> Hi!
>
> On Thu, Nov 24, 2016 at 5:18 PM, Michael Widenius
>  wrote:
>> Hi!
>>
>> On Wed, Nov 23, 2016 at 3:38 PM, kentoku  wrote:
>>> Hi Monty!
>>>
>>> Thank you for starting this task. I appreciate it.
>>>
 Kentoku, do you have patches for the test files, or should I just take
 them from the above spider branch or from somewhere else ?
>>>
>>> I just attached test files into MDEV-7698. Please use it. And please
>>> let me know if you get a error from test. Sometimes, test results are
>>> changed by patches. In this case I should check it.
>
> All spider tests now passes in 10.2
>
> I removed a few changes in test results that was affected by some of
> the not yet pushed patches, like:
>
>   SHOW STATUS LIKE 'Spider_direct_aggregate';
>   Variable_name Value
> ! Spider_direct_aggregate   1
>
> This fails as the above variable doesn't yet exists.
> I will update the test results as soon the variable exists again.
>
> Thanks!
>
> Regards,
> Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

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 :
> Hi!
>
> On Wed, Nov 23, 2016 at 4:31 PM, kentoku  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 >= 10
>>>
>>> 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 : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

Hi!

> If this is ok, I will add this and close MDEV-7700

It looks connect string is visible, but never used. This is different
from what I think.
I just added patch for connect string to MDEV-7698. Please review it.
This changes do not cause error for federated_partition test.

Thanks,
Kentoku

2016-11-28 20:31 GMT+09:00 Michael Widenius :
> Hi!
>
> On Thu, Nov 24, 2016 at 8:21 PM, kentoku  wrote:
>> Hi!
>>
>>> 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?
>>
>> when create a table like the following
>>
>> create table tbl_a(
>>   col_a int,
>>   primary key(col_a)
>> )engine=myisam connection 'some settings for whole table'
>> partition by range(col_a)
>> (
>>   partition pt1 values less than (100) connection 'some settings for pt1',
>>   partition pt2 values less than maxvalue connection 'some settings for pt2'
>> );
>>
>> "connection 'some settings for whole table'" is lost. It is not only
>> behavior of federated_x.
>>
>>> 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.
>>
>> Does it means all options have to write with partition information?
>
> All options that you want to see in SHOW CREATE.
>
>> How about "engine"? Don't you think it is useful if default engine can
>> write as table option and only partition specific engines are write as
>> partition option? I thought just like this about connect string, too.
>
> Normally you don't need to write special options for each partition. In
> this case federatedx is a special case.
>
>>> Where should the connection strings for each partition be stored?
>>
>> I think this is already stored in par file. This is not problem.
>>
>>> this needs to work both with Spider but also with other usage of the
>>> partitioning engine.
>>
>> O.K. I understand.
>>
>>> Where does spider store the tables connect strings ?
>>
>> Spider doesn't store it. It's in frm file.
>>
>>> How do you suggest this should work when spider is not used?
>>
>> I suggest ...
>> These connect strings have priority level. Connect strings of
>> sub-partition are the first priority. Connect strings of partition are
>> the second priority. Connect string of table is the third priority.
>>
>>> Can you create a patch that will not cause federated_partiton.test to fail?
>>
>> Yes, I think I can. I'll create it.
>
> Does this patch do what you need:
>
> This preserve the CONNECT string from the top table level as it was
> originally entered.
>
> I tested this by doing the following create:
>
> create table t1 (s1 int primary key) engine=federated connection="QQQ"
>   partition by list (s1)
>   (partition p1 values in (1,3)
>  connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
>partition p2 values in (2,4)
>  connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');
>
> and show create table was able to show the original connection="QQQ"
>
> --- a/sql/ha_partition.cc
> +++ b/sql/ha_partition.cc
> @@ -3486,13 +3486,15 @@ int ha_partition::open(const char *name, int
> mode, uint test_if_locked)
> file= m_file;
> do
> {
> +  LEX_STRING save_connect_string= table->s->connect_string;
>create_partition_name(name_buff, name, name_buffer_ptr, 
> NORMAL_PART_NAME,
>  FALSE);
>table->s->connect_string = m_connect_string[(uint)(file-m_file)];
> -  if ((error= (*file)->ha_open(table, name_buff, mode,
> -   test_if_locked | HA_OPEN_NO_PSI_CALL)))
> +  error= (*file)->ha_open(table, name_buff, mode,
> +  test_if_locked | HA_OPEN_NO_PSI_CALL);
> +  table->s->connect_string= save_connect_string;
> +  if (error)
>  goto err_handler;
> -  bzero(>s->connect_string, sizeof(LEX_STRING));
>if (m_file == file)
>  m_num_locks= (*file)->lock_count();
>DBUG_ASSERT(m_num_locks == (*file)->lock_count());
>
> If this is ok, I will add this and close MDEV-7700
>
> Regards,
> Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Thu, Nov 24, 2016 at 8:21 PM, kentoku  wrote:
> Hi!
>
>> 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?
>
> when create a table like the following
>
> create table tbl_a(
>   col_a int,
>   primary key(col_a)
> )engine=myisam connection 'some settings for whole table'
> partition by range(col_a)
> (
>   partition pt1 values less than (100) connection 'some settings for pt1',
>   partition pt2 values less than maxvalue connection 'some settings for pt2'
> );
>
> "connection 'some settings for whole table'" is lost. It is not only
> behavior of federated_x.
>
>> 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.
>
> Does it means all options have to write with partition information?

All options that you want to see in SHOW CREATE.

> How about "engine"? Don't you think it is useful if default engine can
> write as table option and only partition specific engines are write as
> partition option? I thought just like this about connect string, too.

Normally you don't need to write special options for each partition. In
this case federatedx is a special case.

>> Where should the connection strings for each partition be stored?
>
> I think this is already stored in par file. This is not problem.
>
>> this needs to work both with Spider but also with other usage of the
>> partitioning engine.
>
> O.K. I understand.
>
>> Where does spider store the tables connect strings ?
>
> Spider doesn't store it. It's in frm file.
>
>> How do you suggest this should work when spider is not used?
>
> I suggest ...
> These connect strings have priority level. Connect strings of
> sub-partition are the first priority. Connect strings of partition are
> the second priority. Connect string of table is the third priority.
>
>> Can you create a patch that will not cause federated_partiton.test to fail?
>
> Yes, I think I can. I'll create it.

Does this patch do what you need:

This preserve the CONNECT string from the top table level as it was
originally entered.

I tested this by doing the following create:

create table t1 (s1 int primary key) engine=federated connection="QQQ"
  partition by list (s1)
  (partition p1 values in (1,3)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
   partition p2 values in (2,4)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');

and show create table was able to show the original connection="QQQ"

--- a/sql/ha_partition.cc
+++ b/sql/ha_partition.cc
@@ -3486,13 +3486,15 @@ int ha_partition::open(const char *name, int
mode, uint test_if_locked)
file= m_file;
do
{
+  LEX_STRING save_connect_string= table->s->connect_string;
   create_partition_name(name_buff, name, name_buffer_ptr, NORMAL_PART_NAME,
 FALSE);
   table->s->connect_string = m_connect_string[(uint)(file-m_file)];
-  if ((error= (*file)->ha_open(table, name_buff, mode,
-   test_if_locked | HA_OPEN_NO_PSI_CALL)))
+  error= (*file)->ha_open(table, name_buff, mode,
+  test_if_locked | HA_OPEN_NO_PSI_CALL);
+  table->s->connect_string= save_connect_string;
+  if (error)
 goto err_handler;
-  bzero(>s->connect_string, sizeof(LEX_STRING));
   if (m_file == file)
 m_num_locks= (*file)->lock_count();
   DBUG_ASSERT(m_num_locks == (*file)->lock_count());

If this is ok, I will add this and close MDEV-7700

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

Hi!

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

when create a table like the following

create table tbl_a(
  col_a int,
  primary key(col_a)
)engine=myisam connection 'some settings for whole table'
partition by range(col_a)
(
  partition pt1 values less than (100) connection 'some settings for pt1',
  partition pt2 values less than maxvalue connection 'some settings for pt2'
);

"connection 'some settings for whole table'" is lost. It is not only
behavior of federated_x.

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

Does it means all options have to write with partition information?
How about "engine"? Don't you think it is useful if default engine can
write as table option and only partition specific engines are write as
partition option? I thought just like this about connect string, too.

> Where should the connection strings for each partition be stored?

I think this is already stored in par file. This is not problem.

> this needs to work both with Spider but also with other usage of the
> partitioning engine.

O.K. I understand.

> Where does spider store the tables connect strings ?

Spider doesn't store it. It's in frm file.

> How do you suggest this should work when spider is not used?

I suggest ...
These connect strings have priority level. Connect strings of
sub-partition are the first priority. Connect strings of partition are
the second priority. Connect string of table is the third priority.

> Can you create a patch that will not cause federated_partiton.test to fail?

Yes, I think I can. I'll create it.

Thanks,
Kentoku


2016-11-24 23:07 GMT+09:00 Michael Widenius :
> Hi!
>
> On Wed, Nov 23, 2016 at 4:31 PM, kentoku  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 >= 10
>>>
>>> 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 

Re: [Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Thu, Nov 24, 2016 at 5:18 PM, Michael Widenius
 wrote:
> Hi!
>
> On Wed, Nov 23, 2016 at 3:38 PM, kentoku  wrote:
>> Hi Monty!
>>
>> Thank you for starting this task. I appreciate it.
>>
>>> Kentoku, do you have patches for the test files, or should I just take
>>> them from the above spider branch or from somewhere else ?
>>
>> I just attached test files into MDEV-7698. Please use it. And please
>> let me know if you get a error from test. Sometimes, test results are
>> changed by patches. In this case I should check it.

All spider tests now passes in 10.2

I removed a few changes in test results that was affected by some of
the not yet pushed patches, like:

  SHOW STATUS LIKE 'Spider_direct_aggregate';
  Variable_name Value
! Spider_direct_aggregate   1

This fails as the above variable doesn't yet exists.
I will update the test results as soon the variable exists again.

Thanks!

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Wed, Nov 23, 2016 at 3:38 PM, kentoku  wrote:
> Hi Monty!
>
> Thank you for starting this task. I appreciate it.
>
>> Kentoku, do you have patches for the test files, or should I just take
>> them from the above spider branch or from somewhere else ?
>
> I just attached test files into MDEV-7698. Please use it. And please
> let me know if you get a error from test. Sometimes, test results are
> changed by patches. In this case I should check it.

I will start working on applying the test files to 10.2-spider now
I was mostly confused by that in the current 10.2 the test and result
files doesn't match.

>> I can't figure out,why we get the above warnings.
>> This is from a patch we discussed at booking.com one year ago.  Any
>> explanation for the above warnings would be appreciated.
>>
>> You can branch 10.2-spider and check the current state.
>
> O.K. I'll check it.

Please send an email when you have figured out why we get the new
warnings. This looks very strange.

Here is the executed code with warnings:
mysql-test-run spider/handler.basic_sql

CREATE TABLE ta_l (
PRIMARY KEY(a)
) ENGINE=Spider DEFAULT CHARSET=utf8 COMMENT='database
"auto_test_remote", table "ta_r"'
CONNECTION='host "localhost", socket
"/home/my/maria-10.2-spider/mysql-test/var/tmp/mysqld.2.1.sock", user
"root",
password ""'
IGNORE SELECT a, b, c FROM tb_l;
Warnings:
Warning1062Duplicate entry '1' for key 'PRIMARY'
Warning1062Duplicate entry '2' for key 'PRIMARY'
Warning1062Duplicate entry '3' for key 'PRIMARY'
Warning1062Duplicate entry '4' for key 'PRIMARY'
Warning1062Duplicate entry '5' for key 'PRIMARY

But when we do a select from the table, it looks like all rows are there.

Is this because the table ta_l is connected to a remote table trough
spider that already has the
rows in it ?
If yes, then it would be good to have a comment about this is in the
test and also insert
data that is different from the original one to make this part clear.

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

On Wed, Nov 23, 2016 at 4:31 PM, kentoku  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 >= 10
>>
>> 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 : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

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. Because when an user writes both table's connect string and
partition's connect string, table's connect string is lost. This
behavior causes a confusion of an user.
In the other hand, Spider can read both table's connect string and
partition's connect string without overwriting. 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 my_free is safe to call with NULL, the above is not needed

ok

> +#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 >= 10
>
> Which is always true in MariaDB 10.x

Is this in Spider code, right?
This is needed for supporting other versions. Spider still supports MySQL 5.5.

Thanks,
Kentoku


2016-11-22 3:25 GMT+09:00 Michael Widenius :
> Hi!
>
> Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff
>
> +++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc2015-10-14
> 01:48:53.392665313 +0900
> @@ -327,7 +327,9 @@ void ha_partition::init_handler_variable
>m_file_buffer= NULL;
>m_name_buffer_ptr= NULL;
>m_engine_array= NULL;
> +/*
>m_connect_string= NULL;
> +*/
>m_file= NULL;
>m_file_tot_parts= 0;
>m_reorged_file= NULL;
> @@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition(
>if ((error= set_up_table_before_create(tbl, part_name, create_info, 
> p_elem)))
>  goto error_create;
>
> +/*
>tbl->s->connect_string = p_elem->connect_string;
> +*/
>
>
> 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.
>
> Here is an example from fedarated_partion.test:
>
> eval create table t1 (s1 int primary key) engine=federated
>   partition by list (s1)
>   (partition p1 values in (1,3)
>  connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
>partition p2 values in (2,4)
>  connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');
>
> The above works in mariadb 10.2 but not in your spider tree.
>
> From this patch I will, for now, only take the code related to
>
> HA_EXTRA_WRITE_CAN_REPLACE
> HA_EXTRA_WRITE_CANNOT_REPLACE
>
> --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc
> 2015-10-13 23:49:10.188129839 +0900
> +++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc2015-10-14
> 01:48:54.296665317 +0900
> @@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st
>*(char**) tgt= my_strdup(value, MYF(0));
>  else
>*(char**) tgt= 0;
> -my_free(old);
> +if (old)
> +  my_free(old);
>
> As my_free is safe to call with NULL, the above is not needed
>
> diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
> ./002_mariadb-10.1.8-spider/sql/sql_priv.h
> --- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
> 2015-10-13 23:49:10.189129839 +0900
> +++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h2015-10-14
> 01:48:54.642665315 +0900
> @@ -27,6 +27,8 @@
>  #ifndef SQL_PRIV_INCLUDED
>  #define SQL_PRIV_INCLUDED
>
> +#define PLUGIN_VAR_CAN_MEMALLOC
> +
>  #ifndef MYSQL_CLIENT
>
> The above is not needed, as all code that is testing this is doing:
>
> #if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 10
>
> Which is always true in MariaDB 10.x
>
> Regards,
> Monty
>
> ___
> Mailing list: https://launchpad.net/~maria-developers
> Post to : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[kentoku]

Hi Monty!

Thank you for starting this task. I appreciate it.

> Kentoku, do you have patches for the test files, or should I just take
> them from the above spider branch or from somewhere else ?

I just attached test files into MDEV-7698. Please use it. And please
let me know if you get a error from test. Sometimes, test results are
changed by patches. In this case I should check it.

> I can't figure out,why we get the above warnings.
> This is from a patch we discussed at booking.com one year ago.  Any
> explanation for the above warnings would be appreciated.
>
> You can branch 10.2-spider and check the current state.

O.K. I'll check it.

Thanks,
Kentoku


2016-11-21 17:30 GMT+09:00 Michael Widenius :
> Hi!
>
> I have now started to work on the spider patches for MariaDB 10.2, MDEV-7698.
>
> I have moved all code from maria-10.1-spider to a new branch
> maria-10.2-spider and added some more patches.
> I have closed all related MDEV's in MDEV-7698 that is now included in
> 10.2-spider.
>
> While doing this, I noticed that spider/handler test was not included
> in the test suite. I added the missing suite.pm and suite.opt files
> and got the tests to work.
>
> However, when I tried to run test to verify my changes, I noticed that
> a lot of test in spider/handler where failing:
>
> mysql-test-run --suite=spider/handler
> produces these failures:
>
> spider/handler.spider3_fixes spider/handler.direct_aggregate
> spider/handler.direct_update spider/handler.spider_fixes
> spider/handler.function spider/handler.ha spider/handler.vp_fixes
>
> All failures are because .test and .result file doesn't match.
>
> I checked the patch file:
> http://spiderformysql.com/downloads/spider-3.2/patch_mariadb-10.1.8.tgz
> but this doesn't include any updates to the handler test files:
> grep mysql_test *  returns nothing.
>
> However the .tar file:
> http://spiderformysql.com/downloads/spider-3.2/mariadb-10.1.8-spider-3.2-vp-1.1.tgz
> Contains a lot of updated .test and .result files.
>
> Kentoku, do you have patches for the test files, or should I just take
> them from the above spider branch or from somewhere else ?
>
> Another question:
> After applying the patches:
>
> 013_mariadb-10.0.15.vp_handler.diff
> 034_mariadb-10.0.15.vp_handler2.diff
> 005_mariadb-10.0.15.hs.diff
> 041_mariadb-10.0.15.vp_handler2.diff
>
> I get the following change in spider/handler/basic_sql.result:
>
> --- a/storage/spider/mysql-test/spider/handler/r/basic_sql.result
> +++ b/storage/spider/mysql-test/spider/handler/r/basic_sql.result
> @@ -70,6 +70,12 @@ CREATE TABLE ta_l (
>  PRIMARY KEY(a)
>  ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT_2_1
>  IGNORE SELECT a, b, c FROM tb_l
> +Warnings:
> +Warning1062Duplicate entry '1' for key 'PRIMARY'
> +Warning1062Duplicate entry '2' for key 'PRIMARY'
> +Warning1062Duplicate entry '3' for key 'PRIMARY'
> +Warning1062Duplicate entry '4' for key 'PRIMARY'
> +Warning1062Duplicate entry '5' for key 'PRIMARY'
>
> I can't figure out,why we get the above warnings.
> This is from a patch we discussed at booking.com one year ago.  Any
> explanation for the above warnings would be appreciated.
>
> You can branch 10.2-spider and check the current state.
>
> Regards,
> Monty
>
> ___
> Mailing list: https://launchpad.net/~maria-developers
> Post to : maria-developers@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

Re: [Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff

+++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc2015-10-14
01:48:53.392665313 +0900
@@ -327,7 +327,9 @@ void ha_partition::init_handler_variable
   m_file_buffer= NULL;
   m_name_buffer_ptr= NULL;
   m_engine_array= NULL;
+/*
   m_connect_string= NULL;
+*/
   m_file= NULL;
   m_file_tot_parts= 0;
   m_reorged_file= NULL;
@@ -1516,4 +1518,6 @@ int ha_partition::prepare_new_partition(
   if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem)))
 goto error_create;

+/*
   tbl->s->connect_string = p_elem->connect_string;
+*/


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.

Here is an example from fedarated_partion.test:

eval create table t1 (s1 int primary key) engine=federated
  partition by list (s1)
  (partition p1 values in (1,3)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_1',
   partition p2 values in (2,4)
 connection='mysql://root@127.0.0.1:$SLAVE_MYPORT/federated/t1_2');

The above works in mariadb 10.2 but not in your spider tree.

>From this patch I will, for now, only take the code related to

HA_EXTRA_WRITE_CAN_REPLACE
HA_EXTRA_WRITE_CANNOT_REPLACE

--- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_plugin.cc
2015-10-13 23:49:10.188129839 +0900
+++ ./002_mariadb-10.1.8-spider/sql/sql_plugin.cc2015-10-14
01:48:54.296665317 +0900
@@ -2757,6 +2757,7 @@ static void update_func_str(THD *thd, st
   *(char**) tgt= my_strdup(value, MYF(0));
 else
   *(char**) tgt= 0;
-my_free(old);
+if (old)
+  my_free(old);

As my_free is safe to call with NULL, the above is not needed

diff -Narup ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
./002_mariadb-10.1.8-spider/sql/sql_priv.h
--- ./001_mariadb-10.1.8-partition_cond_push/sql/sql_priv.h
2015-10-13 23:49:10.189129839 +0900
+++ ./002_mariadb-10.1.8-spider/sql/sql_priv.h2015-10-14
01:48:54.642665315 +0900
@@ -27,6 +27,8 @@
 #ifndef SQL_PRIV_INCLUDED
 #define SQL_PRIV_INCLUDED

+#define PLUGIN_VAR_CAN_MEMALLOC
+
 #ifndef MYSQL_CLIENT

The above is not needed, as all code that is testing this is doing:

#if defined(MARIADB_BASE_VERSION) && MYSQL_VERSION_ID >= 10

Which is always true in MariaDB 10.x

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

[Maria-developers] Working on spider patches, MDEV-7698

[Michael Widenius]

Hi!

I have now started to work on the spider patches for MariaDB 10.2, MDEV-7698.

I have moved all code from maria-10.1-spider to a new branch
maria-10.2-spider and added some more patches.
I have closed all related MDEV's in MDEV-7698 that is now included in
10.2-spider.

While doing this, I noticed that spider/handler test was not included
in the test suite. I added the missing suite.pm and suite.opt files
and got the tests to work.

However, when I tried to run test to verify my changes, I noticed that
a lot of test in spider/handler where failing:

mysql-test-run --suite=spider/handler
produces these failures:

spider/handler.spider3_fixes spider/handler.direct_aggregate
spider/handler.direct_update spider/handler.spider_fixes
spider/handler.function spider/handler.ha spider/handler.vp_fixes

All failures are because .test and .result file doesn't match.

I checked the patch file:
http://spiderformysql.com/downloads/spider-3.2/patch_mariadb-10.1.8.tgz
but this doesn't include any updates to the handler test files:
grep mysql_test *  returns nothing.

However the .tar file:
http://spiderformysql.com/downloads/spider-3.2/mariadb-10.1.8-spider-3.2-vp-1.1.tgz
Contains a lot of updated .test and .result files.

Kentoku, do you have patches for the test files, or should I just take
them from the above spider branch or from somewhere else ?

Another question:
After applying the patches:

013_mariadb-10.0.15.vp_handler.diff
034_mariadb-10.0.15.vp_handler2.diff
005_mariadb-10.0.15.hs.diff
041_mariadb-10.0.15.vp_handler2.diff

I get the following change in spider/handler/basic_sql.result:

--- a/storage/spider/mysql-test/spider/handler/r/basic_sql.result
+++ b/storage/spider/mysql-test/spider/handler/r/basic_sql.result
@@ -70,6 +70,12 @@ CREATE TABLE ta_l (
 PRIMARY KEY(a)
 ) MASTER_1_ENGINE MASTER_1_CHARSET MASTER_1_COMMENT_2_1
 IGNORE SELECT a, b, c FROM tb_l
+Warnings:
+Warning1062Duplicate entry '1' for key 'PRIMARY'
+Warning1062Duplicate entry '2' for key 'PRIMARY'
+Warning1062Duplicate entry '3' for key 'PRIMARY'
+Warning1062Duplicate entry '4' for key 'PRIMARY'
+Warning1062Duplicate entry '5' for key 'PRIMARY'

I can't figure out,why we get the above warnings.
This is from a patch we discussed at booking.com one year ago.  Any
explanation for the above warnings would be appreciated.

You can branch 10.2-spider and check the current state.

Regards,
Monty

___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp