-
Notifications
You must be signed in to change notification settings - Fork 108
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 NftRepository queries in web3 #8070
Optimize NftRepository queries in web3 #8070
Conversation
Signed-off-by: Xin Li <xin@swirldslabs.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8070 +/- ##
============================================
- Coverage 92.33% 92.32% -0.02%
+ Complexity 7182 7180 -2
============================================
Files 893 893
Lines 29100 29100
Branches 3508 3508
============================================
- Hits 26870 26867 -3
- Misses 1447 1449 +2
- Partials 783 784 +1 ☔ View full report in Codecov by Sentry. |
@@ -0,0 +1,3 @@ | |||
drop index if exists nft_history__timestamp_range; | |||
create index if not exists nft_history__account_timestamp_range | |||
on nft_history using gist (account_id, timestamp_range); |
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.
btree_gist
index gives us the best performance and it's much better than different btree indexes tested.
There may be concern if requiring btree_gist
index will break other mirror node operators, I believe it should be fine as it's a trusted extension in default installation.
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.
Adding breaking
label. Let's remember to mention in the release notes upgrading section.
} | ||
|
||
func (d dbParams) toDsn() string { | ||
return fmt.Sprintf("postgres://%s:%s@%s/%s?sslmode=disable", d.username, d.password, d.endpoint, d.name) | ||
return fmt.Sprintf("postgres://%s:%s@%s/%s?sslmode=disable", ownerUsername, d.ownerPassword, d.endpoint, dbName) |
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.
resort to use mirror_node
user in rosetta integration tests due to the cleanup script and deleting data in some test case setup.
ideally since now it's changed to use the same init.sh
as other modules, we should really use mirror_rosetta
which is readonly to run the integration tests. Will create a ticket as follow-up.
Signed-off-by: Xin Li <xin@swirldslabs.com>
3c47825
to
84251dc
Compare
hedera-mirror-rosetta/test/db/db.go
Outdated
} | ||
|
||
options := &dockertest.RunOptions{ | ||
Name: getDbHostname(network.Network), | ||
Repository: "postgres", | ||
Tag: "14-alpine", | ||
Env: env, | ||
Mounts: []string{fmt.Sprintf("%s/../hedera-mirror-importer/src/main/resources/db/scripts/init.sh:/docker-entrypoint-initdb.d/init.sh", moduleRoot)}, |
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.
the module dockertest
requires host path to be absolute
Signed-off-by: Xin Li <xin@swirldslabs.com>
Signed-off-by: Xin Li <xin@swirldslabs.com>
@@ -2196,6 +2193,32 @@ private EntityId nftPersistHistorical( | |||
.timestampRange(Range.openClosed( | |||
historicalBlock.lowerEndpoint() - 1, historicalBlock.upperEndpoint() + 1))) | |||
.persist(); | |||
|
|||
// nft table | |||
domainBuilder |
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.
for any nft in the nft_history
table, there must be the same nft by (token_id, serial) in nft
table.
Signed-off-by: Xin Li <xin@swirldslabs.com>
Quality Gate passedIssues Measures |
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.
LGTM
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
Description:
This PR optimizes queries in web3 NftRepository
gist
index onaccount_id, timestamp_range
and drop the timestamp_range indexinit.sh
to createbtree_gist
extension for v1btree_gist
extension in existing dbinit.sh
Related issue(s):
Fixes #8064
Notes for reviewer:
Performance comparison, the first
explain analyze
is frommain
, the second is from the PR branch:countByAccountIdAndTimestampNotDeleted
nftBalanceByAccountIdTokenIdAndTimestamp
findNftTotalSupplyByTokenIdAndTimestamp
Checklist