Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed MySQL session reset #2801

Open
wants to merge 3 commits into
base: poco-1.10.0
Choose a base branch
from

Conversation

jk8
Copy link

@jk8 jk8 commented Oct 4, 2019

  • In Data/MySQL/src/SessionHandle.cpp changed calling of mysql_reset_connection() to query FLUSH
  • previously used mysql_reset_connection() caused issues with encoding
  • see comment in Data/MySQL/src/SessionHandle.cpp:181 SessionHandle::reset() for more info

Related issues

#2546
#2800

Jan Kukuczka added 3 commits October 3, 2019 14:16
- changed calling of mysql_reset_connection() to query FLUSH
- previously used mysql_reset_connection() caused issues with encoding
- see comment in Data/MySQL/src/SessionHandle.cpp:181 SessionHandle::reset() for more info
@obiltschnig obiltschnig self-assigned this Oct 10, 2019
@obiltschnig obiltschnig added this to the Release 1.10.0 milestone Oct 10, 2019
@obiltschnig
Copy link
Member

obiltschnig commented Jan 22, 2020

FLUSH STATUS seems to affect more than the current session:

This option adds the session status from all active sessions to the global status variables, resets the status of all active sessions, and resets account, host, and user status values aggregated from disconnected sessions. See Section 26.12.14, “Performance Schema Status Variable Tables”. This information may be of use when debugging a query. See Section 1.7, “How to Report Bugs or Problems”.

Not sure if this is the right way to do it.

@frwilckens
Copy link
Member

frwilckens commented Dec 8, 2023

Using "FLUSH STATUS" instead of mysql_reset_connection() is problematic. It may solve a special problem with character encodings, but it leads to other problems. It changes the semantics. For example, the "FLUSH" statement causes an implicit commit, while mysql_reset_connection() rolls back any active transaction and resets autocommit mode. Clearly, you want to do the latter when you return a Session to a SessionPool, not the former.

@frwilckens
Copy link
Member

As an experiment I added test
void MySQLTest::testSessionPoolAndUnicode() from the pull request to the other MySQL tests in the devel branch without changing SessionHandle::reset(), and the test passes on my machine. I'm using MySQL 8.0.23. Of course, this is not a comprehensive test but it raises the question under which conditions the encoding issue comes up in the first place.

aleks-f added a commit that referenced this pull request Dec 22, 2023
 (#4262)

* fix(Data::AbstracSessionImpl): protect autocommit feature handlers #4261

* chore(CI): re-enable mysql

* MySQL SessionImpl: make sure autocommit mode is on when session is openend or reset.

* PostgreSQL SessionImpl: reuse autocommit flag of AbstractSessionImpl.

* Github workflow: re-activated linux-gcc-make-postgres

* Fixed indentation in ci.yml

* Fix for DataTest SQLExecutor: use connector

* Data::Session: when parser is not used and autocommit mode is off, assume any SQL statement begins a transaction.

* PostgreSQL: don't use SQL parser (it currently cannot handle placeholders).

* PostgreSQL: added test sessionTransactionNoAutoCommit

* PostgreSQL test suite: removed reference to generic SQLExecutor

* PostgreSQL: fixes for sessionTransactionNoAutoCommit.

* MySQL: added test sessionPoolAndUnicode (from #2801)

* Fixed #define in sql-parser

* Data generic testsuite: support numbered placeholders

* PostgreSQL test suite: added missing include directory to Makefile.

* Attempt to fix PostgreSQL Makefiles

* PostgreSQL testsuite: added include path to Makefile

* PostgreSQL testsuite: added PocoDataTest library to Makefile

* DataTest SQLExecutor::formatSQL: don't use string_view

* PostgreSQL test suite: delegated most tests to Poco::Data::Test

* Makefile: added dependencies on Data-Tests

* Weaken assumptions about async in generic transaction tests

* Makefile: added dependency for Prometheus samples

* Fix deadlock in DataTest SQLExecutor

* PostgreSQL tests SQLExecutor: cleanup

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

* Brought MySQL backend in line with _autoCommit flag of AbstractSessionImpl.

---------

Co-authored-by: Friedrich Wilckens <frwilckens@gmail.com>
Co-authored-by: Friedrich Wilckens <friedrich.wilckens@ingramcontent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants