Context Navigation

CacheInvalidation

Timestamp:: Mar 29, 2009, 11:03:13 PM (15 years ago)
Author:: Remy Blank
Comment:: Reorganized discussion section.

Legend:

: Unmodified
: Added
: Removed
: Modified

TracDev/Proposals/CacheInvalidation

-              v16
+              v17
 '''Comments and testing are very welcome'''. The implementation is quite complete, except for the missing database upgrade module. I have only tested with several concurrent `tracd` instances so far.
 ==== cboos' feedback ====
+==== cboos: feedback ====
 It's really nice, I like it a lot!
 When reviewing the code, I think I've detected some possible issues in `CacheManager.get`.
  - in case there are multiple "out-of-date" threads, each might trigger a retrieval. An improvement would be to check if the `CacheManager` already has a "newer" generation.
  - in the locked section, if the generation increases after the cached value retrieval and before the fetch of the latest generation, the `CacheManager` may think it is up to date yet have old data.
 Those are admittedly corner cases, I hope I have not missed more important issues while focusing on that ;-) See the first patch attachment:cache-manager_get-corner-cases.patch.
 …
 I've also added more documentation to the decorators and changed the order of the definitions in a top-down way (first the decorators, then the descriptors, ending with the proxy class), as I think it's easier to understand that way.
 ==== rblank ====
+==== rblank: ====
 Thanks for the improvement ideas, I'll integrate them shortly.
  - I'm not sure the DB race condition you describe can actually happen. At least with SQLite, issuing a `SELECT` sets the lock state to `SHARED`, which disallows writes, so it should not be possible to increase the generation between the data retrieval and fetching the generation. I don't know how this behaves with other databases, though. Maybe it's just safer to fetch the generation first.
-    * SELECT statements don't start a transaction in PySqlite, as opposed to the other DML statements. So in my understanding, each retrieval is "atomic" and I think there can indeed be a race condition between the SELECT(s) done for data retrieval and the SELECT for fetching the generation.
-      As this picked my curiosity, I tried to see how multiple SELECTs could be done within a single transaction, and this is indeed possible, but a bit heavyweight: see e.g. pysqlite:IdRange, look for `def get_id_range`. So I think it's better to simply cope with the race.
  - You're right about automating the cache key generation. I didn't want to do it first, because renaming a module or class would have changed the key. But we're not going to rename them, and even if we do, it will only leave an orphaned row in the `cache` table, so it's no big deal. Your patch proposed `{module}.{function}` as the key, I'd like to make it `{module}.{class}.{function}`.
-   * simple oversight on my part; sure, make the class name part of the key.
  - If the keys are auto-generated, the decorators don't need any arguments anymore. This allows simplifying them even more by dropping the `cached()` and `cached_value()` functions, and calling the descriptor classes `cached` and `cached_value` directly.
+   * great, I didn't know one can do that
+==== !CacheManager used in non-Component classes ====
+==== cboos: ====
+ - SELECT statements don't start a transaction in PySqlite, as opposed to the other DML statements. So in my understanding, each retrieval is "atomic" and I think there can indeed be a race condition between the SELECT(s) done for data retrieval and the SELECT for fetching the generation.
+   As this picked my curiosity, I tried to see how multiple SELECTs could be done within a single transaction, and this is indeed possible, but a bit heavyweight: see e.g. pysqlite:IdRange, look for `def get_id_range`. So I think it's better to simply cope with the race.
+ - simple oversight on my part; sure, make the class name part of the key.
+ - great, I didn't know one can do that
+==== cboos: !CacheManager used in non-Component classes ====
 When thinking about how to use the !CacheManager for getting the `youngest_rev` information from !CachedRepository, I saw two additional problems:
  - we can't use the decorators here, as the !CachedRepository is not a Component (and shouldn't be, as we may have many instances per environment)
  - so far we have avoided propagating the `env` to the `CachedRepository`. I think we can no longer afford to do this, if we want to access the !CacheManager conveniently. Having the `env` available would also simplify the `getdb` stuff.
 …
 Do you see a better way to do it?
+ - Yes, by instantiating `CacheProxy` in the constructor and storing it as an instance attribute. This gives it the same interface as if `@cached` was used.
+==== rblank: ====
+Yes, by instantiating `CacheProxy` in the constructor and storing it as an instance attribute. This gives it the same interface as if `@cached` was used.
 {{{
 #!python
     self.metadata = CacheProxy('CachedRepository.metadata:' + self.name, self.get_metadata, env)
 }}}
+   This does indeed require `env`, and changing that will make the `CachedRepository` unit tests a bit more complicated :-/
+==== Update with feedback ====
+This does indeed require `env`, and changing that will make the `CachedRepository` unit tests a bit more complicated :-/
+==== rblank: Update with feedback ====
 The attachment:cache-manager-r7989.patch is an updated patch which should take into account all corner cases described above. Cache keys are now auto-generated from the module, class and attribute name. I have also added the database upgrade code, so the `db_version` is now 22.
 Are there any other issues that should be considered? If not, the next step would be to plan the integration into trunk. Are there any special considerations when upgrading the database version? What else (other than committing) must be done?
