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

sqlite db backend #6570

Closed
wants to merge 8 commits into from
Closed

sqlite db backend #6570

wants to merge 8 commits into from

Conversation

seejee
Copy link
Contributor

@seejee seejee commented May 24, 2022

Change Description

Closes #6176

This PR brings SQLite support to LND. As mentioned in #6176, this PR is a first pass implements sqlite support in a somewhat naive way.

It builds on the postgres support work that was added recently, however to avoid any premature abstraction, I've intentionally duplicated some things between the postgres and sqlite kvdb modules. Specifically, the kvdb/sqlite/readwrite_*.go files are nearly completely identical to the ones in the postgres package, and (thanks for golang's database/sql library) could likely be used as-is for future SQL-based kvdb backends.

The table schema used by sqlite is almost identical to the postgres table schema. It is still very much a key-value approach as opposed to a properly normalized SQL table structure. The intention is that moving to a more normalized table structure could come in the future, along with tooling to migrate from the existing tables or from boltdb, but that would likely come in a future PR.

We're also making use of the https://github.com/mattn/go-sqlite3 package (which relies on C bindings) because of its maturity, as opposed to the less mature, but pure golang sqlite driver: https://pkg.go.dev/modernc.org/sqlite. If requiring CGO is a problem, then we could attempt to use the modernc sqlite package instead.

Tasks:

  • Get basic SQLite support working via unit tests
  • Get integration tests passing with sqlite backend
  • Wire up lncfg support for sqlite
  • Do manual integration testing with the sqlite driver

Steps to Test

TODO

Pull Request Checklist

Testing

  • Your PR passes all CI checks.
  • Tests covering the positive and negative (error paths) are included.
  • Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

@Roasbeef Roasbeef added this to the v0.16.0 milestone May 24, 2022
@Roasbeef Roasbeef added enhancement Improvements to existing features / behaviour database Related to the database/storage of LND sql labels May 24, 2022
@seejee seejee force-pushed the sqlite branch 2 times, most recently from f4e431d to 6c865b3 Compare May 24, 2022 20:14
@blckbx
Copy link

blckbx commented May 27, 2022

👍
Would it be possible to adapt database replication within this inital PR?
Like: db.location=/location1/sqlite3.db:/location2/sqlite3.db

@seejee
Copy link
Contributor Author

seejee commented Jun 6, 2022

Would it be possible to adapt database replication within this initial PR?

@blckbx good question! For the first iteration, I was hoping to keep things simple and not bring that sort of complexity into lnd.

There is a new project (which I've only read about, but haven't used yet) called litestream.io that looks promising as a way to replicate sqlite database to another file locally or to cloud storage. That might be a nice solution because litestream runs as a sidecar process and so none of the replication complexity would have to come into lnd itself.

Curious to hear your thoughts!

@blckbx
Copy link

blckbx commented Jul 12, 2022

Would it be possible to adapt database replication within this initial PR?

@blckbx good question! For the first iteration, I was hoping to keep things simple and not bring that sort of complexity into lnd.

Absolutely true. Best to move this to another PR/discussion. But to get back to litestream ...

There is a new project (which I've only read about, but haven't used yet) called litestream.io that looks promising as a way to replicate sqlite database to another file locally or to cloud storage. That might be a nice solution because litestream runs as a sidecar process and so none of the replication complexity would have to come into lnd itself.

Unfortunately I'm not knowledgable enough to recommend a definite solution. So I just point to the issue CLN devs experienced using litestream, namely crashes. In the end they went away from litestream and implemented direct sqlite3 replication instead.

@saubyk saubyk removed this from the v0.16.0 milestone Aug 27, 2022
@Roasbeef Roasbeef self-requested a review September 28, 2022 19:28
@Roasbeef
Copy link
Member

Roasbeef commented Oct 25, 2022

Hey @seejee are you still interested in continuing this PR? (if not we can pick it up)

This was referenced Dec 14, 2022
@Roasbeef
Copy link
Member

Replaced by #7251

Thanks for getting this started!

@Roasbeef Roasbeef closed this Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Related to the database/storage of LND enhancement Improvements to existing features / behaviour sql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add SQLite3 backend option
4 participants