bzr commit into mysql-trunk branch (guilhem.bichot:3350) WL#5872

[Guilhem Bichot]

#At file:///home/mysql_src/bzrrepos_new/wl5872_2/ based on revid:sven.sandberg@stripped

 3350 Guilhem Bichot	2011-04-30
      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-04-30 18:05:20 +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;
+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-30 18:05:20 +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-30 18:05:20 +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 */


Attachment: [text/bzr-bundle] bzr/[e-mail]