storage/consensus: Add index for transaction type #708
Conversation
ptrus
force-pushed
the
ptrus/feature/consensus-method-idx
branch
from
98c5f7c to
1c78e00
Compare
| @@ -0,0 +1,5 @@ | |||
| BEGIN; | |||
|
|
|||
| CREATE INDEX ix_transactions_method ON chain.transactions (method); | |||
There was a problem hiding this comment.
this kind of index usually ends up needing height at the end, otherwise the api can't get "recent transactions of x method" without walking this index for all transactions of a method and sorting them or ignoring this index and scanning chronologically and filtering by method
There was a problem hiding this comment.
Do you think It might make sense to update the ix_transactions_sender index in the same way?
Most of the accounts probably have a relatively low number of transactions which are fine, but there are likely some accounts with huge amount of transactions which could cause problems for the "recent transactions of x sender" query, since it would need to sort the filtered list of transactions which could be large?
Likely way less problematic, I should probably find the accounts with most transactions and test the queries on a live nexus deployment.
There was a problem hiding this comment.
Re to the above, yes I think we should, after some tests:
- https://testnet.nexus.stg.oasis.io/v1/consensus/transactions?sender=oasis1qq39ldve0caf5z2m0h7ml8xe8jfcuq5wyqqtlh77 took 15+ seconds
- https://testnet.nexus.stg.oasis.io/v1/consensus/transactions?sender=oasis1qzzt2hvv3meptnqymae7ylzsfkxcvwx0s5heeylj took ~10 seconds
- https://testnet.nexus.stg.oasis.io/v1/consensus/transactions?sender=oasis1qq39ldve0caf5z2m0h7ml8xe8jfcuq5wyqqtlh77 ~5 seconds
- https://testnet.nexus.stg.oasis.io/v1/consensus/transactions?sender=oasis1qzj7ju3smh62hpgypmcpj46760uzj3cpegqx4hwr -timeouts
There was a problem hiding this comment.
Adding the ix_transactions_sender_block index fixes the above examples to be quick.
|
ok on removing fee and code filters 🤷 seems like they would have been an easy way to DoS |
ptrus
force-pushed
the
ptrus/feature/consensus-method-idx
branch
2 times, most recently
from
88aa18c to
c671266
Compare
ptrus
force-pushed
the
ptrus/feature/consensus-method-idx
branch
2 times, most recently
from
77add2a to
736cd5f
Compare
| @@ -0,0 +1,8 @@ | |||
| BEGIN; | |||
There was a problem hiding this comment.
Added these indexes might take non-trivial time. Do we have any special requirements or handling of such migrations that aren't super quick?
The specific changes in this migration are not breaking and could actually be made whenever (don't need to happen at deployment), so these can be handled that way, but i'm asking more for any general guidelines.
There was a problem hiding this comment.
We don't have a great general solution.
Specifically for indexes, we sometimes run "CREATE INDEX CONCURRENTLY ..." manually in psql, in advance.
If you do this, you'll want CREATE INDEX IF NOT EXISTS in the migratino.
I'm not sure what happens if you use CONCURRENTLY in the migration, i.e. if the migration will conclude super fast and then the index will be created later, while nexus is running.
There was a problem hiding this comment.
Thanks for the info. For now I updated just update the migration to do CREATE INDEX IF NOT EXISTS so that we are more flexible on how to handle this deployment.
| @@ -0,0 +1,8 @@ | |||
| BEGIN; | |||
There was a problem hiding this comment.
We don't have a great general solution.
Specifically for indexes, we sometimes run "CREATE INDEX CONCURRENTLY ..." manually in psql, in advance.
If you do this, you'll want CREATE INDEX IF NOT EXISTS in the migratino.
I'm not sure what happens if you use CONCURRENTLY in the migration, i.e. if the migration will conclude super fast and then the index will be created later, while nexus is running.
| @@ -0,0 +1,8 @@ | |||
| BEGIN; | |||
|
|
|||
| CREATE INDEX ix_transactions_method_height ON chain.transactions (method, block); | |||
There was a problem hiding this comment.
I often find it hard to understand why we have certain indexes in place. Makes it hard and/or scary to modify or remove them. Let's try to document every index with its intended purpose. E.g. here:
-- `method` is a possible external API parameter; `block` lets us efficiently retrieve the most recent N txs with a given method.
.changelog/708.breaking.md
Outdated
There was a problem hiding this comment.
I forget, when did we say we were going to call a change "breaking"? Is it whenever it introduces a backwards-incompatible DB change? I think this is not breaking because:
- We're breaking the API, but it's the consensus API which is not officially released yet, so we're OK to muck with it. Even if this were an "official" breaking API change, I guess it would mean that we need to introduce api/spec/v2.yaml (and do some thinking about how we'll manage multiple versions), but nexus itself would stay at the same major version? This philosophy is untested :)
- We're not breaking nexus: A current nexus operator can deploy this PR over an existing DB, and it will keep working thanks to migrations. An example of a breaking change here is consolidate migrations #574 which requires an operator to be very careful and hands-on when deploying.
There was a problem hiding this comment.
I noticed that #685 was marked as a breaking change and it is only an API change.
But i agree that the state (consensus part of) nexus is currently in, then most changes would be breaking. I'll make it non breaking, imo it doesn't really matter that much as there's not many external uses of nexus i think.
ptrus
force-pushed
the
ptrus/feature/consensus-method-idx
branch
from
736cd5f to
7d8c75d
Compare
Adds an index on consensus transactions method.
Additionally removes
minFee,maxFeeandcodequery filters from/consensus/transactions. These filters don't seem all that useful (I have confirmed with frontend that these are not needed) and don't have any indexes set, so these aren't usable in practice anyway.