Hi, Nikita! Thanks! Please, see review comments below On Jan 03, Nikita Malyavin wrote:

revision-id: fd18ed07d65 (versioning-1.0.7-3-gfd18ed07d65) parent(s): a643ca815af author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2018-12-03 13:19:18 +1000 message:

MDEV-16973 Application period tables: DELETE

* inject portion of time updates into mysql_delete main loop * triggered case emits delete+insert, no updates * PORTION OF `SYSTEM_TIME` is forbidden * `DELETE HISTORY .. FOR PORTION OF ...` is forbidden as well

diff --git a/mysql-test/suite/period/r/delete.result b/mysql-test/suite/period/r/delete.result new file mode 100644 index 00000000000..30c9220d6f9 --- /dev/null +++ b/mysql-test/suite/period/r/delete.result @@ -0,0 +1,216 @@ +create or replace table t (id int, s date, e date, period for apptime(s,e)); +insert into t values(1, '1999-01-01', '2018-12-12'); +insert into t values(1, '1999-01-01', '2017-01-01'); +insert into t values(1, '2017-01-01', '2019-01-01'); +insert into t values(2, '1998-01-01', '2018-12-12'); +insert into t values(3, '1997-01-01', '2015-01-01'); +insert into t values(4, '2016-01-01', '2020-01-01');

I'd insert also, say, (5, '2010-01-01', '2015-01-01').

+create or replace table t1 (id int, s date, e date, period for apptime(s,e)); +insert t1 select * from t; +create or replace table t2 (id int, s date, e date, period for apptime(s,e)); +insert t2 select * from t; +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +delete from t1 for portion of APPTIME from '2000-01-01' to '2018-01-01'; +select * from t;

try to avoid unsorted selects, the row order is undefined in these cases use --sorted_results command before the select or the order by. --sorted_results is generally better, because it's done on the client, so it doesn't change the execution plan on the server.

+id s e +1 1999-01-01 2000-01-01 +1 1999-01-01 2000-01-01 +1 2018-01-01 2019-01-01 +2 1998-01-01 2000-01-01 +3 1997-01-01 2000-01-01 +4 2018-01-01 2020-01-01 +1 2018-01-01 2018-12-12 +2 2018-01-01 2018-12-12 +select * from t1 order by id, s, e; +id s e +1 1999-01-01 2000-01-01 +1 1999-01-01 2000-01-01 +1 2018-01-01 2018-12-12 +1 2018-01-01 2019-01-01 +2 1998-01-01 2000-01-01 +2 2018-01-01 2018-12-12 +3 1997-01-01 2000-01-01 +4 2018-01-01 2020-01-01 +select * from log_tbl; +log +>DEL: 1, 1999-01-01, 2018-12-12 +INS: 1, 1999-01-01, 2000-01-01 +INS: 1, 2018-01-01, 2018-12-12 +DEL: 1, 1999-01-01, 2017-01-01 +INS: 1, 1999-01-01, 2000-01-01 +DEL: 1, 2017-01-01, 2019-01-01 +INS: 1, 2018-01-01, 2019-01-01 +DEL: 2, 1998-01-01, 2018-12-12 +INS: 2, 1998-01-01, 2000-01-01 +INS: 2, 2018-01-01, 2018-12-12 +DEL: 3, 1997-01-01, 2015-01-01 +INS: 3, 1997-01-01, 2000-01-01 +DEL: 4, 2016-01-01, 2020-01-01 +INS: 4, 2018-01-01, 2020-01-01 +INS: 1, 1999-01-01, 2000-01-01 +INS: 1, 2018-01-01, 2018-12-12 +INS: 1, 1999-01-01, 2000-01-01 +INS: 1, 2018-01-01, 2019-01-01 +INS: 2, 1998-01-01, 2000-01-01 +INS: 2, 2018-01-01, 2018-12-12 +INS: 3, 1997-01-01, 2000-01-01 +INS: 4, 2018-01-01, 2020-01-01 +

The error is good, ER_PARSE_ERROR is very appropriate here. But it should say "right syntax to use near 'for portion of' at line 1"

+delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; +ERROR HY000: period `othertime` is not found in table +delete from t for portion of system_time from '2000-01-01' to '2018-01-01'; +ERROR HY000: period SYSTEM_TIME is not allowed here

could be just ER_PARSE_ERROR too, I suppose

+create or replace table t (id int, str text, s date, e date, +period for apptime(s,e)); +insert into t values(1, 'data', '1999-01-01', '2018-12-12'); +insert into t values(1, 'other data', '1999-01-01', '2018-12-12'); +insert into t values(1, 'deleted', '2000-01-01', '2018-01-01'); +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +show warnings; +Level Code Message +select * from t;

