List:Commits« Previous MessageNext Message »
From:Sven Sandberg Date:May 3 2011 12:26pm
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3350) WL#5872
View as plain text  
Hi Guilhem,

Thanks for taking my suggestions into account. This looks good, I have 
nothing to add now. Patch approved!

/Sven


On 05/02/2011 03:52 PM, Guilhem Bichot wrote:
> #At file:///home/mysql_src/bzrrepos_new/wl5872_2/ based on
> revid:sven.sandberg@stripped
>
>   3350 Guilhem Bichot	2011-05-02
>        WL#5872 "avoid using global heap memory to remember autoincrement values for
> statement-based binlog".
>        As long as WL3404 "new binlog event Insert_id_list_event to store multiple
> auto_increment intervals"
>        is not implemented, Discrete_intervals_list needs to contain only the first
> interval,
>        so it makes sense to store this interval as a member of
> Discrete_intervals_list,
>        rather than allocating it on the global heap with new(). This is to save one
> new()
>        per statement inserting into an auto-increment column, and is a sub-part of
>        WL5774 "Decrease number of malloc's for normal DML queries".
>        Even when WL3404 is implemented, it will still make sense to avoid new()
>        for the first interval, as we expect one-interval lists to be common.
>        Patch includes parts by Sven Sandberg.
>       @ sql/structs.h
>          1) remove get_head()/current()/tail(), because those were used only
>          in private functions where head/current/tail are accessible anyway;
>          head/current/tail are implementation details, users of the class
>          don't need to see them.
>          2) make copy_shallow() and append(Discrete_interval*) private,
>          because they are used only in private functions (same arguments as
>          above).
>          3) Store first interval inside Discrete_intervals_list,
>          not on the heap (no new()).
>
>      modified:
>        mysql-test/extra/rpl_tests/rpl_auto_increment.test
>        mysql-test/suite/rpl/r/rpl_auto_increment.result
>        sql/structs.h
> === modified file 'mysql-test/extra/rpl_tests/rpl_auto_increment.test'
> --- a/mysql-test/extra/rpl_tests/rpl_auto_increment.test	2010-12-21 10:39:20 +0000
> +++ b/mysql-test/extra/rpl_tests/rpl_auto_increment.test	2011-05-02 13:52:51 +0000
> @@ -256,4 +256,19 @@ connection master;
>   DROP TABLE t1;
>   sync_slave_with_master;
>
> +#
> +# WL#5872 "avoid using global heap memory to remember autoincrement
> +# values for statement-based binlog".
> +#
> +connection master;
> +eval create table t1(a int auto_increment primary key) engine=$engine_type;
> +insert into t1 values (null),(null),(1025),(null);
> +sync_slave_with_master;
> +select * from t1;
> +let $diff_tables= master:t1, slave:t1;
> +--source include/diff_tables.inc
> +connection master;
> +drop table t1;
> +sync_slave_with_master;
> +
>   --source include/rpl_end.inc
>
> === modified file 'mysql-test/suite/rpl/r/rpl_auto_increment.result'
> --- a/mysql-test/suite/rpl/r/rpl_auto_increment.result	2010-12-21 04:47:22 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_auto_increment.result	2011-05-02 13:52:51 +0000
> @@ -320,4 +320,14 @@ SELECT * FROM t1;
>   id	data
>   0	2
>   DROP TABLE t1;
> +create table t1(a int auto_increment primary key) engine=innodb;
> +insert into t1 values (null),(null),(1025),(null);
> +select * from t1;
> +a
> +1
> +2
> +1025
> +1026
> +include/diff_tables.inc [master:t1, slave:t1]
> +drop table t1;
>   include/rpl_end.inc
>
> === modified file 'sql/structs.h'
> --- a/sql/structs.h	2010-10-21 09:49:16 +0000
> +++ b/sql/structs.h	2011-05-02 13:52:51 +0000
> @@ -292,105 +292,138 @@ public:
>     };
>   };
>
> -/* List of Discrete_interval objects */
> +/// List of Discrete_interval objects
>   class Discrete_intervals_list {
> +
> +/**
> +   Discrete_intervals_list objects are used to remember the
> +   intervals of autoincrement values that have been used by the
> +   current INSERT statement, so that the values can be written to the
> +   binary log.  However, the binary log can currently only store the
> +   beginning of the first interval (because WL#3404 is not yet
> +   implemented).  Hence, it is currently not necessary to store
> +   anything else than the first interval, in the list.  When WL#3404 is
> +   implemented, we should change the '# define' below.
> +*/
> +#define DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT 1
> +
>   private:
> +  /**
> +    To avoid heap allocation in the common case when there is only one
> +    interval in the list, we store the first interval here.
> +  */
> +  Discrete_interval        first_interval;
>     Discrete_interval        *head;
>     Discrete_interval        *tail;
> -  /*
> +  /**
>       When many intervals are provided at the beginning of the execution of a
>       statement (in a replication slave or SET INSERT_ID), "current" points to
>       the interval being consumed by the thread now (so "current" goes from
>       "head" to "tail" then to NULL).
>     */
>     Discrete_interval        *current;
> -  uint                  elements; // number of elements
> -  void set_members(Discrete_interval *h, Discrete_interval *t,
> -                   Discrete_interval *c, uint el)
> -  {
> -    head= h;
> -    tail= t;
> -    current= c;
> -    elements= el;
> +  uint                  elements;               ///<  number of elements
> +  void operator=(Discrete_intervals_list&);    // prevent use of this
> +  bool append(Discrete_interval *new_interval)
> +  {
> +    if (unlikely(new_interval == NULL))
> +      return true;
> +    DBUG_PRINT("info",("adding new auto_increment interval"));
> +    if (head == NULL)
> +      head= current= new_interval;
> +    else
> +      tail->next= new_interval;
> +    tail= new_interval;
> +    elements++;
> +    return false;
>     }
> -  void operator=(Discrete_intervals_list&);  /* prevent use of these */
> -  Discrete_intervals_list(const Discrete_intervals_list&);
> +  void copy_shallow(const Discrete_intervals_list *other)
> +  {
> +    const Discrete_interval *o_first_interval=&other->first_interval;
> +    first_interval= other->first_interval;
> +    head= other->head == o_first_interval ?&first_interval : other->head;
> +    tail= other->tail == o_first_interval ?&first_interval : other->tail;
> +    current=
> +      other->current == o_first_interval ?&first_interval :
> other->current;
> +    elements= other->elements;
> +  }
> +  Discrete_intervals_list(const Discrete_intervals_list&other)
> +  { copy_shallow(&other); }
>
>   public:
>     Discrete_intervals_list() : head(NULL), current(NULL), elements(0) {};
> -  void empty_no_free()
> -  {
> -    set_members(NULL, NULL, NULL, 0);
> -  }
>     void empty()
>     {
> -    for (Discrete_interval *i= head; i;)
> +    if (head)
>       {
> -      Discrete_interval *next= i->next;
> -      delete i;
> -      i= next;
> +      // first element, not on heap, should not be delete-d; start with next:
> +      for (Discrete_interval *i= head->next; i;)
> +      {
> +#ifdef DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT
> +        DBUG_ASSERT(0);
> +#endif
> +        Discrete_interval *next= i->next;
> +        delete i;
> +        i= next;
> +      }
>       }
> -    empty_no_free();
> +    head= tail= current= NULL;
> +    elements= 0;
>     }
> -  void copy_shallow(const Discrete_intervals_list * dli)
> +  void swap(Discrete_intervals_list *other)
>     {
> -    head= dli->get_head();
> -    tail= dli->get_tail();
> -    current= dli->get_current();
> -    elements= dli->nb_elements();
> +    const Discrete_intervals_list tmp(*other);
> +    other->copy_shallow(this);
> +    copy_shallow(&tmp);
>     }
> -  void swap (Discrete_intervals_list * dli)
> +  const Discrete_interval *get_next()
>     {
> -    Discrete_interval *h, *t, *c;
> -    uint el;
> -    h= dli->get_head();
> -    t= dli->get_tail();
> -    c= dli->get_current();
> -    el= dli->nb_elements();
> -    dli->copy_shallow(this);
> -    set_members(h, t, c, el);
> -  }
> -  const Discrete_interval* get_next()
> -  {
> -    Discrete_interval *tmp= current;
> +    const Discrete_interval *tmp= current;
>       if (current != NULL)
>         current= current->next;
>       return tmp;
>     }
>     ~Discrete_intervals_list() { empty(); };
> +  /**
> +    Appends an interval to the list.
> +
> +    @param start  start of interval
> +    @val   how    many values it contains
> +    @param incr   what increment between each value
> +    @retval true  error
> +    @retval false success
> +  */
>     bool append(ulonglong start, ulonglong val, ulonglong incr)
>     {
> -    DBUG_ENTER("Discrete_intervals_list::append");
> -    /* first, see if this can be merged with previous */
> -    if ((head == NULL) || tail->merge_if_contiguous(start, val, incr))
> +    // If there are no intervals, add one.
> +    if (head == NULL)
>       {
> -      /* it cannot, so need to add a new interval */
> -      Discrete_interval *new_interval= new Discrete_interval(start, val, incr);
> -      DBUG_RETURN(append(new_interval));
> +      first_interval.replace(start, val, incr);
> +      return append(&first_interval);
>       }
> -    DBUG_RETURN(0);
> -  }
> -
> -  bool append(Discrete_interval *new_interval)
> -  {
> -    DBUG_ENTER("Discrete_intervals_list::append");
> -    if (unlikely(new_interval == NULL))
> -      DBUG_RETURN(1);
> -    DBUG_PRINT("info",("adding new auto_increment interval"));
> -    if (head == NULL)
> -      head= current= new_interval;
> -    else
> -      tail->next= new_interval;
> -    tail= new_interval;
> -    elements++;
> -    DBUG_RETURN(0);
> +    // If this interval can be merged with previous, do that.
> +    if (tail->merge_if_contiguous(start, val, incr) == 0)
> +      return false;
> +    // If this interval cannot be merged, append it.
> +#ifdef DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT
> +    /*
> +      We cannot create yet another interval as we already contain one. This
> +      situation can happen. Assume innodb_autoinc_lock_mode>=1 and
> +       CREATE TABLE T(A INT AUTO_INCREMENT PRIMARY KEY) ENGINE=INNODB;
> +       INSERT INTO T VALUES (NULL),(NULL),(1025),(NULL);
> +      Then InnoDB will reserve [1,4] (because of 4 rows) then
> +      [1026,1026]. Only the first interval is important for
> +      statement-based binary logging as it tells the starting point. So we
> +      ignore the second interval:
> +    */
> +    return false;
> +#else
> +    return append(new Discrete_interval(start, val, incr));
> +#endif
>     }
>     ulonglong minimum()     const { return (head ? head->minimum() : 0); };
>     ulonglong maximum()     const { return (head ? tail->maximum() : 0); };
>     uint      nb_elements() const { return elements; }
> -  Discrete_interval* get_head() const { return head; };
> -  Discrete_interval* get_tail() const { return tail; };
> -  Discrete_interval* get_current() const { return current; };
>   };
>
>   #endif /* STRUCTS_INCLUDED */
>
>
>
>
>

Thread
bzr commit into mysql-trunk branch (guilhem.bichot:3350) WL#5872Guilhem Bichot2 May
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3350) WL#5872Sven Sandberg3 May