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);
>>
>>
>>
>>
>>