--sorted_results or order by

+id str s e +1 data 1999-01-01 2000-01-01 +1 other data 1999-01-01 2000-01-01 +1 data 2018-01-01 2018-12-12 +1 other data 2018-01-01 2018-12-12 +drop table t1; +# SQL17 Case 15.7-8.b.i

good! you forgot "part 2" and "general rules", but even without them it only takes a couple of seconds to find this quote.

+# If the column descriptor that corresponds to the i-th field of BR +# describes an identity column, a generated column, a system-time period +# start column, or a system-time period end column, then let V i be +# DEFAULT. +# auto_inrement field is updated

"increment"

+create or replace table t (id int primary key auto_increment, s date, e date, +period for apptime(s, e)); +insert into t values (default, '1999-01-01', '2018-12-12');

please, add a select * here.

+delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select * from t; +id s e +2 1999-01-01 2000-01-01 +3 2018-01-01 2018-12-12 +truncate t; +# same for trigger case +insert into t values (default, '1999-01-01', '2018-12-12'); +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select * from t; +id s e +2 1999-01-01 2000-01-01 +3 2018-01-01 2018-12-12 +select * from log_tbl; +log +>DEL: 1999-01-01, 2018-12-12 +INS: 1999-01-01, 2000-01-01 +INS: 2018-01-01, 2018-12-12 +DEL: 1999-01-01, 2018-12-12 +INS: 1999-01-01, 2000-01-01 +INS: 2018-01-01, 2018-12-12 +DEL: 1999-01-01, 2018-12-12 +INS: 1999-01-01, 2000-01-01 +INS: 2018-01-01, 2018-12-12 +

PORTION_SYM, please

{ "PRECEDES", SYM(PRECEDES_SYM)}, { "PRECEDING", SYM(PRECEDING_SYM)}, { "PRECISION", SYM(PRECISION)}, diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 4f2c3fe7611..0c368b40836 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -1033,6 +1033,7 @@ bool my_yyoverflow(short **a, YYSTYPE **b, size_t *yystacksize); %token PERCENT_RANK_SYM %token PERCENTILE_CONT_SYM %token PERCENTILE_DISC_SYM +%token PORTION

add /* SQL-2016-R */ comment and it seems to be 2016 standard, not 2017 standard, https://en.wikipedia.org/wiki/SQL please change it everywhere, it's too confusing otherwise

%token POSITION_SYM /* SQL-2003-N */ %token PRECISION /* SQL-2003-R */ %token PRIMARY_SYM /* SQL-2003-R */ @@ -13437,23 +13449,26 @@ delete_part2: opt_delete_options single_multi {} | HISTORY_SYM delete_single_table opt_delete_system_time { + MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period()); Lex->last_table()->vers_conditions= Lex->vers_conditions; Lex->pop_select(); //main select } ;

delete_single_table: - FROM table_ident opt_use_partition + FROM table_ident opt_for_portion_of_time_clause opt_use_partition

better put opt_for_portion_of_time_clause after opt_use_partition, I think it's more logical that way. FOR PORTION OF is logical specification, while opt_use_partition is physical one, just like the table name, so it makes sense to keep them together

{ if (unlikely(!Select-> add_table_to_list(thd, $2, NULL, TL_OPTION_UPDATING, YYPS->m_lock_type, YYPS->m_mdl_type, NULL, - $3))) + $4))) MYSQL_YYABORT; YYPS->m_lock_type= TL_READ_DEFAULT; YYPS->m_mdl_type= MDL_SHARED_READ; + if ($3) + Lex->last_table()->period_conditions= Lex->period_conditions; } ;

diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index ceb3a58a427..7ceff1a215e 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7945,3 +7945,9 @@ ER_PERIOD_FIELD_WRONG_ATTRIBUTES

ER_PERIOD_FIELD_HOLDS_MORE_THAN_ONE_PERIOD eng "Field %`s is specified for more than one period" + +ER_PERIOD_NOT_FOUND + eng "period %`s is not found in table"

first letter capitalized, please

+ +ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED + eng "period SYSTEM_TIME is not allowed here"

same here, or, better, use ER_PARSE_ERROR

diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 3ae113a4595..e76eb729536 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -287,7 +287,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, bool return_error= 0; ha_rows deleted= 0; bool reverse= FALSE; - bool has_triggers; + bool has_triggers= false; + bool has_period_triggers= false;

I don't see why you need has_triggers and has_period_triggers, I find it confusing. just make `has_triggers=true` if there's `FOR PORTION OF` and there are insert triggers. In other words, has_triggers should be true if there are any triggers that this statement might need to fire.

