Hi!
I'd insert also, say, (5, '2010-01-01', '2015-01-01'). Ok no problem
+select * from t;
try to avoid unsorted selects, the row order is undefined in these cases use --sorted_results command before the select or the order by.
--sorted_results is generally better, because it's done on the client, so it doesn't change the execution plan on the server.
Added --sorted_result to all plain selects✅
+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
The error is good, ER_PARSE_ERROR is very appropriate here. But it should say "right syntax to use near 'for portion of' at line 1"
Well I just used `MYSQL_YYABORT_UNLESS` in parser, as in many other places, and they all have this problem at this moment. Maybe that's a bug of this macro.
+delete from t for portion of othertime from '2000-01-01' to '2018-01-01'; +ERROR HY000: period `othertime` is not found in table +delete from t for portion of system_time from '2000-01-01' to '2018-01-01'; +ERROR HY000: period SYSTEM_TIME is not allowed here
could be just ER_PARSE_ERROR too, I suppose
I think it worth separate error here. ER_PARSE_ERROR will make "right syntax to use near '' at line 1", just like in the above case. However, these cases differ -- it is very likely that user will try to use SYSTEM_TIME period, and we should tell him this is a voilation.
+select * from t;
--sorted_results or order by
✅
+# SQL17 Case 15.7-8.b.i
good! you forgot "part 2" and "general rules", but even without them it only takes a couple of seconds to find this quote.
To be honest, the whole idea to cite the standard in tests belongs to Alexey Midenkov. I've just pasted his citations. Added the rest ✅
+# auto_inrement field is updated
"increment"
✅
+create or replace table t (id int primary key auto_increment, s date, e date, +period for apptime(s, e)); +insert into t values (default, '1999-01-01', '2018-12-12');
please, add a select * here.
✅
+ { "PORTION", SYM(PORTION)},
PORTION_SYM, please
✅
+%token PORTION
add /* SQL-2016-R */ comment
✅
and it seems to be 2016 standard, not 2017 standard, https://en.wikipedia.org/wiki/SQL please change it everywhere, it's too confusing otherwise
uh yes, really, missed it somehow. Replaced ✅
%token POSITION_SYM /* SQL-2003-N */ %token PRECISION /* SQL-2003-R */ %token PRIMARY_SYM /* SQL-2003-R */ @@ -13437,23 +13449,26 @@ delete_part2: opt_delete_options single_multi {} | HISTORY_SYM delete_single_table opt_delete_system_time { + MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period()); Lex->last_table()->vers_conditions= Lex->vers_conditions; Lex->pop_select(); //main select } ;
delete_single_table: - FROM table_ident opt_use_partition + FROM table_ident opt_for_portion_of_time_clause opt_use_partition
better put opt_for_portion_of_time_clause after opt_use_partition, I think it's more logical that way. FOR PORTION OF is logical specification, while opt_use_partition is physical one, just like the table name, so it makes sense to keep them together
✅
+ER_PERIOD_NOT_FOUND + eng "period %`s is not found in table"
first letter capitalized, please
✅
+ +ER_PERIOD_SYSTEM_TIME_NOT_ALLOWED + eng "period SYSTEM_TIME is not allowed here"
same here, or, better, use ER_PARSE_ERROR
✅
diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc index 3ae113a4595..e76eb729536 100644 --- a/sql/sql_delete.cc +++ b/sql/sql_delete.cc @@ -287,7 +287,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds, bool return_error= 0; ha_rows deleted= 0; bool reverse= FALSE; - bool has_triggers; + bool has_triggers= false; + bool has_period_triggers= false;
I don't see why you need has_triggers and has_period_triggers, I find it confusing.
just make `has_triggers=true` if there's `FOR PORTION OF` and there are insert triggers.
In other words, has_triggers should be true if there are any triggers that this statement might need to fire.
ok ✅
+ conds= table_list->on_expr; + table_list->on_expr= NULL;
Why did you define SELECT_LEX::period_setup_conds to put the condition in TABLE_LIST::on_expr only to hack it up from there? Just make it to return the new condition or NULL in case of an error.
I just wanted to reflect versioning behavior as much as possible. Made it just to return a condition ✅
+ portion_of_time_through_update= !has_period_triggers + && !table->versioned(VERS_TIMESTAMP);
I suspect that with VERS_TRX_ID you'd also need to use delete/insert. Add a test, please.
There is a system versioning test at the end of 'delete.test'. It runs for both 'timestamp' and 'trx_id' cases. But what I suspect is that this optimization is meaningful in case of VERS_TRX_ID. The resulting operation will be converted to update+insert in both cases, so I am also removing it for VERS_TRX_ID. ✅
- if (unique_table(thd, table_list, table_list->next_global, 0)) + if (table_list->has_period()
Really? Why?
Yes, really. Table crashes myisam -- just tested. This is related to that we mix deletes/updates and inserts, and it breaks cursor. Maybe we can do something like Eugene did in latest TABLE::delete_row patch -- i.e. save/restore cursor, but delete_while_scanning= false just works.
+ || unique_table(thd, table_list, table_list->next_global, 0)) *delete_while_scanning= false;
@@ -8302,6 +8302,7 @@ TABLE_LIST *st_select_lex::add_table_to_list(THD *thd, ptr->ignore_leaves= MY_TEST(table_options & TL_OPTION_IGNORE_LEAVES); ptr->sequence= MY_TEST(table_options & TL_OPTION_SEQUENCE); ptr->derived= table->sel; + ptr->vers_conditions.name= "SYSTEM_TIME";
why?
Guess it is junk for now... I was thinking about SELECT...AS OF support, and had get_period_by_name function somewhere in TABLE, but it's gone after several review iterations. I was also trying to not so much distinguish SYSTEM_TIME and application-time period already on this step. Removed from this commit and added note in MDEV-16977 ✅
+void period_update_condition(THD *thd, TABLE_LIST *table, SELECT_LEX *select,
static
✅
+ vers_select_conds_t &conds, bool timestamp)
please, either use pointers or _const_ references. in other words, `conds` here should be a pointer
✅
+ switch (conds.type) + { + case SYSTEM_TIME_UNSPECIFIED: + curr= newx Item_int(thd, ULONGLONG_MAX); + cond1= newx Item_func_eq(thd, conds.field_end, curr);
add here
DBUG_ASSERT(!conds.start.item); DBUG_ASSERT(!conds.end.item);
✅
+ break; + case SYSTEM_TIME_AS_OF: + cond1= newx Item_func_trt_trx_sees_eq(thd, trx_id0, conds.field_start); + cond2= newx Item_func_trt_trx_sees(thd, conds.field_end, trx_id0);
DBUG_ASSERT(!conds.end.item);
✅
+ break; + case SYSTEM_TIME_FROM_TO: + cond1= newx Item_func_trt_trx_sees(thd, trx_id1, conds.field_start); + cond3= newx Item_func_lt(thd, conds.start.item, conds.end.item); + break;
hmm, you don't use trx_id0 here at all. did you forget
cond2= newx Item_func_trt_trx_sees_eq(thd, conds.field_end, trx_id0);
yes, looks so... The tests did not catch this
+int SELECT_LEX::period_setup_conds(THD *thd, TABLE_LIST *tables, Item *where) +{ + DBUG_ENTER("SELECT_LEX::period_setup_conds"); + + if (!thd->stmt_arena->is_conventional() && + !thd->stmt_arena->is_stmt_prepare_or_first_sp_execute()) + { + // statement is already prepared + DBUG_RETURN(0); + } + + if (thd->lex->is_view_context_analysis()) + DBUG_RETURN(0);
I see that you've copied these two if's from SELECT_LEX::vers_setup_conds. I suspect that the first if() is not quite correct here, and I plan to take a closer look at it.
so, it'd be great if you could come up with some way to avoid copying these two if()'s. So that there will be only one place to fix.
May be, extract them into a function or something, I don't know.
Those lines were made for a couple of bugfixes. Eugene's just mentioned the latest one: https://github.com/MariaDB/server/commit/748ef3ec91d35aa6cd5b230c71084f6c83c... I've just extracted those if's to a separate function. Have no better idea.
+ + for (TABLE_LIST *table= tables; table; table= table->next_local)
1. can here be more than one table? you only allow FOR PORTION OF in delete_single_table parser rule. 2. if no, add an assert here that there's only one table in the list That's not the case for UPDATE, so assert will be inappropriate here.
3. and please add a test for DELETE FROM view FOR PORTION OF
✅
+ { + if (!table->table) + continue; + vers_select_conds_t &conds= table->period_conditions; + if (0 == strcasecmp(conds.name.str, "SYSTEM_TIME"))
why not to check it in the parser?
Maybe.. but anyway what's the defference? This place is good for error handling too.
@@ -6426,9 +6426,14 @@ void TABLE::mark_columns_needed_for_delete()
if (s->versioned) { - bitmap_set_bit(read_set, s->vers.start_field(s)->field_index); - bitmap_set_bit(read_set, s->vers.end_field(s)->field_index); - bitmap_set_bit(write_set, s->vers.end_field(s)->field_index); + bitmap_set_bit(read_set, s->vers.start_fieldno); + bitmap_set_bit(read_set, s->vers.end_fieldno); + bitmap_set_bit(write_set, s->vers.end_fieldno); + } + + if (with_period) + { + use_all_columns(); }
Nope, I think this is conceptually wrong. TABLE::mark_columns_needed_for_delete marks columns that are needed to *delete* a row. The fact that you might need to *insert* if there's a FOR PORTION OF, is irrelevant here, it's something the caller should bother about.
I thought this function is to collect all the weird logic of columns marking in one convenient place. A tip is that it's used only in DELETE statement code, but not in replication, or anywhere else.
}
@@ -7834,6 +7839,101 @@ int TABLE::update_default_fields(bool update_command, bool ignore_errors) DBUG_RETURN(res); }
+static int table_update_generated_fields(TABLE *table) +{ + int res= 0; + if (table->found_next_number_field) + { + table->next_number_field= table->found_next_number_field; + res= table->found_next_number_field->set_default(); + table->file->update_auto_increment(); + } + + if (table->default_field) + res= table->update_default_fields(0, false);
why is that? auto_increment fields are allowed
+ + if (likely(!res) && table->vfield) + res= table->update_virtual_fields(table->file, VCOL_UPDATE_FOR_WRITE); + if (likely(!res) && table->versioned()) + table->vers_update_fields();
I believe you need to verify CHECK constraints too. and, please, add a test where they fail here. I think there'll be no such test, because generated fields are forbidden to use application-time period fields. Except auto_increment, but, those are not allowed to be used in virtual fields. The SQL16 standard in 15.7 Effect of deleting rows from base tables, General Rules, 8)c)ii) says: The following <insert statement> is effectively executed without further Access Rule and constraint checking: INSERT INTO TN VALUES (VL1, ..., VLd) NOTE 691 — Constraint checking is performed at the end of triggering DELETE statement.
Not sure what does the NOTE 691 mean exactly, but I interpret the statement as this INSERT command doesn't require any constraint check. BTW what's important, this operation does not require INSERT privilege!
+ return res; +} + +static int period_make_insert(TABLE *table, Item *src, Field *dst) +{ + THD *thd= table->in_use; + + store_record(table, record[1]); + int res= src->save_in_field(dst, true); + + if (likely(!res)) + table_update_generated_fields(table);
perhaps, move store_record() and save_in_field() into table_update_generated_fields() ? less duplicated code. But the function will need to be renamed, like prepare_for_period_insert or something
Because of two duplicating lines?:) I actually don't quite like this idea because store_record is symmetrically paired with restore_record here, and this pairing fits on one screen, which is very convenient for reading.
+ + if (likely(!res) && table->triggers) + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_BEFORE, true); + + if (likely(!res)) + res = table->file->ha_write_row(table->record[0]); + + if (likely(!res) && table->triggers) + res= table->triggers->process_triggers(thd, TRG_EVENT_INSERT, TRG_ACTION_AFTER, true); + + restore_record(table, record[1]); + return res; +} + +int TABLE::update_portion_of_time(THD *thd, vers_select_conds_t &period_conds, + bool *inside_period)
is it used for UPDATE too? or only for DELETE?
Looks like for DELETE only. Does the naming look confusing?
+{ + 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); + + *inside_period= !lcond && !rcond; + if (*inside_period) + return 0; + + int res= 0; + Item *src= lcond ? period_conds.start.item : period_conds.end.item; + uint dst_fieldno= lcond ? s->period.end_fieldno : s->period.start_fieldno; + + store_record(this, record[1]); + if (likely(!res)) + res= src->save_in_field(field[dst_fieldno], true); + + if (likely(!res)) + res= table_update_generated_fields(this); +
may be, an assert that there're no delete/insert triggers in a table?
Hmm maybe. I doubt it will prevent any bug hereafter, but added, let's see how it'll go. Sincerely, Nikita Malyavin