maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hi, Sergei!

чт, 24 янв. 2019 г. в 21:31, Sergei Golubchik <serg@xxxxxxxxxxx>:

> 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.
>

Ugh I didn't realize that, thanks! Now things are clear.


> 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 :)
>
>  What about "for portion of `SYSTEM_TIME` ..."? Is it correct to throw
ER_PARSE_ERROR in this case?

> > > 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? :)
>
added a test to versioning.select ✅


>
> > > > +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?
>
> Just because the check is written in setup_conds, which is called after
period_setup_conds. Well, seems that we'll move the check to parser in
scope of UPDATE discussion, I'll add the assertion now, but that'll break
update tests temporarily.
And it'll be removed in SELECT...AS OF


> > > > @@ -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.
>
> Okay, removed ✅

> > > >  }
> > > >
> > > > @@ -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.
>
>  Oh, I misunderstood the question.
Seems, we don't need it really, and it changes nothing.
Removed✅

> > > +
> > > > +  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.
>
> Good corner case, thanks! It was indeed failing, but because I missed some
error handling.
I've fixed it, and added the test case, please see the update.
Thing is it wasn't CHECK constraint -- it was autoinc check inside
handler::update_auto_increment.
I'm still convinced that CHECK constraints are impossible to fail here.

> > > +  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 :)
>
> Okay!

> > > > +
> > > > +  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*);
>
> Well, technically moving it to sql_delete will
make table_update_generated_fields and period_make_insert extern.
I also like current code arrangement by the following reason: these
functions make similar operations, so better to store them in one place.
I'd actually prefer to have a separate class/module for them, with
TABLE::delete_row and maybe some others, but was afraid It'll be not in
style the rest code is written -- seems, mysql/mariadb developers prefer
not to spawn a lot of files, and to group logical API blocks by comments😐

> > > > +{
> > > > +  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.
>
> Sure

>   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.
>
>  Yes, looks reasonable in such point of view, now i'm convinced. Anyway it
should have been added by previous update

Sincerely,
Nikita Malyavin