maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Anshu!

On Jun 08, Anshu Avinash wrote:
> Hi all,
> 
> As per serg comments on the previous commit, I have modified the code to
> add an init function which will initialize all the cost factors at the
> start of the server. The latest code is at
> https://github.com/igniting/server/commits/selfTuningOptimizer. I have also
> attached the diff here.

Much better! Good work!
See my comments below.

> diff --git a/mysql-test/t/costmodel.test b/mysql-test/t/costmodel.test
> --- /dev/null
> +++ b/mysql-test/t/costmodel.test
> @@ -0,0 +1,37 @@
> +--disable_warnings
> +DROP DATABASE IF EXISTS world;
> +--enable_warnings
> +
> +CREATE DATABASE world;
> +use world;
> +
> +--source include/world_schema.inc
> +--disable_query_log
> +--disable_result_log
> +--disable_warnings
> +--source include/world.inc
> +--enable_query_log
> +--enable_result_log
> +--enable_warnings
> +
> +EXPLAIN
> +  SELECT c.name, ci.name FROM Country c, City ci
> +    WHERE c.capital=ci.id;
> +
> +use mysql;
> +
> +UPDATE optimizer_cost_factors
> +SET const_value=1000.0
> +WHERE const_name='READ_TIME_FACTOR';
> +
> +--enable_reconnect
> +--exec echo "restart" > $MYSQLTEST_VARDIR/tmp/mysqld.1.expect
> +--source include/wait_until_connected_again.inc
> +
> +use world;
> +
> +EXPLAIN
> +  SELECT c.name, ci.name FROM Country c, City ci
> +    WHERE c.capital=ci.id;
> +
> +DROP DATABASE world;

This is a very correctly written test file. Good!

But just to test whether READ_TIME_FACTOR and SCAN_TIME_FACTOR work,
I'd suggest to use a simpler test, not a world database, not a join,
but as I described in another email - one table, minimal structure,
one simple SELECT.

> diff --git a/scripts/mysql_system_tables.sql b/scripts/mysql_system_tables.sql
> --- a/scripts/mysql_system_tables.sql
> +++ b/scripts/mysql_system_tables.sql
> @@ -229,3 +229,10 @@ CREATE TABLE IF NOT EXISTS index_stats (db_name varchar(64) NOT NULL, table_name
>  -- we avoid mixed-engine transactions.
>  set storage_engine=@orig_storage_engine;
>  CREATE TABLE IF NOT EXISTS gtid_slave_pos (domain_id INT UNSIGNED NOT NULL, sub_id BIGINT UNSIGNED NOT NULL, server_id INT UNSIGNED NOT NULL, seq_no BIGINT UNSIGNED NOT NULL, PRIMARY KEY (domain_id, sub_id)) comment='Replication slave GTID position';
> +
> +-- Tables for Self Tuning Cost Optimizer
> +
> +CREATE TABLE IF NOT EXISTS optimizer_cost_factors (const_name varchar(64) NOT NULL, const_value double NOT NULL, PRIMARY KEY (const_name) ) ENGINE=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Constants for optimizer';

please, make it latin1, not utf8. We don't need to support utf8 names for cost factors :)

> +
> +-- Remember for later if all_constants table already existed
> +set @had_optimizer_cost_factors_table= @@warning_count != 0;
> diff --git a/scripts/mysql_system_tables_data.sql b/scripts/mysql_system_tables_data.sql
> --- a/scripts/mysql_system_tables_data.sql
> +++ b/scripts/mysql_system_tables_data.sql
> @@ -53,3 +53,9 @@ INSERT INTO tmp_proxies_priv VALUES ('localhost', 'root', '', '', TRUE, '', now(
>  REPLACE INTO tmp_proxies_priv SELECT @current_hostname, 'root', '', '', TRUE, '', now() FROM DUAL WHERE @current_hostname != 'localhost';
>  INSERT INTO  proxies_priv SELECT * FROM tmp_proxies_priv WHERE @had_proxies_priv_table=0;
>  DROP TABLE tmp_proxies_priv;
> +
> +CREATE TEMPORARY TABLE tmp_optimizer_cost_factors LIKE optimizer_cost_factors;
> +INSERT INTO tmp_optimizer_cost_factors VALUES ('READ_TIME_FACTOR', 1.0);
> +INSERT INTO tmp_optimizer_cost_factors VALUES ('SCAN_TIME_FACTOR', 1.0);
> +INSERT INTO optimizer_cost_factors SELECT * FROM tmp_optimizer_cost_factors WHERE @had_optimizer_cost_factors_table=0;
> +DROP TABLE tmp_optimizer_cost_factors;

Eh. I'd suggest to not use a temporary table and not use WHERE @had_optimizer_cost_factors_table
but simply

  INSERT INTO optimizer_cost_factors VALUES ('READ_TIME_FACTOR', 1.0);
  INSERT INTO optimizer_cost_factors VALUES ('SCAN_TIME_FACTOR', 1.0);

the difference is - if optimizer_cost_factors table already exists, your
code will insert *nothing at all*. But the one I suggest will insert
missing constants while keeping existing values intact. For example,
if the table optimizer_cost_factors exists, and has only one row
{ READ_TIME_FACTOR, 10.0 }. Then after your script it will still have
this one row, while after my suggested solution it'll be

  | READ_TIME_FACTOR | 10.0 |
  | SCAN_TIME_FACTOR | 1.0  |

which, I think, is better. If you'd like you can even use INSERT IGNORE to
ignore duplicate key errors. But they'll be ignored by the client anyway, so
it doesn't matter much in this case.

> diff --git a/sql/handler.h b/sql/handler.h
> --- a/sql/handler.h
> +++ b/sql/handler.h
> @@ -2741,7 +2742,8 @@ public:
>      reset_statistics();
>    }
>    virtual double scan_time()
> -  { return ulonglong2double(stats.data_file_length) / IO_SIZE + 2; }
> +  { return Cost_factors::get_scan_time_factor() *
> +            (ulonglong2double(stats.data_file_length) / IO_SIZE + 2); }

No, this is wrong. You use the scan factor *in the virtual method*,
which means that every storage engine that overwrite handler::scan_time()
will not use Cost_factors::get_scan_time_factor().

The correct solution - see ha_index_init, ha_rnd_next, etc. You make
handler::scan_time() a *protected* method. And create a public non-virtual
method handler::ha_scan_time(). Like this

double ha_scan_time()
{ return Cost_factors::scan_factor() * scan_time(); }

(above I've also renamed the get_scan_time_factor() to be a bit shorter)

>    /**
>       The cost of reading a set of ranges from the table using an index
> @@ -2755,7 +2757,7 @@ public:
>       using an index by calling it using read_time(index, 1, table_size).
>    */
>    virtual double read_time(uint index, uint ranges, ha_rows rows)
> -  { return rows2double(ranges+rows); }
> +  { return Cost_factors::get_read_time_factor() * rows2double(ranges+rows); }

same here. While you have your cost factors in the virtual method,
they won't have any effect on the optimizer, you should be able to
see it in the test case.

>  
>    /**
>      Calculate cost of 'keyread' scan for given index and number of records.
> diff --git a/sql/opt_costmodel.h b/sql/opt_costmodel.h
> --- /dev/null
> +++ b/sql/opt_costmodel.h
> @@ -0,0 +1,25 @@
> +/* Interface to get constants */
> +
> +#ifndef SQL_OPT_COSTMODEL_INCLUDED
> +#define SQL_OPT_COSTMODEL_INCLUDED
> +
> +class Cost_factors
> +{
> +private:
> +  static bool isInitialized;

please, try to stick to naming conventions that we have in MariaDB.
as you might've noticed, we don't use camelCase.

> +  static double read_time_factor;
> +  static double scan_time_factor;
> +
> +public:
> +  static void init();
> +  static inline double get_read_time_factor()
> +  {
> +    return read_time_factor;
> +  }
> +  static inline double get_scan_time_factor()
> +  {
> +    return scan_time_factor;
> +  }
> +};

Ok, so you've made everything static and never instantiated a single instance
of the Cost_factors class. I see. What benefits does this have as compared to
making nothing inside the class static, but creating one static Cost_factors
object?

> +
> +#endif /* SQL_OPT_COSTMODEL_INCLUDED */
> diff --git a/sql/opt_costmodel.cc b/sql/opt_costmodel.cc
> --- /dev/null
> +++ b/sql/opt_costmodel.cc
> @@ -0,0 +1,103 @@
> +#include "sql_base.h"
> +#include "key.h"
> +#include "records.h"
> +#include "opt_costmodel.h"
> +
> +/* Name of database to which the optimizer_cost_factors table belongs */
> +static const LEX_STRING db_name= { C_STRING_WITH_LEN("mysql") };
> +
> +/* Name of all_constants table */
> +static const LEX_STRING table_name = { C_STRING_WITH_LEN("optimizer_cost_factors") };
> +
> +/* Columns in the optimizer_cost_factors_table */
> +enum cost_factors_col
> +{
> +  COST_FACTORS_CONST_NAME,
> +  COST_FACTORS_CONST_VALUE
> +};
> +
> +/* Name of the constants present in table */
> +static const LEX_STRING read_time_factor_name = { C_STRING_WITH_LEN("READ_TIME_FACTOR") };
> +static const LEX_STRING scan_time_factor_name = { C_STRING_WITH_LEN("SCAN_TIME_FACTOR") };
> +
> +/* Helper functions for Cost_factors::init() */
> +
> +static
> +inline int open_table(THD *thd, TABLE_LIST *table,
> +                      Open_tables_backup *backup,
> +                      bool for_write)
> +{
> +  enum thr_lock_type lock_type_arg= for_write? TL_WRITE: TL_READ;
> +  table->init_one_table(db_name.str, db_name.length, table_name.str,
> +                        table_name.length, table_name.str, lock_type_arg);
> +  return open_system_tables_for_read(thd, table, backup);
> +}
> +
> +static inline void clean_up(THD *thd)
> +{
> +  close_mysql_tables(thd);
> +  delete thd;
> +}
> +
> +/* Initialize static class members */
> +bool Cost_factors::isInitialized= false;
> +double Cost_factors::read_time_factor= 1.0;
> +double Cost_factors::scan_time_factor= 1.0;
> +
> +/* Interface functions */
> +
> +void Cost_factors::init()
> +{
> +  TABLE_LIST table_list;
> +  Open_tables_backup open_tables_backup;
> +  READ_RECORD read_record_info;
> +  TABLE *table;
> +  MEM_ROOT mem;
> +  init_sql_alloc(&mem, 1024, 0, MYF(0));
> +  THD *new_thd = new THD;
> +  
> +  if(!new_thd)
> +  {
> +    free_root(&mem, MYF(0));
> +    DBUG_VOID_RETURN;
> +  }
> +
> +  new_thd->thread_stack= (char *) &new_thd;
> +  new_thd->store_globals();
> +  new_thd->set_db(db_name.str, db_name.length);
> +
> +  if(open_table(new_thd, &table_list, &open_tables_backup, FALSE))
> +  {
> +    clean_up(new_thd);
> +    DBUG_VOID_RETURN;

it's ok to do goto err: here and put the err: label at the end
with the cleanup code - this pattern is used very often in MariaDB

> +  }
> +
> +  table= table_list.table;
> +  if(init_read_record(&read_record_info, new_thd, table, NULL, 1, 0, FALSE))
> +  {
> +    clean_up(new_thd);
> +    DBUG_VOID_RETURN;
> +  }
> +
> +  table->use_all_columns();
> +  while (!read_record_info.read_record(&read_record_info))
> +  {
> +    LEX_STRING const_name;
> +    const_name.str= get_field(&mem, table->field[COST_FACTORS_CONST_NAME]);
> +    const_name.length= strlen(const_name.str);

you don't use const_name.length anywhere, you don't need LEX_STRING here.

> +
> +    double const_value;
> +    const_value= table->field[COST_FACTORS_CONST_VALUE]->val_real();
> +    if(!strcmp(const_name.str, read_time_factor_name.str))
> +    {
> +      Cost_factors::read_time_factor= const_value;
> +    }
> +    else if(!strcmp(const_name.str, scan_time_factor_name.str))
> +    {
> +      Cost_factors::scan_time_factor= const_value;
> +    }

To avoid many if/else if you can do like this:

  struct st_factor {
     const char *name;
     double *value;
  } factors = {
  { "READ_TIME_FACTOR", & Cost_factors::read_time_factor },
  { "SCAN_TIME_FACTOR", & Cost_factors::scan_time_factor },
  { 0, 0 }};

or even

  #define FACTOR(X)   { #X, &Cost_factors::X }
  struct st_factor {
     const char *name;
     double *value;
  } factors = {
      FACTOR(read_time_factor),
      FACTOR(scan_time_factor),
      { 0, 0 }};

either way, instead of if/elseif you do

  for (st_factor *f= factors; f->name; f++)
  {
    if (strcasecmp(f->name, const_name) == 0)
    {
      *(f->value)= const_value;
      break;
    }
  }
  if (f->name == 0)
    sql_print_warning("Invalid row in the optimizer_cost_factors table: %s", const_name);

> +  }
> +
> +  clean_up(new_thd);
> +  DBUG_VOID_RETURN;
> +}
Regards,
Sergei