List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:September 29 2010 9:24am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3518)
Bug#56423
View as plain text  
Martin,

The patch looks good, but I have some suggestions. See comments inline

On 09/24/2010 04:46 PM, Martin Hansson wrote:
> #At file:///data0/martin/bzr/bug56423/5.1bt/ based on
> revid:davi.arnaut@stripped
>
>   3518 Martin Hansson	2010-09-24
>        Bug#56423: Different count with SELECT and CREATE SELECT queries
>
>        This is a regression from the fix for bug no 38999. A storage engine capable
>        of reading only a subset of a table's columns updates corresponding bits in
>        the read buffer to signal that it has read NULL values for the corresponding
>        columns. It cannot, and should not, update any other bits. Bug no 38999
>        occurred because the implementation of UPDATE statements compare the NULL
> bits
>        using memcmp, inadvertently comparing bits that were never requested from the
>        storage engine. The regression was caused by the storage engine trying to
>        alleviate the situation by writing to all NULL bits, even those that it had
> no
>        knowledge of. This has devastating effects for the index merge algorithm,
>        which relies on all NULL bits, except those explicitly requested, being left
>        unchanged.
>
>        The fix reverts the fix for bug no 38999 in both InnoDB and InnoDB plugin and
>        changes the server's method of comparing records. For engines that always
> read
>        entire rows, we proceed as usual. For engines capable of reading only select
>        columns, the record buffers are now compared on a column by column basis. An
>        assertion was also added so that non comparable buffers are never read. Some
>        relevant copy-pasted code was also consolidated in a new function.
>
>      modified:
>        mysql-test/include/index_merge2.inc
>        mysql-test/r/index_merge_innodb.result
>        mysql-test/r/index_merge_myisam.result
>        sql/mysql_priv.h
>        sql/sql_insert.cc
>        sql/sql_update.cc
>        storage/innobase/row/row0sel.c
>        storage/innodb_plugin/row/row0sel.c
> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2010-08-09 11:39:59 +0000
> +++ b/sql/sql_update.cc	2010-09-24 14:45:38 +0000
> @@ -25,10 +25,56 @@
> +/**
> +   Compares the input and outbut record buffers of the table to see if a row
> +   has changed. The algorithm iterates over updated columns and if they are
> +   nullable compares NULL bits in the buffer before comparing actual
> +   data. Special care must be taken to compare only the relevant NULL bits and
> +   mask out all others as they may be undefined. The storage engine will not
> +   and should not touch them.
> +
> +   @param table The table to evaluate.
> +
> +   @return false if row hasn't changed.
> +   @return true otherwise.

Isn't it easier to read "@return true if row has changed @return false otherwise"?

> +*/
> +bool compare_records(const TABLE *table)
>   {
> +  DBUG_ASSERT(records_are_comparable(table));
> +
> +  if ((table->file->ha_table_flags()&  HA_PARTIAL_COLUMN_READ) != 0)
> +  {
> +    for (Field **ptr= table->field ; *ptr != NULL; ptr++)
> +    {
> +      Field *field= *ptr;
> +      if (bitmap_is_set(table->write_set, field->field_index))
> +      {
> +        if (field->real_maybe_null())
> +        {
> +          uchar null_byte_index= field->null_ptr - table->record[0];
> +
> +          if (((table->record[0][null_byte_index])&  field->null_bit) !=
> +              ((table->record[1][null_byte_index])&  field->null_bit))
> +            return TRUE;
> +        }
> +        if (field->cmp_binary_offset(table->s->rec_buff_length))
> +          return TRUE;
> +      }
> +    }
> +    return FALSE;
> +  }
> +

Suggested changes (feel free to ignore): "Special care must be taken to compare 
only the relevant NULL bits and mask out all others as they may be undefined." 
-> "Special care is taken to not compare NULL bits of columns not read by the 
storage engine." ("only relevant bits" requires the reader to be in context when 
reading the doc, and that shouldn't be necessary)

and additionaly add something similar to the comments below. Also, I find it 
better to make it explicit that only one of the three parts of the code
will be executed, so I suggest this:

if (...HA_PARTIAL_COLUMN_READ != 0)
{
   /* Storage engine has not read all columns of the record.
      Fields (including NULL bits) not in the write_set may not
      have been read and can therefore not be compared.
   */
...
}
else if (table->s->blob_fields + table->s->varchar_fields == 0)
   // Fixed-size record: do bitwise comparison of the records
   ...
else
{
   /* Compare null bits. The storage engine has read all columns,
      so it's safe to compare all bits including those not in the
      write_set. This is cheaper than the bit-by-bit comparison
      done above
   */
   if (table->s->blob_fields...)
   ...
}
return FALSE;

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3518) Bug#56423Martin Hansson24 Sep
Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3518)Bug#56423Jorgen Loland29 Sep