Implement MySQL transactions without changing the autocommit mode.

[frwilckens]

I ran into the same problem as the author of #2007: starting a MySQL transaction changes the autocommit mode of the Session to false, while commit() and rollback() don't reset it. Session.begin() therefore has surprising side effects. If you work with a session pool and put the session back without restoring the autocommit mode, other users of the pool will get a session without the default autocommit mode on.

If you experiment with the test program, you can see that the "_pSession->begin();" statements have no function. The autocommit mode is already set to false at the start of SQLExecutor::sessionTransaction() and SQLExecutor::transaction(). It is therefore not necessary to call "_pSession->begin();" to start a MySQL transaction; this statement only sets the internal SessionImpl::_inTransaction variable to true. If you remove the _pSession->begin(); statements from the test program, there are still MySQL transactions, but the assertions (_pSession->isTransaction()); will (confusingly) fail. The "local" session will not see the inserted Person records until _pSession is committed, because after the previous _pSession->commit() and _pSession->rollback() MySQL immediately started a new transaction, and they are no longer auto-committed.

MySQL allows to work with transactions even in autocommit mode, see https://dev.mysql.com/doc/refman/8.0/en/innodb-autocommit-commit-rollback.html. These are temporary transactions. After a commit and rollback the session resumes autocommit of each statement.

I therefore reimplemented SessionHandle::startTransaction() to just execute the statement "START TRANSACTION". The modified test program shows that the autocommit mode is not changed by having temporary transactions running. In the test program, autocommit is explicitly set to true, but it works as fine when it's false.

I think this is the correct way to handle it. We can still set the autocommit mode by "setFeature("autoCommit", val);" , but you don't need to in order to get transactions. By the way, it was not possible to execute "START TRANSACTION" before this change: Poco only supports prepared statements, but MySQL (at least the version I have) does not support "START TRANSACTION" as a prepared statement: you have to execute it with "mysql_query" or "msyql_real_query".

I have an equivalent change running in a production system and it works fine. I'm aware, however, that it may break old code that relied on the previous semantic (the side effect of having autocommit mode switched off).

[frwilckens]

I made changes to the test program as well, but this is only to bring out the point that the transactions work in autocommit mode. The modified code passes the unchanged test program.

[aleks-f]

#4167

[aleks-f]

@frwilckens please check this branch and see if it fixes the problem:

[aleks-f]

@frwilckens not devel branch, the 4198-poco-data-fixes-improvements branch. you should try this locally and let me know

[frwilckens]

I'll look into it.

[frwilckens]

@aleks-f This is a large change set, and I'm not sure I'll do it justice. That said, it seems to me that the changes to the Transaction class address the issue described in the first paragraph above: starting a transaction in MySQL sets the autocommmit mode to false, and rollback() and commit() don't set it back. With #4167, Poco now officially sets the autocommmit mode to false during a transaction and restores it afterwards. However, as far as I can see this is only done when you use the Transaction class. If you just use Session::begin() and Session::rollback() or commit(), you still have the problem. I'm only referring to MySQL here - the story may be different for ODBC.

As I see it, the problem is still that MySQL::SessionHandle::startTransaction() is implemented as setting the autocommit mode to false. It follows that in Data::Transaction,

void Transaction::begin()
{
	if (!_rSession.isTransaction())
	{
		if (_autoCommit)
			_rSession.setFeature("autoCommit", false);
		_rSession.begin();
	}
	else
		throw InvalidAccessException("Transaction in progress.");
}

now sets the autocommit mode to false _twice (for MySQL), where the second call is harmless but redundant.

As I argue in #4143, I think starting a transaction and changing the autocommit mode are two different things. We can start a transaction in MSQL with the SQL statement "start transaction". This works in autocommit mode: it will create an "ad hoc" transaction. rollback() and commit() will end it, and MySQL will automatically revert to autocommit mode afterwards. Hence, for MySQL, it is not necessary to manage this manually (as is done for Transaction in this change set). On the other hand, autocommit mode is switched off, it is not even necessary to start a transaction: each rollback and commit will automatically start a new transaction.

I think https://dev.mysql.com/doc/refman/8.0/en/innodb-autocommit-commit-rollback.html is clear about this.

To put it otherwise, in the current implementation Poco does not support ad hoc transactions in autocommit mode, and I think this is needlessly limiting.

[frwilckens]

@aleks-f I was assuming in my previous comment that #4198 was meant as an alternative to #4143, not used in addition to it.

[aleks-f]

@frwilckens I understand.

