Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote:
* system_versioning_insert_history * allows pseudocolumns ROW_START and ROW_END be specified in INSERT * doesn't affect invisible columns * FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better
when system_versioning_insert_history=1
3. Cleaned up select_insert::send_data() from setting vers_write as this parameter is now set on TABLE initialization.
I don't see where it's set. This commit removes two table->vers_write= table->versioned(); and adds one from_field->table->vers_write= false; in Item_field::fix_fields. But where is vers_write set to true? Was it done in some other commit?
shouldn't be visible here, only in insert and load.
please, add tests for insert into t1 values (4) this should work, row_start/row_end must be mentioned explicitly. Also insert into t1 set x=5, row_start=..., row_end=... this should work too. And when only one row_start or row_end is mentioned as well. But update t1 set row_start=... insert into t1 (x, row_start, row_end) values (...) on duplicate key update row_start=... this should fail. While replace load data ... replace should work, but as DELETE+INSERT (I mean that DELETE in versioned tables doesn't actually delete history). If that's too difficult, then an error too.
why is this?
and this?
don't call it vers_modify_sys_field, your previous name vers_modify_history was better. There may be other sys fields that will never be directly writable (like ROWID, for example).
I don't think this is enough to reject insert ... on duplicate key update row_start=xxx
+ if (opt_secure_timestamp >= SECTIME_REPL ||
this is fixed in a later commit, so ok
system_versioning_insert_history
vers_modify_history
does it even make sense? I mean direct insertion of transaction ids.
This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes.
"system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns"
Not really, system_versioning_insert_history should not affect visibility of anything besides ROW_START and ROW_END pseudocolumns in the INSERT and LOAD DATA statements. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei! On Tue, Jun 8, 2021 at 3:04 PM Sergei Golubchik <serg@mariadb.org> wrote:
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).
* FLUSH TABLES requirememnt is rather confusing, but let's have it, at least until I could suggest something better
That was the requirement for force_fields_visible. For system_versioning_insert_history there is no such requirement (as we do not modify anything in TABLE_SHARE).
Edited.
That was the commit 00a254cc for MDEV-20186 which sets vers_write in TABLE::init().
I removed force_fields_visible and hence this effect.
Added.
Added.
Ok.
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. 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?
Looks like a needless artifact. Removed.
Looks like a needless artifact. Removed.
Renamed.
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. While I did that I noticed different error codes for implicit and explicit row_start/row_end which I don't like: --error ER_BAD_FIELD_ERROR insert t1 (x) values (2) on duplicate key update x= 3, row_end= '1970-01-01 00:00:00'; --error ER_WARNING_NON_DEFAULT_VALUE_FOR_GENERATED_COLUMN insert t2 (y) values (1) on duplicate key update y= 3, row_end= '1970-01-01 00:00:00'; That is the result of the pseudocolumn concept.
Renamed to vers_insert_history()
Probably not. This will be deprecated by MDEV-16226. I disabled direct insertion for VERS_TRX_ID.
There are tests for REPLACE now.
Updated.
-- All the best, Aleksey Midenkov @midenok
Hi, Aleksey! On Jun 28, Aleksey Midenkov wrote:
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.
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.
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.
Hmm. I need to see it in the context, will comment on the new patch.
Thanks, will do the next review asap! (but likely after some maintenance bugfixing) Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Aleksey, I've took a look at the whole diff bad1440325ba 9af2883e2692. (but without MDEV-16029) I have a couple of comments about minor issues, and a suggestion, please, see below. Nothing big, really.
As you like. But I'd _suggest_ not to introduce a new HA_ERR_ code for this. You only use it in sql/, it's not something that a storage engine will use, as far as I understand. So there is no need to pollute the API, I would say.
apparently, forgot to update embedded result after renaming the variable
Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc. For example, you've changed OPTION_SETUP_TABLES_DONE from bit 30 to 44. The EXPECTED_OPTIONS check was preventing somebody inadvertenly moving one of OPTIONS_WRITTEN_TO_BIN_LOG flags to a different bit. You can refactor it, if you don't like it. But please keep it in in some form.
I don't quite understand this condition. checking for row_end->is_max() is redundant, because row_start->cmp() on itself is sufficient. So, I suppose you've done it as an optimization? to avoid expensive cmp()? But in that case you also allow zero date in the row_start, and this looks like a bug. I'd suggest to simplify the code and to remove is_max check. Unless you've run benchmarks and it really helps. And unless I've misunderstood its purpose.
Could you try to avoid invoking THD::vers_insert_history() (which isn't inlined) for every fix_fields on every field that's going to be modified (so every INSERT and every UPDATE)?
it's okay to invoke THD::vers_insert_history here, as INVISIBLE_SYSTEM fields are rare
same. it's behind rfield->vers_sys_field() check, so ok.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei, On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik <serg@mariadb.org> wrote:
Replaced with ER_WRONG_VALUE.
Fixed.
Reverted.
And that helps to keep consistency between different versions of master and slave, doesn't it? The above doesn't protect from the swapped values between two flags.
No, you're right. We can insert not only history but current data and row_start there must be checked too. Also, please note this comment: # NOTE: having row_start=0 might be useful and can mean # "there is no information on when the history was started" (an opposite to row_end=MAX_TIMESTAMP) Maybe we should allow it? Just to make the user not invent some values for this purpose (like "1970-01-01 00:00:00").
It turns out this compiles as inline. Fixed.
-- @midenok
Hi, Aleksey, On Aug 03, Aleksey Midenkov wrote:
I'm sorry, but that my comment above is now obsolete. For explicit_defaults_for_timestamp feature I've removed this whole EXPECTED_OPTIONS define. Its purpose was to ensure that the set of "expected options" never changes, it's hard-coded for replication to work. Obviously, assuming it'll never change was a bit... optimistic. Monty already changed it by adding OPTION_IF_EXISTS, you changed it for OPTION_INSERT_HISTORY, and so did I for explicit_defaults_for_timestamp. So now there is no EXPECTED_OPTIONS define, the set of OPTIONS_WRITTEN_TO_BIN_LOG can grow. You can add new flags to it, but you need to record the version when you did it in the method Format_description_log_event::deduct_options_written_to_bin_log(). Like if (server_version_split < Version(10,11,0)) return; options_written_to_bin_log|= OPTION_INSERT_HISTORY; See commit 7b500f04fb0b. Rebase on top of the latest 10.6 to get 7b500f04fb0b and the deduct_options_written_to_bin_log() method.
This would violate the concept that direct inserts are *only* a usability shortcut and does not allow to do anything new. One cannot have row_start=0 with set timestampt=xxx; insert ... values ...(); any other value of row_start can be faked, but not 0.
Also I've looked at your "Review 2" commits in the bb-10.6-midenok branch, no new comments. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik