maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi,

Summary:
1. Three questions (are you sure, what guarantee, why)
2. A couple of suggestions (spilt a commit, move function call out of
   DBUG_RETURN)
3. Two suggestions for unrelated optimizations (don't allocate unneded
   bitmaps, don't modify read_set)

> commit 2734eca05d499bcbcaf428d04d4f1acbb298589f
> Author: Monty <monty@xxxxxxxxxxx>
> Date:   Mon Nov 2 11:52:06 2015 +0200
> 
> diff --git a/sql/table.h b/sql/table.h
> index b150e4f..6ecfdef 100644
> --- a/sql/table.h
> +++ b/sql/table.h
> @@ -1066,9 +1066,11 @@ struct TABLE
>    String        alias;                    /* alias or table name */
>    uchar         *null_flags;
>    MY_BITMAP     def_read_set, def_write_set, def_vcol_set, tmp_set; 
> +  MY_BITMAP     def_rpl_write_set;

I would rather not allocate more bitmaps directly in the TABLE.
They are not always needed, and only increase the TABLE size.
The same is true for def_vcol_set, in fact, it's *even more true* for vcols.
Instead I'd allocate them in the TABLE's memroot as needed.

>    MY_BITMAP     eq_join_set;         /* used to mark equi-joined fields */
>    MY_BITMAP     cond_set;   /* used to mark fields from sargable conditions*/
> -  MY_BITMAP     *read_set, *write_set, *vcol_set; /* Active column sets */
> +  /* Active column sets */
> +  MY_BITMAP     *read_set, *write_set, *vcol_set, *rpl_write_set;
>    /*
>     The ID of the query that opened and is using this table. Has different
>     meanings depending on the table type.
> diff --git a/sql/table.cc b/sql/table.cc
> index 76cb4b8..8816ee0 100644
> --- a/sql/table.cc
> +++ b/sql/table.cc
> @@ -2884,20 +2884,28 @@ enum open_frm_error open_table_from_share(THD *thd, TABLE_SHARE *share,
>    /* Allocate bitmaps */
>  
>    bitmap_size= share->column_bitmap_size;
> -  if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*6)))
> +  if (!(bitmaps= (uchar*) alloc_root(&outparam->mem_root, bitmap_size*7)))
>      goto err;
>    my_bitmap_init(&outparam->def_read_set,
>                (my_bitmap_map*) bitmaps, share->fields, FALSE);
>    my_bitmap_init(&outparam->def_write_set,
> -              (my_bitmap_map*) (bitmaps+bitmap_size), share->fields, FALSE);
> +                 (my_bitmap_map*) (bitmaps+bitmap_size), share->fields,
> +                 FALSE);
>    my_bitmap_init(&outparam->def_vcol_set,
> -              (my_bitmap_map*) (bitmaps+bitmap_size*2), share->fields, FALSE);
> +                 (my_bitmap_map*) (bitmaps+bitmap_size*2), share->fields,
> +                 FALSE);

This is totally wasted memory if the table has no virtual columns.
(that's what I meant by *even more true* - rpl bitmap and
eq_join_set, cond_set bitmaps may or may not be used for this table,
one never knows. But for vcols it's always known in advance whether
the table has them or not).

Still, I'd allocate other bitmaps on demand too.