ORDER *order= (ORDER *) ((order_list && order_list->elements) ? order_list->first : NULL); SELECT_LEX *select_lex= thd->lex->first_select_lex(); @@ -334,6 +338,21 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, table_list->on_expr= NULL; } } + if (table_list->has_period()) + { + if (table_list->is_view_or_derived()) + { + my_error(ER_IT_IS_A_VIEW, MYF(0), table_list->table_name.str); + DBUG_RETURN(true); + } + + int err= select_lex->period_setup_conds(thd, table_list, conds); + if (err) + DBUG_RETURN(true); + + conds= table_list->on_expr; + table_list->on_expr= NULL;

Why did you define SELECT_LEX::period_setup_conds to put the condition in TABLE_LIST::on_expr only to hack it up from there? Just make it to return the new condition or NULL in case of an error.

+ }

if (mysql_handle_list_of_derived(thd->lex, table_list, DT_MERGE_FOR_INSERT)) DBUG_RETURN(TRUE); @@ -727,6 +752,9 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, delete_record= true; }

+ portion_of_time_through_update= !has_period_triggers + && !table->versioned(VERS_TIMESTAMP);

I suspect that with VERS_TRX_ID you'd also need to use delete/insert. Add a test, please.

+ THD_STAGE_INFO(thd, stage_updating); while (likely(!(error=info.read_record())) && likely(!thd->killed) && likely(!thd->is_error())) @@ -969,7 +1013,8 @@ int mysql_prepare_delete(THD *thd, TABLE_LIST *table_list, DBUG_RETURN(TRUE); }

- if (unique_table(thd, table_list, table_list->next_global, 0)) + if (table_list->has_period()

Really? Why?

+ || unique_table(thd, table_list, table_list->next_global, 0)) *delete_while_scanning= false;

if (select_lex->inner_refs_list.elements && diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc index 5f76f36983b..de8ed2f14b3 100644 --- a/sql/sql_parse.cc +++ b/sql/sql_parse.cc @@ -8302,6 +8302,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd, ptr->ignore_leaves= MY_TEST(table_options & TL_OPTION_IGNORE_LEAVES); ptr->sequence= MY_TEST(table_options & TL_OPTION_SEQUENCE); ptr->derived= table->sel; + ptr->vers_conditions.name= "SYSTEM_TIME";

why?

if (!ptr->derived && is_infoschema_db(&ptr->db)) { ST_SCHEMA_TABLE *schema_table; diff --git a/sql/sql_select.cc b/sql/sql_select.cc index 19cea4d6a15..ea1c7834f57 100644 --- a/sql/sql_select.cc +++ b/sql/sql_select.cc @@ -717,12 +717,152 @@ void vers_select_conds_t::print(String *str, enum_query_type query_type) const } }

+void period_update_condition(THD *thd, TABLE_LIST *table, SELECT_LEX *select,

static

+ vers_select_conds_t &conds, bool timestamp)

please, either use pointers or _const_ references. in other words, `conds` here should be a pointer

