MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Frazer Clement Date:March 19 2008 2:11pm
Subject:bk commit into 5.1 tree (frazer:1.2544) BUG#35137
View as plain text  
Below is the list of changes that have just been committed into a local
5.1 repository of frazer.  When frazer 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@stripped, 2008-03-19 14:10:47+00:00, frazer@stripped +5 -0
  Bug#35137 : Query crashed mysqld
  
  Nested MRR reads failed as the second MRR range released the first MRR range's unprocessed operations.
  Fix is to process all outstanding operations upfront before returning first result.
  New testcase added to ndb_read_multi_range 

  include/my_base.h@stripped, 2008-03-19 14:10:20+00:00, frazer@stripped +1 -0
    New per-range flag

  mysql-test/suite/ndb/r/ndb_read_multi_range.result@stripped, 2008-03-19 14:10:22+00:00, frazer@stripped +11 -0
    New testcase for nested MRR

  mysql-test/suite/ndb/t/ndb_read_multi_range.test@stripped, 2008-03-19 14:10:28+00:00, frazer@stripped +13 -0
    New testcase for nested MRR

  sql/ha_ndbcluster.cc@stripped, 2008-03-19 14:10:31+00:00, frazer@stripped +96 -44
    Remove need to keep operation list for MRR iteration by reading into buffer up-front.
    Get rid of unnecessary operation-release-on-execute special case for MRR.
    Fix apparent issue with SKIP_RANGE and buffer increment in read_multi_range_next()

  sql/ha_ndbcluster.h@stripped, 2008-03-19 14:10:33+00:00, frazer@stripped +3 -4
    Remove force flag from execute() methods, remove current MRR operation member

