Bug #29699759: ORDER BY WITH DISTINCT CAUSES OUT-OF-BOUNDS READ

When having two sorts on the same table with no temporary in-between
(e.g. first for DISTINCT, then for ORDER BY), they would both try to
store their buffers in table->sort.

Fix so that SortingIterator owns the buffers, so that the two sorts
have separate results; it doesn't really belong in TABLE anyway.

Change-Id: Ide5bfccc81e13dc657ef6c5f3ab36c3b2f5009c1

Author: Steinar H. Gunderson

sql/filesort.cc

@@ -94,6 +94,7 @@
 #include "sql/psi_memory_key.h"
 #include "sql/row_iterator.h"
 #include "sql/sort_param.h"
+#include "sql/sorting_iterator.h"
 #include "sql/sql_array.h"
 #include "sql/sql_base.h"
 #include "sql/sql_bitmap.h"
@@ -347,12 +348,13 @@ static void trace_filesort_information(Opt_trace_context *trace,
   in sorted order. This should be done with the functions
   in records.cc.
 
-  The result set is stored in table->sort.io_cache or
-  table->sort.sorted_result, or left in the main filesort buffer.
+  The result set is stored in fs_info->io_cache or
+  fs_info->sorted_result, or left in the main filesort buffer.
 
   @param      thd            Current thread
   @param      filesort       How to sort the table
   @param      source_iterator Where to read the rows to be sorted from.
+  @param      fs_info        Owns the buffers for sort_result.
   @param      sort_result    Where to store the sort result.
   @param[out] found_rows     Store the number of found rows here.
                              This is the number of found rows after
@@ -366,7 +368,8 @@ static void trace_filesort_information(Opt_trace_context *trace,
 */
 
 bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
-              Sort_result *sort_result, ha_rows *found_rows) {
+              Filesort_info *fs_info, Sort_result *sort_result,
+              ha_rows *found_rows) {
   int error;
   ulong memory_available = thd->variables.sortbuff_size;
   ha_rows num_rows_found = HA_POS_ERROR;
@@ -424,7 +427,7 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
                            table, thd->variables.max_length_for_sort_data,
                            max_rows, filesort->m_remove_duplicates);
 
-  table->sort.addon_fields = param->addon_fields;
+  fs_info->addon_fields = param->addon_fields;
 
   /*
     TODO: Now that we read from RowIterators, the situation is a lot more
@@ -457,7 +460,7 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
   // However, do note this cannot change the addon fields status,
   // so that we at least know that when checking whether we can skip
   // in-between temporary tables (StreamingIterator).
-  if (check_if_pq_applicable(trace, param, &table->sort, num_rows_estimate,
+  if (check_if_pq_applicable(trace, param, fs_info, num_rows_estimate,
                              memory_available)) {
     DBUG_PRINT("info", ("filesort PQ is applicable"));
     /*
@@ -466,19 +469,19 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
       all pointers here. (We cannot pack fields anyways, so there is no
       point in doing incremental allocation).
      */
-    if (table->sort.preallocate_records(param->max_rows_per_buffer)) {
+    if (fs_info->preallocate_records(param->max_rows_per_buffer)) {
       my_error(ER_OUT_OF_SORTMEMORY, ME_FATALERROR);
       LogErr(ERROR_LEVEL, ER_SERVER_OUT_OF_SORTMEMORY);
       goto err;
     }
 
-    if (pq.init(param->max_rows, param, table->sort.get_sort_keys())) {
+    if (pq.init(param->max_rows, param, fs_info->get_sort_keys())) {
       /*
        If we fail to init pq, we have to give up:
        out of memory means my_malloc() will call my_error().
       */
       DBUG_PRINT("info", ("failed to allocate PQ"));
-      table->sort.free_sort_buffer();
+      fs_info->free_sort_buffer();
       DBUG_ASSERT(thd->is_error());
       goto err;
     }
@@ -506,7 +509,7 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
     param->max_rows_per_buffer =
         min(num_rows_estimate > 0 ? num_rows_estimate : 1, keys);
 
-    table->sort.set_max_size(memory_available, param->max_record_length());
+    fs_info->set_max_size(memory_available, param->max_record_length());
   }
 
   param->sort_form = table;
@@ -515,10 +518,9 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
   // New scope, because subquery execution must be traced within an array.
   {
     Opt_trace_array ota(trace, "filesort_execution");
-    num_rows_found =
-        read_all_rows(thd, param, qep_tab, &table->sort, &chunk_file, &tempfile,
-                      param->using_pq ? &pq : nullptr, source_iterator,
-                      found_rows, &longest_key);
+    num_rows_found = read_all_rows(thd, param, qep_tab, fs_info, &chunk_file,
+                                   &tempfile, param->using_pq ? &pq : nullptr,
+                                   source_iterator, found_rows, &longest_key);
     if (num_rows_found == HA_POS_ERROR) goto err;
   }
 
@@ -536,7 +538,7 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
   {
     ha_rows rows_in_chunk =
         param->using_pq ? pq.num_elements() : num_rows_found;
-    if (save_index(param, rows_in_chunk, &table->sort, sort_result)) goto err;
+    if (save_index(param, rows_in_chunk, fs_info, sort_result)) goto err;
   } else {
     // If deduplicating, we'll need to remember the previous key somehow.
     if (filesort->m_remove_duplicates) {
@@ -545,12 +547,12 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
     }
 
     // We will need an extra buffer in SortFileIndirectIterator
-    if (table->sort.addon_fields != nullptr &&
-        !(table->sort.addon_fields->allocate_addon_buf(param->m_addon_length)))
+    if (fs_info->addon_fields != nullptr &&
+        !(fs_info->addon_fields->allocate_addon_buf(param->m_addon_length)))
       goto err; /* purecov: inspected */
 
-    table->sort.read_chunk_descriptors(&chunk_file, num_chunks);
-    if (table->sort.merge_chunks.is_null()) goto err; /* purecov: inspected */
+    fs_info->read_chunk_descriptors(&chunk_file, num_chunks);
+    if (fs_info->merge_chunks.is_null()) goto err; /* purecov: inspected */
 
     close_cached_file(&chunk_file);
 
@@ -562,23 +564,23 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
     if (reinit_io_cache(outfile, WRITE_CACHE, 0L, 0, 0)) goto err;
 
     param->max_rows_per_buffer = static_cast<uint>(
-        table->sort.max_size_in_bytes() / param->max_record_length());
+        fs_info->max_size_in_bytes() / param->max_record_length());
 
-    Bounds_checked_array<uchar> merge_buf = table->sort.get_contiguous_buffer();
+    Bounds_checked_array<uchar> merge_buf = fs_info->get_contiguous_buffer();
     if (merge_buf.array() == nullptr) {
       my_error(ER_OUT_OF_SORTMEMORY, ME_FATALERROR);
       LogErr(ERROR_LEVEL, ER_SERVER_OUT_OF_SORTMEMORY);
       goto err;
     }
-    if (merge_many_buff(thd, param, merge_buf, table->sort.merge_chunks,
+    if (merge_many_buff(thd, param, merge_buf, fs_info->merge_chunks,
                         &num_chunks, &tempfile))
       goto err;
     if (flush_io_cache(&tempfile) ||
         reinit_io_cache(&tempfile, READ_CACHE, 0L, 0, 0))
       goto err;
     if (merge_index(
             thd, param, merge_buf,
-            Merge_chunk_array(table->sort.merge_chunks.begin(), num_chunks),
+            Merge_chunk_array(fs_info->merge_chunks.begin(), num_chunks),
             &tempfile, outfile))
       goto err;
 
@@ -611,7 +613,7 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
         .add("num_rows_estimate", num_rows_estimate)
         .add("num_rows_found", num_rows_found)
         .add("num_initial_chunks_spilled_to_disk", num_initial_chunks)
-        .add("peak_memory_used", table->sort.peak_memory_used())
+        .add("peak_memory_used", fs_info->peak_memory_used())
         .add_alnum("sort_algorithm", algo_text[param->m_sort_algorithm]);
     if (!param->using_packed_addons())
       filesort_summary.add_alnum(
@@ -628,9 +630,9 @@ bool filesort(THD *thd, Filesort *filesort, RowIterator *source_iterator,
 
 err:
   if (!subselect || !subselect->is_uncacheable()) {
-    if (!sort_result->sorted_result_in_fsbuf) table->sort.free_sort_buffer();
-    my_free(table->sort.merge_chunks.array());
-    table->sort.merge_chunks = Merge_chunk_array(NULL, 0);
+    if (!sort_result->sorted_result_in_fsbuf) fs_info->free_sort_buffer();
+    my_free(fs_info->merge_chunks.array());
+    fs_info->merge_chunks = Merge_chunk_array(NULL, 0);
   }
   close_cached_file(&tempfile);
   close_cached_file(&chunk_file);
@@ -689,10 +691,12 @@ void filesort_free_buffers(TABLE *table, bool full) {
   table->unique_result.sorted_result_in_fsbuf = false;
 
   if (full) {
-    table->sort.free_sort_buffer();
-    my_free(table->sort.merge_chunks.array());
-    table->sort.merge_chunks = Merge_chunk_array(NULL, 0);
-    table->sort.addon_fields = NULL;
+    if (table->sorting_iterator != nullptr) {
+      table->sorting_iterator->CleanupAfterQuery();
+    }
+    if (table->duplicate_removal_iterator != nullptr) {
+      table->duplicate_removal_iterator->CleanupAfterQuery();
+    }
   }
 }
 

sql/filesort.h

@@ -93,7 +93,8 @@ class Filesort {
 };
 
 bool filesort(THD *thd, Filesort *fsort, RowIterator *source_iterator,
-              Sort_result *sort_result, ha_rows *found_rows);
+              Filesort_info *fs_info, Sort_result *sort_result,
+              ha_rows *found_rows);
 void filesort_free_buffers(TABLE *table, bool full);
 void change_double_for_sort(double nr, uchar *to);
 

sql/filesort_utils.h

@@ -39,31 +39,6 @@
 class Cost_model_table;
 class Sort_param;
 
-/*
-  Calculate cost of merge sort
-
-    @param num_rows            Total number of rows.
-    @param num_keys_per_buffer Number of keys per buffer.
-    @param elem_size           Size of each element.
-    @param cost_model          Cost model object that provides cost data.
-
-    Calculates cost of merge sort by simulating call to merge_many_buff().
-
-  @returns
-    Computed cost of merge sort in disk seeks.
-
-  @note
-    Declared here in order to be able to unit test it,
-    since library dependencies have not been sorted out yet.
-
-    See also comments get_merge_many_buffs_cost().
-*/
-
-double get_merge_many_buffs_cost_fast(ha_rows num_rows,
-                                      ha_rows num_keys_per_buffer,
-                                      uint elem_size,
-                                      const Cost_model_table *cost_model);
-
 /**
   Buffer used for storing records to be sorted. The records are stored in
   a series of buffers that are allocated incrementally, growing 50% each
@@ -218,7 +193,10 @@ class Filesort_buffer {
     Gets sorted record number ix. @see get_sort_keys()
     Only valid after buffer has been sorted!
   */
-  uchar *get_sorted_record(size_t ix) { return m_record_pointers[ix]; }
+  uchar *get_sorted_record(size_t ix) {
+    DBUG_ASSERT(ix < m_record_pointers.size());
+    return m_record_pointers[ix];
+  }
 
   /**
     Clears all rows, then returns a contiguous buffer of maximum size.

sql/sorting_iterator.cc

@@ -98,13 +98,7 @@ bool SortFileIndirectIterator::Init() {
     return true;
   }
 
-  /*
-    table->sort.addon_field is checked because if we use addon fields,
-    it doesn't make sense to use cache - we don't read from the table
-    and table->sort.io_cache is read sequentially
-  */
-  if (m_using_cache && !table()->sort.using_addon_fields() &&
-      thd()->variables.read_rnd_buff_size &&
+  if (m_using_cache && thd()->variables.read_rnd_buff_size &&
       !(table()->file->ha_table_flags() & HA_FAST_KEY_READ) &&
       (table()->db_stat & HA_READ_ONLY ||
        table()->reginfo.lock_type <= TL_READ_NO_INSERT) &&
@@ -468,7 +462,17 @@ SortingIterator::SortingIterator(THD *thd, Filesort *filesort,
       m_source_iterator(move(source)),
       m_examined_rows(examined_rows) {}
 
-SortingIterator::~SortingIterator() { ReleaseBuffers(); }
+SortingIterator::~SortingIterator() {
+  ReleaseBuffers();
+  CleanupAfterQuery();
+}
+
+void SortingIterator::CleanupAfterQuery() {
+  m_fs_info.free_sort_buffer();
+  my_free(m_fs_info.merge_chunks.array());
+  m_fs_info.merge_chunks = Merge_chunk_array(NULL, 0);
+  m_fs_info.addon_fields = NULL;
+}
 
 void SortingIterator::ReleaseBuffers() {
   m_result_iterator.reset();
@@ -480,6 +484,8 @@ void SortingIterator::ReleaseBuffers() {
   }
   m_sort_result.sorted_result.reset();
   m_sort_result.sorted_result_in_fsbuf = false;
+
+  // Keep the sort buffer in m_fs_info.
 }
 
 bool SortingIterator::Init() {
@@ -522,42 +528,47 @@ bool SortingIterator::Init() {
   TABLE *table = qep_tab->table();
   if (m_sort_result.io_cache && my_b_inited(m_sort_result.io_cache)) {
     // Test if ref-records was used
-    if (table->sort.using_addon_fields()) {
+    if (m_fs_info.using_addon_fields()) {
       DBUG_PRINT("info", ("using SortFileIterator"));
-      if (table->sort.addon_fields->using_packed_addons())
+      if (m_fs_info.addon_fields->using_packed_addons())
         m_result_iterator.reset(
             new (&m_result_iterator_holder.sort_file_packed_addons)
                 SortFileIterator<true>(thd(), table, m_sort_result.io_cache,
-                                       &table->sort, m_examined_rows));
+                                       &m_fs_info, m_examined_rows));
       else
         m_result_iterator.reset(
             new (&m_result_iterator_holder.sort_file)
                 SortFileIterator<false>(thd(), table, m_sort_result.io_cache,
-                                        &table->sort, m_examined_rows));
+                                        &m_fs_info, m_examined_rows));
     } else {
+      /*
+        m_fs_info->addon_field is checked because if we use addon fields,
+        it doesn't make sense to use cache - we don't read from the table
+        and m_fs_info->io_cache is read sequentially
+      */
+      bool request_cache = !m_fs_info.using_addon_fields();
       m_result_iterator.reset(
           new (&m_result_iterator_holder.sort_file_indirect)
-              SortFileIndirectIterator(thd(), table, m_sort_result.io_cache,
-                                       /*request_cache=*/true,
-                                       /*ignore_not_found_rows=*/false,
-                                       m_examined_rows));
+              SortFileIndirectIterator(
+                  thd(), table, m_sort_result.io_cache, request_cache,
+                  /*ignore_not_found_rows=*/false, m_examined_rows));
     }
     m_sort_result.io_cache =
         nullptr;  // The result iterator has taken ownership.
   } else {
     DBUG_ASSERT(m_sort_result.has_result_in_memory());
-    if (table->sort.using_addon_fields()) {
+    if (m_fs_info.using_addon_fields()) {
       DBUG_PRINT("info", ("using SortBufferIterator"));
       DBUG_ASSERT(m_sort_result.sorted_result_in_fsbuf);
-      if (table->sort.addon_fields->using_packed_addons())
+      if (m_fs_info.addon_fields->using_packed_addons())
         m_result_iterator.reset(
             new (&m_result_iterator_holder.sort_buffer_packed_addons)
-                SortBufferIterator<true>(thd(), table, &table->sort,
+                SortBufferIterator<true>(thd(), table, &m_fs_info,
                                          &m_sort_result, m_examined_rows));
       else
         m_result_iterator.reset(
             new (&m_result_iterator_holder.sort_buffer)
-                SortBufferIterator<false>(thd(), table, &table->sort,
+                SortBufferIterator<false>(thd(), table, &m_fs_info,
                                           &m_sort_result, m_examined_rows));
     } else {
       DBUG_PRINT("info", ("using SortBufferIndirectIterator (sort)"));
@@ -621,7 +632,7 @@ int SortingIterator::DoSort(QEP_TAB *qep_tab) {
   }
 
   ha_rows found_rows;
-  bool error = filesort(thd(), m_filesort, m_source_iterator.get(),
+  bool error = filesort(thd(), m_filesort, m_source_iterator.get(), &m_fs_info,
                         &m_sort_result, &found_rows);
   qep_tab->set_records(found_rows);  // For SQL_CALC_ROWS
   table->set_keyread(false);         // Restore if we used indexes

sql/sorting_iterator.h

@@ -91,6 +91,15 @@ class SortingIterator final : public RowIterator {
 
   std::vector<std::string> DebugString() const override;
 
+  /// Optional (when JOIN::destroy() runs, the iterator and its buffers
+  /// will be cleaned up anyway); used to clean up the buffers a little
+  /// bit earlier.
+  ///
+  /// When we get cached JOIN objects (prepare/optimize once) that can
+  /// live for a long time between queries, calling this will become more
+  /// important.
+  void CleanupAfterQuery();
+
  private:
   int DoSort(QEP_TAB *qep_tab);
   void ReleaseBuffers();
@@ -109,6 +118,9 @@ class SortingIterator final : public RowIterator {
   // using packed addons, etc..
   unique_ptr_destroy_only<RowIterator> m_result_iterator;
 
+  // Holds the buffers for m_sort_result.
+  Filesort_info m_fs_info;
+
   Sort_result m_sort_result;
 
   ha_rows *m_examined_rows;