About the alternative, yes - the proper fix for this is through Data::Session, not directly in the MySQL SessionHandle - that leaves all other back-ends in bad shape. Basically, what I have done in Data::Transaction should have been done in Data::Session.

If you can give me a hand here and implement your fix that way, it will help to release it in 1.13.0. I can't promise I'll get to it myself for that release.

I plan to merge #4198 into devel by Monday and you can branch from there, that's probably the best route to avoid conflicts.

[frwilckens]

@aleks-f OK I'll give it a try. I have to get familiar with the other backends, thus there is some learning involved in order to get it right.

[aleks-f]

@frwilckens you don't need to worry about back ends. On Data::Session::begin(), check if the session has "autoCommit" property; if it does and it's true, on begin() store it and set it to false, then restore it back on commit() or rollback(). Essentially, the logic has to be moved from Data::Transaction to Data::Session - it's all abstract, no need to dig into implementations.

[aleks-f]

@frwilckens devel branch is now ready, you can work from it: 5131fe1

[aleks-f]

@frwilckens I'll close this pull and you can create a new one

[aleks-f]

#4167

[frwilckens]

@aleks-f I think it is not that easy. There are two parts that have to be changed. First, as you mentioned, the abstract logic has to be moved from Transaction to Session. But secondly, something like my fix has to remain in MySQL::SessionHandle, to distinguish between starting a session and changing the autocommit mode. I have some ideas how to reconcile both things. By the way, I discovered that the PostgreSQL and SQLite backends already have something like my proposed implementation for SessionHandle::startTransaction(): they execute the SQL statement "BEGIN", equivalent to "START TRANSACTION" for MySQL. Hence I think that my proposed fix brings the MySQL backend in line with the other backends. The only exception is ODBC about which I don't know enough to have an opinion.

[frwilckens]

See Poco::Data::PostgreSQL::SessionHandle::startTransaction() and Poco::Data::SQLite::SessionImpl::begin().

[aleks-f]

@frwilckens I don't think you understand what I am proposing here. It is clear that implementations have to know how to start/commit/rollback transaction and I am not opposing any changes/fixes thereof. But I want Poco::Data::Session to ensure that a session implementation, if (1) it has autoCommit feature, and (2) if that feature is true, is put into autoCommit=false mode for the duration of the transaction, and on commit/rollback returned back to its original autoCommit=true state. If the session implementation has no "autoCommit" feature, or has it but the feature is set to false, Data::Session will do nothing to it on transaction.

I will implement the Data::Session part tomorrow in devel branch and let you know, and then you can send the MySQL changes you have.

[aleks-f]

Now that I look better at this pull, how did MySQL transactions ever work when there was no starting transaction at all, only setting autocommit to false?

[aleks-f]

And to be clear: a session in autocommit mode needs no transaction because every statement is automatically commited. When a transaction is started, a session in autocommit mode must be taken out of it for the duration of the transaction, otherwise that would not really be a transaction. So, as I'm reading it from the diff, your fix to the SessionHandle is correct, but the test fixes are not, eg. this:

_pSession->begin();
assertTrue (_pSession->getFeature("autoCommit")); // did not change

during the transaction, a session can not be in autocommit mode.

[frwilckens]

MySQL transactions worked because turning off the autocommit mode of a session makes MySQL implicitly start a transaction. If autocommit is off, you are always in a transaction, and rollback and commit do start a new transaction. We indeed have a disagreement here. My understanding is that you can have a transaction in MySQL while autocommit mode is on. By executing "BEGIN" or "START TRANSACTION", you start a working transaction despite autocommit being on. The transaction is ended by either "rollback" or "commit", and from then on each statement is, again, automactically committed. I'm quite sure about that because I have this logic in a production system and it works fine: I use "START TRANSACTION" and "rollback" or "commit" without ever changing the autocommit mode: the sessions remain in the default autocommit mode.

Even more: Starting a transaction is needed only when autocommit mode is on, because when it is off, you are always in a transaction. All you need when autocommit mode is off is executing statements, followed by "rollback" or "commit".

I'm not as familiar with PostgreSQL, but https://www.postgresql.org/docs/current/sql-begin.html states that it behaves the same way: BEGIN implicitly prevents the following statements from being committed as isolated statements, until the next explicit ROLLBACK or COMMIT.

I think what is confusing here is a subtle ambiguity of "the session is in autocommit mode". In one sense it means that outside of explicit BEGIN ... COMMIT/ROLLBACK blocks, each statement is automatically committed. This is what I usually mean by "autocomit mode". But in another sense it just means plainly that each statement is its own transaction and is automatically committed. In this second sense, autocommit mode is switched off during a transaction; but this is done automatically by the database, you don't need to do this explicitly in client code.

[aleks-f]

Alright, that makes sense. I really don't have lots of experience with MySQL or PostgreSQL, those libraries came from external contributors that are long gone now.

So, basically, if a back end implementation does not want the framework to tinker with autocommit, it should not expose the "autoComit" feature. But for consistency from the framework point of view, it would be good to have that feature (implemented in terms of the code you removed), so user can have more control over it.

[frwilckens]

The user still can change the auto-commit mode for MySQL backends with setFeature("autoCommit", val).

[aleks-f]

We need to have consistency across different back ends, which is not an easy thing, but that's the path we took and we should stay on it - otherwise this whole effort makes no sense.

I agree that the current MySQL back-end behavior of taking an autocommit session out of the autocommit mode, without putting it back, is wrong and should be fixed. I also agree that throwing on a transaction begin attempt on autocomit session is not the right thing to do, thus #4167

But I disagree with this statement (although I think it is just a terminology disagreement, I think I understand what you mean, more on it below*):

Starting a transaction is needed only when autocommit mode is on, because when it is off, you are always in a transaction.

MySQL:

If autocommit mode is enabled, each SQL statement forms a single transaction on its own

Postgres:

PostgreSQL executes transactions in “autocommit” mode, that is, each statement is executed in its own transaction

I think we have a disagreement here about how we interpret things. With autocommit on, a session is in a "transaction" mode during a statement execution - a mode accessible to us only for asynchronous statements (or from another thread), and there's possibly some work we need to do there.

But let's first address a simple, synchronous execution, case: a session is in transaction when BEGIN TRANSACTION statement is issued. Thats how our framework works and that is consistent with how databases work - a session which did not begin executing is not considered to be in a transaction. It is only in transaction if BEGIN TRANSACTION was executed, explicitly or implicitly, which makes this valid:

_pSession->setFeature("autoCommit", true);
assertTrue (!_pSession->isTransaction());
_pSession->setFeature("autoCommit", false);
assertTrue (!_pSession->isTransaction());

*I'm guessing what you mean by "being in transaction when autocommit mode is off" is that the first statement carries with it an implicit "BEGIN TRANSACTION", and in order for the statements issued in that mode to have final effect (there are some "intermediate" effects, with visibility depending on the transaction isolation), a commit is needed. I agree. But that's a different thing from saying that session becomes transaction by just changing its autocommit to off; we should say that a session with autocommit off is in a transaction only after a statement was executed, not before - that would make sense. Saying that taking it out of autocommit mode puts it in transaction is not the right thing to do in my opinion and I'm not inclined to allow such logic into the framework.

[frwilckens]

No worries, I'm not proposing something like this. When I wrote, "when autocommit mode is off, you are always in a transaction", this was a loose way of talking. As long as you did not submit a DML statement, there is nothing to commit or rollback, hence in this sense you are not in a transaction. I should have said "when autocommit mode is off, each DML statement implicitly starts a transaction that lasts until the next commit or rollback."

Truth is, I'm not sure how the _pSession->isTransaction() flag should be handled. It's tricky. If autocommit mode is on, I agree with what you wrote above: you need to issue a START TRANSACTION statement. I think if autocommit mode is off, it is harmless to issue a START TRANSACTION in MySQL or PostgreSQL, but it is not needed. (Oracle even gives you an error when you do (see https://stackoverflow.com/questions/35178808/how-do-i-use-transaction-with-oracle-sql). But you are right, if you don't issue a START TRANSACTION, what does _pSession->isTransaction() indicate?

[aleks-f]

Here's how transactions will be handled in the ODBC back-end, it's the behavior that the front-end will assume, so that's how all the back ends should behave because they all reside behind the common front end:

• autoCommit ON, every statement is a transaction - nothing for us to do

• autoCommit OFF, transaction automatically starts on the first statement and ends with Session::commit() or rollback(); whether explicit start transaction was issued or not, we don't know and don't care - ODBC driver knows what to do; in fact, there is no way to explicitly start transaction through ODBC API because there's no need for that. The problem here is that there seems to be no way to find out from ODBC API whether a session is in an ongoing transaction.

  I'm thinking about introducing SQLParser into Data library, and only start transaction if there are data-modifying statements in the query. It won't be a perfect solution, because we can't know what is actually happening in every single case (because of SQL extensions), but it should be better than what we have now - telling user we are not in transaction when we actually are.