-
Notifications
You must be signed in to change notification settings - Fork 403
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(wal): reenables wal mode for litefs after resolution of issue #621
Conversation
97bd258
to
7efa931
Compare
Have you verified that this issue no longer exists with the latest version of LiteFS? #598 (comment) |
Also, thanks for working on this! |
I hadn't looked at this issue yet actually 😅 just read through it. I will go through the steps to reproduce the error with this branch and report back. Thank you for this amazing resource! |
@kentcdodds - I was able to deploy, make a change (similar to the change you made in your tests) and deploy the template app without an issue. Here is the successful GH action run: https://github.com/NGimbal/epic-stack/actions/runs/7981017813/job/21791913762 Let me know if this confirms that this issue has been resolved or if further testing is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! I just would like to check on a few things before merging.
app/utils/db.server.ts
Outdated
@@ -16,6 +16,10 @@ export const prisma = remember('prisma', () => { | |||
{ level: 'warn', emit: 'stdout' }, | |||
], | |||
}) | |||
|
|||
client.$queryRaw`PRAGMA journal_mode = WAL;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand why we're doing this? @benbjohnson, this will run as the first thing the prisma client does when it connects to the sqlite database. Do you see this as problematic or a reasonable workaround? Should we prevent this from running on read-only replicas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'm not sure if this is the right thing to be doing. I added it because I figured we had to set the journal_mode
in order to use WAL mode - I don't think it's on by default in LiteFS. The comment that I linked above is part of a thread where Prisma sqlite users are discussing WAL mode and this seemed like a sensible option. I would definitely defer to @benjohnson.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I realize that this may be dumb because the command in the Dockerfile already does this. I guess my question would be for the local database and the test database? How can we make sure these are configured like the production database?
I may be overthinking this. I will remove this line for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes more sense I believe: 4ef05f5
@@ -1,3 +1,5 @@ | |||
PRAGMA journal_mode = WAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does make more sense. But based on this it looks like prisma will acquire the exclusive lock whenever it connects: superfly/litefs#425 (comment)
I'm just not 100% certain this will work in the long term. And I'm not 100% certain I know how to check for this.
Calling my friends from Prisma and LiteFS!
@rtbenfield and @benbjohnson 🙏
Could you tell me whether this should work around this issue: superfly/litefs#425 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this actually breaks the tests >.< it seems like for some reason the test db is not created properly after running:
prisma migrate reset --force --skip-seed --skip-generate
here.
I'll wait for help from these folks before noodling on this further.
89de11b
to
226837c
Compare
Ok I know I said I would stop noodling... but I didn't. Per the most recent comments I realized that we really only need to be setting I can confirm that tests pass, it deploys in CI, and that the test database and local database do in fact have |
Ok yeah this might be a bad idea - I just tried creating a migration and re-deploying my app with these changes applied and the deployment failed. The logs showed this message: Looks like the underlying issue might not be fixed yet. Full logs:
|
Sorry for the slow reply. I think all you need to do to fix the WAL issue with Prisma is to disable advisory locking by setting an environment variable. It probably makes sense to set it in the
SQLite is a single-writer database so the the advisory lock setting the database in exclusive mode seems like overkill. There shouldn't really be a downside to setting this. |
Very interesting! Thanks @benbjohnson! @NGimbal let me know when you've tried this out and then we can look into re-enabling WAL in the Epic Stack 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to worry about WAL mode locally. So we really only need a single line change in the fly.toml
0e6486f
to
1a2ffb0
Compare
Ok so looks like this works 🥳 Thanks @benbjohnson for the direction here! Succcessful deploy after taking Ben's suggestion: Successful deploy after making a subsequent change: Successful deploy after a migration: Successful deploy after making another change... for good measure: Looks like it's working! |
1a2ffb0
to
9875b04
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super! Thank you so much!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more thing
fly.toml
Outdated
@@ -15,6 +15,10 @@ destination = "/data" | |||
[deploy] | |||
release_command = "node ./other/sentry-create-release" | |||
|
|||
[env] | |||
# For WAL support: https://github.com/prisma/prisma-engines/issues/4675#issuecomment-1914383246 | |||
PRISMA_SCHEMA_DISABLE_ADVISORY_LOCK = "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the more I think about this, the more I think this belongs in the Dockerfile because that's where we set up litefs. It's less a fly issue and more a litefs issue. Do you mind moving this (along with the comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved the env variable to the docker file - one question, do we need to upgrade the litefs version from 0.5.8 to 0.5.11 as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably a good idea to upgrade to v0.5.11. It avoids issues with exclusive locking mode if it ends up being re-enabled for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just deployed to my own site. Let's do this thing!
It looks like the issue that were preventing the Epic Stack from using WAL mode has been resolved. This PR updates the version of LiteFS used to 0.5.11 and un-comments the lines that enable WAL mode.
Enables WAL mode for the local db per this comment. Not sure if this is correct. 👀
I started looking into this because the call to
prisma.verification.upsert
in the functionprepareVerification
started failing for me locally with the following error:Invalid 'prisma.verification.upsert()' invocation: internal error: entered unreachable code
. I started investigating WAL mode because of this GH issue.Test Plan
I believe the existing tests cover this change.
Checklist
Screenshots
No visual changes