-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
CCIP-1053 Replace filtering by created_at with filtering by block_timestamp #10743
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
c375d3b
to
59b9cbc
Compare
59b9cbc
to
725db36
Compare
725db36
to
fddb455
Compare
f80a866
to
303a0eb
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.
I'd want to see benchmarks to ensure we aren't introducing any performance regressions
@@ -122,7 +122,7 @@ func (o *ORM) SelectLatestLogEventSigWithConfs(eventSig common.Hash, address com | |||
WHERE evm_chain_id = $1 | |||
AND event_sig = $2 | |||
AND address = $3 | |||
AND (block_number + $4) <= (SELECT COALESCE(block_number, 0) FROM evm.log_poller_blocks WHERE evm_chain_id = $1 ORDER BY block_number DESC LIMIT 1) | |||
AND block_number <= (SELECT COALESCE(block_number, 0) FROM evm.log_poller_blocks WHERE evm_chain_id = $1 ORDER BY block_number DESC LIMIT 1) - $4 |
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.
Careful! This was set up deliberately for optimization reasons IIRC
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.
I was the one who did these optimizations :P For some reason, I missed this query, therefore fixing it right now
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.
Query plans look good, so I thought perf tests might not be needed. but I can do that. Btw, this query is used only by CCIP, so the impact is very localized. |
@samsondav I created a simple Go Benchmark suite with a populated database. Added outcomes to the PR's description, you can also check benchmark in the code |
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!
SonarQube Quality Gate |
Context
Sorting by
created_at
in LogPoller’s queries is invalid becausecreated_at
is generated when inserting a block into the database, this might be a problem when:Although it sounds like an edge case, impact of this might be very high.
Solution
Instead of sorting by
created_at
, we should sort by theblock_timestamp
. We can run one small subquery that picksblock_number
based on the timestamp and use it for filtering. This should save us some latency. Moreover,block_timestamp
is initialized on the chain so every node running code should get the same values.The first implementation was done here, but it lacks detailed testing, so we decided to start from scratch
Tests
Current query performance (match up to 10k records)
We should not make it worse by introducing new query
Nested query performance - match everything
Nested query performance - match nothing
Main query performance - up to 10k records matched
Go bench results