Fix transaction filter and use transactions for all SQL updates
|Assignee:||Dan Gillean||% Done:|
|Category:||Data model / ORM||Estimated time:||8.00 hours|
|Target version:||Release 2.3.0|
|Google Code Legacy ID:||Tested version:||2.2, 2.3|
On Tue, Dec 15, 2015 at 7:17 AM, José Raddaoui <firstname.lastname@example.org> wrote:
I've taken a quick look to the places where we update the IO's nested set and this is what I've found:
In some cases we're using transactions directly over the connection:
But in other cases we use a transaction filter:
And it looks like that filter is not using transactions properly, as the beginTransaction call is commented:
#4 Updated by José Raddaoui Marín about 6 years ago
- Status changed from New to Code Review
- Assignee changed from José Raddaoui Marín to Nick Wilkinson
- Target version set to Release 2.3.0
Ready for code review in PR 283
This fix will wrap almost the entire request process and the related Propel connection in the same transaction. The changes in the database will be rolled back in case of a fatal error, exception or timeout in the process, or they will be committed in successful requests at the end of the process. PropelPDO class takes care to prevent errors due to already in progress transactions, so other calls to beginTransaction, commit and rollback functions in the same request will be ignored, unless the commit or roll back functions are called in other parts without calling first the beginTransaction function, which should be removed if it ever comes out.
I did several tests to check that everything is working as expected:
No transaction is being used in the entire request process.
The transaction starts in line 11 (after some auth and route queries that come before the filters in some cases), and commits just before exiting the current connection in line 344.
I've tested forcing a timeout, a PHP fatal error and an exception (except sfStopException) in different places, in all cases the transaction is rolled back.
We were using a transaction to update the accession counter, it can be seen in lines 90-93 in the log.
As I said earlier PropelPDO takes care of the current transaction from the filter and the one in QubitAccession is not started.
IMPORTANT NOTE: This will rollback the changes in the database if something wrong happens, but it won't do it with the changes made over the Elasticsearch index. So, if a record is successfully created and added to the ES index, but there is an error between that and the end of the request, it will leave a 'ghost' document in the ES index, which links will ref to a 404 Not Found page. That's not an easy fix and, as always, it will require a re-index to remove the 'ghost' document. Now that we have the AtoM worker, it would be nice if we could find some time to add a job to rebuild the search index through the GUI in the future.
OTHER NOTE: This fix only works in HTTP requests; cli tasks and jobs are not using transactions yet. We could use a similar approach in both cases by adding a transaction wrapper in the arBaseTask execute method and in the arBaseJob run method, calling the runJob and the executeTask methods in the child classes from inside that wrapper. This wrapper is already set for jobs to avoid breaking the Worker (without using transactions), but it's not set for tasks where we call the arBaseTask execute method from the child classes instead of the other way around; also, not all the tasks extend arBaseTask. I think it could take around 2 hours to add it and test it in the jobs and at least 4-6 hours to do the same in the tasks, but I'm almost reaching the estimated for this ticket.
#7 Updated by José Raddaoui Marín about 6 years ago
- Status changed from Feedback to Code Review
- Assignee changed from José Raddaoui Marín to Mike Gale
Mike G. found some PropelException catches in the IO save method that would mess with the transaction filter, avoiding the rollback in some cases. I've removed all those catches (from multiple model classes), please Mike G., could you take a quick look again?