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