Hi Sergei, On Tue, Feb 15, 2022 at 4:35 PM Sergei Golubchik <s...@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 secur...@mariadb.org -- @midenok _______________________________________________ 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