List:Commits« Previous MessageNext Message »
From:Luís Soares Date:April 21 2010 12:56am
Subject:Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)
Bug#49100
View as plain text  
Hi Alfranio,
  Please, review new patch addressing your requests:
  http://lists.mysql.com/commits/106164

Regards,
Luís

On Wed, 2010-04-21 at 00:13 +0100, Luís Soares wrote:
> Hi Alfranio,
> 
>   Before proceeding some considerations about the server 
> behavior before this patch and when tested against the
> tests cases presented:
> 
> case #1: Master and slave(s) become out of sync.
> 
> case #2: Master and first slave in sync, but third slave is out
>          of sync.
> 
> case #3: triggers assertion in 
> #7  0x00000000007ef82d in Rows_log_event::unpack_current_row
> (this=0x32f95d8, rli=0x331ac60, cols=0x32f9660, 
>     abort_on_warning=false) at log_event.h:3631
> 
> case #4: although AI has data only for columns that slave does
>          not know about (thence it should do nothing), it still 
>          reports an error.
> 
> Last_SQL_Error	Could not execute Update_rows event on table test.t1;
> Can't find record in 't1', Error_code: 1032; handler error
> HA_ERR_END_OF_FILE; the event's master log mysqld-bin.000001,
> end_log_pos 555
> 
> case #5: there is no bug here. We are just adding the test case
>          to cover this case. In this case, despite the fact that 
>          there is valid data for columns on the slave's table, the
>          slave cannot find the record because there is no valid 
>          data in the BI to use when searching for the record. 
>          Thence we just test that the slave stops (by adding the 
>          test case).
> 
> Some more comments inline.
> 
> On Tue, 2010-04-20 at 17:33 +0100, Alfranio Correia wrote:
> > Hi Luis,
> > 
> > Not approved.
> > 
> > The code seems ok but we need to improve the comments and clarify
> > some points. See comments in-line
> > 
> > Cheers.
> > 
> > Luis Soares wrote:
> > > #At
> file:///home/lsoares/Workspace/bzr/work/features/wl5092/mysql-5.1-rpl-wl5092/ based on
> revid:luis.soares@stripped
> > > 
> > >  3186 Luis Soares	2010-03-31
> > >       BUG#49100: RBR: Unexpected behavior when AI contains no usable
> > >                  data for slave columns
> > >       
> > >       RBR was not fully compliant with empty before and after
> > >       images (BI and AI) in row events. Several faulty scenarios
> > >       surfaced during WL#5092 implementation, especially when MINIMAL
> > >       images were used:
> > >       
> > >         S1. Inserting an empty row on the master would cause slave to
> > >             skip the event (empty AI);
> > >       
> > >             Expected: slave(s) shall apply the insert and fill its
> > >             table with default values as master did.
> > >       
> > >         S2. Master inserts values just for columns that slave doesn't
> > >             have (unusable AI at the slave). 
> > 
> > Please, describe what was the bug. Most likely, it is equivalent to S1
> > but it is better to make this clear.
> 
> No it's not the same thing. In S1 the event is skipped, while in 
> S2 the event is not skipped.
> 
> > 
> > >       
> > >             Expected: slave(s) shall apply the insert and fill its
> > >             table with default values as master did for columns that
> > >             user did not provide data.
> > >        
> > >         S3. Master updates columns that only it knows about (unusable
> > >             AI at the slave) but references columns that it shares with
> > >             the slave (usable BI at the slave). This would trigger an
> > >             assertion on a third slave in chained replication: m -> s1
> > >             -> s2, s2 would abort.
> > >       
> > >             Expected: slave s1 shall not update any row and slave s2
> > >             shall not assert.
> > >       
> > >         S4. Master updates and references columns that only it knows
> > >             about (unusable AI and BI at the slave).
> > 
> > 
> > The same here. See above.
> 
> No this (S4) is the case the master uses for reading and writing columns
> that only it knows about. Whereas the previous (S3) is master reads
> columns that the slave knows about but writes into columns that only
> master is aware of...
> 
> > >       
> > >             Expected: slave(s) shall skip the event and not update its
> > >             tables with any value.
> > >       
> > >         S5. Master references columns that only it knows
> > >             about (unusable BI at the slave) and updates columns shared
> > >             with the slave (usable AI at the slave). Slave must stop.
> > >       
> > 
> > What was the error before the patch?
> 
> No error (in WL#5092 tree, have not tested in legacy 6.0), we're 
> just adding test coverage in this patch.
> 
> > >             Expected: slave(s) shall fail with HA_ERR_END_OF_FILE.
> > >       
> > >       We fix S1 to S5 by relaxing some asserts that were triggered on
> > >       when processing empty images. Additionally, on the
> > >       Update_rows_log_event::do_exec_row we deployed a check the
> > >       verifies if there is any value for AI. If there is not, then the
> > >       event is skipped (neither BI nor AI is are processed).
> > 
> > Are S3 and S4 only about asserts? I don't think so. Please, update the summary
> of
> > the solutions.
> 
> They are completely different issues. S3 is about the assertion 
> on the 2nd slave. While S4 is about the slave stopping unexpectedly. 
> 
> > >       
> > >       For mysqlbinlog --verbose --base64=decode-rows we print out an
> > >       INSERT INTO t VALUES () when a write rows event contains an
> > >       empty AI.
> > 
> > ok.
> > 
> > >      @ mysql-test/suite/rpl/r/rpl_row_img.result
> > >         Updates to result files.
> > >      @ mysql-test/suite/rpl/t/rpl_row_img.test
> > >         Added test cases.
> > >      @ sql/log_event.cc
> > >         1. Added printout for INSERT INTO ... VALUES () when AI is empty
> for
> > >            Write_rows_log_events.
> > >         2. Changed assertion from 'm_curr_row < ...' to 'm_curr_row
> <= ...'
> > >         3. Calculation of estimated rows for bulk insert is now
> conditional,
> > >            and dependent of whether m_curr_row < m_rows_end or not.
> > >         4. In the Update_rows_log_event, changed it to take into
> consideration
> > >            if there are any values in AI. If there are not, then skip
> event.
> > >      @ sql/log_event.h
> > >         Changed assertion to from 'm_curr_row < ...' to ' m_curr_row
> <= ...'
> > >      @ sql/rpl_record.cc
> > >         Do not unpack if master did not send any bit set in the write_set.
> > > 
> > >     modified:
> > >       mysql-test/suite/rpl/r/rpl_row_img.result
> > >       mysql-test/suite/rpl/t/rpl_row_img.test
> > >       sql/log_event.cc
> > >       sql/log_event.h
> > >       sql/rpl_record.cc
> > > === modified file 'mysql-test/suite/rpl/r/rpl_row_img.result'
> > > --- a/mysql-test/suite/rpl/r/rpl_row_img.result	2010-03-30 16:06:36 +0000
> > > +++ b/mysql-test/suite/rpl/r/rpl_row_img.result	2010-03-30 23:19:25 +0000
> > > @@ -24179,3 +24179,259 @@ FLUSH TABLES;
> > >  SHOW VARIABLES LIKE 'binlog_row_image';
> > >  Variable_name	Value
> > >  binlog_row_image	FULL
> > > +include/stop_slave.inc
> 
> [snip]
> 
> > > @@ -8695,7 +8706,11 @@ Rows_log_event::write_row(const Relay_lo
> > >      /* this is the first row to be inserted, we estimate the rows with
> > >         the size of the first row and use that value to initialize
> > >         storage engine for bulk insertion */
> > > -    ulong estimated_rows= (m_rows_end - m_curr_row) / (m_curr_row_end -
> m_curr_row);
> > > +    DBUG_ASSERT(!(m_curr_row > m_curr_row_end));
> > > +    ulong estimated_rows= 1;
> > > +    if (m_curr_row < m_curr_row_end)
> > > +      estimated_rows= (m_rows_end - m_curr_row) / (m_curr_row_end -
> m_curr_row);
> > > +
> > 
> > 
> > Please, rewrite it as follows:
> > 
> > 
> > +    if (m_curr_row < m_curr_row_end)
> > +      estimated_rows= (m_rows_end - m_curr_row) / (m_curr_row_end -
> m_curr_row);
> > +    else if (m_curr_row = m_curr_row_end)
> > +      ulong estimated_rows= 1;
> > 
> > 
> > It think it would be easier to read.
> 
> OK, will do, but will change 
>   ulong estimated_rows= 1; 
> to 
>   estimated_rows= 1;
> in the last line. ;)
> 
> > >      m_table->file->ha_start_bulk_insert(estimated_rows);
> > >    }
> > >    
> > > @@ -9665,8 +9680,34 @@ int 
> > >  Update_rows_log_event::do_exec_row(const Relay_log_info *const rli)
> > >  {
> > >    DBUG_ASSERT(m_table != NULL);
> > > +  int error= 0;
> > > +
> > > +  /* check if update contains only values (for both BI and AI)
> > > +     for columns that do not exist on the slave. If they do, we
> > > +     can just unpack the rows and return (do nothing on the local 
> > > +     table).
> > > +
> > > +     We can optimize and check only if there are usable values on 
> > > +     the AI. However, if no good values on AI but we have values 
> > > +     on BI, we will not be letting the slave thread to search the
> > > +     SE for the row. This means that we may be loosing a bit on 
> > > +     the safety side!
> > > +     
> > > +     After all, find_row is a consistency check as well.
> > > +  */
> > 
> > These comments are difficult to read. Please, improve them.
> > Does this handle S3 and S4?
> 
> S4. I think you refer to the second paragraph as being hard 
> to read...
> 
> Will improve it.
> 
> > 
> > > +  if (/*!is_any_column_signaled_for_table(m_table, &m_cols)
> &&*/ /* BI */
> > > +      !is_any_column_signaled_for_table(m_table, &m_cols_ai) /* AI
> */)
> > > +  {
> > > +    /* read and discard images, because they don't contain any 
> > > +       useful information for this server table */
> > > +    error = unpack_current_row(rli, &m_cols);
> > > +    m_curr_row= m_curr_row_end;
> > > +    error = error | unpack_current_row(rli, &m_cols_ai);
> > > +
> > > +    return error;
> > > +  }
> > 
> > See above.
> 
> What should I see? Is it hard to read? Do you want me to remove
> the comment in the 'if' ?
> 
> I'll assume that you want me to remove the comment on the 'if'.
> I only added for reviewing purposes anyway...
> 
> > >  
> > > -  int error= find_row(rli); 
> > > +  error= find_row(rli); 
> > >    if (error)
> > >    {
> > >      /*
> > 
> > ok.
> > 
> > > 
> > > === modified file 'sql/log_event.h'
> > > --- a/sql/log_event.h	2010-03-04 17:00:32 +0000
> > > +++ b/sql/log_event.h	2010-03-30 23:19:25 +0000
> > > @@ -3628,7 +3628,7 @@ protected:
> > >      DBUG_ASSERT(m_table);
> > >  
> > >      bool first_row= (m_curr_row == m_rows_buf);
> > > -    ASSERT_OR_RETURN_ERROR(m_curr_row < m_rows_end,
> HA_ERR_CORRUPT_EVENT);
> > > +    ASSERT_OR_RETURN_ERROR(m_curr_row <= m_rows_end,
> HA_ERR_CORRUPT_EVENT);
> > >      int const result= ::unpack_row(rli, m_table, m_width, m_curr_row,
> cols,
> > >                                     &m_curr_row_end,
> &m_master_reclength,
> > >                                     abort_on_warning, first_row);
> > 
> > ok.
> > 
> > > 
> > > === modified file 'sql/rpl_record.cc'
> > > --- a/sql/rpl_record.cc	2010-03-04 17:00:32 +0000
> > > +++ b/sql/rpl_record.cc	2010-03-30 23:19:25 +0000
> > > @@ -208,6 +208,20 @@ unpack_row(Relay_log_info const *rli,
> > >    uchar const *null_ptr= row_data;
> > >    uchar const *pack_ptr= row_data + master_null_byte_count;
> > >  
> > > +  if (bitmap_is_clear_all(cols))
> > > +  {
> > > +    /**
> > > +       There was no data sent from the master, so there is 
> > > +       nothing to unpack. In case we are unpacking an AI (for
> > > +       update or insert purposes), we just use the default 
> > 
> > Is this comment right? Default value for an update?
> 
> On an update one unpacks the row on top of the record that 
> the engine has returned in find_row. See:
> 
> Update_rows_log_event::do_exec_row
> 
> So this comment is misleading...
> 
> > > +       values from the slave to do the insert.
> > 
> > Updates ---> Inserts?
> > 
> > Please, improve these comments.
> 
> Again, these comments are misleading... Perhaps the reference 
> to AI is probably not worth the trouble... I'll remove it.
> 
> As I take it code-wise (apart from the refactoring above in one 'if')
> the patch is ok, you just need me to improve some comments.
> 
> Will rewrite the comments do the small change and recommit.
> 
> Thanks,
> Luís
> 
> > > +    */
> > > +    *row_end= pack_ptr;
> > > +    *master_reclength= 0;
> > > +    DBUG_RETURN(error);
> > > +  }
> > > +
> > > +
> > >    Field **const begin_ptr = table->field;
> > >    Field **field_ptr;
> > >    Field **const end_ptr= begin_ptr + colcnt;
> > > 
> > > 
> > > ------------------------------------------------------------------------
> > > 
> > > This body part will be downloaded on demand.
> > 
> > -- 
> > MySQL Code Commits Mailing List
> > For list archives: http://lists.mysql.com/commits
> > To unsubscribe:    http://lists.mysql.com/commits?unsub=1
> > 
> 
> 
> 


Thread
bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Luis Soares31 Mar
  • Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Alfranio Correia20 Apr
    • Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Luís Soares21 Apr
      • Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Luís Soares21 Apr
      • Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Alfranio Correia23 Apr
        • Re: bzr commit into mysql-5.1-rpl-wl5092 branch (luis.soares:3186)Bug#49100Luís Soares23 Apr