Fix Vote History sorting#5927
Conversation
Signed-off-by: Paweł Perek <pawel.perek@digitalasset.com>
Signed-off-by: Paweł Perek <pawel.perek@digitalasset.com>
Signed-off-by: Paweł Perek <pawel.perek@digitalasset.com>
| createAction = sqlu""" | ||
| create index concurrently if not exists scan_txlog_store_sid_en_vot | ||
| on scan_txlog_store (store_id, entry_number desc) | ||
| create index concurrently if not exists scan_txlog_store_sid_rt_en_vot |
There was a problem hiding this comment.
@rautenrieth-da could you take a look if that's a correct way to remove unused indices? We didn't use IndexAction.Drop before but AFAIU that's the way
There was a problem hiding this comment.
The code looks good to me. Since this is the first time we're using it, we should check if it really works as intended. You won't trigger the code path in integration tests, you need to first deploy an old version of the app and then a version that has your PR merged. At the very least, check logs of CILR after the changes from this PR have been deployed there.
| case Some(a) => Some(sql"""entry_number < $a""") | ||
| case Some(a) => | ||
| Some( | ||
| sql"""(record_time, entry_number) < ((select record_time from #$txLogTableName where store_id = $txLogStoreId and entry_number = $a), $a)""" |
There was a problem hiding this comment.
did you check the plane for this? the SQL query looks weird to me.
There was a problem hiding this comment.
+1 to commenting with the query plan. We just dropped the index on (store_id, entry_number), how can it quickly find the record time from the entry number?
I would have expected that we do either:
- Sort lexicographically by
(record_time, entry_number), and include the record time in the API pagination token (along with the entry number) - Argue why two vote requests can never have the same record time, sort by record time only, and paginate by record time (instead of entry number). If you store the record time as "micros since epoch", you don't even need to change the type of
pageTokenin the API.
There was a problem hiding this comment.
We discussed this exactly (out of band) and decided we should use the (record_time, entry_number) in page token
3 participants
Fixes #5692
Before:
After:
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./upgrade_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines