MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:ramil Date:March 26 2008 10:37am
Subject:bk commit into 5.0 tree (ramil:1.2600) BUG#32426
View as plain text  
Below is the list of changes that have just been committed into a local
5.0 repository of ramil.  When ramil 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, 2008-03-26 14:37:47+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, 2008-03-26 14:37:45+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, 2008-03-26 14:37:45+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, 2008-03-26 14:37:45+04:00, ramil@stripped +121 -81
    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.

  sql/ha_federated.h@stripped, 2008-03-26 14:37:45+04:00, ramil@stripped +10 -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 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.

diff -Nrup a/mysql-test/r/federated.result b/mysql-test/r/federated.result
--- a/mysql-test/r/federated.result	2008-03-20 19:07:16 +04:00
+++ b/mysql-test/r/federated.result	2008-03-26 14:37:45 +04:00
@@ -2071,6 +2071,26 @@ DROP TABLE t1;
 DROP TABLE t1;
 CREATE TABLE t1 (a INT) ENGINE=federated CONNECTION='mysql://@:://';
 DROP TABLE t1;
+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	2008-03-20 19:07:16 +04:00
+++ b/mysql-test/t/federated.test	2008-03-26 14:37:45 +04:00
@@ -1745,4 +1745,22 @@ DROP TABLE t1;
 CREATE TABLE t1 (a INT) ENGINE=federated CONNECTION='mysql://@:://';
 DROP TABLE t1;
 
+#
+# 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	2008-03-20 19:07:16 +04:00
+++ b/sql/ha_federated.cc	2008-03-26 14:37:45 +04:00
@@ -1404,11 +1404,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);
@@ -1428,21 +1427,16 @@ 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));
 }
 
 /*
@@ -2079,8 +2073,7 @@ int ha_federated::index_read(byte *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));
@@ -2112,7 +2105,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);
 }
 
 
@@ -2169,18 +2163,21 @@ 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;
@@ -2240,12 +2237,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;
@@ -2253,14 +2244,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;
@@ -2270,10 +2260,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]));
 }
 
 
@@ -2340,23 +2328,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());
 }
 
 
@@ -2370,11 +2346,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);
 }
@@ -2433,6 +2405,9 @@ int ha_federated::read_next(byte *buf, M
   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)))
@@ -2445,24 +2420,38 @@ 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.
+/**
+  @brief      Store a reference to current row.
 
-  In case of federated, a reference is either a primary key or
-  the whole record.
+  @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().
 
-  Called from filesort.cc, sql_select.cc, sql_delete.cc and sql_update.cc.
+  @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);
+
+  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;
 }
 
@@ -2478,24 +2467,20 @@ 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));
 }
 
 
@@ -2943,6 +2928,61 @@ 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));
+  }
+  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;
 }
 
 #endif /* HAVE_FEDERATED_DB */
diff -Nrup a/sql/ha_federated.h b/sql/ha_federated.h
--- a/sql/ha_federated.h	2007-11-30 17:07:58 +04:00
+++ b/sql/ha_federated.h	2008-03-26 14:37:45 +04:00
@@ -154,6 +154,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;
@@ -187,9 +192,7 @@ 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"; }
   /*
@@ -318,6 +321,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);
+  
+  int reset();
+  MYSQL_RES *store_result(MYSQL *mysql);
+  void free_result();
 };
 
 bool federated_db_init(void);
Thread
bk commit into 5.0 tree (ramil:1.2600) BUG#32426ramil26 Mar
  • Re: bk commit into 5.0 tree (ramil:1.2600) BUG#32426Sergei Golubchik29 Apr