Re: bzr commit into mysql-next-mr-rpl-merge branch (alfranio.correia:3202) WL#2775

[Alfranio Correia]

Hi Luis,

Thank you for the review.
See some comments in-line

Cheers.

On 10/08/2010 04:46 PM, Luís Soares wrote:
> Hi Alfranio,
>
>    Nice Work. I have a few comments. Please find them below.
>
> STATUS
> ------
>
>    Not approved.
>
> REQUIRED CHANGES
> ----------------
>
>    RC1. Rpl_info_factory::decide_repository
>
>         Move the comments to outside the member function and put
>         them in doxygen style.

ok. I will do this.

>
>    RC2. Rpl_info_fields::init()
>
>         1. Clear the condition in the for loop.
>
>           +
>           +  if (!field&&  !(field= new info_field[ninfo]))
>           +      DBUG_RETURN(TRUE);
>           +
>           +  for (int pos= 0; field&&  pos<  ninfo; pos++)
>
>           Why not
>
>           for (int pos= 0; pos<  ninfo; pos++)
>
>           You are already certain that "field" exists because of
>           the previous conditional DBUG_RETURN.

ok. I will do this.

>
>         2. Move everything to one loop
>
>            I see the point on having two loops, but I think we
>            might be able to do everything in one loop. If you
>            think it's worth (init() should be called that often),
>            perhaps we can have a variable that keeps track of the
>            malloc outcome, and use it to decide wether to set the
>            field to zero or not and in the end to return error or
>            not.

ok. I will do this.

>
>    RC3. struct info_field
>
>         Better to place a comment on the fields, just to remove
>         some ambiguity wrt to LEX_STRING internal field - size.
>
>         info_field.value.size -- size of the string
>         info_field.size       -- field buffer size
>
>         Perhaps would be better to rename info_field.size to
>         info_field.buffer_size.

ok. I will do this.

>
>    RC4. Rpl_info_table::do_flush_info
>
>         +  THD *thd= access->create_bootstrap_thd();
>         +
>         +  DBUG_ENTER("Rpl_info_table::do_flush_info");
>         +
>         +  if (!(force || (sync_period&&
>         +      ++(sync_counter)>= sync_period)))
>         +  {
>         +    access->drop_bootstrap_thd(thd);
>         +    DBUG_RETURN(0);
>         +  }
>
>         I think one could avoid creating the bootstrap thd if we
>         are returning from inside the if. IIRC, the thd is only
>         created so that DBUG_ENTER/RETURN is usable. So I have two
>         recommendations, either:
>
>         1. we place a comment to make it clear why we do it
>
>         2. or, do not use DBUG_ENTER before the if.
>
>         ATM, I prefer the first one.

I will analyze if it is possible to implement your suggestion but
in the worst case, I will write a comment explaining why this is
necessary.

>
>    RC5. Rpl_info_table::do_flush_info
>
>         +    /*
>         +      Inserts a new row into rpl_info table.
>         +    */
>         +    if (table->file->ha_write_row(table->record[0]))
>         +    {
>         +      table->file->print_error(error, MYF(0));
>         +      goto end;
>         +    }
>
>         I think there is an "error= " in the if clause:
>
>         if ((error= table->file->ha_write_row(table->record[0])))
>
>         Otherwise, it will always be called with error=1.


You are right. It is missing "error=".

>
>
>    RC6. Rpl_info_table::do_reset_info
>
>         I think we can make this part of the code more clear:
>
>         error= ((res == FOUND_ID || res == NOT_FOUND_ID) ? 0 : 1);
>
>         Like this:
>
>         error= (res == ERROR_ID);

ok. I will do this.

>
>
>    RC7. This deserves a comment:
>
> -    int error= ev->update_pos(rli);
> +    int error= 0;
> +    if (ev->get_type_code() != XID_EVENT || skip_event ||
> +        !rli->is_transactional())
> +      error= ev->update_pos(rli);

ok. I will write a comment.

>
>    RC8. + return TABLE_CATEGORY_LOG;  // Alfranio : TODO - Define a new category
>
>         Will you ? ;)

Yes. I will. I was waiting for suggestion on names. See Jasonh's email on this matter.

>
>
> REQUESTS
> --------
>
>    R1. Rpl_info_table::do_set_info
>
>        +    If a file is manually and not properly changed, this function may
>        +    crash the server.
>
>        Yikes... What can we do to prevent even those cases ?

I will check if this comment is still valid.


>
>
>    R2. Rpl_info_table::do_set_info(const int pos, const Server_ids *value)
>
>        +  int needed_size= (sizeof(::server_id) * 3 + 1) *
>        +                    (1 + value->server_ids.elements);
>
>        Care to ellaborate, comment ?
>
>        I am not following

ok. I will do this.

>
>    R3. Rpl_info_table::do_get_info(const int pos, char *value, const size_t
>
>        Why strmov and not strmake?

I can use strmake if you want to.
>
>
>    R4. I wonder why Rpl_info_table::do_get_info get a paremeter
>        named default_value. They don't seem to be used. What's
>        happening here? If they are just interface sugar, can you
>        please annotate them with the unused ? If they shouldn't be
>        there, just remove them.


I will try to fix this. Further news in another email I will send tomorrow.


>
>    R5. Rpl_info_table_access::find_info_id
>
>        I wonder if we should have a test to check if the primary
>        key is disabled. I also wonder if it matters :), given the
>        assumptions (PK is always in the 0th key and there are no
>        two records with the same value for the field ever - even
>        if PK is disabled). Just trying to avoid things like:
>        BUG#47312.
>

ok. I will do this.

>
>
>
> SUGGESTIONS
> -----------
>
>    S1. In scripts/mysql_install_db.pl.in, scripts/mysql_install_db.sh
>
>        + created using the MyISAM storage engine. However, any storage
>        + engine available to the server may be used. If a crash-safe
>        + slave is desired, the storage engine must be transactional.
>
>        Last sentence, I would try to use the word "required"
>        instead of "desired".

ok. I will do this.

>
>
>    S2. Rpl_info_table::do_prepare_info_for_write
>
>        Maybe we could just call ::do_prepare_info_for_read()
>        instead ? If yes, please leave a comment stating that
>        preparing for read and write is the same procedure.

The function are the same when the repository is a table. However,
they differ when the repository is a file. So I will do what follows:

int Rpl_info_table::do_prepare_info_for_read()
{
   return (do_prepare_info_for_write());
}

int Rpl_info_table::do_prepare_info_for_write()
{
   if (!field_values)
     return TRUE;

   cursor= 1;

   return FALSE;
}

>
> DETAILS
> -------
>   n/a
>

Cheers.