Re: bzr commit into mysql-5.1 branch (v.narayanan:2680) Bug#38338

[Sergei Golubchik]

Hi!

The fix is good.
I have only minor formal comments, see below:

On Aug 17, Narayanan V wrote:
> 
>  2680 Narayanan V	2008-08-17
>       Bug#38338

Please, mention the bug synopsys here, like in

  Bug#38338: REPLACE causes last_insert_id() to return an incorrect value

>       Fix the write_record function to record auto increment
>       values in a consistent way.

(that's right, first bug number and synopsys, then a short description
of a fix, just as you have it)

> === modified file 'sql/sql_insert.cc'
> --- a/sql/sql_insert.cc	2008-07-11 18:51:10 +0000
> +++ b/sql/sql_insert.cc	2008-08-17 17:12:39 +0000
> @@ -1545,6 +1545,17 @@ int write_record(THD *thd, TABLE *table,
>          }
>        }
>      }
> +    
> +    /*
> +        If more than one iteration of the above while loop is done, from the second
> 
> +        one the row being inserted will have an explicit value in the autoinc field,
> 
> +        which was set at the first call of handler::update_auto_increment(). This 
> +        value is saved to avoid thd->insert_id_for_cur_row becoming 0. Use this
> saved
> +        autoinc value.
> +     */
> +    if (table->file->insert_id_for_cur_row <= 0)
> +      table->file->insert_id_for_cur_row= insert_id_for_cur_row;

I understand that you got your <= by reverting the if() condition in the
loop. But this still looks confusing - after all insert_id_for_cur_row
is unsigned and can never be negative. Change to "== 0", please.

And don't forget a test case - this changeset doesn't have a test case
and cannot be pushed without it.

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Engineer/Server Architect
/_/  /_/\_, /___/\___\_\___/  Sun Microsystems GmbH, HRB München 161028
       <___/                  Sonnenallee 1, 85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schroeder, Wolfgang Engels, Dr. Roland Boemer
Vorsitzender des Aufsichtsrates: Martin Häring