+==== cboos: feedback ====
  - Last round of feedback:
    - The API documentation should also mention that `cached` and `cached_value`
      must be used within Component sub-classes, and what the `receiver` method
+     must be used within Component sub-classes, and what the `retriever` method
      should look like
-     - Will do.
    - in `CacheManager.invalidate`, the `SELECT ... `, `if fetchone UPDATE`, `else INSERT`
      is not thread-safe (again for the same reason that a SELECT doesn't start a transaction)
+     so we should rather do `try INSERT `, `except UPDATE`.
+     - That's what I tried first, but the error due to the `INSERT` rolled back the whole transaction. I'll have to find a way to do this in a single statement.
+       - Hm right, that can be problematic. So what about this:
+         {{{
+     so we should rather do `try INSERT`, `except UPDATE`.
+ Both points are minor and could be done on trunk.
+ - What to do next?
+   - maybe send a mail on Trac-dev (in the same thread you started a while ago)
+     saying the topic work is done and ask if anyone has some extra feedback to give
+   - after the commit, warn loudly on the milestone:0.12, on the ["TracDev/ReleaseNotes/0.12"]
+     and ["0.12/TracUpgrade"] pages that the DB version has increased.
+     It's not that it's problematic to do the upgrade, it's rather because
+     it's inconvenient to downgrade. As long as we keep the DB version compatible,
+     users can eventually go back and forth between trunk and 0.11-stable.
+     Once they did an upgrade, it's not that convenient anymore (but still
+     relatively easy to do in this specific case, of course).
+ - We could also think about adding some tests for this, though that might be more involved.
+==== rblank: ====
+ - Replies to last round of feedback:
+   - Will do.
+   - That's what I tried first, but the error due to the `INSERT` rolled back the whole transaction. I'll have to find a way to do this in a single statement.
+==== cboos: ====
+Hm right, that can be problematic. So what about this:
+{{{
 #!python
     cursor.execute("SELECT generation FROM cache WHERE key=%s",
 …
             cursor.execute("UPDATE cache SET generation=generation+1"
                            "WHERE key=%s", (key,))
+           }}}
+         If we were in a transaction, then I suppose the SELECT/INSERT
+         sequence can't fail. Conversely, if it fails, then we were
+         ''not'' in a transaction, and we can follow-up with an UPDATE
+         to recover from the failed INSERT.
+       - That could work, yes. How about this:
+         {{{
+}}}
+If we were in a transaction, then I suppose the SELECT/INSERT
+sequence can't fail. Conversely, if it fails, then we were
+''not'' in a transaction, and we can follow-up with an UPDATE
+to recover from the failed INSERT.
+==== rblank: Alternative for atomic UPSERT ====
+That could work, yes. How about this:
+{{{
 #!python
     cursor.execute("UPDATE cache SET generation=generation+1 "
 …
     if not cursor.fetchone():
         cursor.execute("INSERT INTO cache VALUES (%s, %s)", (key, 0))
+         }}}
+         If the row already exists, it is updated, the `SELECT` returns a row and we're done.
+         If not, the `UPDATE` does nothing except starting a transaction (or we may already be in a transaction), the `SELECT`
+         doesn't return any rows, and we do the `INSERT` in the same transaction. Doesn't the `UPDATE` even return the number
+         of altered rows? That would remove the need for a separate `SELECT`. I'm not sure though that the `UPDATE` starts a transaction
+         if no rows are altered. We may have to use a dummy row that is always updated in addition to the desired row.
+   Both points are minor and could be done on trunk.
+ - What to do next?
+   - maybe send a mail on Trac-dev (in the same thread you started a while ago)
+     saying the topic work is done and ask if anyone has some extra feedback to give
+   - after the commit, warn loudly on the milestone:0.12, on the ["TracDev/ReleaseNotes/0.12"]
+     and ["0.12/TracUpgrade"] pages that the DB version has increased.
+     It's not that it's problematic to do the upgrade, it's rather because
+     it's inconvenient to downgrade. As long as we keep the DB version compatible,
+     users can eventually go back and forth between trunk and 0.11-stable.
+     Once they did an upgrade, it's not that convenient anymore (but still
+     relatively easy to do in this specific case, of course).
+ - We could also think about adding some tests for this, though that might be more involved.
+}}}
+If the row already exists, it is updated, the `SELECT` returns a row and we're done.
+If not, the `UPDATE` does nothing except starting a transaction (or we may already be in a transaction), the `SELECT`
+doesn't return any rows, and we do the `INSERT` in the same transaction. Doesn't the `UPDATE` even return the number
+of altered rows? That would void the need for a separate `SELECT`. I'm not sure though that the `UPDATE` starts a
+transaction if no rows are altered. We may have to use a dummy row that is always updated in addition to the desired row.
 == Idea 2: Cache control ==