bk commit into 5.1 tree (tomas:1.2389) BUG#25387

[tomas]

Below is the list of changes that have just been committed into a local
5.1 repository of tomas. When tomas 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.2389 07/01/17 10:53:42 tomas@stripped +2 -0
  Bug#25387 - added comments in code suggestion during review

  storage/ndb/src/ndbapi/NdbEventOperationImpl.hpp
    1.33 07/01/17 10:53:35 tomas@stripped +60 -2
    Bug#25387 - added comments in code suggestion during review

  storage/ndb/src/ndbapi/NdbEventOperationImpl.cpp
    1.75 07/01/17 10:53:34 tomas@stripped +28 -5
    Bug#25387 - added comments in code suggestion during review

# 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:	tomas
# Host:	poseidon.mysql.com
# Root:	/home/tomas/mysql-5.1-new-ndb

--- 1.74/storage/ndb/src/ndbapi/NdbEventOperationImpl.cpp	2007-01-16 19:22:02 +01:00
+++ 1.75/storage/ndb/src/ndbapi/NdbEventOperationImpl.cpp	2007-01-17 10:53:34 +01:00
@@ -559,6 +559,8 @@
   m_state= EO_EXECUTING;
   mi_type= m_eventImpl->mi_type;
   m_ndb->theEventBuffer->add_op();
+  // add kernel reference
+  // removed on TE_STOP, TE_CLUSTER_FAILURE, or error below
   m_ref_count++;
   DBUG_PRINT("info", ("m_ref_count: %u for op: %p", m_ref_count, this));
   int r= NdbDictionaryImpl::getImpl(*myDict).executeSubscribeEvent(*this);
@@ -571,8 +573,8 @@
         if (r != 0) {
           break;
         }
-        // blob event op now holds reference
-        // cleared by TE_STOP or TE_CLUSTER_FAILURE
+        // add blob reference to main op
+        // removed by TE_STOP or TE_CLUSTER_FAILURE
         m_ref_count++;
         DBUG_PRINT("info", ("m_ref_count: %u for op: %p", m_ref_count, this));
         blob_op = blob_op->m_next;
@@ -583,7 +585,9 @@
       DBUG_RETURN(0);
     }
   }
-  //Error
+  // Error
+  // remove kernel reference
+  // added above
   m_ref_count--;
   DBUG_PRINT("info", ("m_ref_count: %u for op: %p", m_ref_count, this));
   m_state= EO_ERROR;
@@ -1227,6 +1231,8 @@
         EventBufData_list::Gci_ops *gci_ops = m_available_data.first_gci_ops();
         while (gci_ops && op->getGCI() > gci_ops->m_gci)
         {
+          // moved to next gci, check if any references have been
+          // released when completing the last gci
           deleteUsedEventOperations();
           gci_ops = m_available_data.next_gci_ops();
         }
@@ -1254,6 +1260,8 @@
 #endif
 
   // free all "per gci unique" collected operations
