maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #11595
Re: 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@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> 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@xxxxxxxxxxx
Follow ups
