-
Notifications
You must be signed in to change notification settings - Fork 4
Disable journaling if c1z is opened in read only mode. #613
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
Conversation
WalkthroughC1File.init now runs two SQLite PRAGMA statements— Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/dotc1z/c1file.go (1)
270-283: Consider pragma application order to prevent overrides.Custom pragmas (lines 278-283) are applied after disabling journaling for read-only mode, allowing them to override
journal_mode = OFF. If the intent is to enforce journaling disabled in read-only mode, consider either:
- Moving the read-only pragma block after custom pragmas
- Filtering out
journal_modefrom custom pragmas when in read-only mode🔎 Proposed fix: Apply custom pragmas first
err = c.InitTables(ctx) if err != nil { return err } + for _, pragma := range c.pragmas { + _, err := c.db.ExecContext(ctx, fmt.Sprintf("PRAGMA %s = %s", pragma.name, pragma.value)) + if err != nil { + return err + } + } + if c.readOnly { // Disable journaling in read only mode, since we're not writing to the database. _, err = c.db.ExecContext(ctx, "PRAGMA journal_mode = OFF") if err != nil { return err } } - for _, pragma := range c.pragmas { - _, err := c.db.ExecContext(ctx, fmt.Sprintf("PRAGMA %s = %s", pragma.name, pragma.value)) - if err != nil { - return err - } - } - return nil
🧹 Nitpick comments (1)
pkg/dotc1z/c1file.go (1)
270-276: Consider opening the SQLite connection in read-only mode whenc.readOnlyis true.While disabling journaling is appropriate, the underlying SQLite connection at line 97 is not opened in read-only mode. Modify the connection string to include
mode=rowhenc.readOnlyis true—for example,sql.Open("sqlite", "file:" + dbFilePath + "?mode=ro"). This adds database-level read-only protection and aligns with the intent of the read-only flag.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/dotc1z/c1file.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
|
@coderabbitai We do journal_mode = OFF before pragmas so that it can be overridden. Also the SQLite library we're using (go-sqlite) doesn't seem to support opening in read only mode. |
|
Tip For best results, initiate chat on the files or code changes. @ggreer, thank you for the clarification! That makes sense—placing ✏️ Learnings added
🧠 Learnings used |
… to the database.
You can still override this behavior in read only mode since we apply pragmas after this one.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.