maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hi, Sergei!

Jan 9, 2019 06:09, Sergei Golubchik <serg@xxxxxxxxxxx>:

>
> > 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