-
Notifications
You must be signed in to change notification settings - Fork 1k
Split the block cache into block pointer cache and block data cache #6037
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
Changes from 4 commits
727b845
044a125
7c80bfb
a2acdaa
1f7a117
7cb0690
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
use anyhow::anyhow; | ||
use chrono::DateTime; | ||
use diesel::deserialize::FromSql; | ||
use diesel::pg::Pg; | ||
use diesel::serialize::{Output, ToSql}; | ||
|
@@ -7,6 +8,7 @@ use diesel::sql_types::{Bytea, Nullable, Text}; | |
use diesel_derives::{AsExpression, FromSqlRow}; | ||
use serde::{Deserialize, Deserializer}; | ||
use std::convert::TryFrom; | ||
use std::num::ParseIntError; | ||
use std::time::Duration; | ||
use std::{fmt, str::FromStr}; | ||
use web3::types::{Block, H256, U256, U64}; | ||
|
@@ -16,9 +18,9 @@ use crate::components::store::BlockNumber; | |
use crate::data::graphql::IntoValue; | ||
use crate::data::store::scalar::Timestamp; | ||
use crate::derive::CheapClone; | ||
use crate::object; | ||
use crate::prelude::{r, Value}; | ||
use crate::util::stable_hash_glue::{impl_stable_hash, AsBytes}; | ||
use crate::{bail, object}; | ||
|
||
/// A simple marker for byte arrays that are really block hashes | ||
#[derive(Clone, Default, PartialEq, Eq, Hash, FromSqlRow, AsExpression)] | ||
|
@@ -477,10 +479,7 @@ impl TryFrom<(Option<H256>, Option<U64>, H256, U256)> for ExtendedBlockPtr { | |
let block_number = | ||
i32::try_from(number).map_err(|_| anyhow!("Block number out of range"))?; | ||
|
||
// Convert `U256` to `BlockTime` | ||
let secs = | ||
i64::try_from(timestamp_u256).map_err(|_| anyhow!("Timestamp out of range for i64"))?; | ||
let block_time = BlockTime::since_epoch(secs, 0); | ||
let block_time = BlockTime::try_from(timestamp_u256)?; | ||
|
||
Ok(ExtendedBlockPtr { | ||
hash: hash.into(), | ||
|
@@ -497,16 +496,13 @@ impl TryFrom<(H256, i32, H256, U256)> for ExtendedBlockPtr { | |
fn try_from(tuple: (H256, i32, H256, U256)) -> Result<Self, Self::Error> { | ||
let (hash, block_number, parent_hash, timestamp_u256) = tuple; | ||
|
||
// Convert `U256` to `BlockTime` | ||
let secs = | ||
i64::try_from(timestamp_u256).map_err(|_| anyhow!("Timestamp out of range for i64"))?; | ||
let block_time = BlockTime::since_epoch(secs, 0); | ||
let timestamp = BlockTime::try_from(timestamp_u256)?; | ||
|
||
Ok(ExtendedBlockPtr { | ||
hash: hash.into(), | ||
number: block_number, | ||
parent_hash: parent_hash.into(), | ||
timestamp: block_time, | ||
timestamp, | ||
}) | ||
} | ||
} | ||
|
@@ -562,14 +558,67 @@ impl fmt::Display for ChainIdentifier { | |
#[diesel(sql_type = Timestamptz)] | ||
pub struct BlockTime(Timestamp); | ||
|
||
impl Default for BlockTime { | ||
fn default() -> Self { | ||
BlockTime::NONE | ||
} | ||
} | ||
|
||
impl TryFrom<BlockTime> for U256 { | ||
type Error = anyhow::Error; | ||
|
||
fn try_from(value: BlockTime) -> Result<Self, Self::Error> { | ||
if value.as_secs_since_epoch() < 0 { | ||
bail!("unable to convert block time into U256"); | ||
} | ||
|
||
Ok(U256::from(value.as_secs_since_epoch() as u64)) | ||
} | ||
} | ||
|
||
impl TryFrom<U256> for BlockTime { | ||
type Error = anyhow::Error; | ||
|
||
fn try_from(value: U256) -> Result<Self, Self::Error> { | ||
i64::try_from(value) | ||
.map_err(|_| anyhow!("Timestamp out of range for i64")) | ||
.map(|ts| BlockTime::since_epoch(ts, 0)) | ||
} | ||
} | ||
|
||
impl TryFrom<Option<String>> for BlockTime { | ||
type Error = ParseIntError; | ||
|
||
fn try_from(ts: Option<String>) -> Result<Self, Self::Error> { | ||
match ts { | ||
Some(str) => return BlockTime::from_str(&str), | ||
None => return Ok(BlockTime::NONE), | ||
}; | ||
} | ||
} | ||
|
||
impl FromStr for BlockTime { | ||
type Err = ParseIntError; | ||
|
||
fn from_str(ts: &str) -> Result<Self, Self::Err> { | ||
let (radix, idx) = if ts.starts_with("0x") { | ||
(16, 2) | ||
} else { | ||
(10, 0) | ||
}; | ||
|
||
u64::from_str_radix(&ts[idx..], radix).map(|ts| BlockTime::since_epoch(ts as i64, 0)) | ||
} | ||
} | ||
|
||
impl BlockTime { | ||
/// A timestamp from a long long time ago used to indicate that we don't | ||
/// have a timestamp | ||
pub const NONE: Self = Self(Timestamp::NONE); | ||
// /// A timestamp from a long long time ago used to indicate that we don't | ||
// /// have a timestamp | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like some extra comment signs snuck in |
||
pub const NONE: Self = Self::MIN; | ||
|
||
pub const MAX: Self = Self(Timestamp::MAX); | ||
|
||
pub const MIN: Self = Self(Timestamp::MIN); | ||
pub const MIN: Self = Self(Timestamp(DateTime::from_timestamp_nanos(0))); | ||
|
||
/// Construct a block time that is the given number of seconds and | ||
/// nanoseconds after the Unix epoch | ||
|
@@ -586,7 +635,12 @@ impl BlockTime { | |
/// hourly rollups in tests | ||
#[cfg(debug_assertions)] | ||
pub fn for_test(ptr: &BlockPtr) -> Self { | ||
Self::since_epoch(ptr.number as i64 * 45 * 60, 0) | ||
Self::for_test_number(&ptr.number) | ||
} | ||
|
||
#[cfg(debug_assertions)] | ||
pub fn for_test_number(number: &BlockNumber) -> Self { | ||
Self::since_epoch(*number as i64 * 45 * 60, 0) | ||
} | ||
|
||
pub fn as_secs_since_epoch(&self) -> i64 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -579,7 +579,7 @@ pub trait ChainStore: ChainHeadStore { | |
async fn block_number( | ||
&self, | ||
hash: &BlockHash, | ||
) -> Result<Option<(String, BlockNumber, Option<u64>, Option<BlockHash>)>, StoreError>; | ||
) -> Result<Option<(String, BlockNumber, Option<BlockTime>, Option<BlockHash>)>, StoreError>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are all these Also, this method should be renamed to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's not always a timestamp, on the shared storage model it still can be None The option BlockTime is a little weird but I kept it because there is a different between Some(epoch time) and None, it's more idiomatic to have Option than checking BlockTime == BlockTime::NONE or MIN which are also in fact the same value (I didn't really get why). |
||
|
||
/// Do the same lookup as `block_number`, but in bulk | ||
async fn block_numbers( | ||
|
@@ -668,7 +668,7 @@ pub trait QueryStore: Send + Sync { | |
async fn block_number_with_timestamp_and_parent_hash( | ||
&self, | ||
block_hash: &BlockHash, | ||
) -> Result<Option<(BlockNumber, Option<u64>, Option<BlockHash>)>, StoreError>; | ||
) -> Result<Option<(BlockNumber, Option<BlockTime>, Option<BlockHash>)>, StoreError>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this could also just be called |
||
|
||
fn wait_stats(&self) -> PoolWaitStats; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,7 +216,7 @@ impl DataSource { | |
data_source::MappingTrigger::Offchain(trigger.clone()), | ||
self.mapping.handler.clone(), | ||
BlockPtr::new(Default::default(), self.creation_block.unwrap_or(0)), | ||
BlockTime::NONE, | ||
BlockTime::MIN, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why that change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from testing, I'll revert, it's the exact same value, not sure why either |
||
)) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
DATABASE_TEST_VAR_NAME := "THEGRAPH_STORE_POSTGRES_DIESEL_URL" | ||
DATABASE_URL := "postgresql://graph-node:let-me-in@localhost:5432/graph-node" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's a justfile? This should be your local file, not something in the repo There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is similar to a make file, it's intentionally to be in the repo, provides some shortcuts for common operations, you don't need to use it yourself but it's useful to have for others There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
help: | ||
@just -l | ||
|
||
local-deps-up *ARGS: | ||
docker compose -f docker/docker-compose.yml up ipfs postgres {{ ARGS }} | ||
|
||
local-deps-down: | ||
docker compose -f docker/docker-compose.yml down | ||
|
||
test-deps-up *ARGS: | ||
docker compose -f tests/docker-compose.yml up {{ ARGS }} | ||
|
||
test-deps-down: | ||
docker compose -f tests/docker-compose.yml down | ||
|
||
# Requires local-deps, see local-deps-up | ||
test *ARGS: | ||
just _run_in_bash cargo test --workspace --exclude graph-tests -- --nocapture {{ ARGS }} | ||
|
||
runner-test *ARGS: | ||
just _run_in_bash cargo test -p graph-tests --test runner_tests -- --nocapture {{ ARGS }} | ||
|
||
# Requires test-deps to be running, see test-deps-up | ||
it-test *ARGS: | ||
just _run_in_bash cargo test --test integration_tests -- --nocapture {{ ARGS }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These can be just aliases in [alias]
store = "test -p graph-store-postgres"
tst = "test --workspace --exclude graph-tests"
docs = "doc --workspace --document-private-items"
gm = "install --bin graphman --path node --locked"
gmt = "install --bin graphman --path node --locked --root /var/tmp/cargo"
rt = "test -p graph-tests --test runner_tests"
it = "test -p graph-tests --test integration_tests -- --nocapture" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and that's local, this works for everyone. |
||
|
||
local-rm-db: | ||
rm -r docker/data/postgres | ||
|
||
new-migration NAME: | ||
diesel migration generate {{ NAME }} --migration-dir store/postgres/migrations/ | ||
|
||
_run_in_bash *CMD: | ||
#!/usr/bin/env bash | ||
export {{ DATABASE_TEST_VAR_NAME }}={{ DATABASE_URL }} | ||
{{ CMD }} |
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.
This impl is very unintuitive to me, that parsing a string will try to interpret the string as a hex/decimal number.
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.
That's how it was used I just move the implementation somewhere that was easier to find. The previous function was
try_parse_timestamp
or something similar. If it's the naming I can change it a method?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.
renamed function