MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:ramil Date:December 14 2007 6:50am
Subject:bk commit into 5.0 tree (ramil:1.2602) BUG#32426
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of ram. When ram does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

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.

  mysql-test/r/federated.result@stripped, 2007-12-14 10:50:54+04:00, ramil@stripped +20 -0
    Fix for bug #32426: "FEDERATED query returns corrupt results for
    ORDER BY on a TEXT field"
      - test result.

  mysql-test/t/federated.test@stripped, 2007-12-14 10:50:54+04:00, ramil@stripped +18 -0
    Fix for bug #32426: "FEDERATED query returns corrupt results for
    ORDER BY on a TEXT field"
      - test case.

  sql/ha_federated.cc@stripped, 2007-12-14 10:50:54+04:00, ramil@stripped +97 -77
    Fix for bug #32426: "FEDERATED query returns corrupt results for
    ORDER BY on a TEXT field"
      - place all used during a query execution result sets in the "results"
        dynamic array, don't free them during query execution.
      - use as a reference <result set address, data cursor position>.
      - free result sets used at once at the very end of a query execution,
        see ::reset().
      - code cleanup.

  sql/ha_federated.h@stripped, 2007-12-14 10:50:54+04:00, ramil@stripped +9 -3
    Fix for bug #32426: "FEDERATED query returns corrupt results for
    ORDER BY on a TEXT field"
      - place all used during a query execution result sets in the "results"
        dynamic array, don't free them during query execution.
      - use as a reference <result set address, data cursor position>.
      - free result sets used at once at the very end of a query execution,
        see ::reset().
      - code cleanup.

diff -Nrup a/mysql-test/r/federated.result b/mysql-test/r/federated.result
--- a/mysql-test/r/federated.result	2007-08-08 12:34:30 +05:00
+++ b/mysql-test/r/federated.result	2007-12-14 10:50:54 +04:00
@@ -1934,6 +1934,26 @@ select * from federated.t2;
 a
 1
 drop table federated.t1, federated.t2;
+CREATE TABLE federated.t1(a TEXT);
+INSERT INTO federated.t1 VALUES('abc'), ('gh'), ('f'), ('ijk'), ('de');
+CREATE TABLE federated.t1(a TEXT) ENGINE=FEDERATED
+CONNECTION='mysql://root@stripped:SLAVE_PORT/federated/t1';
+SELECT * FROM federated.t1 ORDER BY A;
+a
+abc
+de
+f
+gh
+ijk
+SELECT * FROM federated.t1 ORDER BY A DESC;
+a
+ijk
+gh
+f
+de
+abc
+DROP TABLE federated.t1;
+DROP TABLE federated.t1;
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
 DROP TABLE IF EXISTS federated.t1;
diff -Nrup a/mysql-test/t/federated.test b/mysql-test/t/federated.test
--- a/mysql-test/t/federated.test	2007-08-08 12:34:30 +05:00
+++ b/mysql-test/t/federated.test	2007-12-14 10:50:54 +04:00
@@ -1686,4 +1686,22 @@ insert into federated.t1 (a) values (1);
 select * from federated.t2;
 drop table federated.t1, federated.t2;
 
+#
+# Bug #32426: FEDERATED query returns corrupt results for ORDER BY on a TEXT
+#
+connection slave;
+CREATE TABLE federated.t1(a TEXT);
+INSERT INTO federated.t1 VALUES('abc'), ('gh'), ('f'), ('ijk'), ('de');
+
+connection master;
+--replace_result $SLAVE_MYPORT SLAVE_PORT
+eval CREATE TABLE federated.t1(a TEXT) ENGINE=FEDERATED
+  CONNECTION='mysql://root@stripped:$SLAVE_MYPORT/federated/t1';
+SELECT * FROM federated.t1 ORDER BY A;
+SELECT * FROM federated.t1 ORDER BY A DESC;
+DROP TABLE federated.t1;
+
+connection slave;
+DROP TABLE federated.t1;
+
 source include/federated_cleanup.inc;
