maria-developers team mailing list archive

[Sachin Setiya <sachin.setiya@xxxxxxxxxxx>]

Hi Serg

Thanks for the review!



On Wed, Jun 3, 2020 at 8:48 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> Hi, Sachin!
>
> Two questions:
>
> 1. This looks like there was no replication tests for periods of long
>    uniques at all? Please add them (create an MDEV, like "replication tests
>    of long unique") and add various relevant replication tests in 10.4.
>
> Done.  https://jira.mariadb.org/browse/MDEV-22821

> 2. It feels a bit risky to backport my changes to 10.4. At least you
>    definitely should take the latest code from 10.5. Not cherry-pick one
>    commit, but copy handler::prepare_for_insert() and other relevant
>    code from 10.5.
>
> I will look into it.

>    May be it'd be safer not to backport? What would your fix look like
>    if you wouldn't have prepare_for_insert()?
>
No Fix needed, it works without any issue.
Rest of the patch reply are in context of 10.5

>
> Also, a couple of comments to the code, see below.
>
> On Jun 03, Sachin wrote:
> > revision-id: 0f91c62d332 (mariadb-10.4.11-225-g0f91c62d332)
> > parent(s): c01b3d91690
> > author: Sachin <sachin.setiya@xxxxxxxxxxx>
> > committer: Sachin <sachin.setiya@xxxxxxxxxxx>
> > timestamp: 2020-06-02 08:24:24 +0530
> > message:
> >
> > MDEV-22722 Assertion "inited==NONE" failed in handler::ha_index_init on
> the slave during UPDATE
> >
> > Add missing call for handler->prepare_for_insert() in rows event.
> >
> > diff --git a/mysql-test/main/long_unique_bugs_replication.test
> b/mysql-test/main/long_unique_bugs_replication.test
> > new file mode 100644
> > index 00000000000..5348be3e7d3
> > --- /dev/null
> > +++ b/mysql-test/main/long_unique_bugs_replication.test
> > @@ -0,0 +1,22 @@
> > +#
> > +# Long unique bugs related to master slave replication
> > +#
> > +
> > +#
> > +# MDEV-22722 Assertion "inited==NONE" failed in handler::ha_index_init
> on the slave during UPDATE
> > +#
> > +
> > +--source include/master-slave.inc
> > +--source include/have_binlog_format_row.inc
>
> master-slave.inc should always be included last, after all have_xxx.inc
> files that may cause the test to be skipped.
>
Done.

>
> > +
> > +create table t1 (i1 int, a1 text, unique key i1 (a1)) engine=myisam;
> > +insert into t1 values (1,1);
> > +update t1 set a1 = 'd' limit 1;
> > +
> > +sync_slave_with_master;
> > +connection slave;
> > +
> > +connection master;
> > +drop table t1;
> > +
> > +--source include/rpl_end.inc
> > diff --git a/sql/log_event.cc b/sql/log_event.cc
> > index 3707f73a716..f4c0c546a38 100644
> > --- a/sql/log_event.cc
> > +++ b/sql/log_event.cc
> > @@ -11568,6 +11568,7 @@ int
> Rows_log_event::do_apply_event(rpl_group_info *rgi)
> >    {
> >      master_had_triggers= table->master_had_triggers;
> >      bool transactional_table= table->file->has_transactions();
> > +    table->file->prepare_for_insert();
> >      /*
> >        table == NULL means that this table should not be replicated
> >        (this was set up by Table_map_log_event::do_apply_event()
> > @@ -14344,6 +14345,7 @@ int Rows_log_event::find_row(rpl_group_info *rgi)
> >      issue_long_find_row_warning(get_general_type_code(),
> m_table->alias.c_ptr(),
> >                                  is_index_scan, rgi);
> >    table->default_column_bitmaps();
> > +  table->file->prepare_for_insert();
>
> Why do you need it here, I'd think that do_apply_event() should be
> enough?
>
So it can be done in  Rows_log_event::do_apply_event()
With some logic like this
    if (get_general_type_code() == WRITE_ROWS_EVENT)
      table->file->prepare_for_insert(0);
    else if (get_general_type_code() == UPDATE_ROWS_EVENT ||
             get_general_type_code() == DELETE_ROWS_EVENT)
      table->file->prepare_for_insert(1);
Because table->file->prepare_for_insert(1);  will be inefficient for Write
Rows events
Patch is in bb-10.5-sachin branch.

> >    DBUG_RETURN(error);
> >  }
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx
>


-- 
Regards
Sachin Setiya
Software Engineer at  MariaDB