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

Introduce an access mode for SQLite storage #511

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

djmitche
Copy link
Collaborator

@djmitche djmitche commented Dec 24, 2024

The storage implementation checks this access mode for all methods, but also opens SQLite in read-only mode when appropriate, which causes SQLite to skip creating a WAL log or updating the access time in the database.

As a concession to ease-of-use with a new task database, the database is opened in read-write mode and the schema set up if it does not exist, regardless of the passed AccessMode.

This is a breaking change because:

  • The publicly-visible method SqliteStorage::new now takes a
    different number of parameters.
  • The public enum StorageConfig has been marked #[non_exhaustive].
    Pattern-matching on it outside of its crate must now include a
    wildcard pattern like _, or it will fail to compile.
  • StorageConfig::OnDisk has an additional field, access_mode, which
    has to be included when constructing or matching on this variant.

[edited to add the breaking-change justification]

@djmitche
Copy link
Collaborator Author

I'm not sure about deprecating the StorageConfig thing -- it seems like I'm designing around semver. And, we still have ServerConfig following this model.

I'm open to other ideas for how to add a read-only mode in a backward-compatible fashion, or suggestions to just go to 2.0.0.

@djmitche
Copy link
Collaborator Author

taskchampion::storage::SqliteStorage::new now takes 3 parameters instead of 2, in /home/runner/work/taskchampion/taskchampion/src/storage/sqlite.rs:86

Oops, I guess even this is a breaking change. That type probably shouldn't have been exposed to begin with!

Maybe I should take a hard look at the API and make a 2.0.0 with a bunch of API fixes.

@djmitche djmitche force-pushed the issue479 branch 2 times, most recently from a73c1ee to 17b90ca Compare December 24, 2024 04:46
@djmitche djmitche linked an issue Dec 24, 2024 that may be closed by this pull request
@djmitche djmitche requested a review from ryneeverett December 24, 2024 17:13
},
/// Store the data in memory. This is only useful for testing.
InMemory,
}

#[allow(deprecated)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this needed? From your PR comments I'm under the impression you were going to deprecate something and then changed your mind.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, thanks, I missed this one!

The storage implementation checks this access mode for all methods, but
also opens SQLite in read-only mode when appropriate, which causes
SQLite to skip creating a WAL log or updating the access time in the
database.

As a concession to ease-of-use with a new task database, the database is
opened in read-write mode and the schema set up if it does not exist,
regardless of the passed `AccessMode`.

This is a breaking change because:

 - The publicly-visible method `SqliteStorage::new` now takes a
   different number of parameters.
 - The public enum `StorageConfig` has been marked #[non_exhaustive].
   Pattern-matching on it outside of its crate must now include a
   wildcard pattern like `_`, or it will fail to compile.
 - `StorageConfig::OnDisk` has an additional field, `access_mode`, which
   has to be included when constructing or matching on this variant.
@djmitche djmitche enabled auto-merge (squash) December 24, 2024 19:27
@djmitche djmitche merged commit d25a943 into GothenburgBitFactory:main Dec 24, 2024
16 checks passed
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.

Support opening Replica in read-only mode
2 participants