-
Notifications
You must be signed in to change notification settings - Fork 151
Possible example of a readonly cache. #127
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
diskcache/core.py
Outdated
self.create_tag_index() | ||
else: | ||
self.drop_tag_index() | ||
if not self.sqlite_read_only: |
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 wasn't too sure what this does. Could not locate the documentation.
The original proposal would not protect a writeable DB when the read_only flag is set to true (i've now fixed this). Using a sqlite3 feature it is possible to do so When writing to such a db, one gets an error as Traceback (most recent call last): During handling of the above exception, another exception occurred: Traceback (most recent call last): which mentions read only. There are still something to improve: What do you think? |
For the most part, I like it actually. I was unaware of the SQLite setting and my approach didn’t add the attribute. Do these changes preserve the semantics around pragma setting on the SQLite connection? |
You mentioned pragmas already. What do you mean exactly? The code I have implemented clearly does not update setting if they are different in case of readonly, but still uses the ones provided. |
Take a look at the failing CI test. I think it’s the pragma check. Maybe the readonly setting needs an exemption. |
sqlite_xxx settings are actually sqlite PRAGMAs which can be very powerful (and indeed there is a query_only pragma) but as well confusing
I did not know about the sqlite_ prefix and I did not really need it, so in the latest version it has been renamed to read_only. |
Pragmas should be applied both times (some are connection specific). I would rather use the query_only pragma. I had thought read_only was just such a thing. Seems like a nice added benefit/guarantee. |
If you add logging to Cache._con and to "PRAGMA %s = %s" you will see they are only applied the first time. That this means there is a bug in the PRAGMA code? I can turn the read-only option to a PRAGMA, but it will have to be handled in the same special way. Immediately after the connection is established. |
I thought that bug had been fixed. That’s at least what the test is for. Pragmas have to always be applied to connections because some work on a per-connection basis. The code should use the sqlite_query_only setting/pragma. I’m fine with a few special paths to support the pragma. |
OK, here is a version using the PRAGMA query_only rather than the URI modifier. I think the fact that pragmas are not applied the 2nd time should be addressed anyway. |
The pragmas are set in the connection property. There’s a test for it. |
You are right. The fact is not that pragmas as not applied, I was wrong. The latest version cleans this up. The read-only is never stored to the db, so it needs something special to survive the 2nd connection creation. |
I merged this but later moved it. Checkout the query-only-support branch. It's a work in progress. |
This is a minimal example of a Read Only cache.
It could be made more explicit by checking the flag at the API boundary but I wanted to make it as small a change as possible.
The only issue is with the potential infinite loop which needs to be broken. I have not tried the entire API to ensure it does not hang.
I am not asking to commit as is, just to take it as a suggestion.