Hi, Nikita, On Oct 18, Nikita Malyavin wrote:
On Tue, 18 Oct 2022 at 01:01, Sergei Golubchik <serg@mariadb.org> wrote:
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 422d496d5c3..25705d13641 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -6062,7 +6065,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) */ sql_mode_t saved_sql_mode= thd->variables.sql_mode; if (!is_auto_inc_in_extra_columns()) - thd->variables.sql_mode= (rpl_data.is_online_alter() ? saved_sql_mode :0) + thd->variables.sql_mode= (m_online_alter ? saved_sql_mode :0) | MODE_NO_AUTO_VALUE_ON_ZERO;
No, I didn't mean to ask "why MODE_NO_AUTO_VALUE_ON_ZERO", I meant to ask why not
thd->variables.sql_mode= saved_sql_mode | MODE_NO_AUTO_VALUE_ON_ZERO;
that is why not "to use saved_sql_mode also in normal replication"
Oh, sorry for misinterpretation. I have read what I wanted to read:)
Meanwhile I had a time to think. Imo it should be so in replication. It is a background process, right? So we have no control over sql_mode in the replica nodes (if I understand the design correctly).
So we should make it as permissive as possible. I have no opinion on NO_ZERO_DATE and other "View" (in terms of Model-View-Controller) modes, like EMPTY_STRING_IS_NULL, if they don't affect storage, they are unimportant. For the rest, we should allow as much execution, and as much conversions as possible, which is, as I recall, is achieved by zeroing the mode, and MODE_NO_AUTO_VALUE_ON_ZERO is an exception that was found once.
Yes, agree. You're right.
eh? 1. why it's *in the loop* ? 2. why not simply
|| f == f->table->next_number_field
or, to be extra safe
|| f->table->field[f->field_index] == f->table->next_number_field
Thanks! Turns out there's no need in extra safety, it just works.
by "extra safety" I meant the case when
f != f->table->field[f->field_index]
(here f= key->key_part[p].field). This is the case for varchar/blob prefix keys. But as they cannot be auto-increment anyway, the direct comparison should work, I suppose.
Hmm, thanks for noticing, I really didn't consider this blobs case.
still, you didn't use `f == f->table->next_number_field` but we concluded it's safe, didn't we? === Also, I've looked at your latest branch. What were you optimizing with the commit 3afa3288221 (the one with usable_keys_data)? It's complex and I don't quite see what's the purpose of it. It looks like all you need to do is to decide on the best index to use, once, save it somewhere, e.g. in RPL_TABLE_LIST, and that's it. This can be done, for example, in copy_data_between_tables(). Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org