+  // completed gci, check if any references have been
+  // released when completing the gci
   EventBufData_list::Gci_ops *gci_ops = m_available_data.first_gci_ops();
   while (gci_ops)
   {
@@ -1290,6 +1298,8 @@
   {
     NdbEventOperationImpl *op = &op_f->m_impl;
     DBUG_ASSERT(op->m_ref_count > 0);
+    // remove gci reference
+    // added in inserDataL
     op->m_ref_count--;
     DBUG_PRINT("info", ("m_ref_count: %u for op: %p", op->m_ref_count, op));
     if (op->m_ref_count == 0)
@@ -1807,14 +1817,17 @@
       {
         op->m_node_bit_mask.clear();
         DBUG_ASSERT(op->m_ref_count > 0);
+        // remove kernel reference
+        // added in execute_nolock
         op->m_ref_count--;
         DBUG_PRINT("info", ("_TE_CLUSTER_FAILURE: m_ref_count: %u for op: %p",
                             op->m_ref_count, op));
         if (op->theMainOp)
         {
-          // blob event op, need to clear ref count in main op
           DBUG_ASSERT(op->m_ref_count == 0);
           DBUG_ASSERT(op->theMainOp->m_ref_count > 0);
+          // remove blob reference in main op
+          // added in execute_no_lock
           op->theMainOp->m_ref_count--;
           DBUG_PRINT("info", ("m_ref_count: %u for op: %p",
                               op->theMainOp->m_ref_count, op->theMainOp));
@@ -1826,14 +1839,17 @@
       if (op->m_node_bit_mask.isclear())
       {
         DBUG_ASSERT(op->m_ref_count > 0);
+        // remove kernel reference
+        // added in execute_no_lock
         op->m_ref_count--;
         DBUG_PRINT("info", ("_TE_STOP: m_ref_count: %u for op: %p",
                             op->m_ref_count, op));
         if (op->theMainOp)
         {
-          // blob event op, need to clear ref count in main op
           DBUG_ASSERT(op->m_ref_count == 0);
           DBUG_ASSERT(op->theMainOp->m_ref_count > 0);
+          // remove blob reference in main op
+          // added in execute_no_lock
           op->theMainOp->m_ref_count--;
           DBUG_PRINT("info", ("m_ref_count: %u for op: %p",
                               op->theMainOp->m_ref_count, op->theMainOp));
@@ -2586,6 +2602,8 @@
 #ifndef DBUG_OFF
     i = m_gci_op_count;
 #endif
+    // add gci reference
+    // removed in deleteUsedOperations
     g.op->m_ref_count++;
     DBUG_PRINT("info", ("m_ref_count: %u for op: %p", g.op->m_ref_count, g.op));
     m_gci_op_list[m_gci_op_count++] = g;
@@ -2654,6 +2672,8 @@
     delete tOp;
     DBUG_RETURN(NULL);
   }
+  // add user reference
+  // removed in dropEventOperation
   getEventOperationImpl(tOp)->m_ref_count = 1;
   DBUG_PRINT("info", ("m_ref_count: %u for op: %p",
                       getEventOperationImpl(tOp)->m_ref_count,
getEventOperationImpl(tOp)));
@@ -2706,6 +2726,9 @@
   }
 
   DBUG_ASSERT(op->m_ref_count > 0);
+  // remove user reference
+  // added in createEventOperation
+  // user error to use reference after this
   op->m_ref_count--;
   DBUG_PRINT("info", ("m_ref_count: %u for op: %p", op->m_ref_count, op));
   if (op->m_ref_count == 0)

--- 1.32/storage/ndb/src/ndbapi/NdbEventOperationImpl.hpp	2007-01-16 19:22:02 +01:00
+++ 1.33/storage/ndb/src/ndbapi/NdbEventOperationImpl.hpp	2007-01-17 10:53:35 +01:00
@@ -400,7 +400,59 @@
   Uint32 m_eventId;
   Uint32 m_oid;
 
+  /*
+    m_node_bit_mask keeps track of which ndb nodes have reference to
+    an event op
+
+    - add    - TE_ACTIVE
+    - remove - TE_STOP, TE_NODE_FAILURE, TE_CLUSTER_FAILURE
+
+    TE_NODE_FAILURE and TE_CLUSTER_FAILURE are created as events
+    and added to all event ops listed as active or pending delete
+    in m_dropped_ev_op using insertDataL, includeing the blob
+    event ops referenced by a regular event op.
+    - NdbEventBuffer::report_node_failure
+    - NdbEventBuffer::completeClusterFailed
+
+    TE_ACTIVE is sent from the kernel on initial execute/start of the
+    event op, but is also internally generetad on node connect like
+    TE_NODE_FAILURE and TE_CLUSTER_FAILURE
+    - NdbEventBuffer::report_node_connected
+
+    when m_node_bit_mask becomes clear, the kernel reference is
+    removed from m_ref_count
+   */
+
   Bitmask<(unsigned int)_NDB_NODE_BITMASK_SIZE> m_node_bit_mask;
+
+  /*
+    m_ref_count keeps track of outstanding references to an event
+    operation impl object.  To make sure that the object is not
+    deleted too early.
+
+    If on dropEventOperation there are still references to an
+    object it is queued for delete in NdbEventBuffer::m_dropped_ev_op
+  
+    the following references exists for a _non_ blob event op:
+    * user reference
+    - add    - NdbEventBuffer::createEventOperation
+    - remove - NdbEventBuffer::dropEventOperation
+    * kernel reference
+    - add    - execute_nolock
+    - remove - TE_STOP, TE_CLUSTER_FAILURE
+    * blob reference
+    - add    - execute_nolock on blob event
+    - remove - TE_STOP, TE_CLUSTER_FAILURE on blob event
+    * gci reference
+    - add    - insertDataL/add_gci_op
+    - remove - NdbEventBuffer::deleteUsedEventOperations
+
+    the following references exists for a blob event op:
+    * kernel reference
+    - add    - execute_nolock
+    - remove - TE_STOP, TE_CLUSTER_FAILURE    
+   */
+
   int m_ref_count;
   bool m_mergeEvents;
   
@@ -557,8 +609,14 @@
   Vector<EventBufData_chunk *> m_allocated_data;
   unsigned m_sz;
 
-  // dropped event operations that have not yet
-  // been deleted
+  /*
+    dropped event operations (dropEventOperation) that have not yet
+    been deleted because of outstanding m_ref_count
+
+    check for delete is done on occations when the ref_count may have
+    changed by calling deleteUsedEventOperations:
+    - nextEvent - each time the user has completed processing a gci
+  */
   NdbEventOperationImpl *m_dropped_ev_op;
 
   Uint32 m_active_op_count;