maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

Hi, Sergei!

26 Jan 2019  06:27, Sergei Golubchik <serg@xxxxxxxxxxx>:

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