-
Notifications
You must be signed in to change notification settings - Fork 510
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
/db/history: update the claimable balance SQL query #4398
/db/history: update the claimable balance SQL query #4398
Conversation
…IT depending on whether paging cursor parameters are present or not
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.
After performing several more benchmarking experiments, I think we actually need to place the limit clause based on whether or not the claimants
filter is present.
Regardless of pagination, if the user is filtering on claimants
, the limit clause should be outside the subselect query:
") select " + claimableBalancesSelectStatement + " from cb LIMIT ?"
If the claimants
filter is not present, the LIMIT
clause should always reside in the subselect:
"LIMIT ?) select " + claimableBalancesSelectStatement + " from cb"
I tested with the following combinations:
- filtering only
asset
- filtering
asset
with pagination - filtering only
claimants
- filtering
claimants
with pagination - filtering only
sponsor
- only pagination
- filtering
asset
andclaimants
with pagination
the assets
filter sees great performance improvement by always having the LIMIT
clause within the subselect. Other combinations only had negligible improvement. Conversely, the claimants
filter requires the LIMIT
clause to always exist outside the subselect for optimal performance.
@2opremio , I can adjust this to assemble the sql query based on @sydneynotthecity 's profiling, should I proceed on that or hold off? |
Oh crap, I deleted the branch by mistake. Let me recreate it. |
Done! No, we shouldn't hold off! |
…ce query by claimant id per profiling
@sydneynotthecity @2opremio , I reworked that claimant balance query for LIMIT placement per the profiling results and recommendations. If the query has asset and claimant parameters, then i chose to let that follow the LIMIT placement per the claimant, i.e. outside query. |
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.
One question regarding whether we need to add a space to the SQL syntax, otherwise looks good!
// we need to use WITH syntax and correct LIMIT placement to force the query planner to use the right | ||
// indexes, otherwise when the limit is small, it will use an index scan | ||
// which will be very slow once we have millions of records | ||
limitClausePlacement := "LIMIT ?) select " + claimableBalancesSelectStatement + " from cb" |
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.
Do we need to worry about adding a space before LIMIT
? Or does the original statement insert the necessary space? Like " LIMIT ?) select "
instead
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.
Hello @sydneynotthecity , yes, looks like the squirrel Suffix
takes care of spacing on concatenation, i dumped the final emitted sql under both conditions and looks ok with spacing:
# limit on the outer
WITH cb AS ( SELECT cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags FROM claimable_balances cb WHERE (cb.last_modified_ledger, cb.id) > (?, ?) AND cb.claimants @> '[{"destination": "GC3C4AKRBQLHOJ45U4XG35ESVWRDECWO5XLDGYADO6DPR3L7KIDVUMML"}]' ORDER BY cb.last_modified_ledger asc, cb.id asc ) select cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags from cb LIMIT ?
# limit on the inner
WITH cb AS ( SELECT cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags FROM claimable_balances cb ORDER BY cb.last_modified_ledger asc, cb.id asc LIMIT ?) select cb.id, cb.claimants, cb.asset, cb.amount, cb.sponsor, cb.last_modified_ledger, cb.flags from cb
thanks for double checking on that!
* #4382: refined the placement of LIMIT clause on claimant balance query by claimant id per profiling results.
* #4382: refined the placement of LIMIT clause on claimant balance query by claimant id per profiling results.
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Change the claimable balance SQL Query generation to position LIMIT clause dependent on presence of cursor parameters as that determines how many effective rows and the indexes are chosen by db query planner.
Why
the query planner isn't picking the right indexes to use on claimable balance query due to LIMIT clause
Known limitations