maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Jun 28, Aleksey Midenkov wrote:
> >
> > * system_versioning_insert_history
> > * allows pseudocolumns ROW_START and ROW_END be specified in INSERT
> >   doesn't affect invisible columns
> 
> Done. Probably the name "system_versioning_insert_history" may be
> confused with something else. How about
> "system_versioning_direct_history" or
> "system_versioning_direct_insert"? Even
> "system_versioning_modify_history" looks less confusing (and has space
> for future extension).

system_versioning_direct_history looks strange, I wouldn't understand
what it means if I hadn't known it already.

system_versioning_direct_insert is kind of ok, "allow_insert" might've
been clearer.

system_versioning_modify_history looks reasonable too.

But I think we're reaching the point of diminishing return.
There were a bunch of names that were clearly bad, but the difference
between good ones is rather minimal. I don't think I can say "this one
is much better than all others".

So, system_versioning_insert_history, system_versioning_direct_insert,
system_versioning_allow_insert, system_versioning_modify_history - I'd
say they all can do. If you have a strong preference - use the name you
prefer.

> > While
> >
> >   replace
> >   load data ... replace
> >
> > should work, but as DELETE+INSERT (I mean that DELETE in versioned
> > tables doesn't actually delete history).
> 
> Sorry, I don't understand the bracket part (why is it here). Normal
> REPLACE is DELETE+INSERT, versioned REPLACE is UPDATE+INSERT. For
> history replace only "normal REPLACE" is possible, so it is
> DELETE+INSERT obviously.

I meant that DELETE part in REPLACE should update row_end, not actually
delete a versioned row. Now sure why I wrote that, seems kind of
obvious.

> Added REPLACE, but modifying row_end leads to a new row insert. See
> this comment:
> 
> --echo # Changing row_end via REPLACE is NOT possible, we just insert new row:
> # NOTE: because multiple versions of history row with a=1 may exist,
> so what REPLACE should change?

Yes, right. We want mysqldump to be used to save/restore history.
So adding new rows with arbitrary start/end should be allowed.
But *modifying* existing history rows shouldn't.

> > > diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> > > index d23b190b2c5..93f8a8434e4 100644
> > > --- a/sql/sql_class.cc
> > > +++ b/sql/sql_class.cc
> > > +{
> > > +  if (lex->sql_command != SQLCOM_INSERT &&
> > > +      lex->sql_command != SQLCOM_INSERT_SELECT &&
> > > +      lex->sql_command != SQLCOM_LOAD)
> > > +    return false;
> >
> > I don't think this is enough to reject
> >
> >   insert ... on duplicate key update row_start=xxx
> 
> I set thd->lex->sql_command for check_update_fields() so it is
> possible now to distinguish fix_fields() of INSERT from fix_fields()
> of ODKU. I used the value of SQLCOM_UPDATE though it is probably good
> to add something like SQLCOM_DUPKEY_UPDATE. What do you think?
> Probably there would be no need in `bool update` for fill_record() if
> we have different sql_command for that.

Hmm. I need to see it in the context, will comment on the new patch.

> --
> All the best,
> Aleksey Midenkov
> @midenok

Thanks, will do the next review asap!
(but likely after some maintenance bugfixing)

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx