maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

On Apr 26, Jan Lindström wrote:
> revision-id: 607e22e1426ef3d7e11e3a2e9d7f884d36cf3573 (mariadb-10.0.24-43-g607e22e)
> parent(s): ce38adddfa91949b30956abd0e4cab201effcdef
> committer: Jan Lindström
> timestamp: 2016-04-26 13:47:30 +0300
> message:
> 
> MDEV-8633: information_schema.index_statistics doesn't delete
> item when drop table indexes or drop table;
> 
> Problem was that table and index statistics is removed from
> persistent tables but not from memory cache. Added functions
> to remove table and index statistics from memory cache.

That's of course, ok. Just minor comments, see below:

> diff --git a/sql/sql_show.cc b/sql/sql_show.cc
> index b8926b9..ac4503d 100644
> --- a/sql/sql_show.cc
> +++ b/sql/sql_show.cc
> @@ -3442,6 +3442,100 @@ int fill_schema_table_stats(THD *thd, TABLE_LIST *tables, COND *cond)
>    DBUG_RETURN(0);
>  }
>  
> +/* Remove all indexes from global index statistics */

"all indexes for a given table"

> +static
> +int del_global_index_stats(THD *thd, uchar* cache_key, uint cache_key_length)
> +{
> +  int res = 0;
> +  DBUG_ENTER("del_global_index_stats");
> +
> +  mysql_mutex_lock(&LOCK_global_index_stats);
> +
> +  for (uint i= 0; i < global_index_stats.records && !res;)
> +  {
> +    INDEX_STATS *index_stats =
> +      (INDEX_STATS*) my_hash_element(&global_index_stats, i);
> +
> +    /* We search correct db\0table_name\0 string */
> +    if (index_stats &&
> +	index_stats->index_name_length >= cache_key_length &&
> +	!memcmp(index_stats->index, cache_key, cache_key_length))
> +    {
> +      res= my_hash_delete(&global_index_stats, (uchar*)index_stats);
> +      /*
> +          In our HASH implementation on deletion one elements
> +          is moved into a place where a deleted element was,
> +          and the last element is moved into the empty space.
> +          Thus we need to re-examine the current element, but
> +          we don't have to restart the search from the beginning.
> +      */
> +    }
> +    else
> +      i++;
> +  }
> +
> +  mysql_mutex_unlock(&LOCK_global_index_stats);
> +  DBUG_RETURN(res);
> +}
> +
> +/* Remove a table from global table statistics */
> +
> +int del_global_table_stat(THD *thd, LEX_STRING *db, LEX_STRING *table)
> +{
> +  TABLE_STATS *table_stats;
> +  int res = 0;
> +  uchar *cache_key;
> +  uint cache_key_length;
> +  DBUG_ENTER("del_global_TABLE_stat");

correct letter case: "del_global_table_stat".

> +
> +  cache_key_length= db->length + 1 + table->length + 1;
> +
> +  if(!(cache_key= (uchar *)my_malloc(cache_key_length,
> +                                     MYF(MY_WME | MY_ZEROFILL))))
> +  {
> +    /* Out of memory error already given */
> +    res = 1;
> +    goto end;
> +  }
> +
> +  memcpy(cache_key, db->str, db->length);
> +  memcpy(cache_key + db->length + 1, table->str, table->length);
> +
> +  res= del_global_index_stats(thd, cache_key, cache_key_length);
> +
> +  mysql_mutex_lock(&LOCK_global_table_stats);
> +
> +  if(!res && (table_stats= (TABLE_STATS*) my_hash_search(&global_table_stats,

here and below you, probably, don't want if (!res) - if the table is
dropped, you should delete as many its entries as possible, even if
something fails for an unknown reason.

> +                                                cache_key,
> +                                                cache_key_length)))
> +    res= my_hash_delete(&global_table_stats, (uchar*)table_stats);
> +
> +  my_free(cache_key);
> +  mysql_mutex_unlock(&LOCK_global_table_stats);
> +
> +end:
> +  DBUG_RETURN(res);
> +}
> +
> +/* Remove a index from global index statistics */
> +
> +int del_global_index_stat(THD *thd, TABLE* table, KEY* key_info)


The difference is del_global_index_stat and del_global_index_stats is
rather subtle. Could you rename them to make it clearer?

For example, you can rename the static one to
del_global_index_stats_for_table.

> +{
> +  INDEX_STATS *index_stats;
> +  uint key_length= table->s->table_cache_key.length + key_info->name_length + 1;
> +  int res = 0;
> +  DBUG_ENTER("del_global_index_stat");
> +  mysql_mutex_lock(&LOCK_global_index_stats);
> +
> +  if((index_stats= (INDEX_STATS*) my_hash_search(&global_index_stats,
> +                                                key_info->cache_name,
> +                                                key_length)))
> +    res= my_hash_delete(&global_index_stats, (uchar*)index_stats);
> +
> +  mysql_mutex_unlock(&LOCK_global_index_stats);
> +  DBUG_RETURN(res);
> +}
>  
>  /* Fill information schema table with index statistics */
>  
> diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc
> index e86c840..644ecf2 100644
> --- a/sql/sql_statistics.cc
> +++ b/sql/sql_statistics.cc
> @@ -29,6 +29,7 @@
>  #include "sql_statistics.h"
>  #include "opt_range.h"
>  #include "my_atomic.h"
> +#include "sql_show.h"
>  
>  /*
>    The system variable 'use_stat_tables' can take one of the
> @@ -3193,6 +3194,9 @@ int delete_statistics_for_table(THD *thd, LEX_STRING *db, LEX_STRING *tab)
>        rc= 1;
>    }
>  
> +  if (!rc)

again here and below, better to do it without if (!rc)

> +    rc= del_global_table_stat(thd, db, tab);
> +
>    thd->restore_stmt_binlog_format(save_binlog_format);
>  
>    close_system_tables(thd, &open_tables_backup);
> @@ -3339,6 +3343,9 @@ int delete_statistics_for_index(THD *thd, TABLE *tab, KEY *key_info,
>      }
>    }
>  
> +  if (!rc)
> +    rc= del_global_index_stat(thd, tab, key_info);
> +
>    thd->restore_stmt_binlog_format(save_binlog_format);
>  
Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx