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

Stop throwing exceptions on commit in PHP8+pdo_mysql combo #1131

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

ostrolucky
Copy link
Member

Q A
Type bug
BC Break no
Fixed issues #1104

Summary

I didn't go with exception swallowing after all, because it turned out PDO has inTransaction() which works here.

MySQL commits implicitly after every DDL query, but since PHP 8, mysql_pdo started to throw exception when attempting to explicitly commit after it implicitly committed. This causes to interrupt the migration process even though all queries completed successfully.

Alternative would be to disable transaction for PHP8+pdo_mysql, but that would affect all queries, not only DDL ones like intended, hence going with guard check before each explicit commit.

Hopefully this will be fixed directly in DBAL in future, but so far this doesn't look very likely...
@ostrolucky ostrolucky added this to the 2.3.3 milestone Mar 13, 2021
@albe
Copy link

albe commented Mar 18, 2021

FYI: @ostrolucky @greg0ire Seeing an odd "Transaction commit failed because the transaction has been marked for rollback only." now with 3.1.1 on PGSQL - Tests pass with dependency fixed on 3.1.0 and nothing else changed 😬:
https://github.com/neos/neos-development-collection/pull/3311/checks?check_run_id=2141073265#step:10:30 ✔️
https://github.com/neos/neos-development-collection/runs/2134647627?check_suite_focus=true#step:19:19

I'm not sure why that only occurs with PGSQL and the Behat tests, and also only in the tests of our CMS and not even the underlying Flow Framework:
https://github.com/neos/flow-development-collection/pull/2365/checks#step:10:32

That hints at that it might be very edge-case scenario, but still something behaves unexpectedly different now. I will try to find out more.

@albe
Copy link

albe commented Mar 21, 2021

I spent some time analyzing the cause. See neos/neos-development-collection#3311 (comment) for the steps I did.
One of our migrations we run seems to leave a failed transaction open missing a rollback with 3.1.1
Looking at the code, the only possible explanation I have (so far) is this:
The $wrappedConnection instanceof PDO && ! $wrappedConnection->inTransaction() must match (we use pdo_pgsql, which isn't affected by the bug this fixes so this is a behaviour change) and prevent one transaction from committing. So this creates a discrepancy between started/committed transactions?
Also note how this happens when migrations are run on an already current schema (doctrine:create without marking migrations applied) - so the migration statements are logically failing. So a failing transaction that is not committed at the expected point in time is probably caught somewhere else without a rollback, leading to the "marked for rollback only" transaction error at the next commit()?

Edit: This hardly explains why the same doesn't happen in our Framework tests though, where we also should have applied migrations onto a freshly created schema.

Anyway, the core thing is: The condition right now slightly changes commit behaviour outside of the scope of the original issue (PHP8+pdo_mysql).

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.

4 participants