Fix for bug#32426: "FEDERATED query returns corrupt results

 for ORDER BY on a TEXT or VARCHAR field" backported to 5.1.

mysql-test/suite/federated/federated.result

@@ -2153,6 +2153,29 @@ DROP TABLE t1;
 End of 5.0 tests
 create server 's1' foreign data wrapper 'mysql' options (port 3306);
 drop server 's1';
+#
+# Bug #32426: FEDERATED query returns corrupt results for ORDER BY on a TEXT
+#
+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@127.0.0.1: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;
 End of 5.1 tests
 SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
 SET @@GLOBAL.CONCURRENT_INSERT= @OLD_SLAVE_CONCURRENT_INSERT;

mysql-test/suite/federated/federated.test

@@ -1971,6 +1971,28 @@ connection default;
 create server 's1' foreign data wrapper 'mysql' options (port 3306);
 drop server 's1';
 
+
+--echo #
+--echo # Bug #32426: FEDERATED query returns corrupt results for ORDER BY on a TEXT
+--echo #
+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@127.0.0.1:$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;
+
+connection default;
+
+
 --echo End of 5.1 tests
 SET @@GLOBAL.CONCURRENT_INSERT= @OLD_MASTER_CONCURRENT_INSERT;
 connection slave;

storage/federated/ha_federated.cc

@@ -1621,11 +1621,10 @@ int ha_federated::open(const char *name, int mode, uint test_if_locked)
 
   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);
@@ -1645,21 +1644,17 @@ int ha_federated::open(const char *name, int mode, uint test_if_locked)
 
 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;
-  }
+  free_result();
+  
+  delete_dynamic(&results);
+  
   /* Disconnect from mysql */
   mysql_close(mysql);
   mysql= NULL;
-  retval= free_share(share);
-  DBUG_RETURN(retval);
 
+  DBUG_RETURN(free_share(share));
 }
 
 /*
@@ -2326,8 +2321,7 @@ int ha_federated::index_read(uchar *buf, const uchar *key,
 {
   DBUG_ENTER("ha_federated::index_read");
 
-  if (stored_result)
-    mysql_free_result(stored_result);
+  free_result();
   DBUG_RETURN(index_read_idx_with_result_set(buf, active_index, key,
                                              key_len, find_flag,
                                              &stored_result));
@@ -2359,7 +2353,8 @@ int ha_federated::index_read_idx(uchar *buf, uint index, const uchar *key,
                                               &mysql_result)))
     DBUG_RETURN(retval);
   mysql_free_result(mysql_result);
-  DBUG_RETURN(retval);
+  results.elements--;
+  DBUG_RETURN(0);
 }
 
 
@@ -2415,18 +2410,20 @@ int ha_federated::index_read_idx_with_result_set(uchar *buf, uint index,
     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)))
-    DBUG_RETURN(retval);
-
+  if ((retval= read_next(buf, *result)))
+  {
     mysql_free_result(*result);
+    results.elements--;
     *result= 0;
     table->status= STATUS_NOT_FOUND;
     DBUG_RETURN(retval);
+  }
+  DBUG_RETURN(0);
 
 error:
   table->status= STATUS_NOT_FOUND;
@@ -2486,12 +2483,6 @@ int ha_federated::read_range_first(const key_range *start_key,
   create_where_from_key(&sql_query,
                         &table->key_info[active_index],
                         start_key, end_key, 0, eq_range_arg);
-
-  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;
@@ -2499,14 +2490,13 @@ int ha_federated::read_range_first(const key_range *start_key,
   }
   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;
@@ -2516,10 +2506,8 @@ int ha_federated::read_range_first(const key_range *start_key,
 
 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]));
 }
 
 
@@ -2585,23 +2573,11 @@ 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)
-      goto error;
+    if (real_query(share->select_query, strlen(share->select_query)) ||
+        !(stored_result= store_result(mysql)))
+      DBUG_RETURN(stash_remote_error());
   }
   DBUG_RETURN(0);
-
-error:
-  DBUG_RETURN(stash_remote_error());
 }
 
 
@@ -2615,11 +2591,7 @@ 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;
-  }
+  free_result();
   active_index= MAX_KEY;
   DBUG_RETURN(0);
 }
@@ -2680,6 +2652,9 @@ int ha_federated::read_next(uchar *buf, MYSQL_RES *result)
 
   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);
@@ -2691,24 +2666,38 @@ int ha_federated::read_next(uchar *buf, MYSQL_RES *result)
 }
 
 
-/*
-  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 uchar *record)
+void ha_federated::position(const uchar *record __attribute__ ((unused)))
 {
   DBUG_ENTER("ha_federated::position");
-  if (table->s->primary_key != MAX_KEY)
-    key_copy(ref, (uchar *)record, table->key_info + table->s->primary_key,
-             ref_length);
-  else
-    memcpy(ref, record, ref_length);
+  
+  DBUG_ASSERT(stored_result);
+
+  position_called= TRUE;
+  /* 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;
 }
 
@@ -2724,23 +2713,19 @@ void ha_federated::position(const uchar *record)
 
 int ha_federated::rnd_pos(uchar *buf, uchar *pos)
 {
-  int result;
+  MYSQL_RES *result;
   DBUG_ENTER("ha_federated::rnd_pos");
+  
   ha_statistic_increment(&SSV::ha_read_rnd_count);
-  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));
 }
 
 
@@ -2943,6 +2928,16 @@ int ha_federated::reset(void)
   insert_dup_update= FALSE;
   ignore_duplicates= FALSE;
   replace_duplicates= FALSE;
+
+  /* Free stored result sets. */
+  for (uint i= 0; i < results.elements; i++)
+  {
+    MYSQL_RES *result;
+    get_dynamic(&results, (uchar *) &result, i);
+    mysql_free_result(result);
+  }
+  reset_dynamic(&results);
+
   return 0;
 }
 
@@ -3206,6 +3201,45 @@ bool ha_federated::get_error_message(int error, String* buf)
   DBUG_RETURN(FALSE);
 }
 
+
+/**
+  @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, (uchar*) &result);
+  }
+  position_called= FALSE;
+  DBUG_RETURN(result);
+}
+
+
+void ha_federated::free_result()
+{
+  DBUG_ENTER("ha_federated::free_result");
+  if (stored_result && !position_called)
+  {
+    mysql_free_result(stored_result);
+    stored_result= 0;
+    if (results.elements > 0)
+      results.elements--;
+  }
+  DBUG_VOID_RETURN;
+}
+
+ 
 int ha_federated::external_lock(THD *thd, int lock_type)
 {
   int error= 0;

storage/federated/ha_federated.h

@@ -84,6 +84,11 @@ 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;
+  bool position_called;
   uint fetch_num; // stores the fetch num
   MYSQL_ROW_OFFSET current_position;  // Current position used by ::position()
   int remote_error_number;
@@ -251,6 +256,10 @@ class ha_federated: public handler
   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);
+  
+  MYSQL_RES *store_result(MYSQL *mysql);
+  void free_result();
+  
   int external_lock(THD *thd, int lock_type);
   int connection_commit();
   int connection_rollback();