Add logic for transactional commits #2589

alcaeus · 2023-11-24T10:14:24Z

Q	A
Type	feature
BC Break	no
Fixed issues

Summary

~~This PR contains a commit from #2587, which will be removed when rebasing.~~

After refactoring commit logic in #2580 to no longer clear changesets and scheduled document actions after executing them, this pull request allows for running all UnitOfWork::commit operations within a multi-document transaction. Note that the feature does not check whether the deployment supports transactions, but it always acts if the configuration setting is enabled.

Included in this PR is a test change that enables transactional commit for all tests if the deployment supports it. This effectively tests transactional commit logic when testing with replica sets (introduced in this PR) or sharded clusters (already present). Standalone deployments continue to test legacy logic.

The commit logic makes use of the convenient transaction API, which takes care of starting the transaction, committing it, and restarting the entire transaction if necessary. The convenient API retries transactions for up to 2 minutes when encountering transient errors, which may be undesirable. As the API does not allow changing this timeout by design, we have to decide whether we want to keep using the convenient API, or rather introduce our own logic for retrying transactions that uses a lower timeout. Note that the 2-minute limit has been set since the introduction of the convenient API, and has not been challenged since. This PR may be a reason to suggest changing the specification to allow for lower timeouts, as a 2-minute timeout makes no sense in a web-based environment where we'd rather show the user a quick error page instead of delaying a page response. Either way, this issue should not come up in practice unless you're dealing with highly contentious parallel workloads, which I'd argue is quite rare.

Documentation for this feature will be introduced in a separate PR.

With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour.

jmikola · 2023-11-27T16:25:36Z

Note that the 2-minute limit has been set since the introduction of the convenient API, and has not been challenged since.

This is where client-side operations timeout comes into play. timeoutMS is intended to supersede the non-configurable, two-minute default for withTransaction(). See CSOT: Convenient Transactions API for more details.

jmikola · 2023-11-27T16:30:47Z

lib/Doctrine/ODM/MongoDB/Persisters/DocumentPersister.php

@@ -1562,6 +1563,10 @@ private function getQueryForDocument(object $document): array
     */
    private function getWriteOptions(array $options = []): array
    {
+        if ($this->isInTransaction($options)) {
+            return $options;


Realistically, is writeConcern the only option that would be inherited below? I know specifying per-operation RC/WC options is not permitted, but just want to make sure nothing else is being overlooked from getDefaultCommitOptions() that may be permitted.

If so, this logic should be rewritten so that only the writeConcern gets ignored.

Rethinking this, writeConcern is not the only one. Technically speaking, getDefaultCommitOptions may return a host of options that we can't control. Given that, I think it makes most sense to strip transaction options in DocumentPersister as well, after they've been assembled from default commit options, document options, and the specific commit options. This not only ensures that we never pass forbidden options to a write operation within a transaction, but it also future-proofs this code in case other options are eventually added to default commit options or the document commit options.

lib/Doctrine/ODM/MongoDB/UnitOfWork.php

tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php

tests/Doctrine/ODM/MongoDB/Tests/Functional/AtomicSetTest.php

tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php

tests/Doctrine/ODM/MongoDB/Tests/Functional/DocumentPersisterTest.php

tests/Doctrine/ODM/MongoDB/Tests/Functional/ShardKeyTest.php

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php

jmikola

Suggestion to use "majority" over "2" for the WC test, but LGTM otherwise.

jmikola · 2023-11-28T15:37:46Z

tests/Doctrine/ODM/MongoDB/Tests/UnitOfWorkTest.php

+        $logger = new CommandLogger();
+        $logger->register();
+
+        $this->uow->commit(['writeConcern' => new WriteConcern(2)]);


"majority" may be safer as that would allow the test to run against a single-member replica set.

alcaeus · 2023-11-29T08:58:46Z

phpstan errors are unrelated and are fixed separately in #2593.

* Extract commit logic * Support transactional commit operations * Always use transactional flush if supported With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour. * Test with MongoDB 7.0 * Update test names * Update phpstan baseline * Fix query selection in shard key tests * Flip transaction options constant by default * Use supportsTransaction method when skipping tests * Apply review feedback to tests * Add separate test to check write concern in commit options * Strip write options when in transaction * Use majority write concern in test

* Run CI workflows on feature branches * Refactor commit logic (#2580) * Add tests for commit consistency showing wrong behaviour * Clear scheduled document changes at the end of a commit operation * Rename private variables to communicate intent * Remove obsolete comment * Use different error code * Use single mongos in consistency tests This ensures that the fail points are created on the same server that the write operations take place on, which can't be guaranteed in a sharded cluster with multiple mongoses. * Rename test methods for clarity * Explain reasoning for index error * Extract helper method to create failpoint * Add configuration setting for transactional flush (#2587) * Add configuration setting for transactional flush * Use classic setters for transactional flush setting * Add logic for transactional commits (#2589) * Extract commit logic * Support transactional commit operations * Always use transactional flush if supported With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour. * Test with MongoDB 7.0 * Update test names * Update phpstan baseline * Fix query selection in shard key tests * Flip transaction options constant by default * Use supportsTransaction method when skipping tests * Apply review feedback to tests * Add separate test to check write concern in commit options * Strip write options when in transaction * Use majority write concern in test * Update events to play nice with transactions (#2594) * Pass session and transaction information to event args * Only dispatch lifecycle events once per commit operation * Remove isInTransaction property in event args * Split method signature for readability * Use property promotion for event args classes * Extract construction of eventArgs * Inline spl_object_hash calls * Avoid injecting test instance * Add session to commitOptions in persister * Add session assertions in LifecycleEventManager * Only retry transaction once (#2604) * Only retry transaction once * Rename variable * Update transaction documentation (#2606) * Remove references to transactions where not applicable * Update transaction documentation * Apply suggestions from code review Co-authored-by: Jeremy Mikola <jmikola@gmail.com> * Change title level in event documentation * Add documentation about transactions to event docs --------- Co-authored-by: Jeremy Mikola <jmikola@gmail.com> * Address code review items * Test highest dependencies on MongoDB 7.0 * Regenerate psalm baseline to silence unrelated errors --------- Co-authored-by: Jeremy Mikola <jmikola@gmail.com>

alcaeus added the Feature label Nov 24, 2023

alcaeus added this to the 2.7.0 milestone Nov 24, 2023

alcaeus requested review from jmikola, GromNaN and malarzm November 24, 2023 10:14

alcaeus self-assigned this Nov 24, 2023

alcaeus added 7 commits November 27, 2023 13:50

Extract commit logic

6ab86ce

Support transactional commit operations

d42955e

Always use transactional flush if supported

221e749

With this commit, all tests using the document manager use transactional flush as long as transactions are supported. Certain tests can use the static $allowsTransactions variable to disable this behaviour.

Test with MongoDB 7.0

bc3dd25

Update test names

0b6a73c

Update phpstan baseline

7139226

Fix query selection in shard key tests

7c9b920

alcaeus force-pushed the transactions/transactional-commit branch from 9bf352d to 7c9b920 Compare November 27, 2023 12:53

jmikola reviewed Nov 27, 2023

View reviewed changes

alcaeus added 5 commits November 28, 2023 12:43

Flip transaction options constant by default

51e177c

Use supportsTransaction method when skipping tests

2717102

Apply review feedback to tests

9cd986b

Add separate test to check write concern in commit options

47d204f

Strip write options when in transaction

b253760

alcaeus requested a review from jmikola November 28, 2023 11:44

jmikola approved these changes Nov 28, 2023

View reviewed changes

Use majority write concern in test

925f46b

alcaeus merged commit 7c32c01 into doctrine:feature/transactions Nov 29, 2023
19 of 20 checks passed

alcaeus deleted the transactions/transactional-commit branch November 29, 2023 08:59

alcaeus removed this from the 2.7.0 milestone Jan 17, 2024

alcaeus mentioned this pull request Jan 17, 2024

Transaction support #2586

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logic for transactional commits #2589

Add logic for transactional commits #2589

alcaeus commented Nov 24, 2023 •

edited

Loading

jmikola commented Nov 27, 2023

jmikola Nov 27, 2023

alcaeus Nov 28, 2023

jmikola left a comment

jmikola Nov 28, 2023

alcaeus commented Nov 29, 2023

Add logic for transactional commits #2589

Add logic for transactional commits #2589

Conversation

alcaeus commented Nov 24, 2023 • edited Loading

Summary

jmikola commented Nov 27, 2023

jmikola Nov 27, 2023

Choose a reason for hiding this comment

alcaeus Nov 28, 2023

Choose a reason for hiding this comment

jmikola left a comment

Choose a reason for hiding this comment

jmikola Nov 28, 2023

Choose a reason for hiding this comment

alcaeus commented Nov 29, 2023

alcaeus commented Nov 24, 2023 •

edited

Loading