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@mariadb.org