maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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(&ltime, 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@xxxxxxxxxxx