-
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
Replacing created_at with the block_timestamp #9728
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
core/chains/evm/logpoller/orm.go
Outdated
AND block_number >= (SELECT COALESCE(block_number, 0) FROM evm_log_poller_blocks WHERE evm_chain_id = $1 and block_timestamp > $6 ORDER BY block_number LIMIT 1) | ||
AND (block_number + $7) <= (SELECT COALESCE(block_number, 0) FROM evm_log_poller_blocks WHERE evm_chain_id = $1 ORDER BY block_number DESC LIMIT 1) | ||
ORDER BY created_at ASC`, utils.NewBig(o.chainID), address, eventSig.Bytes(), topicIndex+1, pq.ByteaArray(topicValuesBytes), after, confs) | ||
ORDER BY block_number ASC`, utils.NewBig(o.chainID), address, eventSig.Bytes(), topicIndex+1, pq.ByteaArray(topicValuesBytes), after, confs) |
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.
Understand it might be tricky, can we have a test case for it?
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.
@samsondav please take a look, wrote a couple of tests. Additionally, I reverted changes from orm.go
and run those tests against the old implementation - they did not pass, because of created_at
behavior - which is expected ;)
414a28a
to
ee34bb1
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'm tempted to say we should change the name to SelectLogsAfterBlockTimestamp
to avoid confusion, but I guess probably best to leave as-is to avoid an unnecessary API change.
We can give it a try in a separate PR ;) |
a367f9d
007f14f
to
5dc8bf0
Compare
SonarQube Quality Gate |
…h other log poller functions (block_no, log_index)
5dc8bf0
to
ae5b3d0
Compare
ae5b3d0
to
c62b417
Compare
SonarQube Quality Gate |
There are a couple of issues with current approach in which we perform ordering by the created_at
Sorting by
created_at
is done in memoryThis query can be optimized a little by adding other columns to the index with
includes
or just returning columns from the index (not need to perform heap scan). However, the duration ends up around 100ms, with sorting still being run in memory.created_at
is initialized during runtime by picking the current time withNOW()
function. Those times might differ between nodes causing nodes not reaching a consensus because they might get completely different logs from the log poller.Current insert implementation
Solution
Instead of sorting by date, let's sort by the block number. We can run one small subquery that picks
block_number
based on the timestamp and use it for filtering. This should save us some latency. What is more, instead of usingcreate_at
we should move to theblock_timestamp
which is the value created on chain so every node running code should get the same valuesQuery plans
Subquery picking the block_number based on the timestamp
Entire query