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