Re: bk commit into 5.0 tree (gshchepa:1.2625) BUG#36676

[Sergey Petrunia]

Hi,

On Sun, May 18, 2008 at 02:21:27PM +0500, gshchepa@stripped wrote:
> ChangeSet@stripped, 2008-05-18 14:21:25+05:00, gshchepa@stripped +4 -0
>   Fixed bug#36676: multiupdate using LEFT JOIN updates only
>                    first row or fails with an error:
>     ERROR 1022 (23000): Can't write; duplicate key in table ''
>   
>   The server uses intermediate temporary table to store updated
>   row data.  The first column of this table contains rowid.
>   Current server implementation doesn't reset NULL flag of that
>   column even if the server fills a column with rowid.
>   To keep each rowid unique, there is an unique index.
>   An insertion into an unique index takes into account NULL
>   flag of key value and ignores real data if NULL flag is set.
>   So, insertion of actually different rowids may lead to two
>   kind of problems.  Visible effect of each of these problems
>   depends on an initial engine type of temporary table:
>   
>   1. If multiupdate initially creates temporary table as
>   a MyISAM table (a table contains blob columns, and the
>   create_tmp_table function assumes, that this table is
>   large), it inserts only one single row and updates
>   only rows with one corresponding rowid. Other rows are
>   silently ignored. 
>   
>   2. If multiupdate initially creates MEMORY temporary
>   table, fills it with data and reaches size limit for
>   MEMORY tables (max_heap_table_size), multiupdate
>   converts MEMORY table into MyISAM table and fails
>   with an error:
>     ERROR 1022 (23000): Can't write; duplicate key in table ''
>   
>   
>   Multiupdate has been fixed to update the NULL flag of
>   temporary table rowid columns.
>   
> 
>   mysql-test/r/multi_update_tiny_hash.result@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +45 -0
>     Added test case for bug#36676.
> 
>   mysql-test/r/multi_update_tiny_hash.result@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +0 -0
> 
>   mysql-test/t/multi_update_tiny_hash-master.opt@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +1 -0
>     Added test case for bug#36676.
> 
Why create a separate test file and not just put the test case into 
t/multi_update.test, wrapped with 

SET @@save_max_heap_table_size=16384;
SET max_heap_table_size=16384;

<testcase goes here >

SET max_heap_table_size=@@save_max_heap_table_size;

?
>   mysql-test/t/multi_update_tiny_hash-master.opt@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +0 -0
> 
>   mysql-test/t/multi_update_tiny_hash.test@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +61 -0
>     Added test case for bug#36676.
> 
>   mysql-test/t/multi_update_tiny_hash.test@stripped, 2008-05-18 14:20:39+05:00,
> gshchepa@stripped +0 -0
> 
>   sql/sql_update.cc@stripped, 2008-05-18 14:20:47+05:00, gshchepa@stripped +6 -0
>     Fixed bug#36676: multiupdate using LEFT JOIN updates only
>                      first row or fails with an error:
>       ERROR 1022 (23000): Can't write; duplicate key in table ''
>     
>     The multi_update::send_data method has been modified to reset null bits of
>     fields containing rowids.
> 
> diff -Nrup a/sql/sql_update.cc b/sql/sql_update.cc
> --- a/sql/sql_update.cc	2008-03-27 15:52:54 +04:00
> +++ b/sql/sql_update.cc	2008-05-18 14:20:47 +05:00
> @@ -1440,6 +1440,12 @@ bool multi_update::send_data(List<Item> 
>            tbl->file->position(tbl->record[0]);
>          memcpy((char*) tmp_table->field[field_num]->ptr,
>                 (char*) tbl->file->ref, tbl->file->ref_length);
> +        /*
> +         For outer joins a rowid field may have no NOT_NULL_FLAG,
> +         so we have to reset NULL bit for this field.
> +         (set_notnull() resets NULL bit only if available).
> +        */
> +        tmp_table->field[field_num]->set_notnull();
>          field_num++;
>        } while ((tbl= tbl_it++));

Ok, it seems our initial reaction to this fix was incorrect. We assumed that
if the temptable column is nullable that means that it would try to get
rowids of null-complemented records (which will be NULL), try to update them,
and so forth. That seems not to be so - my experiments in debugger show that 
NULL-complemented records are not put into the temptable.

But if that is not so, one might ask a question why the rowid column in the
temptable is NULLable.

I've investigated, this seems to be a consequence of how create_tmp_table() 
works. Your fix for Bug #28716 "CHECK OPTION expression is evaluated over
expired record buffers" (cset ChangeSet 1.2463.85.1 2007/05/30 12:21:39 
gshchepa@stripped 
 Fixed bug #28716.) tried to address the problem:


gshchepa/uchum  1.217.1.1           List_iterator_fast<TABLE>
tbl_it(unupdated_check_opt_tables);
gshchepa/uchum  1.217.1.1           TABLE *tbl= table;
gshchepa/uchum  1.217.1.1           do
gshchepa/uchum  1.217.1.1           {
gshchepa/uchum  1.217.1.1             Field_string *field= new
Field_string(tbl->file->ref_length, 0,
gshchepa/uchum  1.217.1.1                                                   tbl->alias,
gshchepa/uchum  1.217.1.1                                                   tbl,
&my_charset_bin);
gshchepa/uchum  1.217.1.1             if (!field)
gshchepa/uchum  1.217.1.1               DBUG_RETURN(1);
gshchepa/uchum  1.217.1.1             /*
gshchepa/uchum  1.217.1.1               The field will be converted to varstring when
creating tmp table if
gshchepa/uchum  1.217.1.1               table to be updated was created by mysql 4.1. Deny
this.
gshchepa/uchum  1.217.1.1             */
gshchepa/uchum  1.217.1.1             field->can_alter_field_type= 0;
gshchepa/uchum  1.217.1.1             Item_field *ifield= new Item_field((Field *) field);
gshchepa/uchum  1.217.1.1             if (!ifield)
gshchepa/uchum  1.217.1.1                DBUG_RETURN(1);
gshchepa/uchum  1.217.1.1             ifield->maybe_null= 0;

But without much success as create_tmp_table() detects Item_field objects
and analyzes their underlying field, which in this case refers to the inner
table and therefore is NULLable.

Do the "ifield->maybe_null= 0" serve any other purposes? Please either add
comment that this line would not cause temp. table column field to be not
null, or remove the line altogether.

BR
 Sergey
-- 
Sergey Petrunia, Lead Software Engineer
MySQL AB, www.mysql.com
Office: N/A
Blog: http://s.petrunia.net/blog