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

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 00000000000..a74d68957ef --- /dev/null +++ b/mysql-test/suite/versioning/t/insert.opt @@ -0,0 +1 @@ +--secure-timestamp=yes

why is this?

Looks like a needless artifact. Removed.

...

-- 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?

Looks like a needless artifact. Removed.

...

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).

Renamed.

...

+{ + 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. 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.

...

+ 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

Renamed to vers_insert_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.

Probably not. This will be deprecated by MDEV-16226. I disabled direct insertion for VERS_TRX_ID.

...

{ 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.

There are tests for REPLACE now.

...

{ 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"

Updated.

...

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

-- All the best, Aleksey Midenkov @midenok

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_ONLY NO COMMAND_LINE_ARGUMENT REQUIRED +VARIABLE_NAME FORCE_FIELDS_VISIBLE +VARIABLE_SCOPE SESSION +VARIABLE_TYPE BOOLEAN +VARIABLE_COMMENT Make invisible fields visible on next table open +NUMERIC_MIN_VALUE NULL +NUMERIC_MAX_VALUE NULL +NUMERIC_BLOCK_SIZE NULL +ENUM_VALUE_LIST OFF,ON +READ_ONLY NO +COMMAND_LINE_ARGUMENT OPTIONAL

apparently, forgot to update embedded result after renaming the variable

VARIABLE_NAME FOREIGN_KEY_CHECKS VARIABLE_SCOPE SESSION VARIABLE_TYPE BOOLEAN 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(<ime, Datetime::Options( + 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_START(table_share->db.str, table_share->table_name.str); mark_trx_read_write(); increment_statistics(&SSV::ha_write_count); diff --git a/sql/item.cc b/sql/item.cc index a016f04953c..cf259ebd53c 100644 --- a/sql/item.cc +++ b/sql/item.cc @@ -5986,6 +5986,13 @@ bool Item_field::fix_fields(THD *thd, Item **reference) else if (!from_field) goto error;

+ if (thd->column_usage == MARK_COLUMNS_WRITE && + from_field != view_ref_found && + thd->vers_insert_history(from_field))

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)?

+ { + DBUG_ASSERT(from_field->table->versioned()); + from_field->table->vers_write= false; + } table_list= (cached_table ? cached_table : from_field != view_ref_found ? from_field->table->pos_in_table_list : 0); diff --git a/sql/sql_base.cc b/sql/sql_base.cc index 51e4bcad16b..bf65cd4a279 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -6039,7 +6039,8 @@ find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length,

if (field->invisible == INVISIBLE_SYSTEM && thd->column_usage != MARK_COLUMNS_READ && - thd->column_usage != COLUMNS_READ) + thd->column_usage != COLUMNS_READ && + !thd->vers_insert_history(field))

it's okay to invoke THD::vers_insert_history here, as INVISIBLE_SYSTEM fields are rare

DBUG_RETURN((Field*)0); } else @@ -8534,7 +8535,8 @@ fill_record(THD *thd, TABLE *table_arg, List<Item> &fields, List<Item> &values, if (table->next_number_field && rfield->field_index == table->next_number_field->field_index) table->auto_increment_field_not_null= TRUE; - const bool skip_sys_field= rfield->vers_sys_field(); // TODO: && !thd->vers_modify_history() [MDEV-16546] + const bool skip_sys_field= rfield->vers_sys_field() && + (update || !thd->vers_insert_history(rfield));

same. it's behind rfield->vers_sys_field() check, so ok.

if ((rfield->vcol_info || skip_sys_field) && !value->vcol_assignment_allowed_value() && table->s->table_category != TABLE_CATEGORY_TEMPORARY)

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org

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(<ime, Datetime::Options( + 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 security@mariadb.org