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

feat: sqlite: extract common init and migration utilities #12098

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jun 17, 2024

All of the versioning and migration logic is shared between the 3 sqlite databases we use the other two have the same pending-query problem as was resolved for _meta in #12090, so that's addressed here by making it common.

  • Use the same PRAGMA set across all 3 - this may need to change in future but we can configure that later when we need it.
  • Same _meta versioning and migration logic. Do it all in a migration loop where each migration is wrapped in a transaction, logging and error handling and the calling instance is responsible for feeding in functions that can perform a migration. Versioning and _meta is entirely hidden from the calling instance.
  • Two minor breaking changes in here:
    1. ethhashlookup.NewTransactionHashLookup now takes a context as its first argument.
    2. index.NewMsgIndex now takes a full path to its database as its path (not basePath) argument to make it consistent with the others.

Needs a fresh set of eyes to make sure I haven't stumbled over something in one of these to make it inconsistent. I'll do another pass of this myself later too, before I try deploying this on my calibnet and then mainnet nodes. There should be no meaningful visible changes for a running instance from this, mostly they just get consistent PRAGMAs and possibly some changed logging.

Also closes: #12081

Will need some rebase work after #12080 is landed so this should hold off till that's done.

Copy link

github-actions bot commented Jun 17, 2024

All checks have passed

@aarshkshah1992
Copy link
Contributor

Reviewing this after landing #12080 today.

@rvagg rvagg requested review from ZenGround0 and removed request for Stebalien June 17, 2024 09:31
@BigLep BigLep added this to the DX-Streamline milestone Jun 17, 2024
Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

some questions. Overall looking great. Would be nice to test this on a devnet/calibnet node once rebased on #12080.

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

lgtm

@rvagg
Copy link
Member Author

rvagg commented Jul 15, 2024

OK, so on the use of the message index - it is being used by some large node operators, it's not just a hidden feature that nobody uses it.

The change here, of switching the create+pragma order, means that if you have an existing message index, we are going to change the checkpoint size, which will impact how big the write-ahead-log will be before it tries to compact it:

  • Currently it's block-size (4096 - because pragma was done after db create) x 1025 (default) = 4MiB
  • With this change, for an existing node, it'll be block-size (4096 - until the next migration when we run a vacuum) x 256 = 1MiB
  • With this change for a new node, it'll be block-size (32768) x 256 = 8MiB (consistent with all other dbs)

There's also likely some minor performance impacts from increasing the block-size, but it depends on the exact usage and is likely a factor of how big the chunks are that we're writing. It probably makes the events db faster, but for the other 2 dbs they probably don't benefit from the larger block-size, but I doubt there'll be much cost from it.

@rvagg rvagg added the skip/changelog This change does not require CHANGELOG.md update label Jul 15, 2024
@rvagg rvagg enabled auto-merge (rebase) July 15, 2024 09:09
@rvagg rvagg merged commit 424ae01 into master Jul 15, 2024
79 checks passed
@rvagg rvagg deleted the rvagg/sqlite-refactor branch July 15, 2024 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

Do not enable read_uncommitted by default
3 participants