diff -Nrup a/sql/ha_federated.cc b/sql/ha_federated.cc
--- a/sql/ha_federated.cc	2007-10-23 14:27:08 +05:00
+++ b/sql/ha_federated.cc	2007-12-14 10:50:54 +04:00
@@ -1381,11 +1381,10 @@ int ha_federated::open(const char *name,
 
   DBUG_ASSERT(mysql == NULL);
 
-  ref_length= (table->s->primary_key != MAX_KEY ?
-               table->key_info[table->s->primary_key].key_length :
-               table->s->reclength);
+  ref_length= sizeof(MYSQL_RES *) + sizeof(MYSQL_ROW_OFFSET);
   DBUG_PRINT("info", ("ref_length: %u", ref_length));
 
+  my_init_dynamic_array(&results, sizeof(MYSQL_RES *), 4, 4);
   reset();
 
   DBUG_RETURN(0);
@@ -1405,21 +1404,14 @@ int ha_federated::open(const char *name,
 
 int ha_federated::close(void)
 {
-  int retval;
   DBUG_ENTER("ha_federated::close");
 
-  /* free the result set */
-  if (stored_result)
-  {
-    mysql_free_result(stored_result);
-    stored_result= 0;
-  }
+  delete_dynamic(&results);
+
   /* Disconnect from mysql */
   mysql_close(mysql);
   mysql= NULL;
-  retval= free_share(share);
-  DBUG_RETURN(retval);
-
+  DBUG_RETURN(free_share(share));
 }
 
 /*
@@ -2056,8 +2048,6 @@ int ha_federated::index_read(byte *buf, 
 {
   DBUG_ENTER("ha_federated::index_read");
 
-  if (stored_result)
-    mysql_free_result(stored_result);
   DBUG_RETURN(index_read_idx_with_result_set(buf, active_index, key,
                                              key_len, find_flag,
                                              &stored_result));
@@ -2089,7 +2079,8 @@ int ha_federated::index_read_idx(byte *b
                                               &mysql_result)))
     DBUG_RETURN(retval);
   mysql_free_result(mysql_result);
-  DBUG_RETURN(retval);
+  results.elements--;
+  DBUG_RETURN(0);
 }
 
 
@@ -2146,18 +2137,20 @@ int ha_federated::index_read_idx_with_re
     retval= ER_QUERY_ON_FOREIGN_DATA_SOURCE;
     goto error;
   }
-  if (!(*result= mysql_store_result(mysql)))
+  if (!(*result= store_result(mysql)))
   {
     retval= HA_ERR_END_OF_FILE;
     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);
 
 error:
   table->status= STATUS_NOT_FOUND;
@@ -2199,12 +2192,6 @@ int ha_federated::read_range_first(const
   create_where_from_key(&sql_query,
                         &table->key_info[active_index],
                         start_key, end_key, 0);
-
-  if (stored_result)
-  {
-    mysql_free_result(stored_result);
-    stored_result= 0;
-  }
   if (real_query(sql_query.ptr(), sql_query.length()))
   {
     retval= ER_QUERY_ON_FOREIGN_DATA_SOURCE;
@@ -2212,14 +2199,13 @@ int ha_federated::read_range_first(const
   }
   sql_query.length(0);
 
-  if (!(stored_result= mysql_store_result(mysql)))
+  if (!(stored_result= store_result(mysql)))
   {
     retval= HA_ERR_END_OF_FILE;
     goto error;
   }
 
-  retval= read_next(table->record[0], stored_result);
-  DBUG_RETURN(retval);
+  DBUG_RETURN(read_next(table->record[0], stored_result));
 
 error:
   table->status= STATUS_NOT_FOUND;
@@ -2229,10 +2215,8 @@ error:
 
 int ha_federated::read_range_next()
 {
-  int retval;
   DBUG_ENTER("ha_federated::read_range_next");
-  retval= rnd_next(table->record[0]);
-  DBUG_RETURN(retval);
+  DBUG_RETURN(rnd_next(table->record[0]));
 }
 
 
@@ -2299,17 +2283,10 @@ int ha_federated::rnd_init(bool scan)
 
   if (scan)
   {
-    if (stored_result)
-    {
-      mysql_free_result(stored_result);
-      stored_result= 0;
-    }
-
     if (real_query(share->select_query, strlen(share->select_query)))
       goto error;
 
-    stored_result= mysql_store_result(mysql);
-    if (!stored_result)
+    if (!(stored_result= store_result(mysql)))
       goto error;
   }
   DBUG_RETURN(0);
@@ -2329,11 +2306,6 @@ int ha_federated::rnd_end()
 int ha_federated::index_end(void)
 {
   DBUG_ENTER("ha_federated::index_end");
-  if (stored_result)
-  {
-    mysql_free_result(stored_result);
-    stored_result= 0;
-  }
   active_index= MAX_KEY;
   DBUG_RETURN(0);
 }
@@ -2393,6 +2365,9 @@ int ha_federated::read_next(byte *buf, M
 
   table->status= STATUS_NOT_FOUND;              // For easier return
 
+  /* Save current data cursor position. */
+  current_position= result->data_cursor;
+  
   /* Fetch a row, insert it back in a row format. */
   if (!(row= mysql_fetch_row(result)))
     DBUG_RETURN(HA_ERR_END_OF_FILE);
@@ -2404,24 +2379,35 @@ int ha_federated::read_next(byte *buf, M
 }
 
 
-/*
-  store reference to current row so that we can later find it for
-  a re-read, update or delete.
-
-  In case of federated, a reference is either a primary key or
-  the whole record.
-
-  Called from filesort.cc, sql_select.cc, sql_delete.cc and sql_update.cc.
+/**
+  @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().
+              
+  @param[in]  record  record data (unused)
 */
 
-void ha_federated::position(const byte *record)
+void ha_federated::position(const byte *record __attribute__ ((unused)))
 {
   DBUG_ENTER("ha_federated::position");
-  if (table->s->primary_key != MAX_KEY)
-    key_copy(ref, (byte *)record, table->key_info + table->s->primary_key,
-             ref_length);
-  else
-    memcpy(ref, record, ref_length);
+  DBUG_ASSERT(stored_result);
+  /* Store result set address. */
+  memcpy_fixed(ref, &stored_result, sizeof(MYSQL_RES *));
+  /* Store data cursor position. */
+  memcpy_fixed(ref + sizeof(MYSQL_RES *), &current_position,
+               sizeof(MYSQL_ROW_OFFSET));
   DBUG_VOID_RETURN;
 }
 
@@ -2437,24 +2423,19 @@ void ha_federated::position(const byte *
 
 int ha_federated::rnd_pos(byte *buf, byte *pos)
 {
-  int result;
+  MYSQL_RES *result;
   DBUG_ENTER("ha_federated::rnd_pos");
   statistic_increment(table->in_use->status_var.ha_read_rnd_count,
                       &LOCK_status);
-  if (table->s->primary_key != MAX_KEY)
-  {
-    /* We have a primary key, so use index_read_idx to find row */
-    result= index_read_idx(buf, table->s->primary_key, pos,
-                           ref_length, HA_READ_KEY_EXACT);
-  }
-  else
-  {
-    /* otherwise, get the old record ref as obtained in ::position */
-    memcpy(buf, pos, ref_length);
-    result= 0;
-  }
-  table->status= result ? STATUS_NOT_FOUND : 0;
-  DBUG_RETURN(result);
+
+  /* Get stored result set. */
+  memcpy_fixed(&result, pos, sizeof(MYSQL_RES *));
+  DBUG_ASSERT(result);
+  /* Set data cursor position. */
+  memcpy_fixed(&result->data_cursor, pos + sizeof(MYSQL_RES *),
+               sizeof(MYSQL_ROW_OFFSET));
+  /* Read a row. */
+  DBUG_RETURN(read_next(buf, result));  
 }
 
 
@@ -2902,6 +2883,45 @@ bool ha_federated::get_error_message(int
   }
   DBUG_PRINT("exit", ("message: %s", buf->ptr()));
   DBUG_RETURN(FALSE);
+}
+
+
+int ha_federated::reset()
+{
+  DBUG_ENTER("ha_federated::reset");
+  /* Free stored result sets. */
+  for (uint i= 0; i < results.elements; i++)
+  {
+    MYSQL_RES *result;
+    get_dynamic(&results, (gptr) &result, i);
+    mysql_free_result(result);
+  }
+  reset_dynamic(&results);
+  
+  DBUG_RETURN(extra(HA_EXTRA_RESET));
+}
+
+
+/**
+  @brief      Store a result set.
+  
+  @details    Call @c mysql_store_result() to save a result set then
+              append it to the stored results array.
+  
+  @param[in]  mysql  MySLQ connection structure.
+  
+  @return     Stored result set (MYSQL_RES object).
+*/
+
+MYSQL_RES *ha_federated::store_result(MYSQL *mysql)
+{
+  MYSQL_RES *result= mysql_store_result(mysql);
+  DBUG_ENTER("ha_federated::store_result");
+  if (result)
+  {
+    VOID(insert_dynamic(&results, (gptr) &result));
+  }
+  DBUG_RETURN(result);
 }
 
 #endif /* HAVE_FEDERATED_DB */
diff -Nrup a/sql/ha_federated.h b/sql/ha_federated.h
--- a/sql/ha_federated.h	2007-08-02 05:13:02 +05:00
+++ b/sql/ha_federated.h	2007-12-14 10:50:54 +04:00
@@ -154,6 +154,10 @@ class ha_federated: public handler
   FEDERATED_SHARE *share;    /* Shared lock info */
   MYSQL *mysql; /* MySQL connection */
   MYSQL_RES *stored_result;
+  /**
+    Array of all stored results we get during a query execution.
+  */
+  DYNAMIC_ARRAY results;
   uint fetch_num; // stores the fetch num
   MYSQL_ROW_OFFSET current_position;  // Current position used by ::position()
   int remote_error_number;
@@ -187,9 +191,8 @@ private:
   int real_connect();
 public:
   ha_federated(TABLE *table_arg);
-  ~ha_federated()
-  {
-  }
+  ~ha_federated() {}
+
   /* The name that will be used for display purposes */
   const char *table_type() const { return "FEDERATED"; }
   /*
@@ -317,6 +320,9 @@ public:
   THR_LOCK_DATA **store_lock(THD *thd, THR_LOCK_DATA **to,
                              enum thr_lock_type lock_type);     //required
   bool get_error_message(int error, String *buf);
+
+  int reset();  
+  MYSQL_RES *store_result(MYSQL *mysql);
 };
 
 bool federated_db_init(void);
Thread
bk commit into 5.0 tree (ramil:1.2602) BUG#32426ramil14 Dec
  • Re: bk commit into 5.0 tree (ramil:1.2602) BUG#32426Sergei Golubchik19 Dec