From: Dmitry Lenev
Date: May 7 2011 5:26am
Subject: Re: bzr commit into mysql-5.5 branch (alexander.nozdrin:3458)
 Bug#12362125
List-Archive: http://lists.mysql.com/commits/136874
Message-Id: <20110507052650.GF16830@bandersnatch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii

Hello, Alik!

Here are my comments about your patch:

* Alexander Nozdrin <alexander.nozdrin@stripped> [11/05/06 13:43]:
> #At file:///home/alik/MySQL/bzr/00/bug12362125/mysql-5.5-bug12362125/ based on revid:jon.hauglid@stripped
> 
>  3458 Alexander Nozdrin	2011-05-06
>       Patch for Bug#12362125 (SP INOUT HANDLING IS BROKEN FOR TEXT TYPE).
>       
>       The problem was that BLOBs were not copied to the result table
>       from triggers.

I think the above description of the problem is a bit too vague and
needs to be clarified. How about replacing it with something like:

Attempt to assign valut to a table column from trigger by using
NEW.column_name pseudo-variable resulted in garbled data in cases
when the column had a BLOB-based type (e.g. TEXT) and value being
assigned was retrieved from stored routine variable of the same
type.

The problem was that BLOB values were not copied correctly in this
case. Instead of doing copy of a real value the value's representation
in record buffer was copied. This representation is essentially a
pointer to a buffer associate with virtual table for routine
variables where the real value is stored. Since this buffer got
freed once trigger was left or could have changed its contents when
new value was assigned to corresponding routine variable such a shallow
copying resulted in garbled data in NEW.colum_name column.

?
...


>       
>       It worked in 5.1 due to a subtle bug in create_virtual_tmp_table():
>         - in 5.1 create_virtual_tmp_table() returned a table which
>           had db_low_byte_first == false.
>         - in 5.5 and up create_virtual_tmp_table() returns a table which
>           has db_low_byte_first == true.
>       Actually, db_low_byte_first == false only for ISAM storage engine,
>       which was deprecated and removed in 5.0.
>       
>       Having db_low_byte_first == false led to getting false in the
>       complex condition for the 2nd "if" in field_conv(), which in turn
>       led to copy-blob-behavior as a fall-back strategy:
>         - to->table->s->db_low_byte_first was true (correct value)
>         - from->table->s->db_low_byte_first was false (incorrect value)
>       
>       In 5.5 and up that condition is true, which means blob-values are
>       not copied.

Really nice explanation! But I think that we also add a short sentence
explaining how we have solved this problem. Something like:

This patch solves this problem by forcing code in field_conv() perform
full-blob-copy. To achieve this we temporary set TABLE::copy_blobs flag
for trigger's subject table.

...

> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2011-04-08 13:15:23 +0000
> +++ b/sql/item.cc	2011-05-06 09:32:32 +0000
> @@ -7131,8 +7131,26 @@ bool Item_trigger_field::set_value(THD *
>  {
>    Item *item= sp_prepare_func_item(thd, it);
>  
> -  return (!item || (!fixed && fix_fields(thd, 0)) ||
> -          (item->save_in_field(field, 0) < 0));
> +  if (!item)
> +    return true;
> +
> +  if (!fixed)
> +  {
> +    if (fix_fields(thd, NULL))
> +      return true;
> +  }
> +
> +  // NOTE: field->table->copy_blobs should be false here, but let's
> +  // remember the value at runtime to avoid subtle bugs.

AFAIU our coding style still recommends to use /* ... */ style of comment
for multi-line comments. Am I wrong?

> +  bool copy_blobs_saved= field->table->copy_blobs;
> +
> +  field->table->copy_blobs= true;
> +
> +  int err_code= item->save_in_field(field, 0);
> +
> +  field->table->copy_blobs= copy_blobs_saved;
> +
> +  return err_code < 0;
>  }

Otherwise, I am OK with your patch and think that it can be
pushed after considering the above comments.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification