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/748ef3ec91d35aa6cd5b230c71084f6c83c1c92e 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 _______________________________________________ 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