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