>    my_bitmap_init(&outparam->tmp_set,
> -              (my_bitmap_map*) (bitmaps+bitmap_size*3), share->fields, FALSE);
> +                 (my_bitmap_map*) (bitmaps+bitmap_size*3), share->fields,
> +                 FALSE);
>    my_bitmap_init(&outparam->eq_join_set,
> -              (my_bitmap_map*) (bitmaps+bitmap_size*4), share->fields, FALSE);
> +                 (my_bitmap_map*) (bitmaps+bitmap_size*4), share->fields,
> +                 FALSE);
>    my_bitmap_init(&outparam->cond_set,
> -              (my_bitmap_map*) (bitmaps+bitmap_size*5), share->fields, FALSE);
> +                 (my_bitmap_map*) (bitmaps+bitmap_size*5), share->fields,
> +                 FALSE);
> +  my_bitmap_init(&outparam->def_rpl_write_set,
> +                 (my_bitmap_map*) (bitmaps+bitmap_size*6), share->fields,
> +                 FALSE);
>    outparam->default_column_bitmaps();
>  
>    outparam->cond_selectivity= 1.0;
> @@ -5986,36 +5997,48 @@ void TABLE::mark_columns_needed_for_insert()
>    we only want to log a PK and we needed other fields for
>    execution).
>  */
> +
>  void TABLE::mark_columns_per_binlog_row_image()
>  {
> +  THD *thd= in_use;
>    DBUG_ENTER("mark_columns_per_binlog_row_image");
>    DBUG_ASSERT(read_set->bitmap);
>    DBUG_ASSERT(write_set->bitmap);
>  
> -  THD *thd= current_thd;

Hmm. Are you sure that TABLE::in_use is *always* set at this point?
You've basically replaced

-  THD *thd=current_thd;
-  if (in_use && in_use->is_current_stmt_binlog_format_row)
+  THD *thd=in_use;
+  if (thd->is_current_stmt_binlog_format_row)

And, btw, where's your MDEV for caching rbr checks per statement?


> +  /* If not using row format */
> +  rpl_write_set= write_set;
>  
>    /**
>      If in RBR we may need to mark some extra columns,
>      depending on the binlog-row-image command line argument.
>     */
>    if ((WSREP_EMULATE_BINLOG(thd) || mysql_bin_log.is_open()) &&
> -      in_use &&
> -      in_use->is_current_stmt_binlog_format_row() &&
> +      thd->is_current_stmt_binlog_format_row() &&
>        !ha_check_storage_engine_flag(s->db_type(), HTON_NO_BINLOG_ROW_OPT))
>    {
>      /* if there is no PK, then mark all columns for the BI. */
>      if (s->primary_key >= MAX_KEY)
> +    {
>        bitmap_set_all(read_set);
> +      rpl_write_set= read_set;
> +    }
> +    else
> +    {
>        switch (thd->variables.binlog_row_image) {
>        case BINLOG_ROW_IMAGE_FULL:
> -        if (s->primary_key < MAX_KEY)
>            bitmap_set_all(read_set);
> -        bitmap_set_all(write_set);
> +        /* Set of columns that should be written (all) */
> +        rpl_write_set= read_set;
>          break;
>        case BINLOG_ROW_IMAGE_NOBLOB:
> -        /* for every field that is not set, mark it unless it is a blob */
> +        /* Only write changed columns + not blobs */
> +        rpl_write_set= &def_rpl_write_set;
> +        bitmap_copy(rpl_write_set, write_set);
> +
> +        /*
> +          for every field that is not set, mark it unless it is a blob or
> +          part of a primary key
> +        */
>          for (Field **ptr=field ; *ptr ; ptr++)
>          {
>            Field *my_field= *ptr;
> @@ -6027,23 +6050,24 @@ void TABLE::mark_columns_per_binlog_row_image()
>              If set in the AI, then the blob is really needed, there is
>              nothing we can do about it.
>             */
> -          if ((s->primary_key < MAX_KEY) &&
> -              ((my_field->flags & PRI_KEY_FLAG) ||
> -              (my_field->type() != MYSQL_TYPE_BLOB)))
> +          if ((my_field->flags & PRI_KEY_FLAG) ||
> +              (my_field->type() != MYSQL_TYPE_BLOB))
> +          {
>              bitmap_set_bit(read_set, my_field->field_index);
> -
> -          if (my_field->type() != MYSQL_TYPE_BLOB)
> -            bitmap_set_bit(write_set, my_field->field_index);
> +            bitmap_set_bit(rpl_write_set, my_field->field_index);
>          }
> +        }
>          break;
>        case BINLOG_ROW_IMAGE_MINIMAL:
> -        /* mark the primary key if available in the read_set */
> -        if (s->primary_key < MAX_KEY)
> +        /* mark the primary key */
>            mark_columns_used_by_index_no_reset(s->primary_key, read_set);
> +        /* Only write columns that have changed */
> +        rpl_write_set= write_set;

Where's the guarantee that the primary key is in the write set?

>          break;
>  
>        default:
>          DBUG_ASSERT(FALSE);
> +      }
>      }
>      file->column_bitmaps_signal();
>    }
> diff --git a/sql/opt_range.cc b/sql/opt_range.cc
> index 860426c..6201065 100644
> --- a/sql/opt_range.cc
> +++ b/sql/opt_range.cc
> @@ -11140,26 +11138,21 @@ int QUICK_RANGE_SELECT::reset()
>  int QUICK_RANGE_SELECT::get_next()
>  {
>    range_id_t dummy;
> +  int result;
> +  DBUG_ENTER("QUICK_RANGE_SELECT::get_next");
> +
> +  if (!in_ror_merged_scan)
> +    DBUG_RETURN(file->multi_range_read_next(&dummy));

please don't put function calls inside DBUG_RETURN,
always use a variable to store the return value first

> +
>    MY_BITMAP * const save_read_set= head->read_set;
>    MY_BITMAP * const save_write_set= head->write_set;
> -
> -  DBUG_ENTER("QUICK_RANGE_SELECT::get_next");
> -  if (in_ror_merged_scan)
> -  {
>      /*
>        We don't need to signal the bitmap change as the bitmap is always the
>        same for this head->file
>      */
>      head->column_bitmaps_set_no_signal(&column_bitmap, &column_bitmap);
> -  }
> -
> -  int result= file->multi_range_read_next(&dummy);
> +  result= file->multi_range_read_next(&dummy);
> -
> -  if (in_ror_merged_scan)
> -  {
> -    /* Restore bitmaps set on entry */
>      head->column_bitmaps_set_no_signal(save_read_set, save_write_set);
> -  }
>    DBUG_RETURN(result);
>  }

Ok, all changes in opt_range.cc are unrelated to your rpl_write_set
patch, please commit them separately

> diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc
> index ee222f0..208b2b6 100644
> --- a/sql/rpl_record.cc
> +++ b/sql/rpl_record.cc
> @@ -434,7 +433,7 @@ unpack_row(rpl_group_info *rgi,
>        *master_reclength = table->s->reclength;
>    }
>    
> -  DBUG_RETURN(error);
> +  DBUG_RETURN(0);
>  }
>  
>  /**

also unrelated to rpl_write_set. If you'd like you can put it in the same
commit with opt_range.cc changes, with the sybject "cleanups" or something

> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index 5054357..b238e89 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -6259,11 +6248,11 @@ int THD::binlog_delete_row(TABLE* table, bool is_trans,
>    DBUG_ASSERT(is_current_stmt_binlog_format_row() &&
>              ((WSREP(this) && wsrep_emulate_bin_log) || mysql_bin_log.is_open()));
>    /**
> -    Save a reference to the original read and write set bitmaps.
> -    We will need this to restore the bitmaps at the end.
> +    Save a reference to the original read bitmaps
> +    We will need this to restore the bitmaps at the end as
> +    binlog_prepare_row_images() may changetable->read_set.
>     */

This doesn't make much sense. Here, the code is like that

 THD::binlog_delete_row:
   save table->read_set
   THD::binlog_prepare_row_images
     calculate table->tmp_set
     table->read_set=table->tmp_set
   pack_row(table->read_set)
     for every set bit in the set_arg call field->pack()
   restore table->read_set

So, two thoughts here:
1. tmp_set must be a strict subset of read_set!
   other fields may not be present in the record[0]
   and there're no row reads between binlog_prepare_row_images and field->pack
2. One can pass table->tmp_set directly into pack_row() as an argument

which means there's no need for binlog_prepare_row_images to modify
table->read_set at all

>    MY_BITMAP *old_read_set= table->read_set;
> -  MY_BITMAP *old_write_set= table->write_set;
>  
>    /** 
>       This will remove spurious fields required during execution but
> diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> index 488826a..0661e70 100644
> --- a/sql/sql_insert.cc
> +++ b/sql/sql_insert.cc
> @@ -2867,5 +2867,7 @@ pthread_handler_t handle_delayed_insert(void *arg)
>      /* Tell client that the thread is initialized */
>      mysql_cond_signal(&di->cond_client);
>  
> +    di->table->mark_columns_needed_for_insert();

why?

> +
>      /* Now wait until we get an insert or lock to handle */
>      /* We will not abort as long as a client thread uses this thread */

Regards,
Sergei