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.