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

Enhancement: update indexer table schema #1143

Closed
wants to merge 13 commits into from
Closed

Conversation

shiqizng
Copy link
Contributor

Summary

This PR updates the schema for db tables to use numeric(20) type for columns that could have values larger than bigint type.

Test Plan

Add a unit test for the migration, run indexer daemon to check migration happened, and check db to validate that table schema has been updated.

@shiqizng shiqizng self-assigned this Jul 25, 2022
@shiqizng shiqizng added Enhancement New feature or request Team Lamprey labels Jul 25, 2022
@shiqizng shiqizng changed the title update indexer table schema Enhancement: update indexer table schema Jul 25, 2022
Copy link
Contributor

@Eric-Warehime Eric-Warehime left a comment

Choose a reason for hiding this comment

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

It looks like I messed up your PR by merging in ps://github.com//pull/1144 so you'll have conflicts.

@@ -3,7 +3,7 @@
-- TODO? replace all 'addr bytea' with 'addr_id bigint' and a mapping table? makes addrs an 8 byte int that fits in a register instead of a 32 byte string

CREATE TABLE IF NOT EXISTS block_header (
round bigint PRIMARY KEY,
round numeric(20) PRIMARY KEY,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the rationale behind choosing 20 as opposed to another number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is because uint64 max is 18446744073709551615 and it's a value of 20 digits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it thanks!

require.NoError(t, err)

migrationState, err = db.getMigrationState(context.Background(), nil)
require.NoError(t, err)

assert.Equal(t, types.MigrationState{NextMigration: 6}, migrationState)
}

func TestConvertBigIntType(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is necessary, but should we add a test case that asserts the existing values aren't changed by the type conversion?

i.e. insert round 1,000,000 convert the type and assert that the round is still 1,000,000?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I think this can be covered by running the block validator.

@shiqizng shiqizng requested a review from brianolson July 26, 2022 14:24
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #1143 (e15fe45) into develop (ed2a991) will increase coverage by 0.21%.
The diff coverage is 75.00%.

@@             Coverage Diff             @@
##           develop    #1143      +/-   ##
===========================================
+ Coverage    60.07%   60.29%   +0.21%     
===========================================
  Files           52       52              
  Lines         8752     8764      +12     
===========================================
+ Hits          5258     5284      +26     
+ Misses        3012     2994      -18     
- Partials       482      486       +4     
Impacted Files Coverage Δ
idb/postgres/postgres_migrations.go 57.64% <73.68%> (+13.97%) ⬆️
idb/postgres/postgres.go 61.17% <100.00%> (ø)
fetcher/fetcher.go 50.87% <0.00%> (-1.32%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

@@ -0,0 +1,123 @@
package schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can name this file test something or otherwise mark the fact that it's not the actual current production schema?

It doesn't seem like we're maintaining old versions of the schema for past updates. What was our process for previous updates regarding schema testing?

@shiqizng shiqizng closed this Aug 5, 2022
@shiqizng
Copy link
Contributor Author

shiqizng commented Aug 5, 2022

this PR is replaced by #1166

@shiqizng shiqizng deleted the shiqi/dbschema branch August 5, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants