[Maria-developers] Working on spider patches, MDEV-7701
Hi! Next patch, MDEV 7701 +++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc 2016-05-05 21:25:04.289731712 +0900 @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu } /* Category 9) Operations only used by MERGE */ case HA_EXTRA_ADD_CHILDREN_LIST: + DBUG_RETURN(loop_extra(operation)); case HA_EXTRA_ATTACH_CHILDREN: + int result; + handler **file; + if ((result = loop_extra(operation))) + DBUG_RETURN(result); + m_num_locks = 0; + additional_table_flags = + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE); + file = m_file; + do + { + m_num_locks += (*file)->lock_count(); + additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^ + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE))); The above test could be simple: additional_table_flags &= (ulonglong) ((*file)->ha_table_flags(); Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below. I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much. @@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons handler **file= m_file; COND *res_cond = NULL; DBUG_ENTER("ha_partition::cond_push"); +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS + if (set_top_table_fields) + { + do + { + if ((*file)->set_top_table_and_fields(top_table, + top_table_field, + top_table_fields)) + DBUG_RETURN(cond); + } while (*(++file)); + file= m_file; + } +#endif I added this without the define, but I merged it to the loop for cond_push. Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version. However, I would like to have a clear comment the purpose of the function set_top_table_and_fields(). struct st_mysql_storage_engine partition_storage_engine= { MYSQL_HANDLERTON_INTERFACE_VERSION }; +++ ./003_mariadb-10.2.0-vp/sql/handler.h 2016-05-05 21:25:04.314731713 +0900 @@ -255,6 +257,11 @@ enum enum_alter_inplace_result { */ #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE) +#define HA_CAN_BG_SEARCH (1LL << 45) +#define HA_CAN_BG_INSERT (1LL << 46) +#define HA_CAN_BG_UPDATE (1LL << 47) +#define HA_CAN_MULTISTEP_MERGE (1LL << 48) Do you really need HA_CAN_BG_XXX flags? I checked your spider tree and this is not used anywhere, so I think we should leave them out for now. If we don't need these, we can remove the loop for setting them in HA_EXTRA_ATTACH_CHILDREN too. If we need them, I would like to have a comment for why they are needed and when we will need then add these in a separate patch. @@ -3525,6 +3540,29 @@ public: Pops the top if condition stack, if stack is not empty. */ virtual void cond_pop() { return; }; + virtual int set_top_table_and_fields(TABLE *top_table, + Field **top_table_field, + uint top_table_fields) + { + if (!set_top_table_fields) + { + set_top_table_fields = TRUE; + this->top_table = top_table; + this->top_table_field = top_table_field; + this->top_table_fields = top_table_fields; + } + return 0; + } Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void. I will make it void for now and change the relevant code. We can make it int again if needed in the future. diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc ./003_mariadb-10.2.0-vp/sql/sql_base.cc --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc 2016-04-17 02:47:27.000000000 +0900 +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc 2016-05-05 21:25:04.319731713 +0900 @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table table= table->find_table_for_update(); - if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM) - { - TABLE_LIST *child; + if (table->table && + ( + table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM || + (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE) + ) + ) + continue I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler. This is now pushed into 10.2-spider Regards, Monty
Hi!
The above test could be simple:
additional_table_flags &= (ulonglong) ((*file)->ha_table_flags();
Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below.
ok.
I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much.
ok.
I added this without the define, but I merged it to the loop for cond_push.
ok.
Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version.
ok. Thanks.
However, I would like to have a clear comment the purpose of the function set_top_table_and_fields().
This function is used to get correlating of a parent (table/column) and children (table/column). When conditions are pushed down to child table (like child of myisammarge), child table needs to know about which table/column is my parent for understanding conditions.
Do you really need HA_CAN_BG_XXX flags?
No, I don't. I planed to use this flags, but this flags are not used yet. So, it's ok to remove for now. I'll add it when it is needed.
+ virtual int set_top_table_and_fields(TABLE *top_table, ... cut ... Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void.
I will make it void for now and change the relevant code. We can make it int again if needed in the future.
VP allocates some memories in this function. So this function has possibility of causing out of memory error. That's why this function returns int.
I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler.
ok. And now everyone can create myisammarge type storage engine! Regards, Kentoku 2016-11-28 19:44 GMT+09:00 Michael Widenius <michael.widenius@gmail.com>:
Hi!
Next patch, MDEV 7701
+++ ./003_mariadb-10.2.0-vp/sql/ha_partition.cc 2016-05-05 21:25:04.289731712 +0900 @@ -7238,13 +7239,30 @@ int ha_partition::extra(enum ha_extra_fu } /* Category 9) Operations only used by MERGE */ case HA_EXTRA_ADD_CHILDREN_LIST: + DBUG_RETURN(loop_extra(operation)); case HA_EXTRA_ATTACH_CHILDREN: + int result; + handler **file; + if ((result = loop_extra(operation))) + DBUG_RETURN(result); + m_num_locks = 0; + additional_table_flags = + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE); + file = m_file; + do + { + m_num_locks += (*file)->lock_count(); + additional_table_flags &= ~((ulonglong) ((*file)->ha_table_flags() ^ + (HA_CAN_BG_SEARCH | HA_CAN_BG_INSERT | HA_CAN_BG_UPDATE)));
The above test could be simple:
additional_table_flags &= (ulonglong) ((*file)->ha_table_flags();
Should be the same thing. I did however remove this code, as HA_CAN_BG... are not used anywhere in the current spider code. See comment below.
I also changed so that m_num_locks always contains the upper limit number of locks needed. Without this change, the call to lock_count() would have returneed m_total_parts * 'new-counted-locks' which would be been way too much.
@@ -9123,6 +9141,19 @@ const COND *ha_partition::cond_push(cons handler **file= m_file; COND *res_cond = NULL; DBUG_ENTER("ha_partition::cond_push"); +#ifdef HANDLER_HAS_TOP_TABLE_FIELDS + if (set_top_table_fields) + { + do + { + if ((*file)->set_top_table_and_fields(top_table, + top_table_field, + top_table_fields)) + DBUG_RETURN(cond); + } while (*(++file)); + file= m_file; + } +#endif
I added this without the define, but I merged it to the loop for cond_push.
Instead of having defines in the sql code, I will fix that in spider and vp that I will set the proper defines based on the MariaDB version.
However, I would like to have a clear comment the purpose of the function set_top_table_and_fields().
struct st_mysql_storage_engine partition_storage_engine= { MYSQL_HANDLERTON_INTERFACE_VERSION };
+++ ./003_mariadb-10.2.0-vp/sql/handler.h 2016-05-05 21:25:04.314731713 +0900 @@ -255,6 +257,11 @@ enum enum_alter_inplace_result { */ #define HA_BINLOG_FLAGS (HA_BINLOG_ROW_CAPABLE | HA_BINLOG_STMT_CAPABLE)
+#define HA_CAN_BG_SEARCH (1LL << 45) +#define HA_CAN_BG_INSERT (1LL << 46) +#define HA_CAN_BG_UPDATE (1LL << 47) +#define HA_CAN_MULTISTEP_MERGE (1LL << 48)
Do you really need HA_CAN_BG_XXX flags? I checked your spider tree and this is not used anywhere, so I think we should leave them out for now. If we don't need these, we can remove the loop for setting them in HA_EXTRA_ATTACH_CHILDREN too.
If we need them, I would like to have a comment for why they are needed and when we will need then add these in a separate patch.
@@ -3525,6 +3540,29 @@ public: Pops the top if condition stack, if stack is not empty. */ virtual void cond_pop() { return; }; + virtual int set_top_table_and_fields(TABLE *top_table, + Field **top_table_field, + uint top_table_fields) + { + if (!set_top_table_fields) + { + set_top_table_fields = TRUE; + this->top_table = top_table; + this->top_table_field = top_table_field; + this->top_table_fields = top_table_fields; + } + return 0; + }
Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void.
I will make it void for now and change the relevant code. We can make it int again if needed in the future.
diff -Narup ./002_mariadb-10.2.0-spider/sql/sql_base.cc ./003_mariadb-10.2.0-vp/sql/sql_base.cc --- ./002_mariadb-10.2.0-spider/sql/sql_base.cc 2016-04-17 02:47:27.000000000 +0900 +++ ./003_mariadb-10.2.0-vp/sql/sql_base.cc 2016-05-05 21:25:04.319731713 +0900 @@ -1475,6 +1475,10 @@ unique_table(THD *thd, TABLE_LIST *table
table= table->find_table_for_update();
- if (table->table && table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM) - { - TABLE_LIST *child; + if (table->table && + ( + table->table->file->ht->db_type == DB_TYPE_MRG_MYISAM || + (table->table->file->ha_table_flags() & HA_CAN_MULTISTEP_MERGE) + ) + ) + continue
I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler.
This is now pushed into 10.2-spider
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!
However, I would like to have a clear comment the purpose of the function set_top_table_and_fields().
This function is used to get correlating of a parent (table/column) and children (table/column). When conditions are pushed down to child table (like child of myisammarge), child table needs to know about which table/column is my parent for understanding conditions.
Thanks. I have now added this as a comment to where the function is defined. <cut>
+ virtual int set_top_table_and_fields(TABLE *top_table, ... cut ... Why is this int and not void ? Do you have in mind that this may be possible to fail in the future? If it's never going to change, then better to do this void.
I will make it void for now and change the relevant code. We can make it int again if needed in the future.
VP allocates some memories in this function. So this function has possibility of causing out of memory error. That's why this function returns int.
ok. I will change the code for this.
I added HA_CAN_MULTISTEP_MERGE as a flag to DB_TYPE_MRG_MYISAM, which allwoed me to make the above tests and the following identical ones much simpler.
ok. And now everyone can create myisammarge type storage engine!
good. We do need some tests for this... Regards, Monty
participants (2)
-
kentoku -
Michael Widenius