Comment 9 for bug 552420

Revision history for this message
Jay Pipes (jaypipes) wrote :

The problem lies in this block of code in /drizzled/statement/alter_table.cc (lines 981-1000):

<code>
  /* Create a temporary table with the new format */
  /**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to be
    filled.
  */
  TableIdentifier new_table_as_temporary(original_table_identifier.getSchemaName(),
                                         tmp_name,
                                         create_proto.type() != message::Table::TEMPORARY ? message::Table::INTERNAL :
                                         message::Table::TEMPORARY);

  error= create_temporary_table(session, new_table_as_temporary, create_info, create_proto, alter_info);

  if (error != 0)
  {
    return true;
  }

  /* Open the table so we need to copy the data to it. */
  new_table= open_alter_table(session, table, new_table_as_temporary);

</code>

The reason it is problematic in relation to this bug is the comment here:

/**
    @note we make an internal temporary table unless the table is a temporary table. In last
    case we just use it as is. Neither of these tables require locks in order to be
    filled.
*/

Unfortunately, the only two ways that plugin::TransactionalStorageEngine::doStartTransaction() is called are the following two ways:

1) If it is explicitly called by the kernel (which happens when a START TRANSACTION/BEGIN WORK; SQL statement is issued, or
2) When mysql_lock_tables() is called. The kernel decides in /drizzled/lock.cc which engines are active in a statement and calls plugin::TransactionalStorageEngine::doStartTransaction() for each transactional engine if it has not already started a transaction for the engine

So, clearly, the comment that "Neither of these tables require locks in order to be filled" is a hint here. Indeed, if we trace through the alter_table() method in /drizzled/statement/alter_table.cc, we find that during the copy phase (where the new table is being filled with a copy of the old table's data, that instead of Session::openTableLock() being called (which DOES call mysql_lock_tables()), instead Session::open_temporary_table() is called, which does NOT call mysql_lock_tables().

Therefore, the solution to this is to manually call plugin::TransactionalStorageEngine::doStartTransaction() *somewhere* before the copying of the data from the old table into the new table (when Cursor::rnd_init() is eventually callled...)

I should have a fix for this by end of day today. Just trying to find the "right" place to put the call to doStartTransaction()...