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

Thanks for this work. After your fix after Andrei's review, this looks 
fine. I have one question, one suggestion for simplified design (use it 
if you like, the current patch is cool too), and one suggestion for 
improvement in the test case. See details below.

/Sven


On 04/27/2011 03:56 PM, Guilhem Bichot wrote:
> #At file:///home/mysql_src/bzrrepos_new/wl5872/ based on
> revid:sven.sandberg@stripped
>
>   3350 Guilhem Bichot	2011-04-27
>        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 always contains at most one
> interval,

To be entirely correct, I think we could say "As long as WL#3404 is not 
implemented, only the first interval in the Discrete_intervals_list is 
used," (because it could in fact contain multiple intervals when 
innodb_autoinc_lock_mode >= 1, although only the first interval is used 
by the Intvar_log_event). Is that right?

>        so it makes sense to store the 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".
>       @ sql/structs.h
>          1) ** Edits to the full class **
>          The simplified class has to provide all the interface of the full class.
>          So, shorten the public interface of the full class, to shorten the work
>          needed to code the simplified class.
>          1.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.
>          1.2) make copy_shallow() and append(Discrete_interval*) private,
>          because they are used only in private functions (same arguments as
>          above).
>
>          2) ** implementation of the simplified class **
>          Same public interface as full class, but simpler code, and no new().
>          This class is a "POD", so it can use the default copy constructor,
>          and swap() is simple to write.

This all looks good and I'd be happy to approve this. Just a suggestion: 
an alternative approach would be to use the old class mostly as it is, 
but make it hold the first interval as a member of the class 
(Discrete_interval Discrete_intervals_list::first_interval). This will 
avoid allocation when there is only one interval, which I suppose will 
be good also if we implement WL#3404. Advantages are that the patch is 
smaller and we don't need to duplicate any code (so no risk that the two 
versions diverge):

=== modified file 'sql/structs.h'
--- sql/structs.h	2011-04-26 08:49:10 +0000
+++ sql/structs.h	2011-04-28 12:36:10 +0000
@@ -302,6 +302,8 @@ public:
  /* List of Discrete_interval objects */
  class Discrete_intervals_list {
  private:
+  /* To avoid allocation when there is only one interval, we store the 
first interval here. */
+  Discrete_interval first_interval;
    Discrete_interval        *head;
    Discrete_interval        *tail;
    /*
@@ -331,11 +333,14 @@ public:
    }
    void empty()
    {
-    for (Discrete_interval *i= head; i;)
+    if (head)
      {
-      Discrete_interval *next= i->next;
-      delete i;
-      i= next;
+      for (Discrete_interval *i= head->next; i;)
+      {
+        Discrete_interval *next= i->next;
+        delete i;
+        i= next;
+      }
      }
      empty_no_free();
    }
@@ -368,14 +373,22 @@ public:
    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);
+      DBUG_RETURN(append(&first_interval));
      }
+    /* if this interval can be merged with previous, do that */
+    if (tail->merge_if_contiguous(start, val, incr) == 0)
+      DBUG_RETURN(0);
+    /* if this interval cannot be merged, append it */
+#define DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT 0
+#ifdef DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT
      DBUG_RETURN(0);
+#else
+    DBUG_RETURN(append(new Discrete_interval(start, val, incr)));
+#endif
    }

    bool append(Discrete_interval *new_interval)



Your patch does some other refactorings that are of course sound and 
should be pushed too.

What do you think?

>
>      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-04-27 13:56:11 +0000
> @@ -256,4 +256,17 @@ 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;

I would suggest to also use:

