Re: [Maria-developers] Working on spider patches, MDEV-7698
Hi! Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff +++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc 2015-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.cc 2015-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.h 2015-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 >= 100000 Which is always true in MariaDB 10.x Regards, Monty
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 >= 100000
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 <michael.widenius@gmail.com>:
Hi!
Now working on MDEV-7700 Spiral patch 002_mariadb-10.0.15.spider.diff
+++ ./002_mariadb-10.1.8-spider/sql/ha_partition.cc 2015-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.cc 2015-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.h 2015-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 >= 100000
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
Hi! On Wed, Nov 23, 2016 at 4:31 PM, kentoku <kentokushiba@gmail.com> 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
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 <michael.widenius@gmail.com>:
Hi!
On Wed, Nov 23, 2016 at 4:31 PM, kentoku <kentokushiba@gmail.com> 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
Hi! On Thu, Nov 24, 2016 at 8:21 PM, kentoku <kentokushiba@gmail.com> 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(&table->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
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 <michael.widenius@gmail.com>:
Hi!
On Thu, Nov 24, 2016 at 8:21 PM, kentoku <kentokushiba@gmail.com> 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(&table->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
Hi! On Wed, Nov 30, 2016 at 3:26 AM, kentoku <kentokushiba@gmail.com> wrote:
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.
Let's discuss this today when you are at my place. Some quick comments and questions: - Did you see my patch to remember table->s->connect_string, even if partition tables are using it. Why isn't this fixing the problem? About the patch:
diff -Narup ./server/sql/ha_partition.cc ./server-with-connect-string-patch/sql/ha_partition.cc --- ./server/sql/ha_partition.cc 2016-11-29 19:56:47.037326899 +0900 +++ ./server-with-connect-string-patch/sql/ha_partition.cc 2016-11-30 09:57:24.000000000 +0900 @@ -1515,7 +1515,10 @@ int ha_partition::prepare_new_partition( if ((error= set_up_table_before_create(tbl, part_name, create_info, p_elem))) goto error_create;
+ if (!(file->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION)) + { tbl->s->connect_string = p_elem->connect_string; + }
I am not sure that adding a HTON is the right way. Why isn't enough to in all cases just ensure that the handler for the underlaying tables are not changing the connect string given by the end user? This is what my patch is doing. One benefit of my patch is that we will remember the global the connect string in all cases, which this patch doesn't guarantee.
if ((error= file->ha_create(part_name, tbl, create_info))) { /* @@ -2554,10 +2560,13 @@ int ha_partition::set_up_table_before_cr } info->index_file_name= part_elem->index_file_name; info->data_file_name= part_elem->data_file_name; + if (!(m_file[0]->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION)) + { info->connect_string= part_elem->connect_string; if (info->connect_string.length) info->used_fields|= HA_CREATE_USED_CONNECTION; tbl->s->connect_string= part_elem->connect_string; + }
The above is probably wrong. If the partition engine had before store connect strings for the engine, it should be able to read them in the future too, even if the flags has changed value.
@@ -3488,11 +3497,17 @@ int ha_partition::open(const char *name, { create_partition_name(name_buff, name, name_buffer_ptr, NORMAL_PART_NAME, FALSE); + if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION)) + { table->s->connect_string = m_connect_string[(uint)(file-m_file)]; + }
I assume this is the real reason for your patch. You want to ensure that if the partition doesn't support connect strings and there was no connect strings given to the partition, we should send in the main table's partition string. ok, there is a point. I will fix this.
if ((error= (*file)->ha_open(table, name_buff, mode, test_if_locked | HA_OPEN_NO_PSI_CALL))) goto err_handler; + if (!((*file)->ht->flags & HTON_CAN_READ_CONNECT_STRING_IN_PARTITION)) + { bzero(&table->s->connect_string, sizeof(LEX_STRING)); + }
I think the above is always wrong. Better to always restore the connect_string to it's original value. <cut>
--- ./server/sql/sql_table.cc 2016-11-29 19:56:47.118326899 +0900 +++ ./server-with-connect-string-patch/sql/sql_table.cc 2016-11-30 09:47:28.000000000 +0900 @@ -7978,6 +7978,11 @@ mysql_prepare_alter_table(THD *thd, TABL create_info->comment.str= table->s->comment.str; create_info->comment.length= table->s->comment.length; } + if (!create_info->connect_string.str) + { + create_info->connect_string.str= table->s->connect_string.str; + create_info->connect_string.length= table->s->connect_string.length; + }
We should not need this as we have already in the same function: if (!(used_fields & HA_CREATE_USED_CONNECTION)) create_info->connect_string= table->s->connect_string;
diff -Narup ./server/sql/table.cc ./server-with-connect-string-patch/sql/table.cc --- ./server/sql/table.cc 2016-11-29 19:56:47.129326899 +0900 +++ ./server-with-connect-string-patch/sql/table.cc 2016-11-30 09:50:45.000000000 +0900 @@ -3758,6 +3758,7 @@ void update_create_info_from_table(HA_CR create_info->default_table_charset= share->table_charset; create_info->table_charset= 0; create_info->comment= share->comment; + create_info->connect_string= share->connect_string; create_info->transactional= share->transactional; create_info->page_checksum= share->page_checksum; create_info->option_list= share->option_list;
Why is the above needed? Do you have a test case that breaks if the above is not done? Regards, Monty
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@gmail.com>:
Hi!
On Wed, Nov 23, 2016 at 4:31 PM, kentoku <kentokushiba@gmail.com> 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
participants (2)
-
kentoku -
Michael Widenius