diff -Nrup a/include/my_base.h b/include/my_base.h
--- a/include/my_base.h	2008-02-05 13:41:55 +00:00
+++ b/include/my_base.h	2008-03-19 14:10:20 +00:00
@@ -500,6 +500,7 @@ enum data_file_type {
 #define NULL_RANGE	64
 #define GEOM_FLAG      128
 #define SKIP_RANGE     256
+#define EMPTY_RANGE    512
 
 typedef struct st_key_range
 {
diff -Nrup a/mysql-test/suite/ndb/r/ndb_read_multi_range.result b/mysql-test/suite/ndb/r/ndb_read_multi_range.result
--- a/mysql-test/suite/ndb/r/ndb_read_multi_range.result	2007-08-21 13:32:26 +01:00
+++ b/mysql-test/suite/ndb/r/ndb_read_multi_range.result	2008-03-19 14:10:22 +00:00
@@ -492,3 +492,14 @@ id
 3
 drop trigger kaboom;
 drop table t1;
+create table t1 (a int primary key, b int, key b_idx (b)) engine ndb;
+insert into t1 values(1,1), (2,2), (3,3), (4,4), (5,5);
+select one.a 
+from t1 one left join t1 two 
+on (two.b = one.b) 
+where one.a in (3, 4) 
+order by a;
+a
+3
+4
+drop table t1;
diff -Nrup a/mysql-test/suite/ndb/t/ndb_read_multi_range.test b/mysql-test/suite/ndb/t/ndb_read_multi_range.test
--- a/mysql-test/suite/ndb/t/ndb_read_multi_range.test	2007-08-21 13:32:26 +01:00
+++ b/mysql-test/suite/ndb/t/ndb_read_multi_range.test	2008-03-19 14:10:28 +00:00
@@ -338,3 +338,16 @@ select * from t2 order by id;
 
 drop trigger kaboom;
 drop table t1;
+
+#bug#35137 - self join + mrr
+
+create table t1 (a int primary key, b int, key b_idx (b)) engine ndb;
+insert into t1 values(1,1), (2,2), (3,3), (4,4), (5,5);
+
+select one.a 
+from t1 one left join t1 two 
+on (two.b = one.b) 
+where one.a in (3, 4) 
+order by a;
+
+drop table t1;
diff -Nrup a/sql/ha_ndbcluster.cc b/sql/ha_ndbcluster.cc
--- a/sql/ha_ndbcluster.cc	2008-03-12 14:22:51 +00:00
+++ b/sql/ha_ndbcluster.cc	2008-03-19 14:10:31 +00:00
@@ -276,10 +276,9 @@ int execute_no_commit_ignore_no_key(ha_n
 }
 
 inline
-int execute_no_commit(ha_ndbcluster *h, NdbTransaction *trans,
-		      bool force_release)
+int execute_no_commit(ha_ndbcluster *h, NdbTransaction *trans)
 {
-  h->release_completed_operations(trans, force_release);
+  h->release_completed_operations(trans);
   return h->m_ignore_no_key ?
     execute_no_commit_ignore_no_key(h,trans) :
     trans->execute(NdbTransaction::NoCommit,
@@ -304,10 +303,9 @@ int execute_commit(THD *thd, NdbTransact
 }
 
 inline
-int execute_no_commit_ie(ha_ndbcluster *h, NdbTransaction *trans,
-			 bool force_release)
+int execute_no_commit_ie(ha_ndbcluster *h, NdbTransaction *trans)
 {
-  h->release_completed_operations(trans, force_release);
+  h->release_completed_operations(trans);
   return trans->execute(NdbTransaction::NoCommit,
                         NdbOperation::AO_IgnoreError,
                         h->m_force_send);
@@ -1792,7 +1790,7 @@ int ha_ndbcluster::pk_read(const uchar *
       ERR_RETURN(trans->getNdbError());
   }
 
-  if ((res = execute_no_commit_ie(this,trans,FALSE)) != 0 ||
+  if ((res = execute_no_commit_ie(this,trans)) != 0 ||
       op->getNdbError().code) 
   {
     table->status= STATUS_NOT_FOUND;
@@ -1858,7 +1856,7 @@ int ha_ndbcluster::complemented_read(con
     }
   }
   
-  if (execute_no_commit(this,trans,FALSE) != 0) 
+  if (execute_no_commit(this,trans) != 0) 
   {
     table->status= STATUS_NOT_FOUND;
     DBUG_RETURN(ndb_err(trans));
@@ -2058,7 +2056,7 @@ int ha_ndbcluster::peek_indexed_rows(con
   }
   last= trans->getLastDefinedOperation();
   if (first)
-    res= execute_no_commit_ie(this,trans,FALSE);
+    res= execute_no_commit_ie(this,trans);
   else
   {
     // Table has no keys
@@ -2107,7 +2105,7 @@ int ha_ndbcluster::unique_index_read(con
   if ((res= define_read_attrs(buf, op)))
     DBUG_RETURN(res);
 
-  if (execute_no_commit_ie(this,trans,FALSE) != 0 ||
+  if (execute_no_commit_ie(this,trans) != 0 ||
       op->getNdbError().code) 
   {
     int err= ndb_err(trans);
@@ -2164,7 +2162,7 @@ inline int ha_ndbcluster::fetch_next(Ndb
     */
     if (m_ops_pending && m_blobs_pending)
     {
-      if (execute_no_commit(this,trans,FALSE) != 0)
+      if (execute_no_commit(this,trans) != 0)
         DBUG_RETURN(ndb_err(trans));
       m_ops_pending= 0;
       m_blobs_pending= FALSE;
@@ -2196,7 +2194,7 @@ inline int ha_ndbcluster::fetch_next(Ndb
       {
         if (m_transaction_on)
         {
-          if (execute_no_commit(this,trans,FALSE) != 0)
+          if (execute_no_commit(this,trans) != 0)
             DBUG_RETURN(-1);
         }
         else
@@ -2520,7 +2518,7 @@ int ha_ndbcluster::ordered_index_scan(co
       ERR_RETURN(trans->getNdbError());
   }
 
-  if (execute_no_commit(this,trans,FALSE) != 0)
+  if (execute_no_commit(this,trans) != 0)
     DBUG_RETURN(ndb_err(trans));
   
   DBUG_RETURN(next_result(buf));
@@ -2621,7 +2619,7 @@ int ha_ndbcluster::unique_index_scan(con
   if ((res= define_read_attrs(buf, op)))
     DBUG_RETURN(res);
 
-  if (execute_no_commit(this,trans,FALSE) != 0)
+  if (execute_no_commit(this,trans) != 0)
     DBUG_RETURN(ndb_err(trans));
   DBUG_PRINT("exit", ("Scan started successfully"));
   DBUG_RETURN(next_result(buf));
@@ -2689,7 +2687,7 @@ int ha_ndbcluster::full_table_scan(uchar
   if ((res= define_read_attrs(buf, op)))
     DBUG_RETURN(res);
 
-  if (execute_no_commit(this,trans,FALSE) != 0)
+  if (execute_no_commit(this,trans) != 0)
     DBUG_RETURN(ndb_err(trans));
   DBUG_PRINT("exit", ("Scan started successfully"));
   DBUG_RETURN(next_result(buf));
@@ -2903,7 +2901,7 @@ int ha_ndbcluster::write_row(uchar *reco
     m_bulk_insert_not_flushed= FALSE;
     if (m_transaction_on)
     {
-      if (execute_no_commit(this,trans,FALSE) != 0)
+      if (execute_no_commit(this,trans) != 0)
       {
         m_skip_auto_increment= TRUE;
         no_uncommitted_rows_execute_failure();
@@ -3197,7 +3195,7 @@ int ha_ndbcluster::update_row(const ucha
   */
 
   if ((!cursor || m_update_cannot_batch) && 
-      execute_no_commit(this,trans,false) != 0) {
+      execute_no_commit(this,trans) != 0) {
     no_uncommitted_rows_execute_failure();
     DBUG_RETURN(ndb_err(trans));
   }
@@ -3314,7 +3312,7 @@ int ha_ndbcluster::delete_row(const ucha
   }
 
   // Execute delete operation
-  if (execute_no_commit(this,trans,FALSE) != 0) {
+  if (execute_no_commit(this,trans) != 0) {
     no_uncommitted_rows_execute_failure();
     DBUG_RETURN(ndb_err(trans));
   }
@@ -3818,7 +3816,7 @@ int ha_ndbcluster::close_scan()
       deleteing/updating transaction before closing the scan    
     */
     DBUG_PRINT("info", ("ops_pending: %ld", (long) m_ops_pending));    
-    if (execute_no_commit(this,trans,FALSE) != 0) {
+    if (execute_no_commit(this,trans) != 0) {
       no_uncommitted_rows_execute_failure();
       DBUG_RETURN(ndb_err(trans));
     }
@@ -4246,7 +4244,7 @@ int ha_ndbcluster::end_bulk_insert()
     m_bulk_insert_not_flushed= FALSE;
     if (m_transaction_on)
     {
-      if (execute_no_commit(this, trans,FALSE) != 0)
+      if (execute_no_commit(this, trans) != 0)
       {
         no_uncommitted_rows_execute_failure();
         my_errno= error= ndb_err(trans);
@@ -8642,8 +8640,7 @@ int ha_ndbcluster::write_ndb_file(const 
 }
 
 void 
-ha_ndbcluster::release_completed_operations(NdbTransaction *trans,
-					    bool force_release)
+ha_ndbcluster::release_completed_operations(NdbTransaction *trans)
 {
   if (trans->hasBlobOperation())
   {
@@ -8652,16 +8649,7 @@ ha_ndbcluster::release_completed_operati
     */
     return;
   }
-  if (!force_release)
-  {
-    if (get_thd_ndb(current_thd)->query_state & NDB_QUERY_MULTI_READ_RANGE)
-    {
-      /* We are batching reads and have not consumed all fetched
-	 rows yet, releasing operation records is unsafe 
-      */
-      return;
-    }
-  }
+
   trans->releaseCompletedOperations();
 }
 
@@ -8676,6 +8664,12 @@ ha_ndbcluster::null_value_index_search(K
   ulong reclength= table->s->reclength;
   uchar *curr= (uchar*)buffer->buffer;
   uchar *end_of_buffer= (uchar*)buffer->buffer_end;
+
+  /* All passed ranges whose results could fit into the 
+   * buffer are examined, although some may later be
+   * marked for skipping, wasting buffer space.
+   */
+  assert(!(range->range_flag & SKIP_RANGE));
   
   for (; range<end_range && curr+reclength <= end_of_buffer; 
        range++)
@@ -8780,6 +8774,13 @@ ha_ndbcluster::read_multi_range_first(KE
           We can skip this partition since the key won't fit into any
           partition
         */
+        /* Note that we must increment the buffer pointer, even though
+         * we won't use the space, as the check in null_value_index_search()
+         * assumed that every range would use a buffer slot when deciding
+         * how many ranges to check.  This could be fixed if
+         * null_value_index_search() was made partition (and skip_range)
+         * aware
+         */
         curr += reclength;
         multi_range_curr->range_flag |= SKIP_RANGE;
         continue;
@@ -8886,16 +8887,67 @@ ha_ndbcluster::read_multi_range_first(KE
     buffer->end_of_used_area= curr;
   }
   
-  /*
-   * Set first operation in multi range
-   */
-  m_current_multi_operation= 
-    lastOp ? lastOp->next() : m_active_trans->getFirstDefinedOperation();
-  if (!(res= execute_no_commit_ie(this, m_active_trans,true)))
+  /* Get pointer to first range key operation (not scans) */
+  const NdbOperation* rangeOp= lastOp ? lastOp->next() : 
+    m_active_trans->getFirstDefinedOperation();
+
+  if (!(res= execute_no_commit_ie(this, m_active_trans)))
   {
     m_multi_range_defined= multi_range_curr;
     multi_range_curr= ranges;
     m_multi_range_result_ptr= (uchar*)buffer->buffer;
+
+    /* We must unpack all of the results for any primary and unique key
+     * ranges now, as these operations may be invalidated by 
+     * further execute+releaseOperations calls on this transaction by 
+     * different handler objects.
+     */
+    KEY_MULTI_RANGE* rangeInfo= multi_range_curr;
+    uchar* result_ptr= m_multi_range_result_ptr;
+
+    for (;rangeInfo < m_multi_range_defined; rangeInfo++)
+    {
+      if (rangeInfo->range_flag & SKIP_RANGE)
+      {
+        /* Skip over buffer slot to next range */
+        result_ptr += reclength;
+        continue; 
+      }
+
+      if (rangeInfo->range_flag & UNIQUE_RANGE)
+      {
+        if (rangeOp->getNdbError().code == 0)
+        {
+          /* Successful read, unpack results inplace
+           * in buffer now.
+           */
+          DBUG_PRINT("info", ("Unique range op has result"));
+          setup_recattr(rangeOp->getFirstRecAttr());
+          unpack_record(result_ptr);
+        }
+        else
+        {
+          NdbError err= rangeOp->getNdbError();
+
+          if (err.classification !=
+              NdbError::NoDataFound)
+            ERR_RETURN(err);
+
+          DBUG_PRINT("info", ("Unique range op has no result"));
+          /* Indicate to read_multi_range_next that this
+           * result is empty
+           */
+          rangeInfo->range_flag |= EMPTY_RANGE;
+        }
+        
+        /* Move to next completed operation and buffer 'slot' */
+        rangeOp= m_active_trans->getNextCompletedOperation(rangeOp);
+        result_ptr += reclength;
+      }
+
+      /* For scan ranges, do nothing */
+    }
+
     DBUG_RETURN(read_multi_range_next(found_range_p));
   }
   ERR_RETURN(m_active_trans->getNdbError());
@@ -8920,21 +8972,24 @@ ha_ndbcluster::read_multi_range_next(KEY
   int res;
   int range_no;
   ulong reclength= table_share->reclength;
-  const NdbOperation* op= m_current_multi_operation;
   for (;multi_range_curr < m_multi_range_defined; multi_range_curr++)
   {
     DBUG_MULTI_RANGE(12);
     if (multi_range_curr->range_flag & SKIP_RANGE)
+    {
+      /* Skip over buffer slot to next range */
+      m_multi_range_result_ptr += reclength;
       continue;
+    }
     if (multi_range_curr->range_flag & UNIQUE_RANGE)
     {
-      if (op->getNdbError().code == 0)
+      /* If a row was found, return it */
+      if (!(multi_range_curr->range_flag & EMPTY_RANGE))
       {
         DBUG_MULTI_RANGE(13);
         goto found_next;
       }
       
-      op= m_active_trans->getNextCompletedOperation(op);
       m_multi_range_result_ptr += reclength;
       continue;
     } 
@@ -9052,12 +9107,9 @@ found_next:
    */
   * multi_range_found_p= multi_range_curr;
   memcpy(table->record[0], m_multi_range_result_ptr, reclength);
-  setup_recattr(op->getFirstRecAttr());
-  unpack_record(table->record[0]);
   table->status= 0;
   
   multi_range_curr++;
-  m_current_multi_operation= m_active_trans->getNextCompletedOperation(op);
   m_multi_range_result_ptr += reclength;
   DBUG_RETURN(0);
 }
diff -Nrup a/sql/ha_ndbcluster.h b/sql/ha_ndbcluster.h
--- a/sql/ha_ndbcluster.h	2008-02-04 14:40:00 +00:00
+++ b/sql/ha_ndbcluster.h	2008-03-19 14:10:33 +00:00
@@ -493,12 +493,12 @@ private:
   void no_uncommitted_rows_update(int);
   void no_uncommitted_rows_reset(THD *);
 
-  void release_completed_operations(NdbTransaction*, bool);
+  void release_completed_operations(NdbTransaction*);
 
   friend int execute_commit(ha_ndbcluster*, NdbTransaction*);
   friend int execute_no_commit_ignore_no_key(ha_ndbcluster*, NdbTransaction*);
-  friend int execute_no_commit(ha_ndbcluster*, NdbTransaction*, bool);
-  friend int execute_no_commit_ie(ha_ndbcluster*, NdbTransaction*, bool);
+  friend int execute_no_commit(ha_ndbcluster*, NdbTransaction*);
+  friend int execute_no_commit_ie(ha_ndbcluster*, NdbTransaction*);
 
   void transaction_checks(THD *thd);
   int start_statement(THD *thd, Thd_ndb *thd_ndb, Ndb* ndb);
@@ -559,7 +559,6 @@ private:
   uchar *m_multi_range_result_ptr;
   KEY_MULTI_RANGE *m_multi_ranges;
   KEY_MULTI_RANGE *m_multi_range_defined;
-  const NdbOperation *m_current_multi_operation;
   NdbIndexScanOperation *m_multi_cursor;
   uchar *m_multi_range_cursor_result_ptr;
   int setup_recattr(const NdbRecAttr*);
Thread
bk commit into 5.1 tree (frazer:1.2544) BUG#35137Frazer Clement19 Mar