bk commit into 5.0 tree (guilhem:1.1904) BUG#12381

[guilhem]

Below is the list of changes that have just been committed into a local
5.0 repository of guilhem. When guilhem does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.1904 05/08/05 16:24:09 guilhem@stripped +2 -0
  Fix for BUG#12381 "row-based replication leaks memory in rpl_row_loaddata":
  I replace m_chunks with a MEM_ROOT in table_mapping (achieves the same functionality),
  and add a free_root() which closes the leak. Added comments.

  sql/rpl_tblmap.h
    1.3 05/08/05 16:23:59 guilhem@stripped +29 -3
    table_mapping needn't inherit from Sql_alloc as we don't call "new" on it.
    Putting a MEM_ROOT in replacement of m_chunks, this simplifies allocation and freeing and fixes BUG#12381

  sql/rpl_tblmap.cc
    1.4 05/08/05 16:23:59 guilhem@stripped +23 -7
    instead of using plain "new" (which forwards to malloc(), see my_new.cc), we use a MEM_ROOT because it's adapted:
    the table mapping entries are allocated in chunks, and freed as a whole (BUG#12381 was that it wasn't freed).

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	guilhem
# Host:	gbichot2.local
# Root:	/home/mysql_src/mysql-5.0-wl1012

--- 1.3/sql/rpl_tblmap.cc	2005-08-05 14:44:11 +02:00
+++ 1.4/sql/rpl_tblmap.cc	2005-08-05 16:23:59 +02:00
@@ -27,15 +27,22 @@
 table_mapping::table_mapping()
   : m_free(0)
 {
+  /*
+    No "free_element" function for entries passed here, as the entries are
+    allocated in a MEM_ROOT (freed as a whole in the destructor), they cannot
+    be freed one by one.
+  */
   (void) hash_init(&m_table_ids,&my_charset_bin,TABLE_ID_HASH_SIZE,
 		   offsetof(entry,table_id),sizeof(ulong),
 		   0,0,0);
+  /* We don't preallocate any block, this is consistent with m_free=0 above */
+  init_alloc_root(&m_mem_root, TABLE_ID_HASH_SIZE*sizeof(entry), 0);
 }
 
 table_mapping::~table_mapping()
 {
-  /* must not delete the chunks as they are allocated on the memroot */
   hash_free(&m_table_ids);
+  free_root(&m_mem_root, MYF(0));
 }
 
 st_table* table_mapping::get_table(ulong table_id)
@@ -57,17 +64,22 @@
 
 /*
   Called when we are out of table id entries. Creates TABLE_ID_CHUNK
-  new entries, chains them, and puts them into the list of free entries.
+  new entries, chain them and attach them at the head of the list of free
+  (free for use) entries.
 */
 int table_mapping::expand()
 {
-  entry *tmp= new entry[TABLE_ID_CHUNK];
+  /*
+    If we wanted to use "tmp= new (&m_mem_root) entry[TABLE_ID_CHUNK]",
+    we would have to make "entry" derive from Sql_alloc but then it would not
+    be a POD anymore and we want it to be (see rpl_tblmap.h). So we allocate
+    in C.
+  */
+  entry *tmp= (entry *)alloc_root(&m_mem_root, TABLE_ID_CHUNK*sizeof(entry));
   if (tmp == NULL)
     return ERR_MEMORY_ALLOCATION; // Memory allocation failed
 
-  m_chunks.push_front(tmp);
-
-  // Find the end of the array of entries.
+  /* Find the end of this fresh new array of free entries */
   entry *e_end= tmp+TABLE_ID_CHUNK-1;
   for (entry *e= tmp; e < e_end; e++)
     e->next= e+1;
@@ -109,7 +121,7 @@
   if (e)
   {
     hash_delete(&m_table_ids,(byte *)e);
-    /* we add this entry to the chain of free entries */
+    /* we add this entry to the chain of free (free for use) entries */
     e->next= m_free;
     m_free= e;
     return 0;			// All OK
@@ -117,6 +129,10 @@
   return 1;			// No table to remove
 }
 
+/*
+  Puts all entries into the list of free-for-use entries (does not free any
+  memory), and empties the hash.
+*/
 void table_mapping::clear_tables()
 {
   DBUG_ENTER("table_mapping::clear_tables()");

--- 1.2/sql/rpl_tblmap.h	2005-08-05 14:44:11 +02:00
+++ 1.3/sql/rpl_tblmap.h	2005-08-05 16:23:59 +02:00
@@ -30,7 +30,28 @@
   COLLABORATION
     RELAY_LOG    For mapping table id:s to tables when receiving events.
  */
-class table_mapping : public Sql_alloc {
+
+/*
+  Guilhem to Mats:
+  in the table_mapping class, the memory is allocated and never freed (until
+  destruction). So this is a good candidate for allocating inside a MEM_ROOT:
+  it gives the efficient allocation in chunks (like in expand()). So I have
+  introduced a MEM_ROOT.
+
+  Note that inheriting from Sql_alloc had no effect: it has effects only when
+  "ptr= new table_mapping" is called, and this is never called. And it would
+  then allocate from thd->mem_root which is a highly volatile object (reset
+  from example after executing each query, see dispatch_command(), it has a
+  free_root() at end); as the table_mapping object is supposed to live longer
+  than a query, it was dangerous.
+  A dedicated MEM_ROOT needs to be used, see below.
+*/
+
+class table_mapping {
+
+private:
+  MEM_ROOT m_mem_root;
+
 public:
 
   enum {
@@ -56,7 +77,7 @@
 private:
   /*
     This is a POD (Plain Old Data).  Keep it that way (we apply offsetof() to
-    it! which only works for PODs)
+    it, which only works for PODs)
   */
   struct entry { 
     ulong table_id;
@@ -74,9 +95,14 @@
   }
   int expand();
 
-  List<entry> m_chunks;         // List of allocated chunks
+  /*
+    Head of the list of free entries; "free" in the sense that it's an
+    allocated entry free for use, NOT in the sense that it's freed
+    memory.
+  */
   entry *m_free;
 
+  /* Correspondance between an id (a number) and a TABLE object */
   HASH m_table_ids;
 };