MySQL Bugs: #8650: InnoDB does not rollback SQL statement at an error

Bug #8650	InnoDB does not rollback SQL statement at an error
Submitted:	21 Feb 2005 15:26	Modified:	18 Apr 2005 17:49
Reporter:	Matthias Leich	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S1 (Critical)
Version:	5.0.3-pre	OS:
Assigned to:	Heikki Tuuri	CPU Architecture:	Any

Description:
I have a test case derived from the NIST tests which tries to update 
two records. The update gets the expected error message, 
because of an UNIQUE CONSTRAINT violation.
But a following SELECT reveals that one record was modified.
I set severity to S1 and Priority to P1, because there is no acceptable
workaround for this bug modifying data.

BTW: - It looks like this bug was introduced a bit after 16 February.
        -  I did not check, if a fresh pulled and compiled MySQL 4.1 shows the same bug.

My environment:
   - Intel PC with Linux(SuSE 9.1)
   - MySQL compiled from source
        Version 5.0 ChangeSet@1.1878, 2005-02-21

How to repeat:
Please use my attached test file ml27.test , copy it to mysql-test/t
  ./mysql-test-run ml27

test case

Attachment: ml27.test (application/test, text), 2.56 KiB.

a note: in order to see output of select, I run
mysql-test-run --record ml27 
and then look at ml27.result

the reason is that ha_innobase::external lock does not call

        trans_register_ha(thd, FALSE, &innobase_hton);

for every statement in a transaction.
Being not registered InnoDB does not receive a rollback event.

Sergei,

does InnoDB need to register every start of a statement? I thought that the function only needs to be called to register a new TRANSACTION.

Regards,

Heikki

/*************************************************************************
Registers the InnoDB transaction in MySQL, to receive commit/rollback
events. This function must be called every time InnoDB starts a
transaction internally. */
static
void
register_trans(
/*===========*/
        THD*    thd)    /* in: thd to use the handle */
{
        /* Register the start of the statement */
        trans_register_ha(thd, FALSE, &innobase_hton);

        if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) {

              /* No autocommit mode, register for a transaction */
              trans_register_ha(thd, TRUE, &innobase_hton);
        }
}

register_trans_ha(FALSE) should be called for each statement, register_trans_ha(TRUE) once per transaction.

register_trans() that you quoted does both, registers for the start of transaction and the start of the statement (first statement in a tranaction),

but for the rest of the transaction InnoDB still needs to call register_trans_ha(FALSE).

Hi!

The function could have a more descriptive name. There could be:

trans_register_ha()

and

stmt_register_ha()

If a new SQL statement starts a transaction, do we need to call BOTH trans_register_ha() AND stmt_register_ha()? Or is trans_register_ha() enough?

Is there any harm from calling trans_register_ha() several times during one transaction?

Is there any harm from calling stmt_register_ha() several times before a statement?

Regards,

Heikki

handler.cc:

  DESCRIPTION
    Every storage engine MUST call this function when it starts
    a transaction or a statement (that is it must be called both for the
    "beginning of transaction" and "beginning of statement").
    Only storage engines registered for the transaction/statement
    will know when to commit/rollback it.
*/
void trans_register_ha(THD *thd, bool all, handlerton *ht_arg)
{
  THD_TRANS *trans;
  DBUG_ENTER("trans_register_ha");
  DBUG_PRINT("enter",("%s", all ? "all" : "stmt"));

  if (all)
  {
    trans= &thd->transaction.all;
    thd->server_status|= SERVER_STATUS_IN_TRANS;
  }
  else
    trans= &thd->transaction.stmt;

#ifndef DBUG_OFF
  handlerton **ht=trans->ht;
  while (*ht)
  {
    DBUG_ASSERT(*ht != ht_arg);
    ht++;
  }
#endif
  trans->ht[trans->nht++]=ht_arg;
  trans->no_2pc|=(ht_arg->prepare==0);
  if (thd->transaction.xid.is_null())
    thd->transaction.xid.set(thd->query_id);
  DBUG_VOID_RETURN;
}

ha_innodb.cc:

/*************************************************************************
Registers the InnoDB transaction in MySQL, to receive commit/rollback
events. This function must be called every time InnoDB starts a
transaction internally. */
static
void
register_trans(
/*===========*/
        THD*    thd)    /* in: thd to use the handle */
{
        /* Register the start of the statement */
        trans_register_ha(thd, FALSE, &innobase_hton);

        if (thd->options & (OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) {

              /* No autocommit mode, register for a transaction */
              trans_register_ha(thd, TRUE, &innobase_hton);
        }
}

Hi!

The current code apparently only calls stmt_register_ha() if AUTOCOMMIT=1. But still MySQL commits the WHOLE transaction at the statement end, not just the statement.

Is the rule that in the AUTOCOMMIT=1 mode we should not register the transaction, but register just the statement?

How about the AUTOCOMMIT=0 mode?

It would help a lot if the concepts 'transaction' and 'statement' were separated in handler.cc. A 'transaction' would consist of one or more 'statements'.

Regards,

Heikki

Hi!

Looks like the following code fixes the problems. I still have to think about the of the 5 other places where a transaction can start:

        The flag trx->active_trans is set to 1 in

        1. ::external_lock(),
        2. ::start_stmt(),
        3. innobase_query_caching_of_table_permitted(),
        4. innobase_savepoint(),
        5. ::init_table_handle_for_HANDLER(),
        6. innobase_start_trx_and_assign_read_view(),
        7. ::transactional_table_lock()

Maybe the easiest way to avoid multiple declarations of the same handler for the same statement is to check in ha_register_trans() in handler.cc whether the handler already is declared.

Regards,

Heikki

::start_stmt()

        }

        /* Set the MySQL flag to mark that there is an active transaction */
        if (trx->active_trans == 0) {

                register_trans(thd);
                trx->active_trans = 1;
        } else {
                /* Register the statement started: HOW DO WE KNOW if we have
                registered the statement already? */
                trans_register_ha(thd, FALSE, &innobase_hton);
        }

        return(0);
}

::external_lock()

        if (lock_type != F_UNLCK) {
                /* MySQL is setting a new table lock */

                /* Set the MySQL flag to mark that there is an active
                transaction */
                if (trx->active_trans == 0) {

                        register_trans(thd);
                        trx->active_trans = 1;
                } else if (trx->n_mysql_tables_in_use == 0) {
                        /* Register the statement started */
                        trans_register_ha(thd, FALSE, &innobase_hton);
                }

It would make sense if  indeed the function has a more descriptive name. 
There really should be:

trans_register_ha() to register transaction i.e. this is called once per transaction.

and

stmt_register_ha() to register start of SQL statement i.e. this is called once per
statement.

Regards
    JanL

A patch for this bug has been committed. After review, it may
be pushed to the relevant source trees for release in the next
version. You can access the patch from:

  http://lists.mysql.com/internals/23676

Fixed in 5.0.4