List:Commits« Previous MessageNext Message »
From:Ole John Aske Date:June 19 2012 11:05am
Subject:bzr push into mysql-5.5-cluster-7.2-spj branch (ole.john.aske:3866 to 3867)
View as plain text  
 3867 Ole John Aske	2012-06-19
      This patch is intended to fix possible memory leak when a failure
      occurs in Dbspj::execLQHKEYREQ or Dbspj::execSCAN_FRAGREQ,
      during, or after, Dbspj::build() has completed.
      
      In order to do a proper release of any allocated SPJ resources
      in these cases, we need to call ::cleanup().
      That will take care of releasing any resources allocated by
      the (partial?) build TreeNode structures.
      
      Instead of only calling ::cleanup() in case of 'late failures',
      I have enhanced the consistency of the TreeNode and Request
      structures such that ::cleanup() can be called in any state
      on a partial build Request struct - which seems simpler and cleaner.
      
      This also involved moving the assignment of 'm_send.m_keyInfoPtrI'
      from within :build(), to after ::build() had succeeded. That avoided
      a possible memory leak due to ownership of that memory section being
      in an undefined limbo state.
      
      Also introduce ERROR_INSERT(17013) which simulate such a 
      'late failure' after ::build()

    modified:
      storage/ndb/src/kernel/blocks/dbspj/Dbspj.hpp
      storage/ndb/src/kernel/blocks/dbspj/DbspjMain.cpp
      storage/ndb/test/ndbapi/testSpj.cpp
 3866 Ole John Aske	2012-06-07
      Change classification of SPJ 'out of memory/resources' error from 
      'permanent' to 'temporary' resource errors.
      
      Also change error text for SOJ-error 20006 to contain 
      'LongMessageBuffer' as this is the config variable which has 
      to be adjusted to avoid this error.

    modified:
      storage/ndb/src/ndbapi/ndberror.c
=== modified file 'storage/ndb/src/kernel/blocks/dbspj/Dbspj.hpp'
--- a/storage/ndb/src/kernel/blocks/dbspj/Dbspj.hpp	2012-05-24 11:43:11 +0000
+++ b/storage/ndb/src/kernel/blocks/dbspj/Dbspj.hpp	2012-06-19 11:04:55 +0000
@@ -298,7 +298,6 @@ public:
     Uint32 m_senderRef;  // TC (used for routing)
     Uint32 m_scan_cnt;
     Signal* m_start_signal; // Argument to first node in tree
-    SegmentedSectionPtr m_keyPtr;
 
     TreeNodeBitMask m_scans; // TreeNodes doing scans
 

=== modified file 'storage/ndb/src/kernel/blocks/dbspj/DbspjMain.cpp'
--- a/storage/ndb/src/kernel/blocks/dbspj/DbspjMain.cpp	2012-05-24 11:43:11 +0000
+++ b/storage/ndb/src/kernel/blocks/dbspj/DbspjMain.cpp	2012-06-19 11:04:55 +0000
@@ -426,9 +426,10 @@ void Dbspj::execLQHKEYREQ(Signal* signal
    *      (unless StoredProcId is set, when only paramters are sent,
    *       but this is not yet implemented)
    */
+  SegmentedSectionPtr attrPtr;
   SectionHandle handle = SectionHandle(this, signal);
-  SegmentedSectionPtr ssPtr;
-  handle.getSection(ssPtr, LqhKeyReq::AttrInfoSectionNum);
+  handle.getSection(attrPtr, LqhKeyReq::AttrInfoSectionNum);
+  const Uint32 keyPtrI = handle.m_ptr[LqhKeyReq::KeyInfoSectionNum].i;
 
   Uint32 err;
   Ptr<Request> requestPtr = { 0, RNIL };
@@ -457,7 +458,7 @@ void Dbspj::execLQHKEYREQ(Signal* signal
     Uint32 len_cnt;
 
     {
-      SectionReader r0(ssPtr, getSectionSegmentPool());
+      SectionReader r0(attrPtr, getSectionSegmentPool());
 
       err = DbspjErr::ZeroLengthQueryTree;
       if (unlikely(!r0.getWord(&len_cnt)))
@@ -468,8 +469,8 @@ void Dbspj::execLQHKEYREQ(Signal* signal
     Uint32 cnt = QueryTree::getNodeCnt(len_cnt);
 
     {
-      SectionReader treeReader(ssPtr, getSectionSegmentPool());
-      SectionReader paramReader(ssPtr, getSectionSegmentPool());
+      SectionReader treeReader(attrPtr, getSectionSegmentPool());
+      SectionReader paramReader(attrPtr, getSectionSegmentPool());
       paramReader.step(len); // skip over tree to parameters
 
       Build_context ctx;
@@ -477,31 +478,39 @@ void Dbspj::execLQHKEYREQ(Signal* signal
       ctx.m_savepointId = req->savePointId;
       ctx.m_scanPrio = 1;
       ctx.m_start_signal = signal;
-      ctx.m_keyPtr.i = handle.m_ptr[LqhKeyReq::KeyInfoSectionNum].i;
       ctx.m_senderRef = signal->getSendersBlockRef();
 
       err = build(ctx, requestPtr, treeReader, paramReader);
       if (unlikely(err != 0))
         break;
-    }
 
-    /**
-     * a query being shipped as a LQHKEYREQ may only return finite rows
-     *   i.e be a (multi-)lookup
-     */
-    ndbassert(requestPtr.p->isLookup());
-    ndbassert(requestPtr.p->m_node_cnt == cnt);
-    err = DbspjErr::InvalidRequest;
-    if (unlikely(!requestPtr.p->isLookup() || requestPtr.p->m_node_cnt != cnt))
-      break;
+      /**
+       * Root TreeNode in Request takes ownership of keyPtr
+       * section when build has completed.
+       * We are done with attrPtr which is now released.
+       */
+      Ptr<TreeNode> rootNodePtr = ctx.m_node_list[0];
+      rootNodePtr.p->m_send.m_keyInfoPtrI = keyPtrI;
+      release(attrPtr);
+      handle.clear();
+    }
 
     /**
      * Store request in list(s)/hash(es)
      */
     store_lookup(requestPtr);
 
-    release(ssPtr);
-    handle.clear();
+    /**
+     * A query being shipped as a LQHKEYREQ may return at most a row
+     * per operation i.e be a (multi-)lookup 
+     */
+    if (ERROR_INSERTED_CLEAR(17013) ||
+        unlikely(!requestPtr.p->isLookup() || requestPtr.p->m_node_cnt != cnt))
+    {
+      jam();
+      err = DbspjErr::InvalidRequest;
+      break;
+    }
 
     start(signal, requestPtr);
     return;
@@ -514,9 +523,9 @@ void Dbspj::execLQHKEYREQ(Signal* signal
   if (!requestPtr.isNull())
   {
     jam();
-    m_request_pool.release(requestPtr);
+    cleanup(requestPtr);
   }
-  releaseSections(handle);
+  releaseSections(handle);  // a NOOP, if we reached 'handle.clear()' above
   handle_early_lqhkey_ref(signal, req, err);
 }
 
@@ -729,8 +738,8 @@ Dbspj::execSCAN_FRAGREQ(Signal* signal)
    *              if first op is lookup - contains keyinfo for lookup
    */
   SectionHandle handle = SectionHandle(this, signal);
-  SegmentedSectionPtr ssPtr;
-  handle.getSection(ssPtr, ScanFragReq::AttrInfoSectionNum);
+  SegmentedSectionPtr attrPtr;
+  handle.getSection(attrPtr, ScanFragReq::AttrInfoSectionNum);
 
   Uint32 err;
   Ptr<Request> requestPtr = { 0, RNIL };
@@ -758,7 +767,7 @@ Dbspj::execSCAN_FRAGREQ(Signal* signal)
 
     Uint32 len_cnt;
     {
-      SectionReader r0(ssPtr, getSectionSegmentPool());
+      SectionReader r0(attrPtr, getSectionSegmentPool());
       err = DbspjErr::ZeroLengthQueryTree;
       if (unlikely(!r0.getWord(&len_cnt)))
         break;
@@ -768,8 +777,8 @@ Dbspj::execSCAN_FRAGREQ(Signal* signal)
     Uint32 cnt = QueryTree::getNodeCnt(len_cnt);
 
     {
-      SectionReader treeReader(ssPtr, getSectionSegmentPool());
-      SectionReader paramReader(ssPtr, getSectionSegmentPool());
+      SectionReader treeReader(attrPtr, getSectionSegmentPool());
+      SectionReader paramReader(attrPtr, getSectionSegmentPool());
       paramReader.step(len); // skip over tree to parameters
 
       Build_context ctx;
@@ -780,35 +789,38 @@ Dbspj::execSCAN_FRAGREQ(Signal* signal)
       ctx.m_start_signal = signal;
       ctx.m_senderRef = signal->getSendersBlockRef();
 
+      err = build(ctx, requestPtr, treeReader, paramReader);
+      if (unlikely(err != 0))
+        break;
+
+      /**
+       * Root TreeNode in Request takes ownership of keyPtr
+       * section when build has completed.
+       * We are done with attrPtr which is now released.
+       */
+      Ptr<TreeNode> rootNodePtr = ctx.m_node_list[0];
       if (handle.m_cnt > 1)
       {
         jam();
-        ctx.m_keyPtr.i = handle.m_ptr[ScanFragReq::KeyInfoSectionNum].i;
-      }
-      else
-      {
-        jam();
-        ctx.m_keyPtr.i = RNIL;
+        const Uint32 keyPtrI = handle.m_ptr[ScanFragReq::KeyInfoSectionNum].i;
+        rootNodePtr.p->m_send.m_keyInfoPtrI = keyPtrI;
       }
-
-      err = build(ctx, requestPtr, treeReader, paramReader);
-      if (unlikely(err != 0))
-        break;
+      release(attrPtr);
+      handle.clear();
     }
 
-    ndbassert(requestPtr.p->isScan());
-    ndbassert(requestPtr.p->m_node_cnt == cnt);
-    err = DbspjErr::InvalidRequest;
-    if (unlikely(!requestPtr.p->isScan() || requestPtr.p->m_node_cnt != cnt))
-      break;
-
     /**
      * Store request in list(s)/hash(es)
      */
     store_scan(requestPtr);
 
-    release(ssPtr);
-    handle.clear();
+    if (ERROR_INSERTED_CLEAR(17013) ||
+        unlikely(!requestPtr.p->isScan() || requestPtr.p->m_node_cnt != cnt))
+    {
+      jam();
+      err = DbspjErr::InvalidRequest;
+      break;
+    }
 
     start(signal, requestPtr);
     return;
@@ -817,9 +829,9 @@ Dbspj::execSCAN_FRAGREQ(Signal* signal)
   if (!requestPtr.isNull())
   {
     jam();
-    m_request_pool.release(requestPtr);
+    cleanup(requestPtr);
   }
-  releaseSections(handle);
+  releaseSections(handle);  // a NOOP, if we reached 'handle.clear()' above
   handle_early_scanfrag_ref(signal, req, err);
 }
 
@@ -1919,33 +1931,12 @@ Dbspj::cleanup(Ptr<Request> requestPtr)
       requestPtr.p->m_state = Request::RS_ABORTED;
       return;
     }
-
-#ifdef VM_TRACE
-    {
-      Request key;
-      key.m_transId[0] = requestPtr.p->m_transId[0];
-      key.m_transId[1] = requestPtr.p->m_transId[1];
-      key.m_senderData = requestPtr.p->m_senderData;
-      Ptr<Request> tmp;
-      ndbrequire(m_scan_request_hash.find(tmp, key));
-    }
-#endif
-    m_scan_request_hash.remove(requestPtr);
+    m_scan_request_hash.remove(requestPtr, *requestPtr.p);
   }
   else
   {
     jam();
-#ifdef VM_TRACE
-    {
-      Request key;
-      key.m_transId[0] = requestPtr.p->m_transId[0];
-      key.m_transId[1] = requestPtr.p->m_transId[1];
-      key.m_senderData = requestPtr.p->m_senderData;
-      Ptr<Request> tmp;
-      ndbrequire(m_lookup_request_hash.find(tmp, key));
-    }
-#endif
-    m_lookup_request_hash.remove(requestPtr);
+    m_lookup_request_hash.remove(requestPtr, *requestPtr.p);
   }
   releaseRequestBuffers(requestPtr, false);
   ArenaHead ah = requestPtr.p->m_arena;
@@ -3037,7 +3028,6 @@ Dbspj::lookup_build(Build_context& ctx,
       dst->fragmentData = fragId;
       dst->attrLen = attrLen; // fragdist is in here
 
-      treeNodePtr.p->m_send.m_keyInfoPtrI = ctx.m_keyPtr.i;
       treeNodePtr.p->m_bits |= TreeNode::T_ONE_SHOT;
     }
     return 0;
@@ -4092,6 +4082,7 @@ Dbspj::scanFrag_build(Build_context& ctx
       break;
     }
 
+    treeNodePtr.p->m_info = &g_ScanFragOpInfo;
     treeNodePtr.p->m_scanfrag_data.m_scanFragHandlePtrI = RNIL;
     Ptr<ScanFragHandle> scanFragHandlePtr;
     if (ERROR_INSERTED_CLEAR(17004))
@@ -4115,7 +4106,6 @@ Dbspj::scanFrag_build(Build_context& ctx
     treeNodePtr.p->m_scanfrag_data.m_scanFragHandlePtrI = scanFragHandlePtr.i;
 
     requestPtr.p->m_bits |= Request::RT_SCAN;
-    treeNodePtr.p->m_info = &g_ScanFragOpInfo;
     treeNodePtr.p->m_bits |= TreeNode::T_ATTR_INTERPRETED;
     treeNodePtr.p->m_batch_size = ctx.m_batch_size_rows;
 
@@ -4235,7 +4225,6 @@ Dbspj::scanFrag_build(Build_context& ctx
       ndbassert(dst->transId2 == transId2);
 #endif
 
-      treeNodePtr.p->m_send.m_keyInfoPtrI = ctx.m_keyPtr.i;
       treeNodePtr.p->m_bits |= TreeNode::T_ONE_SHOT;
 
       if (rangeScanFlag)

=== modified file 'storage/ndb/test/ndbapi/testSpj.cpp'
--- a/storage/ndb/test/ndbapi/testSpj.cpp	2012-05-24 11:43:11 +0000
+++ b/storage/ndb/test/ndbapi/testSpj.cpp	2012-06-19 11:04:55 +0000
@@ -118,6 +118,7 @@ runLookupJoinError(NDBT_Context* ctx, ND
   int lookupFaults[] = {
       17001, 17005, 17006, 17008,
       17012, // testing abort in :execDIH_SCAN_TAB_CONF
+      17013, // Simulate DbspjErr::InvalidRequest
       17020, 17021, 17022, // lookup_send() encounter dead node -> NodeFailure
       17030, 17031, 17032, // LQHKEYREQ reply is LQHKEYREF('Invalid..')
       17040, 17041, 17042, // lookup_parent_row -> OutOfQueryMemory
@@ -208,6 +209,7 @@ runScanJoinError(NDBT_Context* ctx, NDBT
   int scanFaults[] = {
       17002, 17004, 17005, 17006, 17008,
       17012, // testing abort in :execDIH_SCAN_TAB_CONF
+      17013, // Simulate DbspjErr::InvalidRequest
       17020, 17021, 17022, // lookup_send() encounter dead node -> NodeFailure
       17030, 17031, 17032, // LQHKEYREQ reply is LQHKEYREF('Invalid..')
       17040, 17041, 17042, // lookup_parent_row -> OutOfQueryMemory

No bundle (reason: useless for push emails).
Thread
bzr push into mysql-5.5-cluster-7.2-spj branch (ole.john.aske:3866 to 3867) Ole John Aske19 Jun