[jira] Commented: (AMQ-1886) Transactions used in a JDBC store WITHOUT a journal will not commit the UOW in an atomic operation

["Torsten Mielke (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/activemq/browse/AMQ-1886?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=45041#action_45041 ] 

Torsten Mielke commented on AMQ-1886:
-------------------------------------

I don't think AMQ-1886 is entirely fixed yet. The error handling still needs improvements. 

Assuming the MySQLJDBCAdapter class uses batchStatements (which seems the default for MySQL and Oracle and surely others), the call to MySQLJDBCAdapter.doAddMessage() only adds the SQL statements to the batch, it does not execute the statements:

{code:title=DefaultJDBCAdapter.doAddMessage()}
if (batchStatments) {
   s.addBatch();
} 
{code}

At the end of MemoryTransactionStore.commit() we call persistenceAdapter.commitTransaction(ctx), which calls into TransactionContext.commit() and this is where we execute the SQL statements in executeBatch(). 
{code:title=TransactionContext.commit()}
public void commit() throws IOException {
        if (!inTx) {
            throw new IOException("Not started.");
        }
        try {
            executeBatch();
            if (!connection.getAutoCommit()) {
                connection.commit();
            }
        } catch (SQLException e) {
            JDBCPersistenceAdapter.log("Commit failed: ", e);
            throw IOExceptionSupport.create(e);
        } finally {
            inTx = false;
            close();
        }
    }
{code}
So far all is fine.

Now, lets assume an sql exception is caught during the call to executeBatch(). Before re-raising the ex, we run the finally clause in TransactionContext.commit(), which currently sets inTx=false prior to calling close().

Inside close() we again check for inTx and since it was set to false already (in finally clause), we run executeBatch() again, this time without a transaction. This will result in some message being written directly to the db until we hit another sql exception.

IMHO, the finally clause in TransactionContext.commit() should swap the order and call close() first before setting inTx=false.

{code:title=TransactionContext.commit()}
...
finally {
  close();
  inTx = false;
}
{code}

What do you think?

Then there is a second problem. When I swapped the order of close() and inTx=false there were no messages written to the db in case of hitting an exception during the call to executeBatch(). However, if the sql exception was raised after message 49 out of 50 msgs to be written, we should somehow call a rollback on the jdbc connection. With the change above, these msgs won't get written to the db as we do not commit the sql insert but we also don't rollback explicitly in our error handling. I presume the db itself will run a rollback at some stage (timeout or whatsoever) but to have a clean implementation, we will need to rollback in our exception handling.

Here is a proposed implementation of TransactionStore.commit():

{code:title=TransactionContext.commit()}
public void commit() throws IOException {
        if (!inTx) {
            throw new IOException("Not started.");
        }
        try {
            executeBatch();
            if (!connection.getAutoCommit()) {
                connection.commit();
            }
        } catch (SQLException e) {
            JDBCPersistenceAdapter.log("Commit failed: ", e);

  	  //we need to rollback before the finally clause, 
	  //either by calling 
          this.rollback(); 
          //or connection.rollback();

        throw IOExceptionSupport.create(e);
        } finally {
            close();
            inTx = false;
        }
    }
{code}
What do you think about this change? I have not tested the explicit call to rollback() and don't claim to understand the source well enough to understand possible side effects of this change. 


> Transactions used in a JDBC store WITHOUT a journal will not commit the UOW in an atomic operation
> --------------------------------------------------------------------------------------------------
>
>                 Key: AMQ-1886
>                 URL: https://issues.apache.org/activemq/browse/AMQ-1886
>             Project: ActiveMQ
>          Issue Type: Bug
>    Affects Versions: 5.1.0
>            Reporter: Hiram Chirino
>            Assignee: Hiram Chirino
>             Fix For: 5.2.0
>
>
> The JDBC store's primary use case has always been to be used in conjunction with the journal.  When the journal is in place it recovers any partially committed transaction and makes the JDBC store consistent.  When the journal is not used, for example when you are setting up an HA solution with an HA JDBC database, then it has been noticed that on a DB failure you may get partial commits of JMS transactions.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.