Hi, Aleksey! On Jun 08, Aleksey Midenkov wrote: > revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) > parent(s): 27d66d644cf > author: Aleksey Midenkov <mide...@gmail.com> > committer: Aleksey Midenkov <mide...@gmail.com> > 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 @@ x y > 9 9001 > 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; > +Table Create 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 00000000000..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 00000000000..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() > { > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2913a4a8350..520ea1d63bf 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -820,6 +820,7 @@ typedef struct system_variables > > vers_asof_timestamp_t vers_asof_timestamp; > ulong vers_alter_history; > + my_bool force_fields_visible; system_versioning_insert_history > } SV; > > /** > @@ -5400,6 +5401,7 @@ class THD: public THD_count, /* this must be first */ > Item *sp_prepare_func_item(Item **it_addr, uint cols= 1); > bool sp_eval_expr(Field *result_field, Item **expr_item_ptr); > > + bool vers_modify_sys_field() const; vers_modify_history > }; > > > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc > index f0b84ab126f..84c2ae4ab36 100644 > --- a/sql/sql_insert.cc > +++ b/sql/sql_insert.cc > @@ -2014,7 +2014,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO > *info, select_result *sink) > !table->file->referenced_by_foreign_key() && > (!table->triggers || !table->triggers->has_delete_triggers())) > { > - if (table->versioned(VERS_TRX_ID)) > + if (table->versioned(VERS_TRX_ID) && table->vers_write) does it even make sense? I mean direct insertion of transaction ids. > { > bitmap_set_bit(table->write_set, > table->vers_start_field()->field_index); > table->file->column_bitmaps_signal(); > @@ -2029,7 +2029,7 @@ int write_record(THD *thd, TABLE *table, COPY_INFO > *info, select_result *sink) > info->deleted++; > if (!table->file->has_transactions()) > thd->transaction->stmt.modified_non_trans_table= TRUE; > - if (table->versioned(VERS_TIMESTAMP)) > + if (table->versioned(VERS_TIMESTAMP) && table->vers_write) This (and the previous hunk) are in the DUP_REPLACE branch. There should be tests for these changes. > { > store_record(table, record[2]); > error= vers_insert_history_row(table); > diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc > index f817f9086a9..4429308a097 100644 > --- a/sql/sys_vars.cc > +++ b/sql/sys_vars.cc > @@ -6737,3 +6737,7 @@ static Sys_var_ulonglong Sys_max_rowid_filter_size( > SESSION_VAR(max_rowid_filter_size), CMD_LINE(REQUIRED_ARG), > VALID_RANGE(1024, (ulonglong)~(intptr)0), DEFAULT(128*1024), > BLOCK_SIZE(1)); > + > +static Sys_var_mybool Sys_force_fields_visible( > + "force_fields_visible", "Make invisible fields visible on next table > open", "system_versioning_insert_history" "Allows direct inserts into ROW_START and ROW_END columns" > + SESSION_VAR(force_fields_visible), CMD_LINE(OPT_ARG), DEFAULT(FALSE)); > diff --git a/sql/table.cc b/sql/table.cc > index f4bdbdeac5a..520f3738c9e 100644 > --- a/sql/table.cc > +++ b/sql/table.cc > @@ -4024,6 +4024,9 @@ enum open_frm_error open_table_from_share(THD *thd, > TABLE_SHARE *share, > { > if (!((*field_ptr)= share->field[i]->clone(&outparam->mem_root, > outparam))) > goto err; > + if (thd->variables.force_fields_visible && > + (*field_ptr)->invisible <= INVISIBLE_SYSTEM) > + (*field_ptr)->invisible= VISIBLE; 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 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