Skip to content

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

Merged
merged 8 commits into from
Oct 13, 2019
Merged

Conversation

audetto
Copy link
Contributor

@audetto audetto commented Oct 10, 2019

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.

@audetto audetto mentioned this pull request Oct 10, 2019
self.create_tag_index()
else:
self.drop_tag_index()
if not self.sqlite_read_only:
Copy link
Contributor Author

@audetto audetto Oct 10, 2019

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.

@audetto
Copy link
Contributor Author

audetto commented Oct 11, 2019

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
https://docs.python.org/3/library/sqlite3.html#sqlite3.connect

When writing to such a db, one gets an error as

Traceback (most recent call last):
File "/home/andrea/projects/cvs/3rdParty/python-diskcache/diskcache/core.py", line 721, in _transact
sql('BEGIN IMMEDIATE')
sqlite3.OperationalError: attempt to write a readonly database

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/home/andrea/projects/cvs/3rdParty/python-diskcache/aaa.py", line 11, in
del a[k]
File "/home/andrea/projects/cvs/3rdParty/python-diskcache/diskcache/core.py", line 1361, in delitem
with self._transact(retry) as (sql, cleanup):
File "/usr/lib64/python3.7/contextlib.py", line 112, in enter
return next(self.gen)
File "/home/andrea/projects/cvs/3rdParty/python-diskcache/diskcache/core.py", line 731, in _transact
raise Timeout
diskcache.core.Timeout

which mentions read only.

There are still something to improve:
-settings are only available after the db is opened. I've preprocessed read-only, but it could be better
-better path->uri conversion

What do you think?

@grantjenks
Copy link
Owner

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?

@audetto
Copy link
Contributor Author

audetto commented Oct 13, 2019

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.

@grantjenks
Copy link
Owner

Take a look at the failing CI test. I think it’s the pragma check. Maybe the readonly setting needs an exemption.

@audetto
Copy link
Contributor Author

audetto commented Oct 13, 2019

sqlite_xxx settings are actually sqlite PRAGMAs which can be very powerful (and indeed there is a query_only pragma) but as well confusing

  • some of them are permanent and some transient (poorly documented in sqlite)
  • diskcache opens the connection twice and only applies PRAGMAs the first time

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.
The only thing which is "special" is that the setting read_only must be processed before everything else (i.e. at connection opening time).

@grantjenks
Copy link
Owner

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.

@audetto
Copy link
Contributor Author

audetto commented Oct 13, 2019

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.

@grantjenks
Copy link
Owner

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.

@audetto
Copy link
Contributor Author

audetto commented Oct 13, 2019

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.

@grantjenks
Copy link
Owner

The pragmas are set in the connection property. There’s a test for it.

@audetto
Copy link
Contributor Author

audetto commented Oct 13, 2019

You are right.

The fact is not that pragmas as not applied, I was wrong.

The latest version cleans this up.
Input settings are merged with db settings, saved to the db and when it is reopened, they are stored in pragmas and in member variables.

The read-only is never stored to the db, so it needs something special to survive the 2nd connection creation.
In the latest version this is a bit clearer.

@grantjenks grantjenks merged commit 60af714 into grantjenks:master Oct 13, 2019
grantjenks pushed a commit that referenced this pull request Oct 13, 2019
@grantjenks
Copy link
Owner

I merged this but later moved it. Checkout the query-only-support branch. It's a work in progress.

grantjenks pushed a commit that referenced this pull request Jul 5, 2020
grantjenks pushed a commit that referenced this pull request Jul 5, 2020
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.

2 participants