+{ + DBUG_ASSERT(table); + DBUG_ASSERT(table->table); +#define newx new (thd->mem_root) + TABLE_SHARE *share= table->table->s; + const TABLE_SHARE::period_info_t *period= conds.period; + + const LEX_CSTRING &fstart= period->start_field(share)->field_name; + const LEX_CSTRING &fend= period->end_field(share)->field_name; + + conds.field_start= newx Item_field(thd, &select->context, + table->db.str, table->alias.str, + thd->make_clex_string(fstart)); + conds.field_end= newx Item_field(thd, &select->context, + table->db.str, table->alias.str, + thd->make_clex_string(fend)); + + Item *cond1= NULL, *cond2= NULL, *cond3= NULL, *curr= NULL; + if (timestamp) + { + MYSQL_TIME max_time; + switch (conds.type) + { + case SYSTEM_TIME_UNSPECIFIED: + thd->variables.time_zone->gmt_sec_to_TIME(&max_time, TIMESTAMP_MAX_VALUE); + max_time.second_part= TIME_MAX_SECOND_PART; + curr= newx Item_datetime_literal(thd, &max_time, TIME_SECOND_PART_DIGITS); + cond1= newx Item_func_eq(thd, conds.field_end, curr); + break; + case SYSTEM_TIME_AS_OF: + cond1= newx Item_func_le(thd, conds.field_start, conds.start.item); + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); + break; + case SYSTEM_TIME_FROM_TO: + cond1= newx Item_func_lt(thd, conds.field_start, conds.end.item); + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); + cond3= newx Item_func_lt(thd, conds.start.item, conds.end.item); + break; + case SYSTEM_TIME_BETWEEN: + cond1= newx Item_func_le(thd, conds.field_start, conds.end.item); + cond2= newx Item_func_gt(thd, conds.field_end, conds.start.item); + cond3= newx Item_func_le(thd, conds.start.item, conds.end.item); + break; + case SYSTEM_TIME_BEFORE: + cond1= newx Item_func_lt(thd, conds.field_end, conds.start.item); + break; + default: + DBUG_ASSERT(0); + } + } + else + { + DBUG_ASSERT(table->table->s && table->table->s->db_plugin); + + Item *trx_id0= conds.start.item; + Item *trx_id1= conds.end.item; + if (conds.start.item && conds.start.unit == VERS_TIMESTAMP) + { + bool backwards= conds.type != SYSTEM_TIME_AS_OF; + trx_id0= newx Item_func_trt_id(thd, conds.start.item, + TR_table::FLD_TRX_ID, backwards); + } + if (conds.end.item && conds.end.unit == VERS_TIMESTAMP) + { + trx_id1= newx Item_func_trt_id(thd, conds.end.item, + TR_table::FLD_TRX_ID, false); + } + + switch (conds.type) + { + case SYSTEM_TIME_UNSPECIFIED: + curr= newx Item_int(thd, ULONGLONG_MAX); + cond1= newx Item_func_eq(thd, conds.field_end, curr);

add here DBUG_ASSERT(!conds.start.item); DBUG_ASSERT(!conds.end.item);

+ break; + case SYSTEM_TIME_AS_OF: + cond1= newx Item_func_trt_trx_sees_eq(thd, trx_id0, conds.field_start); + cond2= newx Item_func_trt_trx_sees(thd, conds.field_end, trx_id0);

DBUG_ASSERT(!conds.end.item);

+ break; + case SYSTEM_TIME_FROM_TO: + cond1= newx Item_func_trt_trx_sees(thd, trx_id1, conds.field_start); + cond3= newx Item_func_lt(thd, conds.start.item, conds.end.item); + break;

hmm, you don't use trx_id0 here at all. did you forget cond2= newx Item_func_trt_trx_sees_eq(thd, conds.field_end, trx_id0);

+ case SYSTEM_TIME_BETWEEN: + cond1= newx Item_func_trt_trx_sees_eq(thd, trx_id1, conds.field_start); + cond2= newx Item_func_trt_trx_sees_eq(thd, conds.field_end, trx_id0); + cond3= newx Item_func_le(thd, conds.start.item, conds.end.item); + break; + case SYSTEM_TIME_BEFORE: + cond1= newx Item_func_trt_trx_sees(thd, trx_id0, conds.field_end); + break; + default: + DBUG_ASSERT(0); + } + } + + if (cond1) + { + cond1= and_items(thd, cond2, cond1); + cond1= and_items(thd, cond3, cond1); + table->on_expr= and_items(thd, table->on_expr, cond1); + } +#undef newx +} + +int SELECT_LEX::period_setup_conds(THD *thd, TABLE_LIST *tables, Item *where) +{ + DBUG_ENTER("SELECT_LEX::period_setup_conds"); + + if (!thd->stmt_arena->is_conventional() && + !thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) + { + // statement is already prepared + DBUG_RETURN(0); + } + + if (thd->lex->is_view_context_analysis()) + DBUG_RETURN(0);

I see that you've copied these two if's from SELECT_LEX::vers_setup_conds. I suspect that the first if() is not quite correct here, and I plan to take a closer look at it. so, it'd be great if you could come up with some way to avoid copying these two if()'s. So that there will be only one place to fix. May be, extract them into a function or something, I don't know.

+ + for (TABLE_LIST *table= tables; table; table= table->next_local)

1. can here be more than one table? you only allow FOR PORTION OF in delete_single_table parser rule. 2. if no, add an assert here that there's only one table in the list 3. and please add a test for DELETE FROM view FOR PORTION OF

