From: Frazer Clement
Date: June 1 2011 8:57am
Subject: Re: bzr commit into mysql-5.1-telco-7.0 branch (magnus.blaudd:4428)
 Bug#37153
List-Archive: http://lists.mysql.com/commits/138507
Message-Id: <4DE5FEF9.7090105@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Magnus,
   Great to see this hacky code I wrote just after joining being 
consigned to the bin!
   Looks good, comments inline below just about explaining the logic a 
bit more.
Frazer

On 01/06/11 09:08, Jonas Oreland wrote:
> comments:
> 1) frazer must look too
> 2) otherwise look "ok" i guess...
>
> /Jonas
> On 06/01/11 09:59, magnus.blaudd@stripped wrote:
>> #At file:///data0/magnus/mysql/7.0-bug37153/ based on revid:jonas@stripped
>>
>>   4428 magnus.blaudd@stripped	2011-06-01
>>        Bug #37153  NDB Cluster reports affected rows incorrectly
>>         - refactor patch according to potential fix 2) to avoid
>>         having to fiddle with MySQL Server internals from the ha_ndbcluster code
>>
>>      modified:
>>        sql/ha_ndbcluster.cc
>> === modified file 'sql/ha_ndbcluster.cc'
>> --- a/sql/ha_ndbcluster.cc	2011-04-29 11:45:56 +0000
>> +++ b/sql/ha_ndbcluster.cc	2011-06-01 07:57:18 +0000
>> @@ -4652,6 +4652,36 @@ int ha_ndbcluster::exec_bulk_update(uint
>>   {
>>     DBUG_ENTER("ha_ndbcluster::exec_bulk_update");
>>     *dup_key_found= 0;
>> +
>> +  // m_handler must be NULL or point to _this_ handler instance
Is m_handler == *this an indicator of 'autocommit'?  If so, can you add 
a comment to state this?
>> +  assert(m_thd_ndb->m_handler == NULL ||
>> +         (m_thd_ndb->m_handler&&  m_thd_ndb->m_handler == this));
>> +
>> +  if (m_thd_ndb->m_handler&&
>> +      m_read_before_write_removal_possible)
>> +  {
>> +    // Commit the auto-commit transaction early(before ndbcluster_commit)
>> +    // in order to:
>> +    // 1) return the correct number of updated and affected rows
>> +    // 2) save one round trip "no-commit+commit" converted to "commit"
Not clear from patch fragment why we 'know' that this is an autocommit 
scenario here.  Is there something you can assert? (! 
OPTION_NOT_AUTOCOMMIT)?
Also, would be good to be sure the Slave code isn't taking this route.
>> +    DBUG_PRINT("info", ("committing auto-commit+rbwr early"));
>> +    const int ignore_error= 1;
>> +    uint ignore_count= 0;
>> +    if (execute_commit(m_thd_ndb, m_thd_ndb->trans,
>> +                       m_thd_ndb->m_force_send, ignore_error,
>> +&ignore_count) != 0)
>> +    {
>> +      no_uncommitted_rows_execute_failure();
>> +      DBUG_RETURN(ndb_err(m_thd_ndb->trans));
>> +    }
>> +    DBUG_PRINT("info", ("ignore_count: %u", ignore_count));
>> +    assert(m_rows_changed>= ignore_count);
>> +    assert(m_rows_updated>= ignore_count);
>> +    m_rows_changed-= ignore_count;
>> +    m_rows_updated-= ignore_count;
>> +    DBUG_RETURN(0);
>> +  }
>> +
>>     if (m_thd_ndb->m_unsent_bytes&&
>>         !thd_allow_batch(table->in_use)&&
>>         (!m_thd_ndb->m_handler ||
>> @@ -5007,6 +5037,35 @@ int ha_ndbcluster::end_bulk_delete()
>>   {
>>     DBUG_ENTER("end_bulk_delete");
>>     assert(m_is_bulk_delete); // Don't allow end() without start()
>> +  m_is_bulk_delete= false;
>> +
>> +  // m_handler must be NULL or point to _this_ handler instance
>> +  assert(m_thd_ndb->m_handler == NULL ||
>> +         (m_thd_ndb->m_handler&&  m_thd_ndb->m_handler == this));
>> +
>> +  if (m_thd_ndb->m_handler&&
>> +      m_read_before_write_removal_possible)
>> +  {
>> +    // Commit the auto-commit transaction early(before ndbcluster_commit)
>> +    // in order to:
>> +    // 1) return the correct number of deleted and affected rows
>> +    // 2) save one round trip "no-commit+commit" converted to "commit"
>> +    DBUG_PRINT("info", ("committing auto-commit+rbwr early"));
>> +    const int ignore_error= 1;
>> +    uint ignore_count= 0;
>> +    if (execute_commit(m_thd_ndb, m_thd_ndb->trans,
>> +                       m_thd_ndb->m_force_send, ignore_error,
>> +&ignore_count) != 0)
>> +    {
>> +      no_uncommitted_rows_execute_failure();
>> +      DBUG_RETURN(ndb_err(m_thd_ndb->trans));
>> +    }
>> +    DBUG_PRINT("info", ("ignore_count: %u", ignore_count));
>> +    assert(m_rows_deleted>= ignore_count);
>> +    m_rows_deleted-= ignore_count;
>> +    DBUG_RETURN(0);
>> +  }
>> +
>>     if (m_thd_ndb->m_unsent_bytes&&
>>         !thd_allow_batch(table->in_use)&&
>>         !m_thd_ndb->m_handler)
>> @@ -5017,13 +5076,11 @@ int ha_ndbcluster::end_bulk_delete()
>>                             &ignore_count) != 0)
>>       {
>>         no_uncommitted_rows_execute_failure();
>> -      m_is_bulk_delete = false;
>>         DBUG_RETURN(ndb_err(m_thd_ndb->trans));
>>       }
>>       assert(m_rows_deleted>= ignore_count);
>>       m_rows_deleted-= ignore_count;
>>     }
>> -  m_is_bulk_delete = false;
>>     DBUG_RETURN(0);
>>   }
>>
>> @@ -7097,58 +7154,9 @@ int ndbcluster_commit(handlerton *hton,
>>       if (thd_ndb->m_handler&&
>>           thd_ndb->m_handler->m_read_before_write_removal_possible)
>>       {
I think we should require that all these conditions with > 1 variable 
being tested are accompanied by a text description of the cases intended 
to be covered, especially as some of the conditions do not indicate 
their meaning on reading (thd_ndb->m_handler).
>> -#ifndef NDB_WITHOUT_READ_BEFORE_WRITE_REMOVAL
>> -      /* Autocommit with read-before-write removal
>> -       * Some operations in this autocommitted statement have not
>> -       * yet been executed
>> -       * They will be executed here as part of commit, and the results
>> -       * (rowcount, message) sent back to the client will then be modified
>> -       * according to how the execution went.
>> -       * This saves a single roundtrip in the autocommit case
>> -       */
>> -      uint ignore_count= 0;
>> -      res= execute_commit(thd_ndb, trans, THDVAR(thd, force_send),
>> -                          TRUE,&ignore_count);
>> -      if (!res&&  ignore_count)
>> -      {
>> -        DBUG_PRINT("info", ("AutoCommit + RBW removal, ignore_count=%u",
>> -                            ignore_count));
>> -        /* We have some rows to ignore, modify recorded results,
>> -         * regenerate result message as required.
>> -         */
>> -        thd->row_count_func-= ignore_count;
>> -
>> -        ha_rows affected= 0;
>> -        char buff[ STRING_BUFFER_USUAL_SIZE ];
>> -        const char* msg= NULL;
>> -        if (thd->lex->sql_command == SQLCOM_DELETE)
>> -        {
>> -          assert(thd_ndb->m_handler->m_rows_deleted>= ignore_count);
>> -          affected= (thd_ndb->m_handler->m_rows_deleted-= ignore_count);
>> -        }
>> -        else
>> -        {
>> -          DBUG_PRINT("info", ("Update : message was %s",
>> -                              thd->main_da.message()));
>> -          assert(thd_ndb->m_handler->m_rows_updated>= ignore_count);
>> -          affected= (thd_ndb->m_handler->m_rows_updated-= ignore_count);
>> -          /* For update in this scenario, we set found and changed to be
>> -           * the same as affected
>> -           * Regenerate the update message
>> -           */
>> -          sprintf(buff, ER(ER_UPDATE_INFO), (ulong)affected, (ulong)affected,
>> -                  (ulong) thd->cuted_fields);
>> -          msg= buff;
>> -          DBUG_PRINT("info", ("Update : message changed to %s",
>> -                              msg));
>> -        }
>> -
>> -        /* Modify execution result + optionally message */
>> -        thd->main_da.modify_affected_rows(affected, msg);
>> -      }
>> -#else
>> -      abort(); // Should never come here without rbwr support
>> -#endif
>> +      // Already committed in exec_bulk_update() or end_bulk_delete()
>> +      DBUG_PRINT("info", ("auto-commit+rbwr, transaction already comitted"));
>> +      assert(trans->commitStatus() == NdbTransaction::Committed);
>>       }
>>       else
>>         res= execute_commit(thd_ndb, trans, THDVAR(thd, force_send), FALSE);
>>
>>
>>
>>
>>