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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp