From: Jon Olav Hauglid
Date: June 30 2011 9:34am
Subject: Re: bzr commit into mysql-trunk branch (magnus.blaudd:3187) WL#5906
List-Archive: http://lists.mysql.com/commits/140132
Message-Id: <4E0C432F.30302@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit


Hello,

On 06/10/2011 12:26 PM, magnus.blaudd@stripped wrote:
>   3187 magnus.blaudd@stripped	2011-06-10
>        WL#5906 read before write removal (RBWR)
>         - Make it possible for storage engines to optimize away
>           read of record before UPDATE/DELETE in order to save
>           roundtrips and allow for batching.

General comments:
- I suggest asking Svoj for second review since the handler interface
   is changed.
- Please use Doxygen comments for the functions you've added.
- There's a whole lot of DBUG_PRINT(), are they all really needed?

> === modified file 'sql/handler.h'
> --- a/sql/handler.h	2011-06-01 09:11:28 +0000
> +++ b/sql/handler.h	2011-06-10 10:24:29 +0000
> @@ -1796,6 +1796,28 @@ public:
>     virtual int extra_opt(enum ha_extra_function operation, ulong cache_size)
>     { return extra(operation); }
>
> +  /*
> +    Informs the handler if this handler support read removal
> +    (could use table_flags, but patch is smaller this way)
> +   */
 > +
> +  virtual bool read_before_write_removal_supported(void) const
> +  { return false;

I have some problems understanding the comment for this function.
"Check if handler supports read before write removal"?

> +  /*
> +    Informs handler that it is possible to optimise away the real read
> +    operation from the handler for the current table and instead
> +    use a generated read to optimise simple UPDATE and DELETEs.
> +  */
 > +
> +  virtual bool read_before_write_removal_possible(void)
> +  { return false; }

The responsibility of this function compared to 
read_before_write_removal_supported() is a bit unclear to me.
Why does this function return bool and not void?
Or alternately, are both functions needed?

optimise => optimize
UPDATE => UPDATEs

> +  /*
> +    Return the number of rows the handler has written while using
> +    read before write removal
> +   */
> +  virtual ha_rows read_before_write_removal_rows_written(void) const
> +  { DBUG_ASSERT(0); return (ha_rows) 0; }

One statement per line.

> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc	2011-05-26 15:20:09 +0000
> +++ b/sql/sql_delete.cc	2011-06-10 10:24:29 +0000
> @@ -59,6 +59,7 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>     bool          const_cond_result;
>     ha_rows	deleted= 0;
>     bool          reverse= FALSE;
> +  bool          read_removal= false;
>     bool          skip_record;
>     ORDER *order= (ORDER *) ((order_list&&  order_list->elements) ?
>                              order_list->first : NULL);
> @@ -293,6 +294,31 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>     else
>       will_batch= !table->file->start_bulk_delete();
>
> +  /*
> +    Read removal is possible if the selected quick read
> +    method is using full unique index
> +  */
> +  if (select&&  select->quick&&
> +      will_batch&&
> +      !using_limit&&
> +      table->file->read_before_write_removal_supported())

What about DELETE IGNORE? According to the WL-description, RBWR should 
not be used with IGNORE.

> +  {
> +    const uint idx = select->quick->index;
> +    DBUG_PRINT("rbwr", ("checking index: %d", idx));
> +    const KEY *key= table->key_info + idx;
> +    if ((key->flags&  HA_NOSAME) == HA_NOSAME)

"if (key->flags & HA_NOSAME)", just to be consistent with how this
check is generally written.

> +    {
> +      DBUG_PRINT("rbwr", ("index is unique"));
> +      bitmap_clear_all(&table->tmp_set);
> +      table->mark_columns_used_by_index_no_reset(idx,&table->tmp_set);
> +      if (bitmap_cmp(&table->tmp_set, table->read_set))
> +      {
> +        DBUG_PRINT("rbwr", ("using whole index, rbwr possible"));
> +        read_removal=
> +          table->file->read_before_write_removal_possible();
> +      }
> +    }
> +  }

Consider extracting the above check to a separate function.

> @@ -355,6 +381,13 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>         table->file->print_error(loc_error,MYF(0));
>       error=1;
>     }
> +  if (read_removal)
> +  {
> +    /* Only handler knows how many records really was written */

was => were

> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2011-06-09 08:58:41 +0000
> +++ b/sql/sql_update.cc	2011-06-10 10:24:29 +0000
> @@ -158,6 +158,37 @@ static bool check_fields(THD *thd, List<
>   }
>
>
> +/*
> +  Check if all expressions in list are constant expressions
> +
> +  SYNOPSIS
> +    check_constant_expressions()
> +    values                       List of expressions
> +
> +  RETURN
> +    TRUE                         Only constant expressions
> +    FALSE                        At least one non-constant expression
> +*/
> +
> +static bool check_constant_expressions(List<Item>  &values)
> +{
> +  Item *value;
> +  List_iterator_fast<Item>  v(values);
> +  DBUG_ENTER("check_constant_expressions");
> +
> +  while ((value= v++))
> +  {
> +    if (!value->const_item())
> +    {
> +      DBUG_PRINT("exit", ("expression is not constant"));
> +      DBUG_RETURN(FALSE);

false

> +    }
> +  }
> +  DBUG_PRINT("exit", ("expression is constant"));
> +  DBUG_RETURN(TRUE);

true

> +}
> +
> +
>   /**
>     Re-read record if more columns are needed for error message.
>
> @@ -423,6 +454,36 @@ int mysql_update(THD *thd,
>       DBUG_RETURN(0);
>     }
>
> +  /*
> +    Read removal is possible if the selected quick read
> +    method is using full unique index
> +
> +    NOTE! table->read_set currently holds the columns which are
> +    used for the WHERE clause(this info is most likely already
> +    available in select->quick, but where?)
> +  */
> +  bool read_removal= false;
> +  if (select&&  select->quick&&
> +      !ignore&&
> +      !using_limit&&
> +      table->file->read_before_write_removal_supported())
> +  {
> +    const uint idx= select->quick->index;
> +    DBUG_PRINT("rbwr", ("checking index: %d", idx));
> +    const KEY *key= table->key_info + idx;
> +    if ((key->flags&  HA_NOSAME) == HA_NOSAME)
> +    {
> +      DBUG_PRINT("rbwr", ("index is unique"));
> +      bitmap_clear_all(&table->tmp_set);
> +      table->mark_columns_used_by_index_no_reset(idx,&table->tmp_set);
> +      if (bitmap_cmp(&table->tmp_set, table->read_set))
> +      {
> +        DBUG_PRINT("rbwr", ("using full index, rbwr possible"));
> +        read_removal= true;
> +      }
> +    }
> +  }

It would be good if the logic for determining read_removal could be 
combined and extracted to a separate function.

I know code below changes table->read_set, but in that case read_removal 
is turned off anyway, right?

>     /* If running in safe sql mode, don't allow updates without keys */
>     if (table->quick_keys.is_clear_all())
>     {
> @@ -595,6 +656,10 @@ int mysql_update(THD *thd,
>       }
>       if (table->key_read)
>         table->restore_column_maps_after_mark_index();
> +
> +    /* Rows are already read ->  not possible to remove */
> +    DBUG_PRINT("rbwr", ("rows are already read, turning off rbwr"));
> +    read_removal= false;
>     }
>
>     if (ignore)
> @@ -634,6 +699,14 @@ int mysql_update(THD *thd,
>     else
>       will_batch= !table->file->start_bulk_update();
>
> +  if (read_removal&&
> +      will_batch&&
> +      check_constant_expressions(values))
> +  {
> +    assert(select&&  select->quick);

DBUG_ASSERT ?

--- Jon Olav