Re: [Maria-developers] c39f74ce0d9: MDEV-16974 Application period tables: UPDATE
Hi, Nikita! See comments below On Jan 06, Nikita Malyavin wrote:
revision-id: c39f74ce0d9 (versioning-1.0.7-4-gc39f74ce0d9) parent(s): fd18ed07d65 author: Nikita Malyavin <nikitamalyavin@gmail.com> committer: Nikita Malyavin <nikitamalyavin@gmail.com> timestamp: 2018-12-03 13:19:18 +1000 message:
MDEV-16974 Application period tables: UPDATE
diff --git a/mysql-test/suite/period/r/delete.result b/mysql-test/suite/period/r/delete.result index 30c9220d6f9..ccb8663bf72 100644 --- a/mysql-test/suite/period/r/delete.result +++ b/mysql-test/suite/period/r/delete.result @@ -83,6 +83,9 @@ log <INS: 3, 1997-01-01, 2000-01-01
INS: 4, 2018-01-01, 2020-01-01 <INS: 4, 2018-01-01, 2020-01-01 +# multi-table DELETE is prohibited +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1; +ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
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 delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; diff --git a/mysql-test/suite/period/r/update.result b/mysql-test/suite/period/r/update.result new file mode 100644 index 00000000000..1f59102c131 --- /dev/null +++ b/mysql-test/suite/period/r/update.result @@ -0,0 +1,244 @@ +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; +update t for portion of apptime from '2000-01-01' to '2018-01-01' + set id=id + 5; +select * from t;
same comment about unsorted selects
+id s e +6 2000-01-01 2018-01-01 +6 2000-01-01 2017-01-01 +6 2017-01-01 2018-01-01 +7 2000-01-01 2018-01-01 +8 2000-01-01 2015-01-01 +9 2016-01-01 2018-01-01 +1 1999-01-01 2000-01-01 +1 2018-01-01 2018-12-12 +1 1999-01-01 2000-01-01 +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 +# Check triggers +update t1 for portion of apptime from '2000-01-01' to '2018-01-01' + set id=id + 5; +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 +6 2000-01-01 2017-01-01 +6 2000-01-01 2018-01-01 +6 2017-01-01 2018-01-01 +7 2000-01-01 2018-01-01 +8 2000-01-01 2015-01-01 +9 2016-01-01 2018-01-01 +select * from log_tbl; +log +>UPD: 1, 1999-01-01, 2018-12-12 -> 6, 2000-01-01, 2018-01-01 +<UPD: 1, 1999-01-01, 2018-12-12 -> 6, 2000-01-01, 2018-01-01 +>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 +>UPD: 1, 1999-01-01, 2017-01-01 -> 6, 2000-01-01, 2017-01-01 +<UPD: 1, 1999-01-01, 2017-01-01 -> 6, 2000-01-01, 2017-01-01 +>INS: 1, 1999-01-01, 2000-01-01 +<INS: 1, 1999-01-01, 2000-01-01 +>UPD: 1, 2017-01-01, 2019-01-01 -> 6, 2017-01-01, 2018-01-01 +<UPD: 1, 2017-01-01, 2019-01-01 -> 6, 2017-01-01, 2018-01-01 +>INS: 1, 2018-01-01, 2019-01-01 +<INS: 1, 2018-01-01, 2019-01-01 +>UPD: 2, 1998-01-01, 2018-12-12 -> 7, 2000-01-01, 2018-01-01 +<UPD: 2, 1998-01-01, 2018-12-12 -> 7, 2000-01-01, 2018-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 +>UPD: 3, 1997-01-01, 2015-01-01 -> 8, 2000-01-01, 2015-01-01 +<UPD: 3, 1997-01-01, 2015-01-01 -> 8, 2000-01-01, 2015-01-01 +>INS: 3, 1997-01-01, 2000-01-01 +<INS: 3, 1997-01-01, 2000-01-01 +>UPD: 4, 2016-01-01, 2020-01-01 -> 9, 2016-01-01, 2018-01-01 +<UPD: 4, 2016-01-01, 2020-01-01 -> 9, 2016-01-01, 2018-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 tr1upd_t2; +drop trigger tr2upd_t2; +update t2 for portion of apptime from '2000-01-01' to '2018-01-01' + set id=id + 5; +select * from t2 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 +6 2000-01-01 2017-01-01 +6 2000-01-01 2018-01-01 +6 2017-01-01 2018-01-01 +7 2000-01-01 2018-01-01 +8 2000-01-01 2015-01-01 +9 2016-01-01 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 +select * from t for portion of apptime from 0 to 1 for system_time all; +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
should be "right syntax to use near 'for portion of apptime from ...'"
+update t for portion of apptime from 0 to 1 for system_time all set id=1; +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 'set id=1' at line 1
why not "right syntax to use near 'for system_time all ...'" ?
+# Modifying period start/end fields is forbidden. +# SQL17:
fix the reference, please (SQL:2016, Part 2, section ... paragraph ...)
+# Neither BSTARTCOL nor BENDCOL shall be an explicit <object column> +# contained in the <set clause list>. +update t for portion of apptime from '2000-01-01' to '2018-01-01' + set id= id + 5, s=subdate(s, 5), e=adddate(e, 5); +ERROR HY000: Column `s` used in period `apptime` specified in update SET list +# Precision timestamps +create or replace table t (id int, s timestamp(5), e timestamp(5), +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'); +update t for portion of apptime from '2000-01-01 00:00:00.00015' + to '2018-01-01 12:34:56.31415' + set id= id + 5; +select * from t; +id s e +6 2000-01-01 00:00:00.00015 2018-01-01 12:34:56.31415 +6 2000-01-01 00:00:00.00015 2017-01-01 00:00:00.00000 +1 1999-01-01 00:00:00.00000 2000-01-01 00:00:00.00015 +1 2018-01-01 12:34:56.31415 2018-12-12 00:00:00.00000 +1 1999-01-01 00:00:00.00000 2000-01-01 00:00:00.00015 +# Strings +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'); +update t for portion of apptime from '2000-01-01' to '2018-01-01' + set id= id + 5; +select * from t; +id str s e +6 data 2000-01-01 2018-01-01 +6 other data 2000-01-01 2018-01-01 +1 data 1999-01-01 2000-01-01 +1 data 2018-01-01 2018-12-12 +1 other data 1999-01-01 2000-01-01 +1 other data 2018-01-01 2018-12-12 +# multi-table UPDATE is prohibited +create or replace table t1(x int); +update t for portion of apptime from '2000-01-01' to '2018-01-01', t1 +set t.id= t.id + 5; +ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
+update t1 set x= (select id from t for portion of apptime from '2000-01-01' to '2018-01-01'); +ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
+# SQL17:
fix the reference, please
+# Let FROMVAL be <point in time 1>. FROMVAL shall not generally contain a +# reference to a column of T or a <routine invocation> +# whose subject routine is an SQL-invoked routine that +# is possibly non-deterministic or that possibly modifies SQL-data. +# ...Same for <point in time 2> (TOVAL) +update t for portion of apptime from 5*(5+s) to 1 set t.id= t.id + 5; +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions +update t for portion of apptime from 1 to e set t.id= t.id + 5; +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions +set @s= '2000-01-01'; +set @e= '2018-01-01'; +create or replace function f() returns date return @e; +create or replace function g() returns date not deterministic return @e; +create or replace function h() returns date deterministic return @e; +update t for portion of apptime from @s to f() set t.id= t.id + 5; +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions +update t for portion of apptime from @s to g() set t.id= t.id + 5; +ERROR HY000: Values in range FOR PORTION OF `apptime` should be constant expressions +# success +update t for portion of apptime from @s to h() set t.id= t.id + 5; +# select value is cached +update t for portion of apptime from (select s from t2 limit 1) to h() set t.id= t.id + 5; +# auto_inrement field is updated +create or replace table t (id int primary key auto_increment, x int, +s date, e date, period for apptime(s, e)); +insert into t values (default, 1, '1999-01-01', '2018-12-12'); +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; +select * from t; +id x s e +1 6 2000-01-01 2018-01-01 +2 1 1999-01-01 2000-01-01 +3 1 2018-01-01 2018-12-12 +truncate t; +insert into t values (default, 1, '1999-01-01', '2018-12-12'); +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= 1; +select * from t; +id x s e +1 1 2000-01-01 2018-01-01 +2 1 1999-01-01 2000-01-01 +3 1 2018-01-01 2018-12-12 +# generated columns are updated +create or replace table t (x int, s date, e date, +xs date as (s) stored, xe date as (e) stored, +period for apptime(s, e)); +insert into t values(1, '1999-01-01', '2018-12-12', default, default); +select * from t; +x s e xs xe +1 1999-01-01 2018-12-12 1999-01-01 2018-12-12 +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; +select *, xs=s and xe=e from t; +x s e xs xe xs=s and xe=e +6 2000-01-01 2018-01-01 2000-01-01 2018-01-01 1 +1 1999-01-01 2000-01-01 1999-01-01 2000-01-01 1 +1 2018-01-01 2018-12-12 2018-01-01 2018-12-12 1 +# system_time columns are updated +create or replace table t (x int, s date, e date, +row_start SYS_TYPE as row start invisible, +row_end SYS_TYPE as row end invisible,
a bit strange to use invisible in tests for strict standard compliance :) but ok, whatever
+period for apptime(s, e), +period for system_time(row_start, row_end)) with system versioning; +insert into t values(1, '1999-01-01', '2018-12-12'), +(2, '1999-01-01', '1999-12-12'); +select row_start into @ins_time from t limit 1; +select * from t order by x, s, e; +x s e +1 1999-01-01 2018-12-12 +2 1999-01-01 1999-12-12 +update t for portion of apptime from '2000-01-01' to '2018-01-01' set x= x + 5; +select *, if(row_start = @ins_time, "OLD", "NEW"), check_row(row_start, row_end) +from t for system_time all +order by x, s, e, row_start; +x s e if(row_start = @ins_time, "OLD", "NEW") check_row(row_start, row_end) +1 1999-01-01 2000-01-01 NEW CURRENT ROW +1 1999-01-01 2018-12-12 OLD HISTORICAL ROW +1 2018-01-01 2018-12-12 NEW CURRENT ROW +2 1999-01-01 1999-12-12 OLD CURRENT ROW +6 2000-01-01 2018-01-01 NEW CURRENT ROW +create or replace database test; diff --git a/mysql-test/suite/period/t/delete.test b/mysql-test/suite/period/t/delete.test index b34de6c37e7..39a68959523 100644 --- a/mysql-test/suite/period/t/delete.test +++ b/mysql-test/suite/period/t/delete.test @@ -34,6 +34,10 @@ drop trigger tr2del_t2; delete from t2 for portion of APPTIME from '2000-01-01' to '2018-01-01'; select * from log_tbl;
+--echo # multi-table DELETE is prohibited +--error ER_PORTION_OF_TIME_NOT_ALLOWED +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1;
move this into your MDEV-16973 commit is possible, please,
+ --error ER_PARSE_ERROR delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01';
diff --git a/mysql-test/suite/period/t/update.opt b/mysql-test/suite/period/t/update.opt new file mode 100644 index 00000000000..92a881bef67 --- /dev/null +++ b/mysql-test/suite/period/t/update.opt @@ -0,0 +1 @@ +--explicit_defaults_for_timestamp
Why? it's best to avoid non-default command line options, unless you actually test this particular option. If you just need it for convenience - don't. every non-default command line options means a server restart, and they're slow. write DEFAULT NULL explicitly when needed.
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt index 7ceff1a215e..c5a359b09e8 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7951,3 +7951,12 @@ ER_PERIOD_NOT_FOUND
ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED eng "period SYSTEM_TIME is not allowed here" + +ER_PORTION_OF_TIME_NOT_ALLOWED + eng "PORTION OF time is not allowed here" + +ER_PERIOD_PORTION_OF_TIME_CONSTANT + eng "Values in range FOR PORTION OF %`s should be constant expressions"
Reuse ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR please. Like ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR -> ER_NOT_CONSTANT_EXPRESSION eng "Expression in RANGE/LIST VALUES must be constant" -> eng "Expression in %s must be constant" and in mysql.h #define ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR ER_NOT_CONSTANT_EXPRESSION
+ +ER_PERIOD_COLUMNS_UPDATED + eng "Column %`s used in period %`s specified in update SET list" diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 7a18ac82236..93d2f417a50 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -1343,13 +1343,16 @@ int Lex_input_stream::lex_token(YYSTYPE *yylval, THD *thd) /* * Additional look-ahead to resolve doubtful cases like: * SELECT ... FOR UPDATE - * SELECT ... FOR SYSTEM_TIME ... . + * SELECT ... FOR SYSTEM_TIME ... + * SELECT ... FOR PORTION OF ... .
Can you show an example of the statement where you need a second look-ahead to distinguish between FOR PORTION OF and FOR SYSTEM_TIME or FOR UPDATE ?
*/ token= lex_one_token(yylval, thd); add_digest_token(token, yylval); switch(token) { case SYSTEM_TIME_SYM: return FOR_SYSTEM_TIME_SYM; + case PORTION: + return FOR_PORTION_SYM; default: /* Save the token following 'FOR_SYM' diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 220fcf6cc34..44989b20fae 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -8121,6 +8121,12 @@ int setup_conds(THD *thd, TABLE_LIST *tables, List<TABLE_LIST> &leaves,
for (table= tables; table; table= table->next_local) { + if (table->has_period() && !thd->lex->portion_of_time_applicable()) + { + my_error(ER_PORTION_OF_TIME_NOT_ALLOWED, MYF(0)); + goto err_no_arena; + }
I don't understand that either, why do you need to check the correct syntax in setup_conds() ?
+ if (select_lex == thd->lex->first_select_lex() && select_lex->first_cond_optimization && table->merged_for_insert && diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index e76eb729536..4726557d762 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -752,6 +752,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, delete_record= true; }
+ /* SQL standard defines DELETE FOR PORTTION OF time as 0-2 INSERTS + DELETE + * We can substitute INSERT+DELETE with one UPDATE, but only if there are + * no triggers set. It is also meaningless for system-versioned table + */
1. format multi-line comments like everywhere in the code, please 2. every time you refer to the standard, say where the standard says that 3. move this comment in your MDEV-16973 commit, if possible
portion_of_time_through_update= !has_period_triggers && !table->versioned(VERS_TIMESTAMP);
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 0c368b40836..10535326ce5 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -11910,12 +11911,13 @@ join_table_parens:
table_primary_ident: - table_ident opt_use_partition opt_for_system_time_clause + table_ident opt_use_partition + opt_for_portion_of_time_clause opt_for_system_time_clause
Oh, no. FOR PORTION OF has very few well defined places where the standard allows it. Don't put it into the generic used-everywhere table_primary_ident. That's why you needed FOR_PORTION_SYM and a second look-ahead.
opt_table_alias_clause opt_key_definition { SELECT_LEX *sel= Select; sel->table_join_options= 0; - if (!($$= Select->add_table_to_list(thd, $1, $4, + if (!($$= Select->add_table_to_list(thd, $1, $5, Select->get_table_join_options(), YYPS->m_lock_type, YYPS->m_mdl_type, diff --git a/sql/table.h b/sql/table.h index 1b897c1b4c6..f104f4fc99c 100644 --- a/sql/table.h +++ b/sql/table.h @@ -2420,7 +2423,7 @@ struct TABLE_LIST
bool has_period() const { - return period_conditions.name; + return period_conditions.is_set();
why?
}
/**
diff --git a/sql/table.cc b/sql/table.cc index 8f1c94dd633..93908a9e2e3 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -6456,7 +6456,7 @@ void TABLE::mark_columns_needed_for_delete(bool with_period) retrieve the row again. */
-void TABLE::mark_columns_needed_for_update() +void TABLE::mark_columns_needed_for_update(bool with_period)
just like in the delete case, I think this is the caller's problem
{ DBUG_ENTER("TABLE::mark_columns_needed_for_update"); bool need_signal= false; @@ -7870,19 +7871,41 @@ static int period_make_insert(TABLE *table, Item *src, Field *dst) }
+int TABLE::cut_fields_for_portion_of_time(THD *thd, const vers_select_conds_t &period_conds)
looks like something that belongs to sql_update.cc, not TABLE
+{ + 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); + + Field *start_field= field[s->period.start_fieldno]; + Field *end_field= field[s->period.end_fieldno]; + + int res= 0; + if (lcond && !start_field->has_explicit_value()) + res= period_conds.start.item->save_in_field(start_field, true); + + if (likely(!res) && rcond && !end_field->has_explicit_value()) + res= period_conds.end.item->save_in_field(end_field, true); + + return res; +} + -int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, +int TABLE::update_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, bool *inside_period)
looks like something that belongs to sql_delete.cc, not TABLE
{ bool lcond= period_conds.field_start->val_datetime_packed(thd) @@ -7917,7 +7940,8 @@ int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, return res; }
-int TABLE::insert_portion_of_time(THD *thd, vers_select_conds_t &period_conds) +int TABLE::insert_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, + ha_rows &rows_inserted)
pointers or _constant_ references. Here, rows_inserted must be a pointer.
{ bool lcond= period_conds.field_start->val_datetime_packed(thd) < period_conds.start.item->val_datetime_packed(thd); diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 27bd6f04670..d5240f5524e 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -177,6 +178,21 @@ static bool check_fields(THD *thd, List<Item> &items, bool update_view) f->set_has_explicit_value(); } } + + if (thd->lex->sql_command == SQLCOM_UPDATE && table->has_period())
what else can sql_command be here? SQLCOM_UPDATE_MULTI? SQLCOM_UPDATE_MULTI cannot have table->has_period().
+ { + for (List_iterator_fast<Item> it(items); (item=it++);) + { + Lex_ident name(item->name); + vers_select_conds_t &period= table->period_conditions; + if (name.streq(period.field_start->name) + || name.streq(period.field_end->name))
Don't compare names, compare fields. The field to be updated is, I guess, item->field_for_view_update()->field
+ { + my_error(ER_PERIOD_COLUMNS_UPDATED, MYF(0), name.str, period.name.str); + return true; + } + } + } return FALSE; }
@@ -323,6 +339,7 @@ int mysql_update(THD *thd, List<Item> all_fields; killed_state killed_status= NOT_KILLED; bool has_triggers, binlog_is_row, do_direct_update= FALSE; + bool has_period_triggers= false;
same as for sql_delete.cc, one has_triggers is enough, second has_period_triggers is quite confusing.
Update_plan query_plan(thd->mem_root); Explain_update *explain; TABLE_LIST *update_source_table; @@ -880,14 +905,25 @@ int mysql_update(THD *thd, explain->tracker.on_record_after_where(); store_record(table,record[1]);
+ if (table_list->has_period()) + table->cut_fields_for_portion_of_time(thd, table_list->period_conditions);
1. this should be done after BEFORE triggers (see SQL:2016, part 2, 15.13 "Effect of replacing rows in base tables") 2. why in cut_fields_for_portion_of_time() you check for has_explicit_value()? fields cannot have explicit values there, you've checked that in check_fields().
+ if (fill_record_n_invoke_before_triggers(thd, table, fields, values, 0, TRG_EVENT_UPDATE)) break; /* purecov: inspected */
found++;
- if (!can_compare_record || compare_record(table)) + bool record_was_same= false; + bool need_update= !can_compare_record || compare_record(table) || + thd->lex->sql_command == SQLCOM_DELETE;
why sql_command would be SQLCOM_DELETE here?
+ + if (need_update) { + if (table->versioned(VERS_TIMESTAMP) && + thd->lex->sql_command == SQLCOM_DELETE) + table->vers_update_end(); + if (table->default_field && table->update_default_fields(1, ignore)) { error= 1; @@ -994,6 +1032,13 @@ int mysql_update(THD *thd, break; }
+ if (need_update && !record_was_same && table_list->has_period())
I suspect this should happen even if record_was_same. The standard never says "if new values are the same as old values, don't update anything"
+ { + restore_record(table, record[1]); + table->insert_portion_of_time(thd, table_list->period_conditions, + rows_inserted); + } + if (!--limit && using_limit) { /*
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Sergei! Jan 9, 2019 06:09, Sergei Golubchik <serg@mariadb.org>:
diff --git a/mysql-test/suite/period/r/delete.result b/mysql-test/suite/period/r/delete.result index 30c9220d6f9..ccb8663bf72 100644 --- a/mysql-test/suite/period/r/delete.result +++ b/mysql-test/suite/period/r/delete.result @@ -83,6 +83,9 @@ log <INS: 3, 1997-01-01, 2000-01-01
INS: 4, 2018-01-01, 2020-01-01 <INS: 4, 2018-01-01, 2020-01-01 +# multi-table DELETE is prohibited +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1; +ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
Resolved in DELETE commit
✅
ERROR 42000: You have an error in your SQL syntax; check the manual
delete history from t2 for portion of apptime from '2000-01-01' to '2018-01-01'; that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; diff --git a/mysql-test/suite/period/r/update.result b/mysql-test/suite/period/r/update.result new file mode 100644 index 00000000000..1f59102c131 --- /dev/null +++ b/mysql-test/suite/period/r/update.result @@ -0,0 +1,244 @@ +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').
Ok. I've also changed id= id+5 to id= id+6 in relevant places, to keep the result set disjoint with original values ✅
+select * from t;
same comment about unsorted selects
Added. ✅
+ERROR 42000: You have an error in your SQL syntax; check the manual
+select * from t for portion of apptime from 0 to 1 for system_time all; that corresponds to your MariaDB server version for the right syntax to use near '' at line 1
should be "right syntax to use near 'for portion of apptime from ...'"
I've got rid of using table_primary_ident, as You suggested below, which resolves it ✅
+ERROR 42000: You have an error in your SQL syntax; check the manual
+update t for portion of apptime from 0 to 1 for system_time all set id=1; that corresponds to your MariaDB server version for the right syntax to use near 'set id=1' at line 1
why not "right syntax to use near 'for system_time all ...'" ?
same as above✅
+# Modifying period start/end fields is forbidden.
+# SQL17:
fix the reference, please (SQL:2016, Part 2, section ... paragraph ...)
+# Neither BSTARTCOL nor BENDCOL shall be an explicit <object column> +# contained in the <set clause list>.
✅
+# multi-table UPDATE is prohibited +create or replace table t1(x int); +update t for portion of apptime from '2000-01-01' to '2018-01-01', t1 +set t.id= t.id + 5; +ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
It's ER_PARSE_ERROR now ✅
+update t1 set x= (select id from t for portion of apptime from '2000-01-01' to '2018-01-01');
+ERROR HY000: PORTION OF time is not allowed here
why not ER_PARSE_ERROR?
It's ER_PARSE_ERROR now ✅
+# SQL17:
fix the reference, please
+# Let FROMVAL be <point in time 1>. FROMVAL shall not generally contain a
+# reference to a column of T or a <routine invocation> +# whose subject routine is an SQL-invoked routine that +# is possibly non-deterministic or that possibly modifies SQL-data. +# ...Same for <point in time 2> (TOVAL)
✅
+# system_time columns are updated
+create or replace table t (x int, s date, e date, +row_start SYS_TYPE as row start invisible, +row_end SYS_TYPE as row end invisible,
a bit strange to use invisible in tests for strict standard compliance :) but ok, whatever
Not exactly:) it's a test for crossover with versioning, which is not so strictly compilant, especially trx_id feature
+--echo # multi-table DELETE is prohibited
+--error ER_PORTION_OF_TIME_NOT_ALLOWED +delete t, t1 from t for portion of apptime from '2000-01-01' to '2018-01-01', t1;
move this into your MDEV-16973 commit is possible, please,
moved ✅
--- /dev/null
+++ b/mysql-test/suite/period/t/update.opt @@ -0,0 +1 @@ +--explicit_defaults_for_timestamp
Why? it's best to avoid non-default command line options, unless you actually test this particular option. If you just need it for convenience - don't.
every non-default command line options means a server restart, and they're slow.
write DEFAULT NULL explicitly when needed.
that's because DEFAULT NOW ON UPDATE NOW is implicitly added to TIMESTAMP fields. I did not know what to do with it first, so created MDEV-17094 to decide what to do with it later. Now I know -- implicit DNUN just should not be added to period fields. I'll make a separate commit related to that task, if You don't mind
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
index 7ceff1a215e..c5a359b09e8 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7951,3 +7951,12 @@ ER_PERIOD_NOT_FOUND
ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED eng "period SYSTEM_TIME is not allowed here" + +ER_PORTION_OF_TIME_NOT_ALLOWED + eng "PORTION OF time is not allowed here" + +ER_PERIOD_PORTION_OF_TIME_CONSTANT + eng "Values in range FOR PORTION OF %`s should be constant expressions"
Reuse ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR please. Like
ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR -> ER_NOT_CONSTANT_EXPRESSION eng "Expression in RANGE/LIST VALUES must be constant" -> eng "Expression in %s must be constant"
and in mysql.h
#define ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR ER_NOT_CONSTANT_EXPRESSION
This error seems to be unused... Considering that, do You still want the macro added to mysql.h?
/* * Additional look-ahead to resolve doubtful cases like: * SELECT ... FOR UPDATE - * SELECT ... FOR SYSTEM_TIME ... . + * SELECT ... FOR SYSTEM_TIME ... + * SELECT ... FOR PORTION OF ... .
Can you show an example of the statement where you need a second look-ahead to distinguish between FOR PORTION OF and FOR SYSTEM_TIME or FOR UPDATE ?
SELECT * FROM t FOR UPDATE was just failing to parse, but that's not appropriate anymore since latest parser changes. This code is removed✅
for (table= tables; table; table= table->next_local) { + if (table->has_period() && !thd->lex->portion_of_time_applicable()) + { + my_error(ER_PORTION_OF_TIME_NOT_ALLOWED, MYF(0)); + goto err_no_arena; + }
I don't understand that either, why do you need to check the correct syntax in setup_conds() ?
Same here. Hunk removed.✅
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index e76eb729536..4726557d762 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -752,6 +752,10 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, delete_record= true; }
+ /* SQL standard defines DELETE FOR PORTTION OF time as 0-2 INSERTS + DELETE + * We can substitute INSERT+DELETE with one UPDATE, but only if there are + * no triggers set. It is also meaningless for system-versioned table + */
1. format multi-line comments like everywhere in the code, please 2. every time you refer to the standard, say where the standard says that 3. move this comment in your MDEV-16973 commit, if possible
done✅
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy index 0c368b40836..10535326ce5 100644 --- a/sql/sql_yacc.yy +++ b/sql/sql_yacc.yy @@ -11910,12 +11911,13 @@ join_table_parens:
table_primary_ident: - table_ident opt_use_partition opt_for_system_time_clause + table_ident opt_use_partition + opt_for_portion_of_time_clause opt_for_system_time_clause
Oh, no. FOR PORTION OF has very few well defined places where the standard allows it. Don't put it into the generic used-everywhere table_primary_ident.
That's why you needed FOR_PORTION_SYM and a second look-ahead.
Yes, thanks. I've reimplemented the parser part to distinguish table definitions and removed all that lexer code.
opt_table_alias_clause opt_key_definition
{ SELECT_LEX *sel= Select; sel->table_join_options= 0; - if (!($$= Select->add_table_to_list(thd, $1, $4, + if (!($$= Select->add_table_to_list(thd, $1, $5,
Select->get_table_join_options(),
YYPS->m_lock_type, YYPS->m_mdl_type, diff --git a/sql/table.h b/sql/table.h index 1b897c1b4c6..f104f4fc99c 100644 --- a/sql/table.h +++ b/sql/table.h @@ -2420,7 +2423,7 @@ struct TABLE_LIST
bool has_period() const { - return period_conditions.name; + return period_conditions.is_set();
why?
Not a big difference, actually. Alexey just wanted is_set to be reused
here. Rebased it into delete.
-void TABLE::mark_columns_needed_for_update() +void TABLE::mark_columns_needed_for_update(bool with_period)
just like in the delete case, I think this is the caller's problem
👍✅
{ DBUG_ENTER("TABLE::mark_columns_needed_for_update"); bool need_signal= false; @@ -7870,19 +7871,41 @@ static int period_make_insert(TABLE *table, Item *src, Field *dst) }
+int TABLE::cut_fields_for_portion_of_time(THD *thd, const vers_select_conds_t &period_conds)
looks like something that belongs to sql_update.cc, not TABLE
moved✅
-int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, +int TABLE::update_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, bool *inside_period)
looks like something that belongs to sql_delete.cc, not TABLE
✅
-int TABLE::insert_portion_of_time(THD *thd, vers_select_conds_t &period_conds) +int TABLE::insert_portion_of_time(THD *thd, const vers_select_conds_t &period_conds, + ha_rows &rows_inserted)
pointers or _constant_ references. Here, rows_inserted must be a pointer.
✅
{ bool lcond= period_conds.field_start->val_datetime_packed(thd) < period_conds.start.item->val_datetime_packed(thd); diff --git a/sql/sql_update.cc b/sql/sql_update.cc index 27bd6f04670..d5240f5524e 100644 --- a/sql/sql_update.cc +++ b/sql/sql_update.cc @@ -177,6 +178,21 @@ static bool check_fields(THD *thd, List<Item> &items, bool update_view) f->set_has_explicit_value(); } } + + if (thd->lex->sql_command == SQLCOM_UPDATE && table->has_period())
what else can sql_command be here? SQLCOM_UPDATE_MULTI? SQLCOM_UPDATE_MULTI cannot have table->has_period().
Yes, it was just to make sure it's true. Extracted to DBUG_ASSERT
+ { + for (List_iterator_fast<Item> it(items); (item=it++);) + { + Lex_ident name(item->name); + vers_select_conds_t &period= table->period_conditions; + if (name.streq(period.field_start->name) + || name.streq(period.field_end->name))
Don't compare names, compare fields. The field to be updated is, I guess, item->field_for_view_update()->field
Seems to be working, thanks! Updated ✅
@@ -323,6 +339,7 @@ int mysql_update(THD *thd,
List<Item> all_fields; killed_state killed_status= NOT_KILLED; bool has_triggers, binlog_is_row, do_direct_update= FALSE; + bool has_period_triggers= false;
same as for sql_delete.cc, one has_triggers is enough, second has_period_triggers is quite confusing.
Removed has_period_triggers ✅
Update_plan query_plan(thd->mem_root); Explain_update *explain; TABLE_LIST *update_source_table; @@ -880,14 +905,25 @@ int mysql_update(THD *thd, explain->tracker.on_record_after_where(); store_record(table,record[1]);
+ if (table_list->has_period()) + table->cut_fields_for_portion_of_time(thd, table_list->period_conditions);
1. this should be done after BEFORE triggers (see SQL:2016, part 2, 15.13 "Effect of replacing rows in base tables")
Disagree. Pls take a loot at 14.14 <update statement: searched> 7)a)vi) ❗
2. why in cut_fields_for_portion_of_time() you check for
has_explicit_value()? fields cannot have explicit values there, you've checked that in check_fields().
Seems to be left from the time I tried to implement using explicit values as an extension to the standard. Extracted them to DBUG_ASSERT. ✅
BTW, maybe we can run CHECK constraints in DBUG_ASSERT in table_update_generated_fields also. ...Added; let's see how will it stay.
- if (!can_compare_record || compare_record(table)) + bool record_was_same= false; + bool need_update= !can_compare_record || compare_record(table) || + thd->lex->sql_command == SQLCOM_DELETE;
why sql_command would be SQLCOM_DELETE here?
Can't be in this branch. Orphan; removed✅
+ if (need_update && !record_was_same && table_list->has_period())
I suspect this should happen even if record_was_same. The standard never says "if new values are the same as old values, don't update anything"
Yes, looks like it never says so. And it was implemented in that way. Those two checks -- need_update && !record_was_same -- could be safely omitted, they don't change the behavior. Because in that way lcond and rcond are false, anyway.
Maybe it's better to remove them -- the optimization is quite arguable here. Up to You, ok? Thanks for the review! Nikita Malyavin
Hi, Nikita! On Jan 30, Nikita Malyavin wrote:
#define ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR ER_NOT_CONSTANT_EXPRESSION
This error seems to be unused... Considering that, do You still want the macro added to mysql.h?
Okay, if it's never issued, then no need to add the define.
+ if (need_update && !record_was_same && table_list->has_period())
I suspect this should happen even if record_was_same. The standard never says "if new values are the same as old values, don't update anything"
Yes, looks like it never says so. And it was implemented in that way. Those two checks -- need_update && !record_was_same -- could be safely omitted, they don't change the behavior. Because in that way lcond and rcond are false, anyway.
Maybe it's better to remove them -- the optimization is quite arguable here. Up to You, ok?
yes, let's remove. it's not an optimization even, but a bug, because it changes behavior from what the standard says. Regards, Sergei Chief Architect MariaDB and security@mariadb.org
On Fri, Feb 1, 2019 at 3:16 AM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita!
On Jan 30, Nikita Malyavin wrote:
#define ER_NO_CONST_EXPR_IN_RANGE_OR_LIST_ERROR
ER_NOT_CONSTANT_EXPRESSION
This error seems to be unused... Considering that, do You still want the macro added to mysql.h?
Okay, if it's never issued, then no need to add the define.
+ if (need_update && !record_was_same && table_list->has_period())
I suspect this should happen even if record_was_same. The standard never says "if new values are the same as old values, don't update anything"
Yes, looks like it never says so. And it was implemented in that way. Those two checks -- need_update && !record_was_same -- could be safely omitted, they don't change the behavior. Because in that way lcond and rcond are false, anyway.
Maybe it's better to remove them -- the optimization is quite arguable here. Up to You, ok?
yes, let's remove. it's not an optimization even, but a bug, because it changes behavior from what the standard says.
It doesn't change the bahavior. Proof. Let's consider record_was_same: it is true iff the result from ha_update_row was HA_ERR_RECORD_IS_THE_SAME => record[0] equals record[1] => neither BSTARTVAL nor BENDVAL changed. Consequently, 15.3 <Effect of replacing rows in base tables>, General Rules, 10) c)ii and d)ii do not apply, so no INSERTs should be done. And
✅ they are not done anyway, because BSTARTVAL check (along with BENDVAL) is done inside insert_portion_of_time. Same applies to need_update with the following note: if it's false, then no ha_update_row is called, because record[0] will be equal record[1]. The thing here is that BSTARTVAL is updated to FROMVAL (BENDVAL same) before that check That all was quite useless of course, because anyway i'm going to remove those checks, because the only thing they are doing is avoiding two datetime compaisons known to be false in that case:) -- Yours truly, Nikita Malyavin
On Wed, Jan 30, 2019 at 3:50 AM Nikita Malyavin <nikitamalyavin@gmail.com> wrote:
Jan 9, 2019 06:09, Sergei Golubchik <serg@mariadb.org>:
--- /dev/null +++ b/mysql-test/suite/period/t/update.opt @@ -0,0 +1 @@ +--explicit_defaults_for_timestamp
Why? it's best to avoid non-default command line options, unless you actually test this particular option. If you just need it for convenience - don't.
every non-default command line options means a server restart, and they're slow.
write DEFAULT NULL explicitly when needed.
that's because DEFAULT NOW ON UPDATE NOW is implicitly added to TIMESTAMP fields. I did not know what to do with it first, so created MDEV-17094 to decide what to do with it later. Now I know -- implicit DNUN just should not be added to period fields. I'll make a separate commit related to that task, if You don't mind
Surprisingly, the fix is already there, inside CREATE commit:) You can take a look at it in sql_table.cc changes. So i've removed the .opt file✅ -- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin
-
Sergei Golubchik