Re: [Maria-developers] 66f931bcd69: MDEV-18727 improve DML operation of System Versioning
![](https://secure.gravatar.com/avatar/39b623a1559cf9c69ac3d9d4fb44e7fe.jpg?s=120&d=mm&r=g)
Hi, Aleksey! Looks ok, thanks! No requests to change anything, but still a couple of "why" questions, see below On Jun 07, Aleksey Midenkov wrote:
revision-id: 66f931bcd69 (mariadb-10.3.12-100-g66f931bcd69) parent(s): f4484dfdbf2 author: Aleksey Midenkov
committer: Aleksey Midenkov timestamp: 2019-03-26 15:04:52 +0300 message: MDEV-18727 improve DML operation of System Versioning MDEV-18957 UPDATE with LIMIT clause is wrong for versioned partitioned tables
UPDATE, DELETE: replace linear search of current/historical records with vers_setup_conds().
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index b8ed887b52d..a00cc2a8fb0 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -4975,8 +4975,10 @@ mysql_execute_command(THD *thd) { result= new (thd->mem_root) multi_delete(thd, aux_tables, lex->table_count); - if (unlikely(result)) + if (likely(result))
heh, good catch
{ + if (unlikely(select_lex->vers_setup_conds(thd, aux_tables))) + goto multi_delete_error; res= mysql_select(thd, select_lex->get_table_list(), select_lex->with_wild, diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 6915d4d23ca..28b54e0b3b1 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -779,20 +781,35 @@ int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables) } }
+ bool is_select= false; + switch (thd->lex->sql_command) + { + case SQLCOM_SELECT: + case SQLCOM_INSERT_SELECT: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_DELETE_MULTI: + case SQLCOM_UPDATE_MULTI: + is_select= true; + default: + break; + } + for (table= tables; table; table= table->next_local) { - if (!table->table || !table->table->versioned()) + if (!table->table || table->is_view() || !table->table->versioned())
why?
continue;
vers_select_conds_t &vers_conditions= table->vers_conditions;
#ifdef WITH_PARTITION_STORAGE_ENGINE - /* - if the history is stored in partitions, then partitions - themselves are not versioned - */ - if (table->partition_names && table->table->part_info->vers_info) + Vers_part_info *vers_info; + if (is_select && table->table->part_info && + (vers_info= table->table->part_info->vers_info)) + { + if (table->partition_names)
Why is that? You don't seem to use vers_info anywhere. Nor I see a reason for two nested if()'s intead of one, as before (with an additional `is_select &&` condition)
{ + /* If the history is stored in partitions, then partitions + themselves are not versioned. */ if (vers_conditions.is_set()) { my_error(ER_VERS_QUERY_IN_PARTITION, MYF(0), table->alias.str);
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
![](https://secure.gravatar.com/avatar/8fa4efb81153950d325fbc3930a3d4fd.jpg?s=120&d=mm&r=g)
Hi Sergei!
On Fri, Jun 7, 2019 at 8:45 PM Sergei Golubchik
Hi, Aleksey!
Looks ok, thanks! No requests to change anything, but still a couple of "why" questions, see below
On Jun 07, Aleksey Midenkov wrote:
revision-id: 66f931bcd69 (mariadb-10.3.12-100-g66f931bcd69) parent(s): f4484dfdbf2 author: Aleksey Midenkov
committer: Aleksey Midenkov timestamp: 2019-03-26 15:04:52 +0300 message:
...
diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 6915d4d23ca..28b54e0b3b1 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -779,20 +781,35 @@ int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables) } }
+ bool is_select= false; + switch (thd->lex->sql_command) + { + case SQLCOM_SELECT: + case SQLCOM_INSERT_SELECT: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_DELETE_MULTI: + case SQLCOM_UPDATE_MULTI: + is_select= true; + default: + break; + } + for (table= tables; table; table= table->next_local) { - if (!table->table || !table->table->versioned()) + if (!table->table || table->is_view() || !table->table->versioned())
why?
VIEW conditions are processed inside mysql_derived_prepare(). Btw, this patch was wrong about VIEW update since there was no appropriate cases in view.test. New patch is here: https://github.com/MariaDB/server/pull/1332/commits/a65299bb284636dfac3530e8... as well as additional test cases. And I attached just a fix for VIEW update bug in this letter.
continue;
vers_select_conds_t &vers_conditions= table->vers_conditions;
#ifdef WITH_PARTITION_STORAGE_ENGINE - /* - if the history is stored in partitions, then partitions - themselves are not versioned - */ - if (table->partition_names && table->table->part_info->vers_info) + Vers_part_info *vers_info; + if (is_select && table->table->part_info && + (vers_info= table->table->part_info->vers_info)) + { + if (table->partition_names)
Why is that? You don't seem to use vers_info anywhere. Nor I see a reason for two nested if()'s intead of one, as before (with an additional `is_select &&` condition)
Looks like an outdated patch. In new patch there is branch: else if (!vers_conditions.is_set()) { But I removed vers_info variable as it doesn't help anything.
{ + /* If the history is stored in partitions, then partitions + themselves are not versioned. */ if (vers_conditions.is_set()) { my_error(ER_VERS_QUERY_IN_PARTITION, MYF(0), table->alias.str);
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok
participants (2)
-
Aleksey Midenkov
-
Sergei Golubchik