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 <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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 security@mariadb.org
Hi Sergei! On Tue, Jun 8, 2021 at 3:04 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jun 08, Aleksey Midenkov wrote:
revision-id: b3844107287 (mariadb-10.5.2-424-gb3844107287) parent(s): 27d66d644cf author: Aleksey Midenkov <midenok@gmail.com> committer: Aleksey Midenkov <midenok@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
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! 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
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 Sergei, On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik <serg@mariadb.org> 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_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
Fixed.
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.
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(<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").
+ 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)?
It turns out this compiles as inline. Fixed.
+ { + 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
-- @midenok
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
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik