Hi, Sergei!
Jan 9, 2019 06:09, Sergei Golubchik <s...@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
✅
> 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').
>
> 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. ✅
> +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 ...'"
>
> I've got rid of using table_primary_ident, as You suggested below, which
resolves it
✅
> +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 ...'" ?
>
> 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
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
