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