Re: bk commit into 5.0 tree (ramil:1.2602) BUG#32426

[Sergei Golubchik]

Hi!

On Dec 14, ramil@stripped wrote:
> ChangeSet@stripped, 2007-12-14 10:50:56+04:00, ramil@stripped +4 -0
>   Fix for bug #32426: "FEDERATED query returns corrupt results for
>   ORDER BY on a TEXT field"
>   
>   Problem: storing a reference to a row which is used in fielsort 
>   we miss BLOB/TEXT data.
>   
>   Fix: store a reference as current result set address and data 
>   cursor position.
> 
> @@ -2146,18 +2137,20 @@ int ha_federated::index_read_idx_with_re
>      goto error;
>    }
> -  if (!(retval= read_next(buf, *result)))
> +  if ((retval= read_next(buf, *result)))
> +  {
> +    mysql_free_result(*result);
> +    results.elements--;
> +    *result= 0;
> +    table->status= STATUS_NOT_FOUND;
>      DBUG_RETURN(retval);
> -
> -  mysql_free_result(*result);
> -  *result= 0;
> -  table->status= STATUS_NOT_FOUND;
> -  DBUG_RETURN(retval);
> +  }
> +  DBUG_RETURN(0);

why ?
  
>  error:
>    table->status= STATUS_NOT_FOUND;
    DBUG_RETURN(0);
> @@ -2404,24 +2379,35 @@ int ha_federated::read_next(byte *buf, M
>  
> +/**
> +  @brief      Store a reference to current row.
> +  
> +  @details    During a query execution we may have different result sets (RS),
> +              e.g. for different ranges. All the RS's used are stored in 
> +              memory and placed in @c results dynamic array. At the end of 
> +              execution all stored RS's are freed at once in the
> +              @c ha_federated::reset().
> +              So, in case of federated, a reference to current row is a 
> +              stored result address and current data cursor position.
> +              As we keep all RS in memory during a query execution,
> +              we can get any record using the reference any time until
> +              @c ha_federated::reset() is called.
> +              TODO: we don't have to store all RS's rows but only those
> +              we call @c ha_federated::position() for, so we can free memory
> +              where we store other rows in the @c ha_federated::index_end().

oh, you put it in TODO :(
Would it be difficult to do it now ?

Currently ha_federated already has the code to free result set
everywhere where necessary. In your patch you remove all that, and later
for this TOD you'll add it again, possibly missing a place or two, which
would introduce bugs, which would need to be fixed, and so on.
Safer and easier to build on what we have now.

> +              
> +  @param[in]  record  record data (unused)
>  */
>  
> -void ha_federated::position(const byte *record)
> +void ha_federated::position(const byte *record __attribute__ ((unused)))

Regards / Mit vielen Grüssen,
Sergei

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /   Sergei Golubchik <serg@stripped>
 / /|_/ / // /\ \/ /_/ / /__  Principal Software Developer
/_/  /_/\_, /___/\___\_\___/  MySQL GmbH, Dachauer Str. 37, D-80335 München
       <___/                  Geschäftsführer: Kaj Arnö - HRB
München 162140