MDEV-22303: Incorrect ordering with REGEXP_REPLACE and OFFSET/LIMIT

For character sets and collation where character to weight mapping > 1, there we need to make sure while creating a sort key, a temporary buffer is created to store the value of the item by val_str function and then copy that value back to the sort buffer. In this case when using a priority queue Sort_param::tmp_buffer was not allocated. Minor refactoring: Changed Sort_param::tmp_buffer from char* to String
MariaDB · Jun 1, 2020 · ade8253 · ade8253
1 parent 132d582
commit ade8253
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 32 deletions.
diff --git a/mysql-test/main/order_by.result b/mysql-test/main/order_by.result
@@ -3866,3 +3866,22 @@ cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc
 dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd
 NULL
 DROP TABLE t1;
+#
+# MDEV-22303: Incorrect ordering with REGEXP_REPLACE and OFFSET/LIMIT
+#
+CREATE TABLE t1 (name VARCHAR(20) CHARACTER SET utf8 NOT NULL);
+INSERT INTO t1 (name) VALUES ('Charles Dickens'), ('Roald Dahl');
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC;
+name	surname_first
+Roald Dahl	Dahl, Roald
+Charles Dickens	Dickens, Charles
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC LIMIT 1;
+name	surname_first
+Roald Dahl	Dahl, Roald
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC LIMIT 1 OFFSET 1;
+name	surname_first
+Charles Dickens	Dickens, Charles
+DROP TABLE t1;
diff --git a/mysql-test/main/order_by.test b/mysql-test/main/order_by.test
@@ -2413,3 +2413,21 @@ insert into t1 select repeat('d', 256);
 
 SELECT IF( 0, NULL, a ) AS f FROM t1 GROUP BY f WITH ROLLUP;
 DROP TABLE t1;
+
+--echo #
+--echo # MDEV-22303: Incorrect ordering with REGEXP_REPLACE and OFFSET/LIMIT
+--echo #
+
+CREATE TABLE t1 (name VARCHAR(20) CHARACTER SET utf8 NOT NULL);
+INSERT INTO t1 (name) VALUES ('Charles Dickens'), ('Roald Dahl');
+
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC;
+
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC LIMIT 1;
+
+SELECT name, REGEXP_REPLACE(name, '^(.*) (.*)$', '\\2, \\1') AS surname_first FROM t1
+ORDER BY surname_first ASC LIMIT 1 OFFSET 1;
+
+DROP TABLE t1;
diff --git a/sql/filesort.cc b/sql/filesort.cc
@@ -58,7 +58,6 @@ static bool save_index(Sort_param *param, uint count,
                        SORT_INFO *table_sort);
 static uint suffix_length(ulong string_length);
 static uint sortlength(THD *thd, Sort_keys *sortorder,
-                       bool *multi_byte_charset,
                        bool *allow_packing_for_sortkeys);
 static Addon_fields *get_addon_fields(TABLE *table, uint sortlength,
                                       uint *addon_length,
@@ -200,7 +199,7 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort,
   ha_rows num_rows= HA_POS_ERROR;
   IO_CACHE tempfile, buffpek_pointers, *outfile; 
   Sort_param param;
-  bool multi_byte_charset, allow_packing_for_sortkeys;
+  bool allow_packing_for_sortkeys;
   Bounded_queue<uchar, uchar> pq;
   SQL_SELECT *const select= filesort->select;
   ha_rows max_rows= filesort->limit;
@@ -248,8 +247,7 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort,
   sort->found_rows= HA_POS_ERROR;
 
   param.sort_keys= sort_keys;
-  uint sort_len= sortlength(thd, sort_keys, &multi_byte_charset,
-                            &allow_packing_for_sortkeys);
+  uint sort_len= sortlength(thd, sort_keys, &allow_packing_for_sortkeys);
 
   param.init_for_filesort(sort_len, table, max_rows, filesort->sort_positions);
 
@@ -309,12 +307,6 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort,
     tracker->report_sort_keys_format(param.using_packed_sortkeys());
     param.using_pq= false;
 
-    if ((multi_byte_charset || param.using_packed_sortkeys()) &&
-        !(param.tmp_buffer= (char*) my_malloc(key_memory_Sort_param_tmp_buffer, param.sort_length,
-                                              MYF(MY_WME | MY_THREAD_SPECIFIC))))
-      goto err;
-
-
     size_t min_sort_memory= MY_MAX(MIN_SORT_MEMORY,
                                    param.sort_length*MERGEBUFF2);
     set_if_bigger(min_sort_memory, sizeof(Merge_chunk*)*MERGEBUFF2);
@@ -345,6 +337,9 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort,
     tracker->report_addon_fields_format(param.using_packed_addons());
   }
 
