Hi, Sergei!

26 Jan 2019  06:27, Sergei Golubchik <serg@mariadb.org>:
> > > > > +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?

Yes, I'd think that

  You have an error in your SQL syntax; ... use near 'SYSTEM_TIME' at line 1

would be quite reasonable
Ok, did it. Please see the last update.

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

Well, yes, and because update_portion_of_time() is a delete-specific
functionality, it should be logically with the rest of the delete code
and not polluting global namespace and shared objects.

And if period_make_insert() is used both for UPDATE and DELETE, then
it'd be quite logical to make it extern and use both from UPDATE and
from DELETE.
 
Well, still quite arguable for me, but ok. I made it in the same manner TABLE::delete_row written -- just moved the method in sql_delete.cc and marked it inline. Is this solution OK for You?

With regards,
Nikta Malyavin