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
Transactional Database Mode #281
Conversation
added function documentation transaction-support for PostgreSQL added
good idea! |
Travis fails because the mysqli extension didn't support begin_transaction() until php 5.5. |
Interesting, 6144d08 passed on Travis. Were the later commits necessary? Did you benchmark running the tests? Travis gives the impression that tests are taking longer to run with this PR. |
Sadly yes - i haven't found a way yet to setup a local test-suite, so several errors slipped through. Intrestingly 6144d08 passed, but i don't know why .. it should have failed, as there was still the PHP 5.5+ Implementation of MySQLi begin_transaction() Benchmark ? I considered it an coincidence that tests ran longer - but i wasn't able to find how to have them retestet by Mr. Travis, as |
I restarted the test and it's passed now! Very good idea to use transactions!! This results in speed improvements, because the Innodb INDEXes are written only once when the transaction is committed, rather than being written after each UPDATE/INSERT queries. |
I am happy you like it. This is not a factor yet - but in case it ever will, i'd like to suggest to return a "transaction-ID" on beginTransaction, which has to be submittet in the commit-call ( and therefore to be saved ) .. what's your oppinion on that? |
@@ -25,6 +25,8 @@ class Mysqli extends Db | |||
protected $username; | |||
protected $password; | |||
protected $charset; | |||
private $_activeTransaction = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not prefix variables with _ so please name member: $activeTransaction
Good point, I don't see that we will use transactions within Tracking API appart from this wrapping around Bulk requests. |
Before merging here is one change that is important:
PS: I can make the change if you're not comfortable with it, just let me know! |
Will do as suggested by you. |
No we should not use transactions for "single call", let's only use transaction in Bulk mode (see previous comment). I could make this change if you wish, no problem... |
please review again. |
Matt, any reason why you did not merge? |
@medic123de the release was coming and didn't want to make such big change prior to our major release. I'll investigate soon and will change it to use transactions only when count($requests) >= 2 |
Oh, sorry. I've been worried code quality isn't sufficient, or there were some issues left I missed. |
I created a ticket for this change here: http://dev.piwik.org/trac/ticket/5275#ticket |
; Whether Bulk tracking requests will be wrapped within a DB Transaction. ; This greatly increases performance of Log Analytics and in general any Bulk Tracking API requests. bulk_requests_use_transaction = 1
I made the couple small changes in a pull request on your fork https://github.com/medic123de/piwik/pull/2/files - once you merge it I think we can merge this PR! |
Only use transactions for bulk requests (more than one request)
Use transactions for Tracking API requests in bulk. fixes #5275 Thanks for the pull request, it's an Awesome Performance Improvement!
@medic123de Well done, this was an epic change 🎱 |
Standard MySQL ( InnoDB ):
without-optimization:
Total time: 350 seconds
Requests imported per second: 13.41 requests per second
without-optimization + transaction:
Total time: 78 seconds
Requests imported per second: 543.68 requests per second