Skip to content
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

Optimize queries for account_tx to work around SQLite query planner #2312

Closed
wants to merge 1 commit into from

Conversation

mtrippled
Copy link
Collaborator

No description provided.

@mtrippled mtrippled requested review from bachase and ximinez December 31, 2017 20:35
@ripplelabs-jenkins
Copy link
Collaborator

ripplelabs-jenkins commented Dec 31, 2017

Jenkins Build Summary

Built from this commit

Built at 20180101 - 05:16:58

Test Results

Build Type Result Status
clang.debug.unity 970 cases, 0 failed, t: 381s PASS ✅
coverage 970 cases, 0 failed, t: 614s PASS ✅
clang.debug.nounity 968 cases, 0 failed, t: 290s PASS ✅
gcc.debug.nounity 968 cases, 0 failed, t: 307s PASS ✅
gcc.debug.unity 970 cases, 0 failed, t: 431s PASS ✅
clang.release.unity 969 cases, 0 failed, t: 470s PASS ✅
gcc.release.unity 969 cases, 0 failed, t: 493s PASS ✅

@mtrippled
Copy link
Collaborator Author

The unit test is failing because the manual filter threw off the LIMIT clause's results. I have to rethink this.

@mtrippled
Copy link
Collaborator Author

I rewrote the SQL in the original instead of making a manual filter. It passes unit tests and improves performance.

Copy link
Contributor

@nbougalis nbougalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no SQL optimization expert, but the new query should generate the same results and looks reasonable.

@sublimator
Copy link
Contributor

How big is a full history sqlitedb these days? Is it even still possible?

@bachase
Copy link
Collaborator

bachase commented Jan 2, 2018

👍

@mtrippled mtrippled added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jan 2, 2018
@ximinez
Copy link
Collaborator

ximinez commented Jan 2, 2018

Nit: Remove the . from the commit message.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm honestly a little surprised that the implicit join (FROM AccountTransactions, Transactions) is faster than the INNER JOIN used in prefix, but data don't lie.

👍

@mtrippled
Copy link
Collaborator Author

@ximinez You're right that the original query should be slightly faster because it only joins on those 2 fields once, whereas the new version essentially takes the union of 2 distinct search queries that each join on those 2 fields. But the query planner for the original version wasn't picking up the index for one or both of the operands of the OR expression in the query.

@nbougalis nbougalis changed the title Filter SQL results in lieu of a search clause. Optimize queries for account_tx to work around SQLite query planner Jan 8, 2018
@bradsavon
Copy link

This is referenced in https://ripple.com/dev-blog/rippled-version-0-81-0/ but not merged?

@mtrippled
Copy link
Collaborator Author

@bradsavon It will get merged into the develop branch, which will turn into 0.90. This was put into the current production code because it will contribute to stability right away.

@bradsavon
Copy link

Great, thanks for clarification

@nbougalis nbougalis mentioned this pull request Jan 10, 2018
@seelabs
Copy link
Collaborator

seelabs commented Jan 16, 2018

In 0.90.0-b3

@seelabs seelabs closed this Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants