From: Date: August 15 2008 2:18pm Subject: bzr commit into mysql-6.0-bugteam branch (ramil:2767) Bug#32426 List-Archive: http://lists.mysql.com/commits/51725 X-Bug: 32426 Message-Id: <200808151218.m7FCICIw011156@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit #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 . - 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 *), ¤t_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();