MySQL Bugs: #38261: Online backup: BACKUP command does implicit commit.

Bug #38261	Online backup: BACKUP command does implicit commit.
Submitted:	21 Jul 2008 14:37	Modified:	18 Oct 2008 15:47
Reporter:	Rafal Somla	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: Backup	Severity:	S3 (Non-critical)
Version:	mysql-6.0-backup	OS:	Any
Assigned to:	Rafal Somla	CPU Architecture:	Any

Description:
There is code in backup kernel which performs implicit commit during RESTORE operation (introduced ad fix for BUG#36828). In latest merge, this code was moved to a wrong place (Backup_restore_ctx::close() method) and as a result, it happens also during BACKUP command which is wrong.

How to repeat:
Code inspection.

Suggested fix:
Move the implicit commit inside Backup_restore_ctx::do_restore() method.

Here is a patch fixing this issue: http://lists.mysql.com/commits/50097

Fixed synopis.

Test script showing that BACKUP does implicit commit

Attachment: bug.test (application/octet-stream, text), 43 bytes.

Test script showing that BACKUP does implicit commit (corrected).

Attachment: bug.test (application/octet-stream, text), 285 bytes.

REFINED PROBLEM DESCRIPTIONS

See the attached test. After doing BACKUP in a connection, all data changes made in that connection are commited which is wrong.

There are actually 2 reasons for that.

First reason is that an implicit commit, which was originally done only for RESTORE, now is done also after BACKUP. This is because of an incorrect manual merge of the code.

Another reason is that the built-in CS backup driver is starting a consistent snapshot transaction internally. This happens in the same thread which executed BACKUP command. Starting new transaction commits any on-going transactions so the result is that BACKUP commits implicitly if CS driver is used.

Here is the code from be_snapshot.cc which starts the transaction:

result_t Backup::lock()
{
  DBUG_ENTER("Snapshot_backup::lock()");
  /*
    We must fool the locking code to think this is a select because
    any other command type places the engine in a non-consistent read
    state. 
  */
  locking_thd->m_thd->lex->sql_command= SQLCOM_SELECT; 
  locking_thd->m_thd->lex->start_transaction_opt|=
    MYSQL_START_TRANS_OPT_WITH_CONS_SNAPSHOT;
  int res= begin_trans(locking_thd->m_thd);      <====== HERE
  if (res)
    DBUG_RETURN(ERROR);
  m_trans_start= TRUE;
  locking_thd->lock_state= LOCK_ACQUIRED;
  DBUG_ASSERT(locking_thd->m_thd == current_thd);
  DEBUG_SYNC(locking_thd->m_thd, "after_backup_cs_locked");
  DBUG_RETURN(OK);
}

The patch submitted before fixes the first problem. That is, it moves the implicit commit so that it is done only inside RESTORE, not in BACKUP.

The problem with implicit commit done by CS driver remains unsolved.

Note: tehere is a new code developed in the server, which does implicit commit at the beginning and end of a statement if appropriate flags are set. When merging with this code, any explicit commit statements should be removed from the backup code.

On a second thought, I think it is ok and even desirable that BACKUP also performs implicit commit. The point is that it will commit ongoing transaction only in the connection which executes BACKUP. As in this scenario:

BEGIN
<change some data (1)>
BACKUP ...;
<change some data (2)>
COMMIT

If BACKUP does not commit the ongoing transaction, then according to the rules all data changes coming from that transaction will not be included in the backup image. This means that data changes (1) will not be present in the backup image which would probably be not what the user expects.

Instead, the above sequence should be equivalent to

BEGIN
<change some data (1)>
COMMIT
BACKUP ...;
BEGIN
<change some data (2)>
COMMIT

so that changes (1) are included in the backup image. Of course, any transactions running in *other* connections should not be disturbed by BACKUP.

1. I think backup and restore should both do implicit commit.  

   By "implicit commit" it is assumed that it is for the *same
   connection* as the BACKUP and RESTORE command.

2. BACKUP/RESTORE should never do commit for any other connection.

I guess the fix for this has hit main. As part of WL#4284, a new flag was introduced to signal that certain commands will ensure a implicit command before and after its execution. This flag was add to BACKUP and RESTORE commands.

TODO

1. Remove any commit statements from backup code - rely on the parser to do implicit commits, marking BACKUP/RESTORE statements with appropriate flags.

2. Extend backup_commit_restore test so that it also checks BACKUP behaviour.

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/commits/53164

2688 Rafal Somla	2008-09-03
      BUG#38261 (Online backup: BACKUP command does implicit commit)
      
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
      
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
      
      The backup_commit_restore test is extended to check that also BACKUP does
      an implicit commit. A new test scenario is added to check how BACKUP and
      RESTORE commands interact with transactions running in other connections.

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/commits/53827

2688 Rafal Somla	2008-09-11
      BUG#38261 (Online backup: BACKUP command does implicit commit)
            
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
            
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
            
      The patch adds new test backup_commit_backup which tests that BACKUP command 
      performs implicit commit for the current transaction and does not interfere with 
      transactions in other connections.

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/commits/53993

2688 Rafal Somla	2008-09-12
      BUG#38261 (Online backup: BACKUP command does implicit commit)
      
      It was clarified that it is OK for BACKUP to perform implicit commit, so it 
      is not a problem. Also, the server was modified so that SQL commands marked with 
      CF_AUTO_COMMIT_TRANS flag automatically perform implicit commits at the 
      beginning and at the end of execution. The BACKUP and RESTORE commands are 
      already marked with the flag.
      
      Hence, there is no need for doing any commits inside backup kernel code - calls 
      to ha_autocommit_or_rollback() and end_active_trans() are removed from kernel.cc 
      by this patch.
      
      The patch adds new test backup_commit_backup which tests that BACKUP command 
      performs implicit commit for the current transaction and does not interfere with 
      transactions in other connections.

Good to push

Patch looks good. Backup tests run successful. Good to push.

Pushed into mysql-6.0-backup tree.

Pushed into 6.0.8-alpha  (revid:rafal.somla@sun.com-20080912171902-syt5bn26wjmxr2qq) (version source revid:oystein.grovlen@sun.com-20080928182351-x4ojdursc9gl8z4o) (pib:5)

Noted in 6.0.8 changelog.

BACKUP DATABASE now performs an implicit commit, like RESTORE.

Also added BACKUP DATABASE and RESTORE to the list of statements that perform implicit commit:
http://dev.mysql.com/doc/refman/6.0/en/implicit-commit.html