+  if (param.tmp_buffer.alloc(param.sort_length))
+    goto err;
+
   if (open_cached_file(&buffpek_pointers,mysql_tmpdir,TEMP_PREFIX,
 		       DISK_BUFFER_SIZE, MYF(MY_WME)))
     goto err;
@@ -438,7 +433,6 @@ SORT_INFO *filesort(THD *thd, TABLE *table, Filesort *filesort,
   error= 0;
 
   err:
-  my_free(param.tmp_buffer);
   if (!subselect || !subselect->is_uncacheable())
   {
     if (!param.using_addon_fields())
@@ -1101,10 +1095,8 @@ Type_handler_string_result::make_sort_key_part(uchar *to, Item *item,
 
   if (maybe_null)
     *to++= 1;
-  char *tmp_buffer= param->tmp_buffer ? param->tmp_buffer : (char*) to;
-  String tmp(tmp_buffer, param->tmp_buffer ? param->sort_length :
-                                             sort_field->length, cs);
-  String *res= item->str_result(&tmp);
+
+  String *res= item->str_result(&param->tmp_buffer);
   if (!res)
   {
     if (maybe_null)
@@ -2181,8 +2173,6 @@ Type_handler_decimal_result::sort_length(THD *thd,
   @param thd			  Thread handler
   @param sortorder		  Order of items to sort
   @param s_length	          Number of items to sort
-  @param[out] multi_byte_charset Set to 1 if we are using multi-byte charset
-                                 (In which case we have to use strxnfrm())
   @param allow_packing_for_sortkeys [out]  set to false if packing sort keys is not
                                      allowed
 
@@ -2196,11 +2186,9 @@ Type_handler_decimal_result::sort_length(THD *thd,
 */
 
 static uint
-sortlength(THD *thd, Sort_keys *sort_keys, bool *multi_byte_charset,
-           bool *allow_packing_for_sortkeys)
+sortlength(THD *thd, Sort_keys *sort_keys, bool *allow_packing_for_sortkeys)
 {
   uint length;
-  *multi_byte_charset= 0;
   *allow_packing_for_sortkeys= true;
   bool allow_packing_for_keys= true;
 
@@ -2226,10 +2214,8 @@ sortlength(THD *thd, Sort_keys *sort_keys, bool *multi_byte_charset,
       sortorder->cs= cs;
 
       if (use_strnxfrm((cs=sortorder->field->sort_charset())))
-      {
-        *multi_byte_charset= true;
         sortorder->length= (uint) cs->strnxfrmlen(sortorder->length);
-      }
+
       if (sortorder->is_variable_sized() && allow_packing_for_keys)
       {
         allow_packing_for_keys= sortorder->check_if_packing_possible(thd);
@@ -2243,17 +2229,12 @@ sortlength(THD *thd, Sort_keys *sort_keys, bool *multi_byte_charset,
     }
     else
     {
-      CHARSET_INFO *cs;
       sortorder->item->type_handler()->sort_length(thd, sortorder->item,
                                                    sortorder);
       sortorder->type= sortorder->item->type_handler()->is_packable() ?
                        SORT_FIELD_ATTR::VARIABLE_SIZE :
                        SORT_FIELD_ATTR::FIXED_SIZE;
-      if (use_strnxfrm((cs=sortorder->item->collation.collation)))
-      {
-        *multi_byte_charset= true;
-      }
-      sortorder->cs= cs;
+      sortorder->cs= sortorder->item->collation.collation;
       if (sortorder->is_variable_sized() && allow_packing_for_keys)
       {
         allow_packing_for_keys= sortorder->check_if_packing_possible(thd);
@@ -2565,8 +2546,7 @@ Type_handler_string_result::make_packed_sort_key_part(uchar *to, Item *item,
   if (maybe_null)
     *to++= 1;
 
-  String tmp(param->tmp_buffer, param->sort_length, cs);
-  String *res= item->str_result(&tmp);
+  String *res= item->str_result(&param->tmp_buffer);
   if (!res)
   {
     if (maybe_null)

diff --git a/sql/sql_sort.h b/sql/sql_sort.h
@@ -536,14 +536,15 @@ class Sort_param {
 
   uchar *unique_buff;
   bool not_killable;
-  char* tmp_buffer;
+  String tmp_buffer;
   // The fields below are used only by Unique class.
   qsort2_cmp compare;
   BUFFPEK_COMPARE_CONTEXT cmp_context;
 
   Sort_param()
   {
     memset(reinterpret_cast<void*>(this), 0, sizeof(*this));
+    tmp_buffer.set_thread_specific();
   }
   void init_for_filesort(uint sortlen, TABLE *table,
                          ha_rows maxrows, bool sort_positions);