Re: [Maria-developers] fd18ed07d65: MDEV-16973 Application period tables: DELETE
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 <nikitamalyavin@gmail.com> committer: Nikita Malyavin <nikitamalyavin@gmail.com> 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 +<DEL: 1, 1999-01-01, 2018-12-12 +>INS: 1, 1999-01-01, 2000-01-01 +<INS: 1, 1999-01-01, 2000-01-01 +>INS: 1, 2018-01-01, 2018-12-12 +<INS: 1, 2018-01-01, 2018-12-12 +>DEL: 1, 1999-01-01, 2017-01-01 +<DEL: 1, 1999-01-01, 2017-01-01 +>INS: 1, 1999-01-01, 2000-01-01 +<INS: 1, 1999-01-01, 2000-01-01 +>DEL: 1, 2017-01-01, 2019-01-01 +<DEL: 1, 2017-01-01, 2019-01-01 +>INS: 1, 2018-01-01, 2019-01-01 +<INS: 1, 2018-01-01, 2019-01-01 +>DEL: 2, 1998-01-01, 2018-12-12 +<DEL: 2, 1998-01-01, 2018-12-12 +>INS: 2, 1998-01-01, 2000-01-01 +<INS: 2, 1998-01-01, 2000-01-01 +>INS: 2, 2018-01-01, 2018-12-12 +<INS: 2, 2018-01-01, 2018-12-12 +>DEL: 3, 1997-01-01, 2015-01-01 +<DEL: 3, 1997-01-01, 2015-01-01 +>INS: 3, 1997-01-01, 2000-01-01 +<INS: 3, 1997-01-01, 2000-01-01 +>DEL: 4, 2016-01-01, 2020-01-01 +<DEL: 4, 2016-01-01, 2020-01-01 +>INS: 4, 2018-01-01, 2020-01-01 +<INS: 4, 2018-01-01, 2020-01-01 +# INSERT trigger only also works +drop trigger tr1del_t2; +drop trigger tr2del_t2; +delete from t2 for portion of APPTIME from '2000-01-01' to '2018-01-01'; +select * from log_tbl; +log +>INS: 1, 1999-01-01, 2000-01-01 +<INS: 1, 1999-01-01, 2000-01-01 +>INS: 1, 2018-01-01, 2018-12-12 +<INS: 1, 2018-01-01, 2018-12-12 +>INS: 1, 1999-01-01, 2000-01-01 +<INS: 1, 1999-01-01, 2000-01-01 +>INS: 1, 2018-01-01, 2019-01-01 +<INS: 1, 2018-01-01, 2019-01-01 +>INS: 2, 1998-01-01, 2000-01-01 +<INS: 2, 1998-01-01, 2000-01-01 +>INS: 2, 2018-01-01, 2018-12-12 +<INS: 2, 2018-01-01, 2018-12-12 +>INS: 3, 1997-01-01, 2000-01-01 +<INS: 3, 1997-01-01, 2000-01-01 +>INS: 4, 2018-01-01, 2020-01-01 +<INS: 4, 2018-01-01, 2020-01-01 +delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
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 +<DEL: 1999-01-01, 2018-12-12 +>INS: 1999-01-01, 2000-01-01 +<INS: 1999-01-01, 2000-01-01 +>INS: 2018-01-01, 2018-12-12 +<INS: 2018-01-01, 2018-12-12 +# generated columns are updated +create or replace table t (s date, e date, +xs date as (s) stored, xe date as (e) stored, +period for apptime(s, e)); +insert into t values('1999-01-01', '2018-12-12', default, default); +select * from t; +s e xs xe +1999-01-01 2018-12-12 1999-01-01 2018-12-12 +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select * from t; +s e xs xe +1999-01-01 2000-01-01 1999-01-01 2000-01-01 +2018-01-01 2018-12-12 2018-01-01 2018-12-12 +truncate t; +# same for trigger case +insert into t values('1999-01-01', '2018-12-12', default, default); +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select * from t; +s e xs xe +1999-01-01 2000-01-01 1999-01-01 2000-01-01 +2018-01-01 2018-12-12 2018-01-01 2018-12-12 +select * from log_tbl; +log +>DEL: 1999-01-01, 2018-12-12 +<DEL: 1999-01-01, 2018-12-12 +>INS: 1999-01-01, 2000-01-01 +<INS: 1999-01-01, 2000-01-01 +>INS: 2018-01-01, 2018-12-12 +<INS: 2018-01-01, 2018-12-12 +# system_time columns are updated +create or replace table t ( +s date, e date, +row_start SYS_TYPE as row start invisible, +row_end SYS_TYPE as row end invisible, +period for apptime(s, e), +period for system_time (row_start, row_end)) with system versioning; +insert into t values('1999-01-01', '2018-12-12'), +('1999-01-01', '1999-12-12'); +select row_start into @ins_time from t limit 1; +select * from t order by s, e; +s e +1999-01-01 1999-12-12 +1999-01-01 2018-12-12 +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) +from t for system_time all +order by s, e, row_start; +s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) +1999-01-01 1999-12-12 OLD CURRENT ROW +1999-01-01 2000-01-01 NEW CURRENT ROW +1999-01-01 2018-12-12 OLD HISTORICAL ROW +2018-01-01 2018-12-12 NEW CURRENT ROW +# same for trigger case +delete from t; +delete history from t; +insert into t values('1999-01-01', '2018-12-12'), +('1999-01-01', '1999-12-12'); +select row_start into @ins_time from t limit 1; +select * from t order by s, e; +s e +1999-01-01 1999-12-12 +1999-01-01 2018-12-12 +delete from t for portion of apptime from '2000-01-01' to '2018-01-01'; +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) +from t for system_time all +order by s, e, row_start; +s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) +1999-01-01 1999-12-12 OLD CURRENT ROW +1999-01-01 2000-01-01 NEW CURRENT ROW +1999-01-01 2018-12-12 OLD HISTORICAL ROW +2018-01-01 2018-12-12 NEW CURRENT ROW +select * from log_tbl; +log +>DEL: 1999-01-01, 2018-12-12 +<DEL: 1999-01-01, 2018-12-12 +>INS: 1999-01-01, 2000-01-01 +<INS: 1999-01-01, 2000-01-01 +>INS: 2018-01-01, 2018-12-12 +<INS: 2018-01-01, 2018-12-12 +create or replace database test; diff --git a/sql/lex.h b/sql/lex.h index d336c273a18..883316ab988 100644 --- a/sql/lex.h +++ b/sql/lex.h @@ -476,6 +476,7 @@ static SYMBOL symbols[] = { { "POINT", SYM(POINT_SYM)}, { "POLYGON", SYM(POLYGON)}, { "PORT", SYM(PORT_SYM)}, + { "PORTION", SYM(PORTION)},
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
Hi!
I'd insert also, say, (5, '2010-01-01', '2015-01-01'). Ok no problem
+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.
Added --sorted_result to all plain selects✅
+delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
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"
Well I just used `MYSQL_YYABORT_UNLESS` in parser, as in many other places, and they all have this problem at this moment. Maybe that's a bug of this macro.
+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
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case. However, these cases differ -- it is very likely that user will try to use SYSTEM_TIME period, and we should tell him this is a voilation.
+select * from t;
--sorted_results or order by
✅
+# 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.
To be honest, the whole idea to cite the standard in tests belongs to Alexey Midenkov. I've just pasted his citations. Added the rest ✅
+# 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.
✅
+ { "PORTION", SYM(PORTION)},
PORTION_SYM, please
✅
+%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
uh yes, really, missed it somehow. Replaced ✅
%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
✅
+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.
ok ✅
+ 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.
I just wanted to reflect versioning behavior as much as possible. Made it just to return a condition ✅
+ 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.
There is a system versioning test at the end of 'delete.test'. It runs for both 'timestamp' and 'trx_id' cases. But what I suspect is that this optimization is meaningful in case of VERS_TRX_ID. The resulting operation will be converted to update+insert in both cases, so I am also removing it for VERS_TRX_ID. ✅
- if (unique_table(thd, table_list, table_list->next_global, 0)) + if (table_list->has_period()
Really? Why?
Yes, really. Table crashes myisam -- just tested. This is related to that we mix deletes/updates and inserts, and it breaks cursor. Maybe we can do something like Eugene did in latest TABLE::delete_row patch -- i.e. save/restore cursor, but delete_while_scanning= false just works.
+ || unique_table(thd, table_list, table_list->next_global, 0)) *delete_while_scanning= false;
@@ -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?
Guess it is junk for now... I was thinking about SELECT...AS OF support, and had get_period_by_name function somewhere in TABLE, but it's gone after several review iterations. I was also trying to not so much distinguish SYSTEM_TIME and application-time period already on this step. Removed from this commit and added note in MDEV-16977 ✅
+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
✅
+ 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);
yes, looks so... The tests did not catch this
+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.
Those lines were made for a couple of bugfixes. Eugene's just mentioned the latest one: https://github.com/MariaDB/server/commit/748ef3ec91d35aa6cd5b230c71084f6c83c... I've just extracted those if's to a separate function. Have no better idea.
+ + 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 That's not the case for UPDATE, so assert will be inappropriate here.
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?
Maybe.. but anyway what's the defference? This place is good for error handling too.
@@ -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.
I thought this function is to collect all the weird logic of columns marking in one convenient place. A tip is that it's used only in DELETE statement code, but not in replication, or anywhere else.
}
@@ -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? auto_increment fields are allowed
+ + 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. I think there'll be no such test, because generated fields are forbidden to use application-time period fields. Except auto_increment, but, those are not allowed to be used in virtual fields. The SQL16 standard in 15.7 Effect of deleting rows from base tables, General Rules, 8)c)ii) says: The following <insert statement> is effectively executed without further Access Rule and constraint checking: INSERT INTO TN VALUES (VL1, ..., VLd) NOTE 691 — Constraint checking is performed at the end of triggering DELETE statement.
Not sure what does the NOTE 691 mean exactly, but I interpret the statement as this INSERT command doesn't require any constraint check. BTW what's important, this operation does not require INSERT privilege!
+ 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
Because of two duplicating lines?:) I actually don't quite like this idea because store_record is symmetrically paired with restore_record here, and this pairing fits on one screen, which is very convenient for reading.
+ + 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?
Looks like for DELETE only. Does the naming look confusing?
+{ + 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?
Hmm maybe. I doubt it will prevent any bug hereafter, but added, let's see how it'll go. Sincerely, Nikita Malyavin
Hi, Nikita! On Jan 24, Nikita Malyavin wrote:
+delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
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"
Well I just used `MYSQL_YYABORT_UNLESS` in parser, as in many other places, and they all have this problem at this moment. Maybe that's a bug of this macro.
No, it's how your grammar works. You wrote | HISTORY_SYM delete_single_table opt_delete_system_time { MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period()); so, the parser consumes HISTORY, table name, FOR PORTION OF, etc. and *only then* you force a ER_PARSE_ERROR. At that moment the current parsing position is at the end of the statement. The correct fix is not to use delete_single_table (that allows FOR PORTION OF) in the DELETE HISTORY rule (where FOR PORTION OF is not allowed).
+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
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case.
it depends on how you write the grammar :)
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; + conds= table_list->on_expr; + table_list->on_expr= NULL; + portion_of_time_through_update= !has_period_triggers + && !table->versioned(VERS_TIMESTAMP); - if (unique_table(thd, table_list, table_list->next_global, 0)) + if (table_list->has_period()
Really? Why?
Yes, really. Table crashes myisam -- just tested. This is related to that we mix deletes/updates and inserts, and it breaks cursor. Maybe we can do something like Eugene did in latest TABLE::delete_row patch -- i.e. save/restore cursor, but delete_while_scanning= false just works.
I see. Please add a comment here that one cannot mix write_row, update_row, and delete_row while scanning.
+ || unique_table(thd, table_list, table_list->next_global, 0)) *delete_while_scanning= false;
+ 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);
yes, looks so... The tests did not catch this
add a new test, perhaps? :)
+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.
Those lines were made for a couple of bugfixes. Eugene's just mentioned the latest one: https://github.com/MariaDB/server/commit/748ef3ec91d35aa6cd5b230c71084f6c83c...
Yes, I know.
I've just extracted those if's to a separate function. Have no better idea.
Thanks, that'll do.
+ + 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 That's not the case for UPDATE, so assert will be inappropriate here.
Can there be many tables with UPDATE? How?
@@ -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.
I thought this function is to collect all the weird logic of columns marking in one convenient place. A tip is that it's used only in DELETE statement code, but not in replication, or anywhere else.
Well, it's "mark_columns_needed_for_delete" and the comment says "Mark columns needed for doing an delete of a row". It only check engine capabilities - some engines need primary key columns to be always selected, if they need to delete a row, for example. The fact that _upper layer_ might want to insert or update is not what this function should be bothered with.
}
@@ -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? auto_increment fields are allowed
I don't understand, sorry. TABLE::update_default_fields does not update auto_increment fields, as far as I can see.
+ + 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. I think there'll be no such test, because generated fields are forbidden to use application-time period fields. Except auto_increment, but, those are not allowed to be used in virtual fields. The SQL16 standard in 15.7 Effect of deleting rows from base tables, General Rules, 8)c)ii) says: The following <insert statement> is effectively executed without further Access Rule and constraint checking: INSERT INTO TN VALUES (VL1, ..., VLd) NOTE 691 — Constraint checking is performed at the end of triggering DELETE statement.
Not sure what does the NOTE 691 mean exactly, but I interpret the statement as this INSERT command doesn't require any constraint check. BTW what's important, this operation does not require INSERT privilege!
Constraint checking is performed. The point is, when the statement has finished, all constraints must be satisfied. A simple test could be CREATE TABLE t1 (id AUTO_INCREMENT, ..., CHECK (id < 10)) and by using DELETE ... FOR PORTION OF you can generate new id's until the constraint fails. Also, please try CREATE TABLE t1 (id TINYINT AUTO_INCREMENT, ...); INSERT t1 VALUES (254, ...) here id will quickly run out of values, and DELETE FOR PORTION OF will fail too.
+ 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
Because of two duplicating lines?:)
I actually don't quite like this idea because store_record is symmetrically paired with restore_record here, and this pairing fits on one screen, which is very convenient for reading.
"perhaps" was a stylistic suggestion. Sure, it's your code, so up to you :)
+ + 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?
Looks like for DELETE only. Does the naming look confusing?
No, I wanted to ask to move it from a TABLE method to a static function in sql_delete.cc: static int update_portion_of_time(THD*, TABLE*, vers_select_conds_t&, bool*);
+{ + 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?
Hmm maybe. I doubt it will prevent any bug hereafter, but added, let's see how it'll go.
One of the benefits of asserts that I like, they document the code. assert(table->triggers == NULL); is a self-enforcing version of /* here the table must have no triggers */ and unlike the comment it'll never be out of date. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! чт, 24 янв. 2019 г. в 21:31, Sergei Golubchik <serg@mariadb.org>:
Hi, Nikita!
+delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; +ERROR 42000: You have an error in your SQL syntax; check the manual
On Jan 24, Nikita Malyavin wrote: that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
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"
Well I just used `MYSQL_YYABORT_UNLESS` in parser, as in many other places, and they all have this problem at this moment. Maybe that's a bug of this macro.
No, it's how your grammar works. You wrote
| HISTORY_SYM delete_single_table opt_delete_system_time { MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period());
so, the parser consumes HISTORY, table name, FOR PORTION OF, etc. and *only then* you force a ER_PARSE_ERROR. At that moment the current parsing position is at the end of the statement.
Ugh I didn't realize that, thanks! Now things are clear.
The correct fix is not to use delete_single_table (that allows FOR PORTION OF) in the DELETE HISTORY rule (where FOR PORTION OF is not allowed).
✅
+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
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case.
it depends on how you write the grammar :)
What about "for portion of `SYSTEM_TIME` ..."? Is it correct to throw ER_PARSE_ERROR in this case?
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; + conds= table_list->on_expr; + table_list->on_expr= NULL; + portion_of_time_through_update= !has_period_triggers + && !table->versioned(VERS_TIMESTAMP); - if (unique_table(thd, table_list, table_list->next_global, 0)) + if (table_list->has_period()
Really? Why?
Yes, really. Table crashes myisam -- just tested. This is related to that we mix deletes/updates and inserts, and it breaks cursor. Maybe we can do something like Eugene did in latest TABLE::delete_row patch -- i.e. save/restore cursor, but delete_while_scanning= false just works.
I see. Please add a comment here that one cannot mix write_row, update_row, and delete_row while scanning.
✅
+ || unique_table(thd, table_list, table_list->next_global, 0)) *delete_while_scanning= false;
+ 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);
yes, looks so... The tests did not catch this
add a new test, perhaps? :)
added a test to versioning.select ✅
+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.
Those lines were made for a couple of bugfixes. Eugene's just mentioned the latest one:
https://github.com/MariaDB/server/commit/748ef3ec91d35aa6cd5b230c71084f6c83c...
Yes, I know.
I've just extracted those if's to a separate function. Have no better idea.
Thanks, that'll do.
+ + 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 That's not the case for UPDATE, so assert will be inappropriate here.
Can there be many tables with UPDATE? How?
Just because the check is written in setup_conds, which is called after
period_setup_conds. Well, seems that we'll move the check to parser in scope of UPDATE discussion, I'll add the assertion now, but that'll break update tests temporarily. And it'll be removed in SELECT...AS OF
@@ -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.
I thought this function is to collect all the weird logic of columns marking in one convenient place. A tip is that it's used only in DELETE statement code, but not in replication, or anywhere else.
Well, it's "mark_columns_needed_for_delete" and the comment says "Mark columns needed for doing an delete of a row". It only check engine capabilities - some engines need primary key columns to be always selected, if they need to delete a row, for example.
The fact that _upper layer_ might want to insert or update is not what this function should be bothered with.
Okay, removed ✅
}
@@ -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? auto_increment fields are allowed
I don't understand, sorry. TABLE::update_default_fields does not update auto_increment fields, as far as I can see.
Oh, I misunderstood the question. Seems, we don't need it really, and it changes nothing. Removed✅
+
+ 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. I think there'll be no such test, because generated fields are forbidden to use application-time period fields. Except auto_increment, but, those are not allowed to be used in virtual fields. The SQL16 standard in 15.7 Effect of deleting rows from base tables, General Rules, 8)c)ii) says: The following <insert statement> is effectively executed without further Access Rule and constraint checking: INSERT INTO TN VALUES (VL1, ..., VLd) NOTE 691 — Constraint checking is performed at the end of triggering DELETE statement.
Not sure what does the NOTE 691 mean exactly, but I interpret the statement as this INSERT command doesn't require any constraint check. BTW what's important, this operation does not require INSERT privilege!
Constraint checking is performed. The point is, when the statement has finished, all constraints must be satisfied.
A simple test could be
CREATE TABLE t1 (id AUTO_INCREMENT, ..., CHECK (id < 10))
and by using DELETE ... FOR PORTION OF you can generate new id's until the constraint fails. Also, please try
CREATE TABLE t1 (id TINYINT AUTO_INCREMENT, ...); INSERT t1 VALUES (254, ...)
here id will quickly run out of values, and DELETE FOR PORTION OF will fail too.
Good corner case, thanks! It was indeed failing, but because I missed some error handling. I've fixed it, and added the test case, please see the update. Thing is it wasn't CHECK constraint -- it was autoinc check inside handler::update_auto_increment. I'm still convinced that CHECK constraints are impossible to 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
Because of two duplicating lines?:)
I actually don't quite like this idea because store_record is symmetrically paired with restore_record here, and this pairing fits on one screen, which is very convenient for reading.
"perhaps" was a stylistic suggestion. Sure, it's your code, so up to you :)
Okay!
+ + 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?
Looks like for DELETE only. Does the naming look confusing?
No, I wanted to ask to move it from a TABLE method to a static function in sql_delete.cc:
static int update_portion_of_time(THD*, TABLE*, vers_select_conds_t&, bool*);
Well, technically moving it to sql_delete will make table_update_generated_fields and period_make_insert extern. I also like current code arrangement by the following reason: these functions make similar operations, so better to store them in one place. I'd actually prefer to have a separate class/module for them, with TABLE::delete_row and maybe some others, but was afraid It'll be not in style the rest code is written -- seems, mysql/mariadb developers prefer not to spawn a lot of files, and to group logical API blocks by comments😐
+{ + 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?
Hmm maybe. I doubt it will prevent any bug hereafter, but added, let's see how it'll go.
One of the benefits of asserts that I like, they document the code.
Sure
assert(table->triggers == NULL);
is a self-enforcing version of
/* here the table must have no triggers */
and unlike the comment it'll never be out of date.
Yes, looks reasonable in such point of view, now i'm convinced. Anyway it should have been added by previous update
Sincerely, Nikita Malyavin
Hi, Nikita! On Jan 25, Nikita Malyavin wrote:
+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
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case.
it depends on how you write the grammar :)
What about "for portion of `SYSTEM_TIME` ..."? Is it correct to throw ER_PARSE_ERROR in this case?
Yes, I'd think that You have an error in your SQL syntax; ... use near 'SYSTEM_TIME' at line 1 would be quite reasonable
+ 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?
Looks like for DELETE only. Does the naming look confusing?
No, I wanted to ask to move it from a TABLE method to a static function in sql_delete.cc:
static int update_portion_of_time(THD*, TABLE*, vers_select_conds_t&, bool*);
Well, technically moving it to sql_delete will make table_update_generated_fields and period_make_insert extern. I also like current code arrangement by the following reason: these functions make similar operations, so better to store them in one place. I'd actually prefer to have a separate class/module for them, with TABLE::delete_row and maybe some others, but was afraid It'll be not in style the rest code is written -- seems, mysql/mariadb developers prefer not to spawn a lot of files, and to group logical API blocks by comments
Well, yes, and because update_portion_of_time() is a delete-specific functionality, it should be logically with the rest of the delete code and not polluting global namespace and shared objects. And if period_make_insert() is used both for UPDATE and DELETE, then it'd be quite logical to make it extern and use both from UPDATE and from DELETE. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! 26 Jan 2019 06:27, Sergei Golubchik <serg@mariadb.org>:
+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
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case.
it depends on how you write the grammar :)
What about "for portion of `SYSTEM_TIME` ..."? Is it correct to throw ER_PARSE_ERROR in this case?
Yes, I'd think that
You have an error in your SQL syntax; ... use near 'SYSTEM_TIME' at line 1
would be quite reasonable
Ok, did it. Please see the last update.
+ 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?
Looks like for DELETE only. Does the naming look confusing?
No, I wanted to ask to move it from a TABLE method to a static function in sql_delete.cc:
static int update_portion_of_time(THD*, TABLE*, vers_select_conds_t&, bool*);
Well, technically moving it to sql_delete will make table_update_generated_fields and period_make_insert extern. I also like current code arrangement by the following reason: these functions make similar operations, so better to store them in one place. I'd actually prefer to have a separate class/module for them, with TABLE::delete_row and maybe some others, but was afraid It'll be not in style the rest code is written -- seems, mysql/mariadb developers prefer not to spawn a lot of files, and to group logical API blocks by comments
Well, yes, and because update_portion_of_time() is a delete-specific functionality, it should be logically with the rest of the delete code and not polluting global namespace and shared objects.
And if period_make_insert() is used both for UPDATE and DELETE, then it'd be quite logical to make it extern and use both from UPDATE and from DELETE.
Well, still quite arguable for me, but ok. I made it in the same manner TABLE::delete_row written -- just moved the method in sql_delete.cc and marked it inline. Is this solution OK for You? With regards, Nikta Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik