Skip to content
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

Store synced_at_block_number when a deployment syncs #5610

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

encalypto
Copy link
Contributor

This is a follow up to #5566

We use both INTEGER and NUMERIC types in the database to match how other block numbers are stored in the same table.

We use both `INTEGER` and `NUMERIC` types in the database to match how
other block numbers are stored in the same table.
@encalypto encalypto marked this pull request as ready for review August 20, 2024 17:40
@fordN fordN requested a review from isum August 26, 2024 15:49
@@ -0,0 +1,2 @@
ALTER TABLE subgraphs.subgraph_deployment ADD COLUMN synced_at_block_number NUMERIC;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to make that an int4, too. The fact that latest_ethereum_block_number is numeric is more a historical accident than anything we should continue doing for other places where we store block numbers. In an ideal world, we'd change al the columns that store block numbers and use numeric to int4 but that might be a bit delicate and nobody's done that, obviously.

use subgraph_deployment as d;

// Work around a Diesel issue with serializing BigDecimals to numeric
let number = format!("{}::int4", block_ptr.number);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting

Copy link
Contributor

@zorancv zorancv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants