Hi, Nikita! On Jan 24, Nikita Malyavin wrote: > > > +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.
No, it's how your grammar works. You wrote | HISTORY_SYM delete_single_table opt_delete_system_time { MYSQL_YYABORT_UNLESS(!Lex->last_table()->has_period()); so, the parser consumes HISTORY, table name, FOR PORTION OF, etc. and *only then* you force a ER_PARSE_ERROR. At that moment the current parsing position is at the end of the statement. The correct fix is not to use delete_single_table (that allows FOR PORTION OF) in the DELETE HISTORY rule (where FOR PORTION OF is not allowed). > > > +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. it depends on how you write the grammar :) > > > 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; > > > + conds= table_list->on_expr; > > > + table_list->on_expr= NULL; > > > + portion_of_time_through_update= !has_period_triggers > > > + && !table->versioned(VERS_TIMESTAMP); > > > - 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. I see. Please add a comment here that one cannot mix write_row, update_row, and delete_row while scanning. > > > + || unique_table(thd, table_list, table_list->next_global, 0)) > > > *delete_while_scanning= false; > > > > > > > + 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 add a new test, perhaps? :) > > > +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 Yes, I know. > I've just extracted those if's to a separate function. Have no better idea. Thanks, that'll do. > > > + > > > + 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. Can there be many tables with UPDATE? How? > > > @@ -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. Well, it's "mark_columns_needed_for_delete" and the comment says "Mark columns needed for doing an delete of a row". It only check engine capabilities - some engines need primary key columns to be always selected, if they need to delete a row, for example. The fact that _upper layer_ might want to insert or update is not what this function should be bothered with. > > > } > > > > > > @@ -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 I don't understand, sorry. TABLE::update_default_fields does not update auto_increment fields, as far as I can see. > > > + > > > + 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! Constraint checking is performed. The point is, when the statement has finished, all constraints must be satisfied. A simple test could be CREATE TABLE t1 (id AUTO_INCREMENT, ..., CHECK (id < 10)) and by using DELETE ... FOR PORTION OF you can generate new id's until the constraint fails. Also, please try CREATE TABLE t1 (id TINYINT AUTO_INCREMENT, ...); INSERT t1 VALUES (254, ...) here id will quickly run out of values, and DELETE FOR PORTION OF will fail too. > > > + 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. "perhaps" was a stylistic suggestion. Sure, it's your code, so up to you :) > > > + > > > + 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? No, I wanted to ask to move it from a TABLE method to a static function in sql_delete.cc: static int update_portion_of_time(THD*, TABLE*, vers_select_conds_t&, bool*); > > > +{ > > > + 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. One of the benefits of asserts that I like, they document the code. assert(table->triggers == NULL); is a self-enforcing version of /* here the table must have no triggers */ and unlike the comment it'll never be out of date. Regards, Sergei Chief Architect MariaDB and secur...@mariadb.org _______________________________________________ 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