Bug #9401

Fix transaction filter and use transactions for all SQL updates

Added by David Juhasz over 6 years ago. Updated about 6 years ago.

Status:VerifiedStart date:02/03/2016
Priority:HighDue date:
Assignee:Dan Gillean% Done:

0%

Category:Data model / ORMEstimated time:8.00 hours
Target version:Release 2.3.0
Google Code Legacy ID: Tested version:2.2, 2.3
Sponsored:No Requires documentation:

Description

On Tue, Dec 15, 2015 at 7:17 AM, José Raddaoui <> wrote:

Hi David,

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:

https://github.com/artefactual/atom/blob/qa/2.3.x/lib/model/om/BaseInformationObject.php#L813

But in other cases we use a transaction filter:

https://github.com/artefactual/atom/blob/qa/2.3.x/lib/model/om/BaseInformationObject.php#L603
https://github.com/artefactual/atom/blob/qa/2.3.x/lib/model/om/BaseInformationObject.php#L698

And it looks like that filter is not using transactions properly, as the beginTransaction call is commented:

https://github.com/artefactual/atom/blob/qa/2.3.x/lib/filter/QubitTransactionFilter.class.php#L30


Related issues

Related to Access to Memory (AtoM) - Bug #4194: Move an information object can break the nested set Verified
Related to Access to Memory (AtoM) - Bug #9533: QubitJob sometimes has a race condition due to SQL transa... Verified 03/04/2016

History

#1 Updated by David Juhasz over 6 years ago

  • Related to Bug #4194: Move an information object can break the nested set added

#2 Updated by Jesús García Crespo over 6 years ago

  • Assignee deleted (Jesús García Crespo)

#3 Updated by Nick Wilkinson over 6 years ago

  • Assignee set to José Raddaoui Marín

#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:

IO creation before the fix (log):

No transaction is being used in the entire request process.

IO creation after the fix (log):

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.

Failed IO creation after the fix (log):

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.

Accession creation before the fix (log):

We were using a transaction to update the accession counter, it can be seen in lines 90-93 in the log.

Accession creation after the fix (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.

#5 Updated by Nick Wilkinson about 6 years ago

  • Assignee changed from Nick Wilkinson to Mike Gale

Hi Mike, assigning to you for CR.

#6 Updated by Mike Gale about 6 years ago

  • Status changed from Code Review to Feedback
  • Assignee changed from Mike Gale to José Raddaoui Marín

lgtm

#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?

#8 Updated by Mike Gale about 6 years ago

  • Status changed from Code Review to Feedback
  • Assignee changed from Mike Gale to José Raddaoui Marín

looks good

#9 Updated by José Raddaoui Marín about 6 years ago

  • Status changed from Feedback to QA/Review
  • Assignee changed from José Raddaoui Marín to Dan Gillean

Merged in qa/2.3.x

#10 Updated by Mike Gale about 6 years ago

  • Related to Bug #9533: QubitJob sometimes has a race condition due to SQL transactions added

#11 Updated by José Raddaoui Marín about 6 years ago

A side effect of using transactions in the jobs will be that the info about each job in the manage jobs page will only be updated at the beginning and at the end of the job.

#12 Updated by Dan Gillean about 6 years ago

Is there some way we should be testing this, Radda? We've had the ticket open for a month, during which we've done a lot of other testing... so far so good. But if there are specific areas I should look at, please let me know.

#13 Updated by Jesús García Crespo about 6 years ago

  • Status changed from QA/Review to Verified

General testing is all we needed. I'll mark as verified, thanks Fiver.

Also available in: Atom PDF