From: Ole John Aske Date: June 20 2011 1:26pm Subject: bzr push into mysql-5.1-telco-7.0 branch (ole.john.aske:4467 to 4468) List-Archive: http://lists.mysql.com/commits/139522 Message-Id: <20110620132617.ED69C224@fimafeng09.norway.sun.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 4468 Ole John Aske 2011-06-20 SPJ API change: Relax lifetime dependencies between 'NdbQueryDef' and its instantiated NdbQuery objects. The application is now allowed to destruct the NdbQueryDef after all its instantiated NdbQuery objects has been '::close()'ed. modified: storage/ndb/src/ndbapi/NdbQueryBuilder.hpp storage/ndb/src/ndbapi/NdbQueryOperation.cpp storage/ndb/src/ndbapi/NdbQueryOperation.hpp storage/ndb/src/ndbapi/NdbQueryOperationImpl.hpp 4467 Jonas Oreland 2011-06-20 ndb - fix bug where row-count could be reported incorrectly if in transaction performing delete of non-existing record using a primary/unique key and then selecting count(*) added: mysql-test/suite/ndb/r/ndb_select_count.result mysql-test/suite/ndb/t/ndb_select_count.test modified: sql/ha_ndbcluster.cc === modified file 'storage/ndb/src/ndbapi/NdbQueryBuilder.hpp' --- a/storage/ndb/src/ndbapi/NdbQueryBuilder.hpp 2011-06-16 09:32:43 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryBuilder.hpp 2011-06-20 13:25:48 +0000 @@ -479,13 +479,8 @@ private: * times. It is valid until it is explicitely released(). * * The NdbQueryDef *must* be keept alive until the last thread - * which executing a query based on this NdbQueryDef has completed execution - * *and* result handling. Used from multiple threads this implies either: - * - * - Keep the NdbQueryDef until all threads terminates. - * - Implement reference counting on the NdbQueryDef. - * - Use the supplied copy constructor to give each thread its own copy - * of the NdbQueryDef. + * which executing a query based on this NdbQueryDef has called + * NdbQuery::close(). * * A NdbQueryDef is scheduled for execution by appending it to an open * transaction - optionally together with a set of parameters specifying === modified file 'storage/ndb/src/ndbapi/NdbQueryOperation.cpp' --- a/storage/ndb/src/ndbapi/NdbQueryOperation.cpp 2011-06-16 09:32:43 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryOperation.cpp 2011-06-20 13:25:48 +0000 @@ -1389,7 +1389,7 @@ NdbQueryImpl::NdbQueryImpl(NdbTransactio m_state(Initial), m_tcState(Inactive), m_next(NULL), - m_queryDef(queryDef), + m_queryDef(&queryDef), m_error(), m_transaction(trans), m_scanTransaction(NULL), @@ -1452,9 +1452,13 @@ NdbQueryImpl::NdbQueryImpl(NdbTransactio NdbQueryImpl::~NdbQueryImpl() { - - // Do this to check that m_queryDef still exists. - assert(getNoOfOperations() == m_queryDef.getNoOfOperations()); + /** BEWARE: + * Don't refer NdbQueryDef or NdbQueryOperationDefs after + * NdbQuery::close() as at this stage the appliaction is + * allowed to destruct the Def's. + */ + assert(m_state==Closed); + assert(m_rootFrags==NULL); // NOTE: m_operations[] was allocated as a single memory chunk with // placement new construction of each operation. @@ -1465,8 +1469,6 @@ NdbQueryImpl::~NdbQueryImpl() } m_operations = NULL; } - delete[] m_rootFrags; - m_rootFrags = NULL; m_state = Destructed; } @@ -1478,6 +1480,9 @@ NdbQueryImpl::postFetchRelease() { m_operations[i].postFetchRelease(); } } + delete[] m_rootFrags; + m_rootFrags = NULL; + m_rowBufferAlloc.reset(); m_tupleSetAlloc.reset(); m_resultStreamAlloc.reset(); @@ -1957,7 +1962,7 @@ NdbQueryImpl::awaitMoreResults(bool forc assert(m_applFrags.getCurrent() == NULL); /* Check if there are any more completed fragments available.*/ - if (m_queryDef.isScanQuery()) + if (getQueryDef().isScanQuery()) { assert (m_scanTransaction); assert (m_state==Executing); @@ -2052,7 +2057,7 @@ NdbQueryImpl::awaitMoreResults(bool forc assert(m_pendingFrags == 0); assert(m_finalBatchFrags == getRootFragCount()); return FetchResult_noMoreData; - } // if(m_queryDef.isScanQuery()) + } // if(getQueryDef().isScanQuery()) } //NdbQueryImpl::awaitMoreResults @@ -2131,32 +2136,41 @@ NdbQueryImpl::close(bool forceSend) int res = 0; assert (m_state >= Initial && m_state < Destructed); - Ndb* const ndb = m_transaction.getNdb(); - - if (m_tcState != Inactive) + if (m_state != Closed) { - /* We have started a scan, but we have not yet received the last batch - * for all root fragments. We must therefore close the scan to release - * the scan context at TC.*/ - res = closeTcCursor(forceSend); - } + if (m_tcState != Inactive) + { + /* We have started a scan, but we have not yet received the last batch + * for all root fragments. We must therefore close the scan to release + * the scan context at TC.*/ + res = closeTcCursor(forceSend); + } - // Throw any pending results - m_fullFrags.clear(); - m_applFrags.clear(); + // Throw any pending results + m_fullFrags.clear(); + m_applFrags.clear(); - if (m_scanTransaction != NULL) - { - assert (m_state != Closed); - assert (m_scanTransaction->m_scanningQuery == this); - m_scanTransaction->m_scanningQuery = NULL; - ndb->closeTransaction(m_scanTransaction); - ndb->theRemainingStartTransactions--; // Compensate; m_scanTransaction was not a real Txn - m_scanTransaction = NULL; + Ndb* const ndb = m_transaction.getNdb(); + if (m_scanTransaction != NULL) + { + assert (m_state != Closed); + assert (m_scanTransaction->m_scanningQuery == this); + m_scanTransaction->m_scanningQuery = NULL; + ndb->closeTransaction(m_scanTransaction); + ndb->theRemainingStartTransactions--; // Compensate; m_scanTransaction was not a real Txn + m_scanTransaction = NULL; + } + + postFetchRelease(); + m_state = Closed; // Even if it was previously 'Failed' it is closed now! } - postFetchRelease(); - m_state = Closed; // Even if it was previously 'Failed' it is closed now! + /** BEWARE: + * Don't refer NdbQueryDef or its NdbQueryOperationDefs after ::close() + * as the application is allowed to destruct the Def's after this point. + */ + m_queryDef= NULL; + return res; } //NdbQueryImpl::close @@ -2819,7 +2833,7 @@ NdbQueryImpl::sendFetchMore(NdbRootFragm { assert(getRoot().m_resultStreams!=NULL); assert(!emptyFrag.finalBatchReceived()); - assert(m_queryDef.isScanQuery()); + assert(getQueryDef().isScanQuery()); const Uint32 fragNo = emptyFrag.getFragNo(); emptyFrag.reset(); @@ -2892,7 +2906,7 @@ NdbQueryImpl::sendFetchMore(NdbRootFragm int NdbQueryImpl::closeTcCursor(bool forceSend) { - assert (m_queryDef.isScanQuery()); + assert (getQueryDef().isScanQuery()); NdbImpl* const ndb = m_transaction.getNdb()->theImpl; const Uint32 timeout = ndb->get_waitfor_timeout(); === modified file 'storage/ndb/src/ndbapi/NdbQueryOperation.hpp' --- a/storage/ndb/src/ndbapi/NdbQueryOperation.hpp 2011-04-06 14:16:13 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryOperation.hpp 2011-06-20 13:25:48 +0000 @@ -180,7 +180,15 @@ public: NdbTransaction* getNdbTransaction() const; /** - * Close query + * Close query. + * + * Will release most of the internally allocated objects owned + * by this NdbQuery and detach itself from the NdbQueryDef + * used to instantiate it. + * + * The application may destruct the NdbQueryDef after + * ::close() has been called on *all* NdbQuery objects + * instantiated from it. */ void close(bool forceSend = false); === modified file 'storage/ndb/src/ndbapi/NdbQueryOperationImpl.hpp' --- a/storage/ndb/src/ndbapi/NdbQueryOperationImpl.hpp 2011-04-06 14:16:13 +0000 +++ b/storage/ndb/src/ndbapi/NdbQueryOperationImpl.hpp 2011-06-20 13:25:48 +0000 @@ -142,7 +142,8 @@ public: /** Close query: * - Release datanode resources, * - Discard pending result sets, - * - optionaly dealloc NdbQuery structures + * - Delete internal buffer and structures for receiving results. + * - Disconnect with NdbQueryDef - it might now be destructed . */ int close(bool forceSend); @@ -191,7 +192,10 @@ public: /** Get the (transaction independent) definition of this query. */ const NdbQueryDefImpl& getQueryDef() const - { return m_queryDef; } + { + assert(m_queryDef); + return *m_queryDef; + } /** Process TCKEYCONF message. Return true if query is complete. */ bool execTCKEYCONF(); @@ -213,7 +217,7 @@ public: */ void setStartIndicator() { - assert(!m_queryDef.isScanQuery()); + assert(!getQueryDef().isScanQuery()); m_startIndicator = true; } @@ -224,7 +228,7 @@ public: */ void setCommitIndicator() { - assert(!m_queryDef.isScanQuery()); + assert(!getQueryDef().isScanQuery()); m_commitIndicator = true; } @@ -415,7 +419,7 @@ private: /** Next query in same transaction.*/ NdbQueryImpl* m_next; /** Definition of this query.*/ - const NdbQueryDefImpl& m_queryDef; + const NdbQueryDefImpl* m_queryDef; /** Possible error status of this query.*/ NdbError m_error; No bundle (reason: useless for push emails).