MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Ramil Kalimullin Date:August 15 2008 12:18pm
Subject:bzr commit into mysql-6.0-bugteam branch (ramil:2767) Bug#32426
View as plain text  
#At file:///home/ram/mysql/mysql-6.0-bugteam/

 2767 Ramil Kalimullin	2008-08-15
        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.
modified:
  mysql-test/r/federated.result
  mysql-test/t/federated.test
  storage/federated/ha_federated.cc
  storage/federated/ha_federated.h

per-file messages:
  storage/federated/ha_federated.cc
        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 if position() 
              was called.
            - 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.
=== modified file 'mysql-test/r/federated.result'
--- a/mysql-test/r/federated.result	2008-05-01 10:31:23 +0000
+++ b/mysql-test/r/federated.result	2008-08-15 12:18:06 +0000
@@ -2121,6 +2121,27 @@ DROP TABLE t1;
 create server 's1' foreign data wrapper 'mysql' options (port 3306);
 drop server 's1';
 End of 5.1 tests
+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;
+End of 6.0 tests
 DROP TABLE IF EXISTS federated.t1;
 DROP DATABASE IF EXISTS federated;
 DROP TABLE IF EXISTS federated.t1;

=== modified file 'mysql-test/t/federated.test'
--- a/mysql-test/t/federated.test	2008-05-01 10:31:23 +0000
+++ b/mysql-test/t/federated.test	2008-08-15 12:18:06 +0000
@@ -1864,6 +1864,28 @@ create server 's1' foreign data wrapper 
 drop server 's1';
 
 --echo End of 5.1 tests
+
+
+#
+# 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;
+
+--echo End of 6.0 tests
+
 source include/federated_cleanup.inc;
 
 connection default;

=== modified file 'storage/federated/ha_federated.cc'
--- a/storage/federated/ha_federated.cc	2008-07-18 12:23:25 +0000
+++ b/storage/federated/ha_federated.cc	2008-08-15 12:18:06 +0000
@@ -1618,11 +1618,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);
@@ -1642,21 +1641,17 @@ 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;
-  }
+  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));
 }
 
 /*
@@ -2323,8 +2318,7 @@ int ha_federated::index_read(uchar *buf,
 {
   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));
@@ -2356,7 +2350,8 @@ int ha_federated::index_read_idx(uchar *
                                               &mysql_result)))
     DBUG_RETURN(retval);
   mysql_free_result(mysql_result);
-  DBUG_RETURN(retval);
+  results.elements--;
+  DBUG_RETURN(0);
 }
 
 
@@ -2412,18 +2407,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;
@@ -2483,12 +2480,6 @@ int ha_federated::read_range_first(const
   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;
@@ -2496,14 +2487,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;
@@ -2513,10 +2503,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]));
 }
 
 
@@ -2582,23 +2570,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());
 }
 
 
@@ -2612,11 +2588,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);
 }
@@ -2676,6 +2648,9 @@ int ha_federated::read_next(uchar *buf, 
   DBUG_ENTER("ha_federated::read_next");
 
   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)))
@@ -2688,24 +2663,38 @@ int ha_federated::read_next(uchar *buf, 
 }
 
 
-/*
-  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;
 }
 
@@ -2721,23 +2710,19 @@ void ha_federated::position(const uchar 
 
 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));
 }
 
 
@@ -2942,6 +2927,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;
 }
 
@@ -3205,6 +3200,45 @@ bool ha_federated::get_error_message(int
   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;

=== modified file 'storage/federated/ha_federated.h'
--- a/storage/federated/ha_federated.h	2008-05-29 15:33:33 +0000
+++ b/storage/federated/ha_federated.h	2008-08-15 12:18:06 +0000
@@ -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 @@ 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);
+  
+  MYSQL_RES *store_result(MYSQL *mysql);
+  void free_result();
+  
   int external_lock(THD *thd, int lock_type);
   int connection_commit();
   int connection_rollback();

Thread
bzr commit into mysql-6.0-bugteam branch (ramil:2767) Bug#32426Ramil Kalimullin15 Aug