+ { + if (!table->table) + continue; + vers_select_conds_t &conds= table->period_conditions; + if (0 == strcasecmp(conds.name.str, "SYSTEM_TIME"))

why not to check it in the parser?

+ { + my_error(ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED, MYF(0)); + DBUG_RETURN(-1); + } + if (!table->table->s->period.name.streq(conds.name)) + { + my_error(ER_PERIOD_NOT_FOUND, MYF(0), conds.name.str); + DBUG_RETURN(-1); + } + + conds.period= &table->table->s->period; + period_update_condition(thd, table, this, conds, true); + table->on_expr= and_items(thd, where, table->on_expr); + } + DBUG_RETURN(0); +} + int SELECT_LEX::vers_setup_conds(THD *thd, TABLE_LIST *tables) { DBUG_ENTER("SELECT_LEX::vers_setup_cond"); -#define newx new (thd->mem_root) - - TABLE_LIST *table;

if (!thd->stmt_arena->is_conventional() && !thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) diff --git a/sql/table.cc b/sql/table.cc index 87fa1ba4d61..8f1c94dd633 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -6426,9 +6426,14 @@ void TABLE::mark_columns_needed_for_delete()

if (s->versioned) { - bitmap_set_bit(read_set, s->vers.start_field(s)->field_index); - bitmap_set_bit(read_set, s->vers.end_field(s)->field_index); - bitmap_set_bit(write_set, s->vers.end_field(s)->field_index); + bitmap_set_bit(read_set, s->vers.start_fieldno); + bitmap_set_bit(read_set, s->vers.end_fieldno); + bitmap_set_bit(write_set, s->vers.end_fieldno); + } + + if (with_period) + { + use_all_columns(); }

Nope, I think this is conceptually wrong. TABLE::mark_columns_needed_for_delete marks columns that are needed to *delete* a row. The fact that you might need to *insert* if there's a FOR PORTION OF, is irrelevant here, it's something the caller should bother about.

}

@@ -7834,6 +7839,101 @@ int TABLE::update_default_fields(bool update_command, bool ignore_errors) DBUG_RETURN(res); }

+static int table_update_generated_fields(TABLE *table) +{ + int res= 0; + if (table->found_next_number_field) + { + table->next_number_field= table->found_next_number_field; + res= table->found_next_number_field->set_default(); + table->file->update_auto_increment(); + } + + if (table->default_field) + res= table->update_default_fields(0, false);

why is that?

+ + if (likely(!res) && table->vfield) + res= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE); + if (likely(!res) && table->versioned()) + table->vers_update_fields();

I believe you need to verify CHECK constraints too. and, please, add a test where they fail here.

+ return res; +} + +static int period_make_insert(TABLE *table, Item *src, Field *dst) +{ + THD *thd= table->in_use; + + store_record(table, record[1]); + int res= src->save_in_field(dst, true); + + if (likely(!res)) + table_update_generated_fields(table);

perhaps, move store_record() and save_in_field() into table_update_generated_fields() ? less duplicated code. But the function will need to be renamed, like prepare_for_period_insert or something

+ + if (likely(!res) && table->triggers) + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_BEFORE, true); + + if (likely(!res)) + res = table->file->ha_write_row(table->record[0]); + + if (likely(!res) && table->triggers) + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_AFTER, true); + + restore_record(table, record[1]); + return res; +} + +int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, + bool *inside_period)

is it used for UPDATE too? or only for DELETE?

+{ + bool lcond= period_conds.field_start->val_datetime_packed(thd) + < period_conds.start.item->val_datetime_packed(thd); + bool rcond= period_conds.field_end->val_datetime_packed(thd) + > period_conds.end.item->val_datetime_packed(thd); + + *inside_period= !lcond && !rcond; + if (*inside_period) + return 0; + + int res= 0; + Item *src= lcond ? period_conds.start.item : period_conds.end.item; + uint dst_fieldno= lcond ? s->period.end_fieldno : s->period.start_fieldno; + + store_record(this, record[1]); + if (likely(!res)) + res= src->save_in_field(field[dst_fieldno], true); + + if (likely(!res)) + res= table_update_generated_fields(this); +

may be, an assert that there're no delete/insert triggers in a table?

+ if(likely(!res)) + res= file->ha_update_row(record[1], record[0]); + + restore_record(this, record[1]); + + if (likely(!res) && lcond && rcond) + res= period_make_insert(this, period_conds.end.item, + field[s->period.start_fieldno]); + + return res; +} + +int TABLE::insert_portion_of_time(THD *thd, vers_select_conds_t &period_conds) +{ + bool lcond= period_conds.field_start->val_datetime_packed(thd) + < period_conds.start.item->val_datetime_packed(thd); + bool rcond= period_conds.field_end->val_datetime_packed(thd) + > period_conds.end.item->val_datetime_packed(thd); + + int res= 0; + if (lcond) + res= period_make_insert(this, period_conds.start.item, + field[s->period.end_fieldno]); + if (likely(!res) && rcond) + res= period_make_insert(this, period_conds.end.item, + field[s->period.start_fieldno]); + + return res; +}

void TABLE::vers_update_fields() {

Regards, Sergei Chief Architect MariaDB and security@mariadb.org