Auto start transaction

[leandrogehlen]

Avoid rewrite transactions method using SaveRelationsTrait

[juban]

Hi @leandrogehlen ,

Thank you (again) for your pull request.
I'm not too sure about that one.
Having relations defined doesn't necessary mean that one would want the transaction to start automaticaly.
Adding a property at the behavior level seem a better approach to me and let the developper decide if he wants transactions to start or not.

Thoughts?

[leandrogehlen]

The user, can decide even with this implementation, the only thing changed is, when user use the SaveRelationsTrait, the default behavior will be start a transaction

Adding a property at the behavior level seem a better approach to me and let the developper decide if he wants transactions to start or not.

I thought about it too, but adding property or rewrite the method, i think is the same work.

[leandrogehlen]

But if you think better, to create property, to keep backward compatibility, it's simple implementation.

[juban]

Indeed. I would prefer to add an autoStartTransactions boolean property at the behavior level and let the developer decide if he wants that to rely on the native ActiveRecord implementation or not.

[juban]

Sorry to bother you but in my opinion, the trait method should not be mandatory. I think that we should rather modify the isModelTransactional method directly to deal with the new property and return true if autoStartTransactions is set to true.

[leandrogehlen]

Sorry, i did not pay attention about this methods. But really don't think this methods are necessary, because ActveReccord handle this behavior.
Furthermore, this method(https://github.com/la-haute-societe/yii2-save-relations-behavior/blob/master/src/SaveRelationsBehavior.php#L302) runs before validation, it means that validations like unique or exists will be execuded in transactions and it's a bad thing

[juban]

The start transaction mechanism is there for a purpose: some newly created has one relations need to be saved prior to the main model in order to be correctly validated and linked. If you remove the transaction, you could end up with unwanted orphan records.
So, no, the startTransactionForModel method will surely not be removed. As is, that is needed. Sorry.

[leandrogehlen]

I understand, but applications with big number of records, and a big request number, to execute select command inside transaction is a big problem. You can close this PR i'am using my fork to solve this problem.

[juban]

I'm opened to suggestions ;)

• How would you deal with Has one relations if you don't start a transaction?
• I'm interested in your performance issue. Could you tell me more about that? For instance, how bad the performance decrease? What DBMS are you using?

[leandrogehlen]

How would you deal with Has one relations if you don't start a transaction?

My suggestion is hasOne relations must be saved beforeSave where transaction has been started.

I'm interested in your performance issue. Could you tell me more about that? For instance, how bad the performance decrease?

The links bellow has some issues with transactions

https://www.simononsoftware.com/are-long-running-transactions-bad/
https://stackoverflow.com/questions/1968437/are-long-living-transactions-acceptable

What DBMS are you using?

I'm using mysql/postgres but the transaction issue is common for any DBMS

[juban]

Thank you for the answers. Moving start transaction to beforeSave seems doable though I remember that there was an issue doing this during that event instead of beforeValidate. Let me think about it and I will get back to you as soon as possible. Thanks again.

[leandrogehlen]

Moving start transaction to beforeSave

You don't need to control transactions if you to use beforeSave, beacuse
insertInternal and updateInternal runs beforeSave event
https://github.com/yiisoft/yii2/blob/master/framework/db/ActiveRecord.php#L545
https://github.com/yiisoft/yii2/blob/master/framework/db/BaseActiveRecord.php#L794

and transactions will be started before by ActiveRecord:

https://github.com/yiisoft/yii2/blob/master/framework/db/ActiveRecord.php#L628
https://github.com/yiisoft/yii2/blob/master/framework/db/ActiveRecord.php#L518

Thinking about it, i found another issue:

$modelWithRelation->save(false);

In this case, the beforeValidade will not be executed

[juban]

Hi @leandrogehlen,
Unfortunately, I don't see an easy way to move that in the beforeSave instead of beforeValidate because main model will fail to validate if the foreign key to a Has One relation is set as required.
In that case, Has One model needs to be saved prior to the main model validation (that was the issue I was referring to in my previous comment).
So, one possible solution would be to start transaction during the beforeValidate method only if needed in that specific case.
Can't find a better way. What do you think about it?

[juban]

@leandrogehlen I've just pushed a quick fix. Let me know.

[leandrogehlen]

i don't have deep knowledge about your code.

I think a little strange to save hasOne relation this way,
I think better hasOne entities must be saved before it, in another moment.

If is really necessary this feature, i don't see a solution.
But it is not safe, example:

Request 1

time	action
00:00	start transaction
00:01	executes unique validation (as transaction is started don't see record inserted in request 2)
00:05	insert duplicated record

Request 2

time	action
00:00	start transaction
00:01	executes unique validation
00:03	insert will be executed before request 1, because unique validation take less time then request 1

[juban]

Well, I guess you’re right about the fact that it’s kind of a edge scenario. But not impossible. I agree the transition can lead to issues too for validation purposes.
With the latest commit however, that will happen only if a new has one relation needs to be saved then. I will think of a better way to be able to delete the newly created has one record if anything goes wrong during the main model saving without the transaction mechanism.

[leandrogehlen]

Well, I guess you’re right about the fact that it’s kind of a edge scenario. But not impossible.

I think a crital issue, and if you have big an application with many request, this situation happens frequently

[juban]

Hi @leandrogehlen,
So, I removed the transaction mechanism completely and changed the way new Has One records are handled if they that scenario occurs (rewrote the rollback function in particular). You can use the latest master branch to test that.
I think (and hope) that solution is kind of best of both worlds and suits your needs ;)
Let me know.

[leandrogehlen]

I guess, it was a great solution, and it solve both situations.
Do you would like to keep this PR? I can resolve the conflicts.

[juban]

I will close that one as the initial purpose is kind of out of scope.
Anyway, thank you very much for you useful inputs.