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/748ef3ec91d35aa6cd5b230c71084f6c83c...
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 security@mariadb.org