--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-04-27 13:56:11 +0000
> @@ -320,4 +320,13 @@ 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
> +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-04-27 13:56:11 +0000
> @@ -292,7 +292,119 @@ public:
>     };
>   };
>
> -/* List of Discrete_interval objects */
> +/**
> +  As long as statement-based binary logging does not support multiple
> +  autoincrement intervals, we can use the simplified version of
> +  Discrete_intervals_list which supports only 0 or 1 elements.
> +*/
> +#define DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT 1
> +
> +#ifdef DISCRETE_INTERVAL_LIST_HAS_MAX_ONE_ELEMENT
> +
> +/**
> +  This is a simplified version of Discrete_intervals_list: it's a degenerate
> +  list which supports only 0 or 1 element. Compared to the full version (which
> +  is after #else), it avoids one allocation on the global heap. Any fix done
> +  here must also be done in the full version.
> +  Implemented as part of WL#.
> +*/
> +class Discrete_intervals_list {
> +private:
> +  uint elements;                             ///<  number of elements (0 or 1)
> +  /// Storage for our only element. Meaningless if elements == 0.
> +  Discrete_interval single_interval;
> +  /**
> +    Says whether the next get_next() call should return our only element. This
> +    is false if:
> +    - either the list is empty
> +    - or the list has one element but we already returned it in a previous
> +    get_next().
> +  */
> +  bool current_is_positioned;
> +
> +public:
> +  Discrete_intervals_list()  { empty(); }
> +  /// Empties the list
> +  void empty()
> +  {
> +    elements= 0;
> +    current_is_positioned= false;
> +  }
> +  void swap (Discrete_intervals_list *dli)
> +  {
> +    Discrete_intervals_list tmp(*dli);
> +    *dli= *this;
> +    *this= tmp;
> +  }
> +  /**
> +    Gets next interval from list.
> +    @returns next interval, or NULL if no next interval.
> +  */
> +  const Discrete_interval* get_next()
> +  {
> +    if (current_is_positioned)
> +    {
> +      /*
> +        Positioned on single interval. We return it, and after this there is
> +        no 'next' so we won't be positioned.
> +      */
> +      current_is_positioned= false;
> +      return&single_interval;
> +    }
> +    else
> +      return NULL;
> +  }
> +  /**
> +    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)
> +  {
> +    if (elements == 1)
> +    {
> +      if (unlikely(single_interval.merge_if_contiguous(start, val, incr)))
> +      {
> +        /*
> +          The merge was impossible. But 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
> +    {
> +      DBUG_ASSERT(elements == 0);
> +      // need to add a new (first) interval:
> +      elements= 1;
> +      single_interval.replace(start, val, incr);
> +      // If get_next() is called it should return new interval:
> +      current_is_positioned= true;
> +    }
> +    return false;
> +  }
> +  ulonglong minimum()     const { return (elements ? single_interval.minimum() : 0);
> };
> +  ulonglong maximum()     const { return (elements ? single_interval.maximum() : 0);
> };
> +  uint      nb_elements() const { return elements; }
> +};
> +
> +#else
> +
> +/**
> +  List of Discrete_interval objects. This is the full version which supports
> +  any number of intervals.
> +  Implemented as part of WL#3146.
> +*/
>   class Discrete_intervals_list {
>   private:
>     Discrete_interval        *head;
> @@ -316,12 +428,34 @@ private:
>     void operator=(Discrete_intervals_list&);  /* prevent use of these */
>     Discrete_intervals_list(const Discrete_intervals_list&);
>
> -public:
> -  Discrete_intervals_list() : head(NULL), current(NULL), elements(0) {};
>     void empty_no_free()
>     {
>       set_members(NULL, NULL, NULL, 0);
>     }
> +  void copy_shallow(const Discrete_intervals_list * dli)
> +  {
> +    head= dli->head;
> +    tail= dli->tail;
> +    current= dli->current;
> +    elements= dli->elements;
> +  }
> +  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);
> +  }
> +
> +public:
> +  Discrete_intervals_list() : head(NULL), current(NULL), elements(0) {};
>     void empty()
>     {
>       for (Discrete_interval *i= head; i;)
> @@ -332,21 +466,14 @@ public:
>       }
>       empty_no_free();
>     }
> -  void copy_shallow(const Discrete_intervals_list * dli)
> -  {
> -    head= dli->get_head();
> -    tail= dli->get_tail();
> -    current= dli->get_current();
> -    elements= dli->nb_elements();
> -  }
>     void swap (Discrete_intervals_list * dli)
>     {
>       Discrete_interval *h, *t, *c;
>       uint el;
> -    h= dli->get_head();
> -    t= dli->get_tail();
> -    c= dli->get_current();
> -    el= dli->nb_elements();
> +    h= dli->head;
> +    t= dli->tail;
> +    c= dli->current;
> +    el= dli->elements;
>       dli->copy_shallow(this);
>       set_members(h, t, c, el);
>     }
> @@ -370,27 +497,11 @@ public:
>       }
>       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);
> -  }
> +
>     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
>
>   #endif /* STRUCTS_INCLUDED */
>
>
>
>
>

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