Thread safe histograms loadingbb-10.1-svoj-MDEV-19061

Previously multiple threads were allowed to load histograms concurrently.
There were no known problems caused by this. But given amount of data
races in this code, it'd happen sooner or later.

To avoid scalability bottleneck, histograms loading is protected by
per-TABLE_SHARE atomic variable.

Whenever histograms were loaded by preceding statement (hot-path), a
scalable load-acquire check is performed.

Whenever histograms have to be loaded anew, mutual exclusion for loaders
is established by atomic variable. If histograms are being loaded
concurrently, statement waits until load is completed.

- Table_statistics::total_hist_size moved to TABLE_STATISTICS_CB: only
  meaningful within TABLE_SHARE (not used for collected stats).
- TABLE_STATISTICS_CB::histograms_can_be_read and
  TABLE_STATISTICS_CB::histograms_are_read are replaced with a tri state
  atomic variable.
- Simplified away alloc_histograms_for_table_share().

Note: there's still likely a data race if a thread attempts accessing
histograms data after it failed to load it (because of concurrent load).
It was there previously and goes out of the scope of this effort. One way
of fixing it could be reviving TABLE::histograms_are_read and adding
appropriate checks whenever it is needed.

Part of MDEV-19061 - table_share used for reading statistical tables is
                     not protected

4 files changed, 34 insertions, 103 deletions

diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc
index a99dcca89e4..ae02254c745 100644
--- a/sql/sql_statistics.cc
+++ b/sql/sql_statistics.cc
@@ -2281,78 +2281,6 @@ static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share)
 
 
 /**
-  @brief 
-  Allocate memory for the histogram used by a table share
-
-  @param
-  thd         Thread handler
-  @param
-  table_share Table share for which the memory for histogram data is allocated
-  @param
-  is_safe     TRUE <-> at any time only one thread can perform the function
-
-  @note
-  The function allocates the memory for the histogram built for a table in the
-  table's share memory with the intention to read the data there from the
-  system persistent statistical table mysql.column_stats,
-  The memory is allocated in the table_share's mem_root.
-  If the parameter is_safe is TRUE then it is guaranteed that at any given time
-  only one thread is executed the code of the function.
-
-  @retval
-  0     If the memory for all statistical data has been successfully allocated  
-  @retval
-  1     Otherwise
-
-  @note
-  Currently the function always is called with the parameter is_safe set
-  to FALSE. 
-*/      
-
-static
-int alloc_histograms_for_table_share(THD* thd, TABLE_SHARE *table_share, 
-                                     bool is_safe)
-{
-  TABLE_STATISTICS_CB *stats_cb= &table_share->stats_cb;
-
-  DBUG_ENTER("alloc_histograms_for_table_share");
-
-  if (!is_safe)
-    mysql_mutex_lock(&table_share->LOCK_share);
-
-  if (stats_cb->histograms_can_be_read)
-  {
-    if (!is_safe)
-      mysql_mutex_unlock(&table_share->LOCK_share);
-    DBUG_RETURN(0);
-  }
-
-  Table_statistics *table_stats= stats_cb->table_stats;
-  ulong total_hist_size= table_stats->total_hist_size;
-
-  if (total_hist_size && !table_stats->histograms)
-  {
-    uchar *histograms= (uchar *) alloc_root(&stats_cb->mem_root,
-                                            total_hist_size);
-    if (!histograms)
-    {
-      if (!is_safe)
-        mysql_mutex_unlock(&table_share->LOCK_share);
-      DBUG_RETURN(1);
-    }
-    memset(histograms, 0, total_hist_size);
-    table_stats->histograms= histograms;
-    stats_cb->histograms_can_be_read= TRUE;
-  }
-
-  if (!is_safe)
-    mysql_mutex_unlock(&table_share->LOCK_share);
-
-  DBUG_RETURN(0);
-
-}
-
-/**
   @brief
   Initialize the aggregation fields to collect statistics on a column
 
@@ -2925,7 +2853,7 @@ int read_statistics_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables)
     column_stat.get_stat_values();
     total_hist_size+= table_field->read_stats->histogram.get_size();
   }
-  read_stats->total_hist_size= total_hist_size;
+  table_share->stats_cb.total_hist_size= total_hist_size;
 
   /* Read statistics from the statistical table index_stats */
   stat_table= stat_tables[INDEX_STAT].table;
@@ -3059,26 +2987,25 @@ void delete_stat_values_for_table_share(TABLE_SHARE *table_share)
 static
 int read_histograms_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables)
 {
-  TABLE_SHARE *table_share= table->s;
-  
+  TABLE_STATISTICS_CB *stats_cb= &table->s->stats_cb;
   DBUG_ENTER("read_histograms_for_table");
 
-  if (!table_share->stats_cb.histograms_can_be_read)
+  if (stats_cb->start_histograms_load())
   {
-    (void) alloc_histograms_for_table_share(thd, table_share, FALSE);
-  }
-  if (table_share->stats_cb.histograms_can_be_read &&
-      !table_share->stats_cb.histograms_are_read)
-  {
-    Field **field_ptr;
-    uchar *histogram= table_share->stats_cb.table_stats->histograms;
-    TABLE *stat_table= stat_tables[COLUMN_STAT].table;
-    Column_stat column_stat(stat_table, table);
-    for (field_ptr= table_share->field; *field_ptr; field_ptr++)
+    uchar *histogram= (uchar *) alloc_root(&stats_cb->mem_root,
+                                           stats_cb->total_hist_size);
+    if (!histogram)
+    {
+      stats_cb->abort_histograms_load();
+      DBUG_RETURN(1);
+    }
+    memset(histogram, 0, stats_cb->total_hist_size);
+
+    Column_stat column_stat(stat_tables[COLUMN_STAT].table, table);
+    for (Field **field_ptr= table->s->field; *field_ptr; field_ptr++)
     {
       Field *table_field= *field_ptr;
-      uint hist_size= table_field->read_stats->histogram.get_size();
-      if (hist_size)
+      if (uint hist_size= table_field->read_stats->histogram.get_size())
       {
         column_stat.set_key_fields(table_field);
         table_field->read_stats->histogram.set_values(histogram);
@@ -3086,8 +3013,9 @@ int read_histograms_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables)
         histogram+= hist_size;
       }
     }
+    stats_cb->end_histograms_load();
   }
- 
+  table->histograms_are_read= true;
   DBUG_RETURN(0);
 }
 
@@ -3178,8 +3106,8 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables)
           if (!tl->table->stats_is_read)
             dump_stats_from_share_to_table(tl->table);
           tl->table->histograms_are_read=
-            table_share->stats_cb.histograms_are_read;
-          if (table_share->stats_cb.histograms_are_read ||
+            table_share->stats_cb.histograms_are_ready();
+          if (table_share->stats_cb.histograms_are_ready() ||
               thd->variables.optimizer_use_condition_selectivity <= 3)
             continue;
         }
@@ -3217,14 +3145,8 @@ int read_statistics_for_tables(THD *thd, TABLE_LIST *tables)
         else
           continue;
       }
-      if (thd->variables.optimizer_use_condition_selectivity > 3 &&
-          !table_share->stats_cb.histograms_are_read)
-      {
+      if (thd->variables.optimizer_use_condition_selectivity > 3)
         (void) read_histograms_for_table(thd, tl->table, stat_tables);
-        table_share->stats_cb.histograms_are_read= TRUE;
-      }
-      if (table_share->stats_cb.histograms_are_read)
-        tl->table->histograms_are_read= TRUE;
     }
   }
 
diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h
index 71d727eab07..151618ca365 100644
--- a/sql/sql_statistics.h
+++ b/sql/sql_statistics.h
@@ -280,7 +280,6 @@ public:
   Column_statistics *column_stats;  /* Array of statistical data for columns */
   Index_statistics *index_stats;    /* Array of statistical data for indexes */
   ulong *idx_avg_frequency;   /* Array of records per key for index prefixes */
-  ulong total_hist_size;            /* Total size of all histograms */
   uchar *histograms;                /* Sequence of histograms       */                    
 };
 
diff --git a/sql/table.cc b/sql/table.cc
index 1d3bebcc5d5..20fc86e35f0 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -416,8 +416,6 @@ void TABLE_SHARE::destroy()
 
   delete_stat_values_for_table_share(this);
   free_root(&stats_cb.mem_root, MYF(0));
-  stats_cb.histograms_can_be_read= FALSE;
-  stats_cb.histograms_are_read= FALSE;
 
   /* The mutexes are initialized only for shares that are part of the TDC */
   if (tmp_table == NO_TMP_TABLE)
diff --git a/sql/table.h b/sql/table.h
index 85ba7b29ce2..93795113fab 100644
--- a/sql/table.h
+++ b/sql/table.h
@@ -659,13 +659,25 @@ class TABLE_STATISTICS_CB
   };
 
   class Statistics_state stats_state;
+  class Statistics_state hist_state;
 
 public:
   MEM_ROOT  mem_root; /* MEM_ROOT to allocate statistical data for the table */
   Table_statistics *table_stats; /* Structure to access the statistical data */
-  bool histograms_can_be_read;
-  bool histograms_are_read;   
+  ulong total_hist_size;         /* Total size of all histograms */
 
+  bool histograms_are_ready() const
+  {
+    return !total_hist_size || hist_state.is_ready();
+  }
+
+  bool start_histograms_load()
+  {
+    return total_hist_size && hist_state.start_load();
+  }
+
+  void end_histograms_load() { hist_state.end_load(); }
+  void abort_histograms_load() { hist_state.abort_load(); }
   bool stats_are_ready() const { return stats_state.is_ready(); }
   bool start_stats_load() { return stats_state.start_load(); }
   void end_stats_load() { stats_state.end_load(); }