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

[Gleb Shchepa]

Hello Sergey,

Please see my answer inline and below.

Sergey Petrunia wrote:
> 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;
> 
> ?


By analogy with count_distinct2-master.opt.


>>   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.


No.

First of all, my fix for #28716 didn't invented that, it is very old thing
(2003-08-20). My fix only added additional rowid columns to temporary table
for VIEW CHECK OPTION needs, so, the first rowid column that acts like primary
key of temporary table was not affected. My name in front of this line is
a result of indentation.


Original source code:

monty   1.54.1.25           /*
monty   1.54.1.25             Create a temporary table to store all fields that are
changed for this
monty   1.54.1.25             table. The first field in the temporary table is a pointer
to the
monty   1.54.1.25             original row so that we can find and update it
monty   1.54.1.25           */
monty   1.54.1.25
monty   1.54.1.25           /* ok to be on stack as this is not referenced outside of this
func */
monty   1.54.1.25           Field_string offset(table->file->ref_length, 0,
"offset",
monty   1.54.1.25                               table, 1);
Sinisa  1.54.1.37           if (!(If=new Item_field(((Field *) &offset))))
Sinisa  1.54.1.37             DBUG_RETURN(1);
Sinisa  1.54.1.37           If->maybe_null=0;


AFAIU this string ("...->maybe_null=0" where "..." is a pointer to rowid
Item_field) was added to synchronise the Item_field::maybe_null flags with
the corresponding Field::flag (that has NOT_NULL_FLAG bit set after the
Field_string constructor call with the null_ptr_arg argument set to NULL).

Secondly, this Item_field::maybe_null flag does not affect NULLness of
temporary table rowid fields, because it is overridden in Field::new_field
from  create_tmp_table (see create_tmp_field_from_field) by whole table
st_table::maybe_null flag:


Field *Field::new_field(MEM_ROOT *root, struct st_table *new_table,
                         bool keep_type __attribute__((unused)))
{
   Field *tmp;
   if (!(tmp= (Field*) memdup_root(root,(char*) this,size_of())))
     return 0;

   if (tmp->table->maybe_null)
     tmp->flags&= ~NOT_NULL_FLAG;


Whole table st_table::maybe_null flag is set for outer joins
at make_join_statistics:


   if (join->outer_join)
   {
     /*
        Build transitive closure for relation 'to be dependent on'.
        This will speed up the plan search for many cases with outer joins,
        as well as allow us to catch illegal cross references/
        Warshall's algorithm is used to build the transitive closure.
        As we use bitmaps to represent the relation the complexity
        of the algorithm is O((number of tables)^2).
     */
     for (i= 0, s= stat ; i < table_count ; i++, s++)
     {
       for (uint j= 0 ; j < table_count ; j++)
       {
         table= stat[j].table;
         if (s->dependent & table->map)
           s->dependent |= table->reginfo.join_tab->dependent;
       }
       if (s->dependent)
         s->table->maybe_null= 1;


AFAIU the line "ifield->maybe_null= 0;" is not related to
bug #36676, and I have no idea what kind of comment may be
added near that. Of course I don't want to remove that line.

Thank you,
Gleb.