Allow skipping setAutoCommit() call in JdbcTransaction

[katsumiy]

Why resetAutoCommit() in close is necessary?

When connection is newly created or popped from pool, setDesiredAutoCommit is called and AutoCommit is set to desired state, anyway.
When connection is not pooled and really closed, resetting AutoCommit appears to be useless. Because it's going to be closed.
So in both pooled and non pooled case, resetAutoCommit appears to be useless.
And setting when popped and resetting when pushed, it exec “SET AUTOCOMMIT” and it’s taking some time to process.
Shouldn't resetAutoCommit in close be removed?

[katsumiy]

SET AUTOCOMMIT is causing peformance hit in our case.

[harawata]

Hello @katsumiy ,

Please provide more information about the performance issue.
We basically need to reproduce the problem on our end before fixing it.
A small demo project like these would be perfect.

[katsumiy]

public class Test {
    public static void main(String[] args) throws Exception {
        try (InputStream in = Test.class.getResourceAsStream("/mybatis-config.xml")) {
            SqlSessionFactory factory = new SqlSessionFactoryBuilder().build(in);

            long startTime = System.currentTimeMillis();

            for (int i = 0; i < 1000; i++) {
                try (SqlSession session = factory.openSession()) {
                    int result = session.selectOne("sample.mybatis.selectTest");
                }
            }

            long endTime = System.currentTimeMillis();

            System.out.println("time :" + (endTime - startTime) + " ms");        }
    }
}
---------------------------------------------
<?xml version="1.0" encoding="UTF-8" ?>
<!DOCTYPE mapper
        PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN"
        "http://mybatis.org/dtd/mybatis-3-mapper.dtd">
<mapper namespace="sample.mybatis">
    <select id="selectTest" resultType="int">
        select 1
    </select>
</mapper>

please use regular pooled data source. We used mysql.

with resetAutoCommit()
this takes 26.614 seconds.

without resetAutoCommit()
this takes 24.235 seconds.

It's 10% diffrence in our test environment.
Our query is always very simple like this sample. So executing SET AUTOCOMMIT twice for every one simple query cause performance hit.
If network between client and server is slower and round robin time is longer, this diffrence would be bigger.

[harawata]

Hi @katsumiy ,

I'm sorry about the belated reply.

This is about JdbcTransaction#resetAutoCommit(), right?
The comment in the method explains the reason why it's necessary.

mybatis-3/src/main/java/org/apache/ibatis/transaction/jdbc/JdbcTransaction.java

Lines 117 to 121 in 395be63

	// MyBatis does not call commit/rollback on a connection if just selects were performed.
	// Some databases start transactions with select statements
	// and they mandate a commit/rollback before closing the connection.
	// A workaround is setting the autocommit to true before closing the connection.
	// Sybase throws an exception here.

So, resetAutoCommit should not be removed because it could break backward compatibility.

The sample code you posted opens a new session for each execution.
You should consider reusing the same session instead.

[katsumiy]

ummm, I think using AutoCommit to commit/rollback active transaction is not good idea.
I don't think commiting active transaction is guaranteed when AutoCommit is changed from false to true. I will check whether changing autocommit value affect active transaction.
And If commit or rollback is really required, commit or rollback should be called explicitly.

also when connection is returned to pool, active transaction should be rolled back not commited.

And if this is work around for Sybase JDBC driver, I think we should have code only for Sybase. Causing performance hit just to work around unknown version and not widely used Sybase driver is not good option. Do we know which version of Sybase driver cause exception? Have we tried latest driver?

The sample code is reduced pseudo code. The DB access code in real code are spread wide and called so often. So session can't be combined.

As you can see below bug for commons DBCP, they have same problem and they put new parameter to disable setting and resettiing auto commit for each borrow.
You should consider this performance hit seriously since they consider seriously.

https://issues.apache.org/jira/browse/DBCP-351

[harawata]

As stated in the comment, MyBatis does not call commit/rollback on a connection if just selects were performed.
This is the documented behavior, so I don't think we can change it at this stage.

And the reason why it calls setAutoCommit(true) seems to be this unique behavior of Derby.

I'm open to suggestions, but the new solution 1) must not call commit() or rollback() in the close() method and 2) must not break Derby compatibility.

Alternatively, you can copy and modify the built-in JdbcTransaction and tell MyBatis to use it instead.
Please see the "transactionManager" section in the doc.
https://mybatis.org/mybatis-3/configuration.html#environments

[harawata]

@katsumiy ,

How about adding a flag as a property of JdbcTransactionFactory?
Please take a look at #2516 and see if it satisfies your requirement.

[katsumiy]

Yes, this definitely fix our issue. Please proceed releasing.

[harawata]

@katsumiy ,
I've merged the PR. You can test it with the latest 3.5.10-SNAPSHOT.
Thank you for your contribution!