Re: [Maria-developers] 66f931bcd69: MDEV-18727 improve DML operation of System Versioning

Hi Sergei! On Fri, Jun 7, 2019 at 8:45 PM Sergei Golubchik wrote:

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