Re: bzr commit into mysql-5.1-rep+3 branch (alfranio.correia:3145) WL#2775

[Luís Soares]

Hi Alfranio

  This is a follow up review after you addressed my comments
from the previews review round: 
  http://lists.mysql.com/commits/109507

  There are some minor things left to address, nothing big I
think.

STATUS
------
 
  Not approved.

REQUIRED CHANGES
----------------

  RC1. In rpl_info_factory.h, hunk @@ -19,6 +19,16 @@, please
       rename the tables as suggested by Mats.

  RC2. In rpl_info_fields.cc:

       "Restore the pointer from "saved" to "use". This method
       must be called (...)".

       I believe that in C++ the correct terminology is "member
       function". Please change it: "method --> member function".

REQUESTS
--------
 
  R1. This is a request for clarification! (I may have already
      asked you this before, so sorry if I did, but I don't
      remember.)

      In hunk @@ -5490,6 +5538,11 @@ Xid_log_event::do_shall_skip
      what motivated this:

      +
      +int Xid_log_event::do_update_pos(Relay_log_info *rli)
      +{
      +  return (0);
      +}


  R2. This is a request for clarification! 

      In rpl_info_table.cc, hunk @@ -0,0 +1,484 @@, more
      precisely in Rpl_info_table::do_flush_info:

      +  THD *thd= access->create_fake_thd();
      +
      +  DBUG_ENTER("Rpl_info_table::do_flush_info");
      +
      +  if (!(force || (sync_period &&
      +      ++(sync_counter) >= sync_period)))
      +  {
      +    access->drop_fake_thd(thd, 0);
      +    DBUG_RETURN(0);
      +  }

      Can't we just move the access->create_fake_thd to *after*
      the condition checking (ie, the "if")? Then we could remove
      the access->drop_fake_thd from the "if" block as well,
      right?

  R3. do_init_info assumes that access->find_info_id returns
      error only when it cannot find the row. I think that there
      are other cases as well. Can you please double check what
      are the possible return values for:
      - table->file->index_read_idx_map

      Actually, you have this in find_indo_id:

      +  if (!(table->field[idx]->flags & PRI_KEY_FLAG))
      +    DBUG_RETURN(TRUE);

      So I believe that errors can be returned for cases that the
      row exists but something else, "out of the ordinary",
      happened.

      So basically, we have:

      1. Rpl_info_table::do_init_info

         Finds the row in the info table by calling
         find_info_id. If find_info_id returns != 0, then assume
         that it does not exist*. 

      2. Rpl_info_table::do_flush_info
 
         Finds the row in the info table by calling
         find_info_id*. If it returns != 0 then a new row is
         inserted.

      * The problem: Rpl_info_table_access::find_info_id may
        return != 0 when other than missing row errors ocurr.
        Am I missing something?


SUGGESTIONS
-----------
 n/a

DETAILS 
-------
 n/a


On Tue, 2010-06-01 at 08:19 +0000, Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql.new/bzrwork/wl-2775/mysql-5.1-rep%2B3.crash-safe.2775/
> based on revid:alfranio.correia@stripped
> 
>  3145 Alfranio Correia	2010-06-01 [merge]
>       WL#2775