maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

On Oct 18, Nikita Malyavin wrote:
> On Tue, 18 Oct 2022 at 01:01, Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx