maria-developers team mailing list archive

Thread
Date
Re: c39f74ce0d9: MDEV-16974 Application period tables: UPDATE

To: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
From: Sergei Golubchik <serg@xxxxxxxxxxx>
Date: Tue, 8 Jan 2019 21:09:30 +0100
Cc: maria-developers@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4wUJccRCyHI.A.DqF.0pfMcB@meddwl>
User-agent: Mutt/1.7.2 (2016-11-26)
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@xxxxxxxxx>
> committer: Nikita Malyavin <nikitamalyavin@xxxxxxxxx>
> 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@xxxxxxxxxxx
Follow ups

Re: c39f74ce0d9: MDEV-16974 Application period tables: UPDATE
From: Nikita Malyavin, 2019-01-29