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.

Sign up for GitHub

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

Jump to bottom

Fix transactions and data consistency #2041

Closed
wants to merge 12 commits into from
Closed

Fix transactions and data consistency #2041

wants to merge 12 commits into from

Conversation

UlrichThomasGabor
Copy link
Contributor

@UlrichThomasGabor UlrichThomasGabor commented Aug 27, 2021 •
edited by OliverSkroblin

@OliverSkroblin As discussed. #1668 should be merged first. After that I can rebase my branch to the then-current trunk.

1. Why is this change necessary?

This PR fixes various transaction issues and improves data consistency.

2. What does this change do, exactly?

  • All calls to beginTransaction have been replaced with the new RetryableTransaction class, as it provides a cleaner interface anyway and the previous code did not execute rollback on exceptions.
  • MultiInsertQueryQueue now uses RetryableTransaction as well as there is no use case where it is desirable that half of the inserts is in the DB and then the execution of the script stops and the other half is left in the wide nothingness.
  • Introduced transactions at multiple code positions to improve data consistency. Also removed all attempts to be smarter than the DBMS in case of a deadlock and try non-batchy execution of queries; executing single statements not within a transaction can lead to data inconsistency; executing single statements inside of a transaction is in no case beneficial to a batch statement. Catching exceptions also breaks the transaction control flow, i.e. they are not reverted correctly.

3. Describe each step to reproduce the issue or behaviour.

4. Please link to the relevant issues (if any).

This fixes all issues I mentioned in the pull request #1668.
This also fixes https://issues.shopware.com/issues/NEXT-15805.

5. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • I have created a changelog file with all necessary information about my changes
  • I have written or adjusted the documentation according to my changes
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfill them.

6. To-Do-List

  • If you have code-style enforcers, you might want to make sure that nobody introduces new beginTransaction, commit or rollback calls. All transactions should be implemented using RetryableTransaction.
  • I've encountered some other occurrences of catch (\Exception) or catch (\Throwable). This is code-smell, i.e. one likely never wants to catch ALL exceptions. You might want to review (i.e. remove) these.
  • The last commit ("Make batch behave like previous non-batch") makes the code behave like previously, but I don't think it is a good idea to ignore all errors. The best way would be to fix the code such that the IGNOREs are not necessary anymore. The second-best solution is to change RetryableTransaction to pass an argument to the transactional closure (I guess this has to be done via Closure::bind to not interfere with Doctrine), passing in its retry $counter so the closure can determine if this is the first execution or a following one. In the following cases, it could perform an INSERT IGNORE instead.
  • Call $connection->setNestTransactionsWithSavepoints(true) globally.

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2021 •
edited

CLA assistant check
All committers have signed the CLA.

@UlrichThomasGabor UlrichThomasGabor force-pushed the fix-transactions branch 3 times, most recently from 5c081d0 to e22341a Compare August 28, 2021 21:58
@NiklasLimberg
Copy link
Contributor

Hi @UlrichThomasGabor,
thanks for your contrubution I enabled the Pipeline for you as a first-time contributer. This PR will likely go to @Dominik28111 since he has also the PR that your depends on.

@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-16970

Please use this issue to track the state of your pull request.

hanneswernery and others added 12 commits September 6, 2021 17:46
@Dominik28111
Copy link
Contributor

I close this PR since it was merged with 43fe96e.

@UlrichThomasGabor
Copy link
Contributor Author

@Dominik28111 There is a to-do-list at the end and especially:

Call $connection->setNestTransactionsWithSavepoints(true) globally.

is important because otherwise the new RetryableTransaction will not work correctly.

@UlrichThomasGabor UlrichThomasGabor deleted the fix-transactions branch September 27, 2021 11:33
@digitalkaoz
Copy link
Contributor

@UlrichThomasGabor is this fix already released?

@UlrichThomasGabor
Copy link
Contributor Author

@digitalkaoz The PR yes. I did not keep track of the setNestTransactionsWithSavepoints issue

@UlrichThomasGabor
Copy link
Contributor Author

@digitalkaoz 0e3d7f1

@digitalkaoz
Copy link
Contributor

Happens to me a lot when using the sync API at scale. Any workarounds? Downgrading PHP would bei possible but not a solution

@UlrichThomasGabor
Copy link
Contributor Author

What happens a lot (error message)? Since that was merged I encountered not a single transaction-related fault

@digitalkaoz
Copy link
Contributor

a lot of "There is no active transaction" when transaction is autocommitted (during sync API usage)

@digitalkaoz
Copy link
Contributor

Related to this yiisoft/yii2#18406 or this doctrine/migrations#1202 (all related to the same PHP 8 commit)

@digitalkaoz
Copy link
Contributor

Dunno If its even related to your changes, i guessed that

@UlrichThomasGabor
Copy link
Contributor Author

You are using Shopware >= 6.4.5?

My changes should have fixed all that.

The error can still happen if exceptions are caught, which should not be caught, i.e. catching all exceptions (catch \Exception) { // do not rethrow } is very bad. Same goes for DBAL-related exceptions (basically everything in that namespace). You can search all of Shopware and the plugins you use for that pattern. Maybe something was missed (or re-introduced...)

If it's not that, than something else is still done wrong...

@digitalkaoz
Copy link
Contributor

digitalkaoz commented Feb 8, 2022 via email

@digitalkaoz
Copy link
Contributor

digitalkaoz commented Feb 8, 2022 via email •
edited

@UlrichThomasGabor
Copy link
Contributor Author

This issue should be fixed, if the RetryableTransaction class is correctly used. If exceptions are caught, and do not reach the mentioned class; or somebody did something weird, it may occur again anytime.

Like I said, I'd start by looking at all catch blocks

@UlrichThomasGabor
Copy link
Contributor Author

Using commit, beginTransaction or rollback by hand can also break the transaction flow. They should not be used outside of RetryableTransaction

@digitalkaoz
Copy link
Contributor

digitalkaoz commented Feb 8, 2022 via email

@UlrichThomasGabor
Copy link
Contributor Author

Might be that we forgot something last time or something was done wrongly since then. Shopware is changing a lot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Dominik28111 Dominik28111

Labels
Accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@UlrichThomasGabor @CLAassistant @NiklasLimberg @shopwareBot @Dominik28111 @digitalkaoz @mitelg @hanneswernery