-
Notifications
You must be signed in to change notification settings - Fork 117
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
tapdb: Add Universe indices, optimize SQL queries, RWMutex
for cache
#1174
tapdb: Add Universe indices, optimize SQL queries, RWMutex
for cache
#1174
Conversation
8d3ca1f
to
1188e50
Compare
Pull Request Test Coverage Report for Build 11937864141Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
note: incorporating postgres functionality now for the test |
632ee1f
to
d3561e5
Compare
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.
Awesome work on analyzing and optimizing the universe tables! Did a first pass just to get some early feedback in.
tapdb/sqlc/migrations/000024_universe_optimization_indexes_queries.up.sql
Outdated
Show resolved
Hide resolved
tapdb/sqlc/migrations/000024_universe_optimization_indexes_queries.up.sql
Outdated
Show resolved
Hide resolved
tapdb/sqlc/migrations/000024_universe_optimization_indexes_queries.up.sql
Outdated
Show resolved
Hide resolved
708cb3a
to
3410624
Compare
64937c7
to
c50ed7f
Compare
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.
Looks good, thanks for the updates!
We're mostly down to code style, so getting closer.
b4507b3
to
c43e3ae
Compare
@guggero thanks so much for the feedback. I've made those changes, which include some sweeping changes to both the indices and the performance test. I opted to use the |
Can you please squash the two middle commits so it's a bit easier to review? Going to take another look next week. |
c43e3ae
to
0b43e3f
Compare
d6d801b
to
ea678ae
Compare
Sounds good. I've also updated the PR description to reflect the lock changes and analysis from the query plans. |
RWMutex
for cache
ea678ae
to
42a2449
Compare
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.
Very nice! Extremely close, going to assign another reviewer as soon as the build tags are working (see commit I pushed).
8579403
to
d0d0ba5
Compare
d0d0ba5
to
de6da1f
Compare
@guggero thanks for the round of feedback! I've patched that commit onto the performance test commit in this PR. I've also updated the code style based on the formatting rules doc. Thanks a bunch for this reference. |
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.
Nice, thanks a lot! LGTM 🎉
) | ||
SELECT asset_id, supply, COUNT(*) as num_leaves | ||
FROM asset_supply | ||
GROUP BY asset_id, supply` |
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.
why not compile these queries with sqlc?
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.
(nit, non blocking)
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.
They would then be part of the "production" code, which we don't necessarily want. So I think in tests it's fine to have pure SQL, as long as it's compatible with both SQLite and Postgres.
kicking itests, probably flake |
Universe Query Performance Optimization and Lock Safety
This PR introduces two key improvements:
tapdb
universe queriesLock Safety Improvements
Key changes:
Performance Optimizations: Strategic Indexing
Key Changes
Performance Impact
PostgreSQL
Asset Stats Query: 4.58x improvement (205ms → 44.8ms)
Universe Leaves Query: 1.24x improvement (20.9ms → 16.8ms)
Root Query: 1.05x improvement (17.7ms → 16.9ms)
SQLite
Asset Stats Query: 2.02x improvement (55.9ms → 27.7ms)
Universe Leaves and Root Queries: Maintain baseline performance
Write Performance
Technical Details
Query Cost Analysis
PostgreSQL shows significant cost reductions:
The improvements come from:
Implementation Considerations
Testing
Next Steps
The PR delivers two key improvements: significantly better query performance through strategic indexing and enhanced concurrency safety through improved locking patterns. Both changes maintain reasonable overhead while providing substantial benefits for production deployments.