"It can be left for another patch, but we should really be doing these mass changes in a single database transaction so that all of the changes are either successful or a failure as an atomic unit. This will take some work in the AbstractDao class to make it possible." - bd808
This will take some work in the AbstractDao class to make it possible.
@Nehajha has noted in an email that Wikimedia\Slimapp\Dao\AbstractDao already uses transactions internally in its update and insert methods. MySQL does not allow nested transactions, so if we want to allow grouping of multiple update and/or insert actions in a single transaction some changes will be needed in AbstractDao. There are several ways that this could be done. One that comes to mind would be to add methods to AbstractDao for starting, committing, and rolling back a transaction. If the start and commit methods implemented a form of counting semaphore then commit could be made to only actually commit the whole transaction when the count of commit calls matches the count of start calls. AbstractDao could continue to wrap its implementation of update and insert in AbstractDao::transactionStart/AbstractDao::transactionCommit calls. This would work the same as it does today under most circumstances. In the grant review application it would then be possible to make a call to AbstractDao::transactionStart before the foreach loop in Wikimedia\IEGReview\Dao\Reviews::insertOrUpdateReview and then a final AbstractDao::transactionCommit at the end.
This work should be done as a follow up to D894: Updating iegreview codebase to use wikimedia slimapp library which @Niharika and I still have not properly reviewed. Sorry about that. I'll try to get to it soon.
I think you can start on the patch for slimapp that will make it's AbstractDao class capable of simulating nested transactions. Then you can use that feature in the grants app after we merge the conversion patch from D894.
Did you mean to keep a track of the locks acquired by the transaction in AbstractDao::transactionStart/transactionCommit?
Yes, one way to do this would be to keep an open transaction counter in the AbstractDao class. If the count is 0 and transactionStart is called then the class would call $this->dbh->begintransaction(); and in either case increment the counter. When transactionCommit is called it would decrement the counter and if resulting count is 0 it would call $this->dbh->commit();. If transactionRollback is called it would call $this->dbh->rollBack(); and reset the counter to 0.
This particular abstraction will allow a group of AbstractDao::update and AbstractDao::insert calls to be wrapped in a set of AbstractDao::transactionStart and AbstractDao::transactionCommit calls and committed atomically if desired. While double checking some of the details of this solution I found a description of the same basic algorithm with sample code in a comment on the PDO::beginTransaction help page.
A related, but more complex, solution is described another comment on the PDO::beginTransaction help page. That example uses the SAVEPOINT feature of MySQL's InnoDB table type to implement a more complete nested transaction capability where inner sections can be rolled back independently. I don't think that we need this additional complexity now, but it would be easy enough to add later if desired.
@Nehajha: I am resetting the assignee of this task because there has not been progress lately (please correct me if I am wrong!). Resetting the assignee avoids the impression that somebody is already working on this task. It also allows others to potentially work towards fixing this task. Please claim this task again when you plan to work on it (via Add Action... → Assign / Claim in the dropdown menu) - it would be welcome. Thanks for your understanding!