maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Jan 25, Nikita Malyavin wrote:
> > > > > +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
 
> > > > > +  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.

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx