-
Notifications
You must be signed in to change notification settings - Fork 29
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 postgresql queries for API Server scanner #1878
Conversation
OBorce
commented
Feb 5, 2025
•
edited
Loading
edited
- added a cache table for the latest delegation states so that we can efficiently retrieve all the latest data for each delegation of a pool
- added delegations balance in the pool endpoints
6e25173
to
1b1d9de
Compare
api-server/api-server-common/src/storage/impls/postgres/queries.rs
Outdated
Show resolved
Hide resolved
2df3f28
to
36c2f3e
Compare
- add a cache table to keep the latest delegations
5f7cbf7
to
39a0c55
Compare
39a0c55
to
9babfb2
Compare
async fn get_address_balances( | ||
&self, | ||
address: &str, | ||
) -> Result<Vec<(CoinOrTokenId, Amount, u8)>, ApiServerStorageError> { |
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 u8
is cryptic. In such a case it's better to at least leave a comment, e.g.
Result<Vec<(CoinOrTokenId, Amount, /*number of decimals*/ u8)>
(though rustfmt may not like such comments sometimes)
But since this tuple appears in public interfaces, it's better to make it a struct with named fields and use the struct everywhere.
P.S. it's also a bit strange that a vec is returned instead of a map with CoinOrTokenId as the key.
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.
replaced with DecimalAmount
api-server/api-server-common/src/storage/impls/postgres/queries.rs
Outdated
Show resolved
Hide resolved
api-server/api-server-common/src/storage/impls/postgres/queries.rs
Outdated
Show resolved
Hide resolved
self.tx | ||
.execute( | ||
"INSERT INTO ml.coin_or_token_decimals (coin_or_token_id, block_height, number_of_decimals) VALUES ($1, $2, $3) ON CONFLICT (coin_or_token_id) DO NOTHING;", | ||
&[&coin_or_token_id.encode(), &height, &(issuance.number_of_decimals as i16)], |
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.
"ON CONFLICT (coin_or_token_id) DO NOTHING" IMO looks strange.
In the context of this particular function probably UPDATE would make more sense? I mean, what does set_fungible_token_issuance
mean in the context of the storage - the token somehow got "re-issued", so probably we should just update the data.
In general, storage methods should make sense on their own. And same methods from the postgres and in-memory storage should probably behave in the same way.
And e.g. the in-memory storage makes no particular assumptions on when a token re-issuance can occur - it just adds a new issuance object for the provided token_id
/block_height
.
So I guess postgres should do the same, assuming that the same token can be issued multiple times at different heights. I.e. coin_or_token_decimals
should have block_height
as a part of the key and queries that use coin_or_token_decimals
should take this into account.
But then the next question arises - why isn't token decimals value a part of ml.fungible_token
? And the coin decimals never change, so there isn't much sense in putting them into the db. So coin_or_token_decimals
seems redundant.
P.S. another weird thing about coin_or_token_decimals
is that it's partially exposed from the storage interface via del_coin_or_token_decimals_above_height
.
Where the in-memory storage implementation just does nothing.
IMO it's another sign that it should go.
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.
Removed the on conflict as a token can be issued only once, if there are multiple then it should be an error.
separated the set_fungible_token_issuance into 2 set_fungible_token_issuance and set_fungible_token_data, first for the initial issuance that can happen only once per token_id and the later for any modification to the token data from mint/change token authority ...
The new table is just a cache for faster lookups when updating address balances.
.await | ||
.map_err(|e| ApiServerStorageError::LowLevelStorageError(e.to_string()))?; | ||
|
||
let coin_or_token_id = CoinOrTokenId::TokenId(token_id); | ||
self.tx | ||
.execute( | ||
"INSERT INTO ml.coin_or_token_decimals (coin_or_token_id, block_height, number_of_decimals) VALUES ($1, $2, $3);", | ||
&[&coin_or_token_id.encode(), &height, &0_i16], | ||
) | ||
.await | ||
.map_err(|e| ApiServerStorageError::LowLevelStorageError(e.to_string()))?; | ||
|
||
Ok(()) |
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.
Same comment.
And btw probably the above "INSERT INTO ml.nft_issuance" should also have "ON CONFLICT ... UPDATE" so that it behaves just like its in-memory counterpart.
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.
Doesn't have an on conflict because it cannot happen same as the insert of the nft above it.
Added a panic in the in-memory counterpart.
ec854aa
to
4186a49
Compare
4186a49
to
2b01a6b
Compare
…ken-authority Add address token authority endpoint to API Server