feat(Data::AbstractSessionImpl): add autoCommit property and tests #4261

[aleks-f]

No description provided.

[frwilckens]

This is getting complex and easily confusing. We now have AbstractSessionImpl::setAutoCommit and getAutoCommit which set the flag and don't call any vendor API function. Since AbstractSessionImpl calls

addFeature("autoCommit", &AbstractSessionImpl<C>::setAutoCommit, &AbstractSessionImpl<C>::getAutoCommit);

setFeature("autoCommit", val) now sets this internal flag. In addition, we have Data::MySQL::SessionImpl::autoCommit(string, val) and isAutoCommit(string) which execute SQL statements. Since method open() calls
addFeature("autoCommit", &SessionImpl::autoCommit, &SessionImpl::isAutoCommit);
the previous call of addFeature in AbstractSessionImpl is overriden. Hence the internal flag _autoCommit will not be changed by setFeature("autoCommit"). Thus I changed the implementation of autoCommit and isAutoCommit to heed the flag. autoCommit will call an API function only if the flag really changes. isAutoCommit just returns the internal flag, not calling "select @@autocommit". Hence we rely on keeping track of the autocommit mode for SQL.

[frwilckens]

This fixes the errors in DataMySQL-testrunner. By the way, MySQL has an API call for changing the autocommit mode: mysql_autocommit, which is now used instead of the prepared statements "set autocommit = 1" resp "= 0". By MySQL does not have an API call to get the autocommit mode. You have to issue "select @@autocommit".

[frwilckens]

This solution may work, but it is still problematic. We rely on our book-keeping of the flag AbstractSessionImpl::_autoCommit. Note also that we enforce the semantics of the framework which sets autocommit mode to false during transactions. MySQL does not by itself. It is not an error to do so, but it's also not needed. MySQL also does not tell you if a transaction is under way or not. The framework tries to keep track of this itself. MySQL is rather permissive: "begin" does not change the autocommit mode. (check it out: "begin; select @@transaction; " selects 1) "autocommit = 0" can be submitted after a begin; to no effect on the ongoing transaction. "set autocommit = 1" during a transaction is also fine: it implicitly ends the transaction with a commit.

[aleks-f]

@frwilckens a back end doesn't have to heed the AbstractSessionImpl::_autoCommit flag at all - if it has its own capabilities, it registers its own set/get handlers, which replace the parent class handlers in the handler map and they become inaccessible dead code.

One could argue a purpose of having those flags in the parent at all; I agree that they are most likely not needed and potentially confusing, so if you can get by without them for MySQL and PostgreSQL, we can remove them and, if a need ever arises, re-introduce.

[aleks-f]

@frwilckens I have protected the autoCommit handlers in AbstractSesssionImpl, so they can't be accessed outside of the class hierarchy.

Feel free to implement MySQL autocommit directly in the MySQL session implementation and completely replace the parent class ones - that's exactly what ODBC back end does:

poco/Data/ODBC/src/SessionImpl.cpp

Lines 110 to 112 in 23463b2

	addFeature("autoCommit",
	&SessionImpl::autoCommit,
	&SessionImpl::isAutoCommit);

[frwilckens]

@aleks-f Your comments clarify the issue, thanks. I also replace the implementation of get/setFeature("autoCommit") in the MySQL SessionImpl, like in the ODBC SessionImpl, but I make use of the flag in AbstractSessionImpl to evade calling "select @@autocommit" which led to an error in Statement::checkBeginTransAction(). I'm not sure if there is another solution since MySQL refuses to execute "select @@autocommit" in the contexts where the framework calls checkBeginTranscation. As I said, it may well work fine that way.

[aleks-f]

@frwilckens when PostgreSQL is ready, just uncomment the CI so we can see if it passes:

poco/.github/workflows/ci.yml

Lines 393 to 415 in a2d10df

	# TODO tests sometimes failling on testTransaction and testReconnect
	# linux-gcc-make-postgres:
	# runs-on: ubuntu-22.04
	# services:
	# postgres:
	# image: postgres:16.0
	# env:
	# POSTGRES_PASSWORD: postgres
	# ports:
	# - 5432:5432
	# steps:
	# - uses: actions/checkout@v3
	# - run: sudo apt -y update && sudo apt -y install libssl-dev unixodbc-dev libmysqlclient-dev odbc-postgresql
	# - run: ./configure --everything --no-samples --omit=ActiveRecord,ApacheConnector,CppParser,Crypto,Data/MySQL,Data/ODBC,Data/SQLite,Encodings,JSON,JWT,MongoDB,Net,NetSSL_OpenSSL,NetSSL_Win,PDF,PageCompiler,PocoDoc,ProGen,Prometheus,Redis,SevenZip,Util,XML,Zip && make all -s -j4 && sudo make install
	# - uses: ./.github/actions/retry-action
	# with:
	# timeout_minutes: 90
	# max_attempts: 3
	# retry_on: any
	# command: >-
	# sudo -s
	# EXCLUDE_TESTS="ActiveRecord ApacheConnector CppParser CppUnit Crypto Data Data/ODBC Data/MySQL Data/SQLite Encodings Foundation JSON JWT MongoDB Net NetSSL_OpenSSL NetSSL_Win PDF PageCompiler PocoDoc ProGen Prometheus Redis SevenZip Util XML Zip"
	# ./ci/runtests.sh

[frwilckens]

The postgreSQL changes are ready, but I don’t have the permissions to change the Git workflows.Sent from my iPhoneOn Nov 13, 2023, at 11:01 PM, Aleksandar Fabijanic ***@***.***> wrote: @frwilckens when PostgreSQL is ready, just uncomment the CI so we can see if it passes: https://github.com/pocoproject/poco/blob/a2d10dffe8fb1f6021a4b3bfb175974c9be58465/.github/workflows/ci.yml#L393-L415 —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[aleks-f]

@frwilckens if you can, please add transaction tests. There is now Data/testsuite/DataTest library, which is a generic testing facility for all backends, so we can align them in terms of behavior (as much as is reasonably possible, of course). This will break windows build, so if you can at least adjust the CMake buildsystem, I'll take care of the VS solutions

poco/Data/testsuite/DataTest/include/Poco/Data/Test/SQLExecutor.h

Lines 215 to 218 in ac7e39f

	void sessionTransaction(const std::string& connector, const std::string& connect);
	void sessionTransactionNoAutoCommit(const std::string& connector, const std::string& connect);
	void transaction(const std::string& connector, const std::string& connect);
	void transactor();

[frwilckens]

@aleks-f With the last commits I added last week the pull request is ready if you agree with the changes. As explained elsewhere, I deactivated SQL parsing for PostgreSQL because the current parser does not handle PostgreSQL placeholders. I also changed the generic transaction logic when autocommit is switched off: if parsing is deactivated, any SQL statement starts a transaction. I also did not use the generic tests for PostgreSQL for now, again because of the peculiar placeholder syntax.

All tests pass and I'm not planning further changes for now.

[frwilckens]

I submitted several changes. The current state of the pull request may be debatable, but it passes all tests and I think its choices can be defended as sensible.

[frwilckens]

@aleks-f I'm working on support for numbered placeholders in the generic Data test suite, so that we can finally delegate PostgreSQL tests to it.

[frwilckens]

@aleks-f Most PostgreSQL tests now delegate to Poco::Data::Test. A few could not be delegated because the corresponding generic test did not pass (sometimes for very mundane reasons).

I had to weaken a few assertions for transactions in Poco::Data::Test because the locking behavior is different for different backends. For ODBC, SQL statements block while a transaction in another session is going on, for PostgreSQL they don't. (Oracle also has a lock-free implementation of transactions.) The PostgreSQL behavior is of course more desirable, so we should not require the ODBC behavior in general. The generic tests now leave open if the statement was executed before or after the other session was committed or rolled back.

I did not touch the other backends yet.

[aleks-f]

@aleks-f Most PostgreSQL tests now delegate to Poco::Data::Test. A few could not be delegated because the corresponding generic test did not pass (sometimes for very mundane reasons).

I had to weaken a few assertions for transactions in Poco::Data::Test because the locking behavior is different for different backends. For ODBC, SQL statements block while a transaction in another session is going on, for PostgreSQL they don't. (Oracle also has a lock-free implementation of transactions.) The PostgreSQL behavior is of course more desirable, so we should not require the ODBC behavior in general. The generic tests now leave open if the statement was executed before or after the other session was committed or rolled back.

I did not touch the other backends yet.

That's ok, thanks. I have resolved conflicts and will merge this.