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

indexer: backups are now a deterministic set of SQL statements #1338

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

altergui
Copy link
Contributor

@altergui altergui commented Jun 13, 2024

this is much less performant that simply exporting/importing the raw binary database, but makes the backup deterministic, allowing StateSync to pull the snapshot from many nodes at the same time

@coveralls
Copy link

coveralls commented Jun 13, 2024

Pull Request Test Coverage Report for Build 9498381141

Details

  • 46 of 69 (66.67%) changed or added relevant lines in 2 files are covered.
  • 12 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.03%) to 60.918%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 44 67 65.67%
Files with Coverage Reduction New Missed Lines %
cmd/end2endtest/account.go 1 66.43%
api/accounts.go 2 54.42%
apiclient/account.go 2 38.89%
vochain/appsetup.go 2 55.45%
vochain/indexer/indexer.go 5 67.55%
Totals Coverage Status
Change from base Build 9495839404: -0.03%
Covered Lines: 16080
Relevant Lines: 26396

💛 - Coveralls

@altergui altergui force-pushed the feat/deterministic-indexer-backups branch 2 times, most recently from 062a1a5 to cc46776 Compare June 13, 2024 11:22
@coveralls
Copy link

coveralls commented Jun 13, 2024

Pull Request Test Coverage Report for Build 9498587081

Details

  • 40 of 62 (64.52%) changed or added relevant lines in 2 files are covered.
  • 114 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.05%) to 60.903%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 38 60 63.33%
Files with Coverage Reduction New Missed Lines %
cmd/end2endtest/account.go 1 66.43%
api/accounts.go 2 54.42%
apiclient/account.go 2 38.89%
vochain/appsetup.go 2 55.45%
vochain/indexer/indexer.go 107 67.55%
Totals Coverage Status
Change from base Build 9495839404: -0.05%
Covered Lines: 16076
Relevant Lines: 26396

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 13, 2024

Pull Request Test Coverage Report for Build 9498735272

Details

  • 49 of 77 (63.64%) changed or added relevant lines in 2 files are covered.
  • 8 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.03%) to 60.924%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 47 75 62.67%
Files with Coverage Reduction New Missed Lines %
cmd/end2endtest/account.go 1 66.43%
vochain/indexer/indexer.go 1 68.09%
api/accounts.go 2 54.42%
apiclient/account.go 2 38.89%
vochain/appsetup.go 2 55.45%
Totals Coverage Status
Change from base Build 9495839404: -0.03%
Covered Lines: 16098
Relevant Lines: 26423

💛 - Coveralls

Copy link
Contributor

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I said I would, but haven't had the time. LGTM with some nits.

Did you consider a test? Just to make sure that we don't regress with any other non-determinism in SQL backups in the future.

Also, I wonder if we should rename the indexer sql "backup" feature to "snapshot", as the latter is perhaps more well understood to mean "deterministic".

statement.WriteString(line)
statement.WriteString("\n")

if strings.HasSuffix(line, ";") {
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure that the entire text ends with ;? what if it's like

foo;
bar;
baz

in that case you would ignore baz, so perhaps add a sanity check at the end that there's no remaining statement.

vochain/indexer/indexer.go Outdated Show resolved Hide resolved
vochain/indexer/indexer.go Outdated Show resolved Hide resolved
@p4u
Copy link
Member

p4u commented Jun 16, 2024

Could you explain why this is now deterministic? thanks

@altergui altergui force-pushed the feat/deterministic-indexer-backups branch from cc46776 to 47767ee Compare June 17, 2024 11:07
@coveralls
Copy link

coveralls commented Jun 17, 2024

Pull Request Test Coverage Report for Build 9546598969

Details

  • 60 of 88 (68.18%) changed or added relevant lines in 3 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 61.32%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vochain/indexer/indexer.go 51 79 64.56%
Files with Coverage Reduction New Missed Lines %
vochain/indexer/indexer.go 5 67.7%
Totals Coverage Status
Change from base Build 9546446432: 0.02%
Covered Lines: 15939
Relevant Lines: 25993

💛 - Coveralls

@altergui altergui marked this pull request as draft June 17, 2024 13:46
@altergui
Copy link
Contributor Author

it's deterministic because the SQL statements produced by each node are the same (bit-by-bit) . this was not the case with the raw db format.

they are a (zstd compressed) set of SQL statements

also, relevant methods now use io.Reader and io.Writer
@altergui altergui force-pushed the feat/deterministic-indexer-backups branch from 47767ee to f345045 Compare September 11, 2024 09:02
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.

deterministic indexer backup so that all nodes produce the same bit-by-bit snapshot
4 participants