Skip to content

Add transaction support #1904

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

Closed
wants to merge 51 commits into from
Closed

Add transaction support #1904

wants to merge 51 commits into from

Conversation

klinson
Copy link
Contributor

@klinson klinson commented Jan 8, 2020

requires mongodb version V4.0 or more and deployment replica sets or sharded clusters
transaction support create/insert,update,delete,etc operation
Supports infinite-level nested transactions, but outside transaction rollbacks do not affect the commit of inside transactions

DB::beginTransaction();
DB::collection('users')->where('name', 'klinson')->update(['age' => 18]);
DB::transaction(function () {
    DB::collection('users')->where('name', 'mongodb')->update(['age' => 30]);
});
DB::rollBack();

…ployment replica sets or sharded clusters

transaction support create/insert,update,delete,etc operation
Supports infinite-level nested transactions, but outside transaction rollbacks do not affect the commit of inside transactions
```
DB::beginTransaction();
DB::collection('users')->where('name', 'klinson')->update(['age' => 18]);
DB::transaction(function () {
    DB::collection('users')->where('name', 'mongodb')->update(['age' => 30]);
});
DB::rollBack();
```
@Smolevich
Copy link
Contributor

@klinson check failed tests in travis build please

@klinson
Copy link
Contributor Author

klinson commented Jan 9, 2020

@Smolevich Because transaction requires mongodb version V4.0 or more and deployment replica sets or sharded clusters, but your environment doesn't.

@Smolevich
Copy link
Contributor

@klinson, Not my environment, it is common configuration based on official docker images, you can change if it's needed

@Smolevich
Copy link
Contributor

@klinson if you work on PR, add 'WIP' to title of PR

@klinson klinson changed the title add transaction support [WIP] Add transaction support Jan 10, 2020
@klinson klinson changed the title [WIP] Add transaction support Add transaction support Jan 13, 2020
@klinson
Copy link
Contributor Author

klinson commented Jan 13, 2020

@Smolevich I've modified the environment to support it

@klinson klinson requested a review from jenssegers January 17, 2020 02:33
@Smolevich
Copy link
Contributor

@jenssegers i prepare Smolevich/demo-mongo-application#9 for demo. It is not finished, but today i finish

toocool
toocool previously approved these changes Jan 17, 2020
@Smolevich
Copy link
Contributor

@klinson do you have plans to add changes for PR?

@klinson
Copy link
Contributor Author

klinson commented Jan 22, 2020

I will continue to change it

@josemiguelq
Copy link
Contributor

josemiguelq commented Mar 26, 2021

Any news guys? Can you review please @jenssegers ?

@klinson
Copy link
Contributor Author

klinson commented Mar 27, 2021

review please @Smolevich

@klinson klinson requested review from divine and jenssegers March 30, 2021 10:10
@klinson klinson requested a review from Smolevich April 23, 2021 03:03
@mattg66

This comment has been minimized.

@redbonzai

This comment has been minimized.

@MageloPST

This comment was marked as spam.

@salvationarinze

This comment was marked as spam.

@vinkev

This comment was marked as spam.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented, I would remove hard-coding of transaction options as well as the ability to start multiple transactions in parallel.

In addition to that, this PR is missing the implementation of the DB::transaction() helper. While the default logic would work, the MongoDB\with_transaction function shipped with the MongoDB driver contains logic designed to avoid re-invoking the callback for errors that only require a second commit command. The retry logic of that function is a little different from the default logic, but that difference should not have any negative effects.

Comment on lines +311 to +313
'readPreference' => new ReadPreference(ReadPreference::RP_PRIMARY),
'writeConcern' => new WriteConcern(1),
'readConcern' => new ReadConcern(ReadConcern::LOCAL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid hardcoding these options this way, especially with the given values. There is no prior art for passing options (as most methods in the query builder don't accept any options), but an optional $transactionOptions parameter for this method would be more sensible than hardcoding these values here.

public function beginTransaction()
{
$this->session_key = uniqid();
$this->sessions[$this->session_key] = $this->connection->startSession();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the session from the connection instance is applied automatically to all operations sent through the query builder, I'd refrain from starting multiple sessions. Looking at the transaction logic used for PDO, the first call to beginTransaction will start a new transaction, while subsequent only create save points (or no-op if those are not supported). I generally believe this to be a better alternative that should be used instead.

@henriquetroiano
Copy link
Contributor

Any update on this?

@alcaeus
Copy link
Member

alcaeus commented Nov 10, 2022

I have an almost-done version of this locally and will aim to have a new pull request up in the coming days. Stay tuned...

@henriquetroiano
Copy link
Contributor

I have an almost-done version of this locally and will aim to have a new pull request up in the coming days. Stay tuned...

Thank you @alcaeus

@alcaeus alcaeus mentioned this pull request Nov 14, 2022
@divine
Copy link
Contributor

divine commented Nov 27, 2022

Closed in favor of #2465, thank you for your work!

@divine divine closed this Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet