Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey, On Aug 03, Aleksey Midenkov wrote: > > > diff --git a/sql/log_event.h b/sql/log_event.h > > > index 3adc7a26d93..dc269955c5f 100644 > > > --- a/sql/log_event.h > > > +++ b/sql/log_event.h > > > @@ -535,16 +535,12 @@ class String; > > > */ > > > #define OPTIONS_WRITTEN_TO_BIN_LOG \ > > >(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \ > > > - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | > > > OPTION_IF_EXISTS) > > > + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | > > > OPTION_IF_EXISTS | \ > > > + OPTION_INSERT_HISTORY) > > > > > > -/* Shouldn't be defined before */ > > > -#define EXPECTED_OPTIONS \ > > > - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL << > > > 28)) > > > - > > > -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS > > > -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values! > > > +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32) > > > +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits! > > > > Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS > > is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc. > > Reverted. 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. > > > diff --git a/sql/handler.cc b/sql/handler.cc > > > index 393f6234653..4bbb40abb5b 100644 > > > --- a/sql/handler.cc > > > +++ b/sql/handler.cc > > > @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf) > > >DBUG_RETURN(error); > > >} > > > > > > + if (table->versioned() && !table->vers_write) > > > + { > > > +Field *row_start= table->vers_start_field(); > > > +Field *row_end= table->vers_end_field(); > > > +MYSQL_TIME ltime; > > > + > > > +bitmap_set_bit(table->read_set, row_start->field_index); > > > +bitmap_set_bit(table->read_set, row_end->field_index); > > > + > > > +/* > > > + Inserting the history row directly, check ROW_START <= ROW_END and > > > + ROW_START is non-zero. > > > +*/ > > > +if (!row_end->is_max() && ( > > > + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) || > > > + row_start->get_date(> > +TIME_NO_ZERO_DATE, > > > time_round_mode_t(time_round_mode_t::FRAC_NONE) > > > > 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. > > 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"). 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. > > > + DBUG_RETURN(HA_ERR_WRONG_ROW_TIMESTAMP); > > > + } > > > + > > >MYSQL_INSERT_ROW_START(table_share->db.str, > > > table_share->table_name.str); > > >mark_trx_read_write(); > > >increment_statistics(&SSV::ha_write_count); 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 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
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi Sergei, On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik wrote: > > 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. > > > diff --git a/include/my_base.h b/include/my_base.h > > index 053bf3fbb69..f1c0d8dbc99 100644 > > --- a/include/my_base.h > > +++ b/include/my_base.h > > @@ -527,7 +527,8 @@ enum ha_base_keytype { > > #define HA_ERR_TABLESPACE_MISSING 194 /* Missing Tablespace */ > > #define HA_ERR_SEQUENCE_INVALID_DATA 195 > > #define HA_ERR_SEQUENCE_RUN_OUT 196 > > -#define HA_ERR_LAST 196 /* Copy of last error nr * */ > > +#define HA_ERR_WRONG_ROW_TIMESTAMP 197 /* System Versioning row_end > > <= row_start */ > > +#define HA_ERR_LAST 197 /* Copy of last error nr * */ > > 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. Replaced with ER_WRONG_VALUE. > > > > > /* Number of different errors */ > > #define HA_ERR_ERRORS(HA_ERR_LAST - HA_ERR_FIRST + 1) > > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > > b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > > index b28f3c567ff..6c09e0a7f23 100644 > > --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > > @@ -982,6 +982,16 @@ NUMERIC_BLOCK_SIZE 1 > > ENUM_VALUE_LIST NULL > > READ_ONLYNO > > COMMAND_LINE_ARGUMENTREQUIRED > > +VARIABLE_NAMEFORCE_FIELDS_VISIBLE > > +VARIABLE_SCOPE SESSION > > +VARIABLE_TYPEBOOLEAN > > +VARIABLE_COMMENT Make invisible fields visible on next table open > > +NUMERIC_MIN_VALUENULL > > +NUMERIC_MAX_VALUENULL > > +NUMERIC_BLOCK_SIZE NULL > > +ENUM_VALUE_LIST OFF,ON > > +READ_ONLYNO > > +COMMAND_LINE_ARGUMENTOPTIONAL > > apparently, forgot to update embedded result after renaming the variable Fixed. > > > VARIABLE_NAMEFOREIGN_KEY_CHECKS > > VARIABLE_SCOPE SESSION > > VARIABLE_TYPEBOOLEAN > > diff --git a/sql/log_event.h b/sql/log_event.h > > index 3adc7a26d93..dc269955c5f 100644 > > --- a/sql/log_event.h > > +++ b/sql/log_event.h > > @@ -535,16 +535,12 @@ class String; > > */ > > #define OPTIONS_WRITTEN_TO_BIN_LOG \ > >(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \ > > - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS) > > + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS > > | \ > > + OPTION_INSERT_HISTORY) > > > > -/* Shouldn't be defined before */ > > -#define EXPECTED_OPTIONS \ > > - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL << > > 28)) > > - > > -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS > > -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values! > > +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32) > > +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits! > > Well, no. This check makes sure that OPTION_RELAXED_UNIQUE_CHECKS > is 1<<27, that OPTION_AUTO_IS_NULL is 1<<14, etc. Reverted. > > 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. 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. > > You can refactor it, if you don't like it. > But please keep it in in some form. > > > #endif > > -#undef EXPECTED_OPTIONS /* You shouldn't use this one */ > > > > #define CHECKSUM_CRC32_SIGNATURE_LEN 4 > > /** > > diff --git a/sql/handler.cc b/sql/handler.cc > > index 393f6234653..4bbb40abb5b 100644 > > --- a/sql/handler.cc > > +++ b/sql/handler.cc > > @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf) > >DBUG_RETURN(error); > >} > > > > + if (table->versioned() && !table->vers_write) > > + { > > +Field *row_start= table->vers_start_field(); > > +Field *row_end= table->vers_end_field(); > > +MYSQL_TIME ltime; > > + > > +bitmap_set_bit(table->read_set, row_start->field_index); > > +bitmap_set_bit(table->read_set, row_end->field_index); > > + > > +/* > > + Inserting the history row directly, check ROW_START <= ROW_END and > > + ROW_START is non-zero. > > +*/ > > +if (!row_end->is_max() && ( > > + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) || > > + row_start->get_date(> +TIME_NO_ZERO_DATE, > > time_round_mode_t(time_round_mode_t::FRAC_NONE) > > I don't quite understa
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
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. > diff --git a/include/my_base.h b/include/my_base.h > index 053bf3fbb69..f1c0d8dbc99 100644 > --- a/include/my_base.h > +++ b/include/my_base.h > @@ -527,7 +527,8 @@ enum ha_base_keytype { > #define HA_ERR_TABLESPACE_MISSING 194 /* Missing Tablespace */ > #define HA_ERR_SEQUENCE_INVALID_DATA 195 > #define HA_ERR_SEQUENCE_RUN_OUT 196 > -#define HA_ERR_LAST 196 /* Copy of last error nr * */ > +#define HA_ERR_WRONG_ROW_TIMESTAMP 197 /* System Versioning row_end <= > row_start */ > +#define HA_ERR_LAST 197 /* Copy of last error nr * */ 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. > > /* Number of different errors */ > #define HA_ERR_ERRORS(HA_ERR_LAST - HA_ERR_FIRST + 1) > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > index b28f3c567ff..6c09e0a7f23 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_embedded.result > @@ -982,6 +982,16 @@ NUMERIC_BLOCK_SIZE 1 > ENUM_VALUE_LIST NULL > READ_ONLYNO > COMMAND_LINE_ARGUMENTREQUIRED > +VARIABLE_NAMEFORCE_FIELDS_VISIBLE > +VARIABLE_SCOPE SESSION > +VARIABLE_TYPEBOOLEAN > +VARIABLE_COMMENT Make invisible fields visible on next table open > +NUMERIC_MIN_VALUENULL > +NUMERIC_MAX_VALUENULL > +NUMERIC_BLOCK_SIZE NULL > +ENUM_VALUE_LIST OFF,ON > +READ_ONLYNO > +COMMAND_LINE_ARGUMENTOPTIONAL apparently, forgot to update embedded result after renaming the variable > VARIABLE_NAMEFOREIGN_KEY_CHECKS > VARIABLE_SCOPE SESSION > VARIABLE_TYPEBOOLEAN > diff --git a/sql/log_event.h b/sql/log_event.h > index 3adc7a26d93..dc269955c5f 100644 > --- a/sql/log_event.h > +++ b/sql/log_event.h > @@ -535,16 +535,12 @@ class String; > */ > #define OPTIONS_WRITTEN_TO_BIN_LOG \ >(OPTION_AUTO_IS_NULL | OPTION_NO_FOREIGN_KEY_CHECKS | \ > - OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS) > + OPTION_RELAXED_UNIQUE_CHECKS | OPTION_NOT_AUTOCOMMIT | OPTION_IF_EXISTS | > \ > + OPTION_INSERT_HISTORY) > > -/* Shouldn't be defined before */ > -#define EXPECTED_OPTIONS \ > - ((1ULL << 14) | (1ULL << 26) | (1ULL << 27) | (1ULL << 19) | (1ULL << 28)) > - > -#if OPTIONS_WRITTEN_TO_BIN_LOG != EXPECTED_OPTIONS > -#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT change their values! > +#if OPTIONS_WRITTEN_TO_BIN_LOG >= (1ULL << 32) > +#error OPTIONS_WRITTEN_TO_BIN_LOG must NOT exceed 32 bits! 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. > #endif > -#undef EXPECTED_OPTIONS /* You shouldn't use this one */ > > #define CHECKSUM_CRC32_SIGNATURE_LEN 4 > /** > diff --git a/sql/handler.cc b/sql/handler.cc > index 393f6234653..4bbb40abb5b 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -7493,6 +7496,26 @@ int handler::ha_write_row(const uchar *buf) >DBUG_RETURN(error); >} > > + if (table->versioned() && !table->vers_write) > + { > +Field *row_start= table->vers_start_field(); > +Field *row_end= table->vers_end_field(); > +MYSQL_TIME ltime; > + > +bitmap_set_bit(table->read_set, row_start->field_index); > +bitmap_set_bit(table->read_set, row_end->field_index); > + > +/* > + Inserting the history row directly, check ROW_START <= ROW_END and > + ROW_START is non-zero. > +*/ > +if (!row_end->is_max() && ( > + (row_start->cmp(row_start->ptr, row_end->ptr) >= 0) || > + row_start->get_date(+TIME_NO_ZERO_DATE, > time_round_mode_t(time_round_mode_t::FRAC_NONE) 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. > + DBUG_RETURN(HA_ERR_WRONG_ROW_TIMESTAMP); > + } > + >MYSQL_INSERT_ROW_
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
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
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi Sergei! On Tue, Jun 8, 2021 at 3:04 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Jun 08, Aleksey Midenkov wrote: > > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > > parent(s): 27d66d644cf > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2021-03-01 17:43:34 +0300 > > message: > > > > MDEV-16546 System versioning setting to allow history modification > > > > 1. force_fields_visible session variable makes system-invisible and > > user-invisible fields visible on next table open. FLUSH TABLES > > required for already opened tables. > > * 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). > * 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). > > > 2. If secure_timestamp allows to modify timestamp variable then > > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > > specify row_start and row_end system field values. > > when system_versioning_insert_history=1 Edited. > > > 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? That was the commit 00a254cc for MDEV-20186 which sets vers_write in TABLE::init(). > > > diff --git a/mysql-test/suite/versioning/r/insert.result > > b/mysql-test/suite/versioning/r/insert.result > > index 2645d0184e8..00d1bb705c4 100644 > > --- a/mysql-test/suite/versioning/r/insert.result > > +++ b/mysql-test/suite/versioning/r/insert.result > > @@ -112,3 +112,95 @@ xy > > 99001 > > drop table t1; > > drop table t2; > > +# > > +# MDEV-16546 System versioning setting to allow history modification > > +# > > +# restart: --secure-timestamp=NO > > +set @@session.time_zone='+00:00'; > > +create table t1(x int) with system versioning; > > +insert into t1(x) values (1); > > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', > > '1980-01-01 00:00:01'); > > +ERROR 42S22: Unknown column 'row_start' in 'field list' > > +set session force_fields_visible= 1; > > +flush tables; > > +show create table t1; > > +TableCreate Table > > +t1 CREATE TABLE `t1` ( > > + `x` int(11) DEFAULT NULL, > > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, > > shouldn't be visible here, only in insert and load. I removed force_fields_visible and hence this effect. > > > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', > > '1980-01-01 00:00:01'); > > please, add tests for > >insert into t1 values (4) > > this should work, row_start/row_end must be mentioned explicitly. Added. > Also > >insert into t1 set x=5, row_start=..., row_end=... > > this should work too. Added. > 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. Ok. > 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. 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? > If that's too difficult, then > an error too. > > > +update t1 set x= x + 1; > > +select x, check_row_ts(row_start, row_end) from t1 for system_time all > > order by x; > ... > > diff --git a/mysql-test/suite/versioning/t/insert.opt > > b/mysql-test/suite/versioning/t/insert.opt > > new file mode 100644 > > index 000..a74d68957ef > > --- /dev/null > > +++ b/mysql-test/suite/versioning/t/insert.opt > > @@ -0,
Re: [Maria-developers] b3844107287: MDEV-16546 System versioning setting to allow history modification
Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-03-01 17:43:34 +0300 > message: > > MDEV-16546 System versioning setting to allow history modification > > 1. force_fields_visible session variable makes system-invisible and > user-invisible fields visible on next table open. FLUSH TABLES > required for already opened tables. * 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 > 2. If secure_timestamp allows to modify timestamp variable then > following DML commands: INSERT, INSERT..SELECT and LOAD DATA can > specify row_start and row_end system field values. 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? > diff --git a/mysql-test/suite/versioning/r/insert.result > b/mysql-test/suite/versioning/r/insert.result > index 2645d0184e8..00d1bb705c4 100644 > --- a/mysql-test/suite/versioning/r/insert.result > +++ b/mysql-test/suite/versioning/r/insert.result > @@ -112,3 +112,95 @@ xy > 99001 > drop table t1; > drop table t2; > +# > +# MDEV-16546 System versioning setting to allow history modification > +# > +# restart: --secure-timestamp=NO > +set @@session.time_zone='+00:00'; > +create table t1(x int) with system versioning; > +insert into t1(x) values (1); > +insert into t1(x, row_start, row_end) values (2, '1980-01-01 00:00:00', > '1980-01-01 00:00:01'); > +ERROR 42S22: Unknown column 'row_start' in 'field list' > +set session force_fields_visible= 1; > +flush tables; > +show create table t1; > +TableCreate Table > +t1 CREATE TABLE `t1` ( > + `x` int(11) DEFAULT NULL, > + `row_start` timestamp(6) GENERATED ALWAYS AS ROW START, > + `row_end` timestamp(6) GENERATED ALWAYS AS ROW END, shouldn't be visible here, only in insert and load. > + PERIOD FOR SYSTEM_TIME (`row_start`, `row_end`) > +) ENGINE=DEFAULT_ENGINE DEFAULT CHARSET=latin1 WITH SYSTEM VERSIONING > +insert into t1(x, row_start, row_end) values (3, '1980-01-01 00:00:00', > '1980-01-01 00:00:01'); 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. > +update t1 set x= x + 1; > +select x, check_row_ts(row_start, row_end) from t1 for system_time all order > by x; ... > diff --git a/mysql-test/suite/versioning/t/insert.opt > b/mysql-test/suite/versioning/t/insert.opt > new file mode 100644 > index 000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes why is this? > -- source suite/versioning/common_finish.inc > diff --git a/mysql-test/suite/versioning/t/insert2.opt > b/mysql-test/suite/versioning/t/insert2.opt > new file mode 100644 > index 000..a74d68957ef > --- /dev/null > +++ b/mysql-test/suite/versioning/t/insert2.opt > @@ -0,0 +1 @@ > +--secure-timestamp=yes and this? > 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 > @@ -7604,6 +7604,20 @@ void THD::set_last_commit_gtid(rpl_gtid >id) > #endif > } > > +bool THD::vers_modify_sys_field() const 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). > +{ > + 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 > + if (opt_secure_timestamp >= SECTIME_REPL || this is fixed in a later commit, so ok > + (opt_secure_timestamp == SECTIME_SUPER && > + !(security_ctx->master_access & SUPER_ACL))) > +return false; > + return true; > +} > + > + > void > wait_for_commit::reinit() > {