DB caches #3320

aatkin · 2024-08-16T14:01:18Z

This change implements low-level caching for db entities. Motivation is greatly improved performance, especially for db entities that are often queried but rarely changed, due to the overhead in fetching data and serializing it. Most db entities have small memory footprint as well, so they can be cached entirely.

Most cache needs are very basic, but having a standard way of accessing the cache is probably beneficial, for example making sure that db entity cache is ready for use without explicit instructions in every db namespace. With this change nearly all db namespaces implement a common refreshable cache that uses clojure.core.cache.wrapped under hood. Interface is mostly similar, implementation takes care that cache is always up-to-date when accessed.

Renamed references to rems.db.* and rems.service.* so that they are always fully qualified. Many places used ambiguous naming when referring to either db or service functions.

Follow up

test that checks parallel cache update (e.g. transactions test)
clean up SQL that is no longer needed: catalogue get-catalogue-items
clean up localization db tables Refactor catalogue item localizations and license localizations into JSON blobs (and refactor localized text fields depending on these). #3330
decide on naming for coerce-from-db and coerce-to-db
remove catalogue item endt (:expired) Remove catalogue item start and end columns #2286
remove deprecated :form/title Remove deprecated API #3047
dont return {:success true} from service layer, rather throw on exception and let API decide return type
dont bind unnecessarily context/*request* in middleware wrap-context

Possible follow ups

add db schemas where none exist (e.g. catalogue)
catalogue api ; only admins get enabled and disabled items
- is this filtering still needed?
~~detect potential issues with caches~~ (edit: transaction tests now implemented)
- ~~while all tests should pass, there could still be hidden bugs~~
analyze absolute performance
- e.g. new events per second, created applications per second ...
- detect bottlenecks and possibilities for improvement
write-up ADR on how it differs from pre-cache implementation
rename users db table into user attributes, and remove users db entity so that user service is the logical source for "full user"?
remove deprecated resource licenses db table
full-stack test that verifies cache works properly (e.g. 2 requests where one resets, one reloads, what is the result)

Other noteworthy changes

raw db access

should now be limited to db namespaces, with some exceptions. raw db querying is fine (in theory), but updates should be strictly through db namespace
db namespaces should implement mostly similar "copy-paste" db loading and parse functions

rems.db.catalogue / rems.service.catalogue

quite big changes due to db select that implements various query params, implemented "stupider" db select that takes in everything and formats before entering cache
service implements query filter and join that should be 100% compatible with existing API
added all various edit functions to db so that they update cache

rems.db.applications / rems.service.application

removed injections cache, since all db entities now implement their own cache. injections are still in place, not necessarily because they are needed but there is a lot of code that depends on those injections existing.
removed periodic applications cache reloader since injections cache is no more
created application service layer for serving API functions
removed rems.service.todos and moved all related functionality to application service

rems.common.dependency

new namespace that wraps com.stuartsierra.dependency (used in e.g. mondo, caches, dependencies)

rems.db.attachments / rems.service.attachment

relocated license attachment functions here (from license db/service namespace)
separation of concerns: moved several checks and "fix filename" from db level to service level

rems.kaocha

relocated namespace that implements kaocha test runner plugins
implemented second plugin rems.kaocha/cache-statistics-plugin that prints table of cache CRUD statistics after successful test run. usable from CLI like lein kaocha integration --plugin rems.kaocha/cache-statistics-plugin
added cache statistics plugin to both integration and browser test suites in CircleCI for tracking potential issues in cache usage

example table data from real test run

:id	%	:get	:upsert	:evict	:reload	:reset
:rems.db.user-settings/user-settings-cache	100.00	11676	13	1	1	0
> rems.test-browser/test-processed-applications-and-paging	84.77	9909	1	0	0	0
> rems.test-browser/test-applicant-member-invite-action	11.21	1309	2	0	0	0
> rems.test-browser/test-handling	2.02	236	0	0	0	0
> rems.test-browser/test-edit-catalogue-item	1.31	146	6	0	1	0
> rems.test-browser/test-workflow-create-edit	0.43	48	2	0	0	0

:rems.db.users/user-cache	100.00	11622	58	0	1	0
> rems.test-browser/test-processed-applications-and-paging	84.05	9813	5	0	0	0
> rems.test-browser/test-applicant-member-invite-action	11.50	1330	13	0	0	0
> rems.test-browser/test-handling	2.15	243	8	0	0	0
> rems.test-browser/test-edit-catalogue-item	1.61	167	20	0	1	0
> rems.test-browser/test-workflow-create-edit	0.36	41	1	0	0	0

Implemented DB caches

dependent cache = actual cache but uses cache DAG to reset when main cache updates
for time being = actual cache implemented but not sure if want to keep):

rems.db.attachments
- rems.db.attachments/by-application-id dependent cache
- rems.db.attachments/license-attachments-cache (for time being)
rems.db.blacklist
rems.db.catalogue
- rems.db.catalogue/catalogue-item-localizations-cache (for time being)
rems.db.form
rems.db.licenses
- rems.db.licenses/license-localizations-cache (for time being)
rems.db.organization
rems.db.resource
rems.db.roles
- rems.db.roles/users-by-role dependent cache
rems.db.user-mappings
- rems.db.user-mappings/by-extidattribute dependent cache
- rems.db.user-mappings/by-extidvalue dependent cache
rems.db.user-settings (for time being)
rems.db.users
rems.db.workflow

Not yet implemented:

rems.db.entitlements
rems.db.invitation

Existing cache implementations:

rems.db.api-key/api-key-memo
rems.db.events/event-cache

Checklist for author

Remove items that aren't applicable, check items that are done.

Reviewability

Link to issue

Backwards compatibility

API is backwards compatible or completely new

Documentation

Update changelog if necessary
API is documented and shows up in Swagger UI
Update docs/ (if applicable)
Update manual/ (if applicable)
ADR for major architectural decisions or experiments

Testing

Complex logic is unit tested
Valuable features are integration / browser / acceptance tested automatically

Follow-up

New tasks are created for pending or remaining tasks

Dependent cache reset should happen as soon as the cache is updated, but cache update algorithm is not strictly dependent on *when* it happens. add-watch seems like a convenient utility here.

Macroz · 2024-09-30T17:09:31Z

src/cljs/rems/administration/status_flags.cljs

@@ -104,51 +104,70 @@
       (not (:expired item))
       (not (:archived item))))

+(defn- plus-others [item-count]
+  (when (> item-count 5)
+    [:li (str "... plus " (- item-count 5) " more")]))


Localization?

Fix this later. Let's get it merged!

test/clj/rems/api/test_permissions.clj

Macroz · 2024-10-01T08:36:24Z

test/clj/rems/application/test_commands.clj

+             (getx {1 {:resource-id 1}
+                    2 {:resource-id 2}
+                    3 {:resource-id 3 :formid 2}
+                    4 {:resource-id 4 :wfid 2}
+                    5 {:resource-id 5 :formid 2 :wfid 2}
+                    6 {:resource-id 5 :formid nil}
+                    7 {:resource-id 6 :enabled false}


Maybe sometime later we can get to :resource/id, :form/id and :workflow/id for all these.

Macroz · 2024-10-01T12:51:19Z

test/clj/rems/db/test_users.clj

+    (testing "user has different userid in userattrs"
+      ;; NB: raw db call due testing backwards compatible behavior
+      (db/add-user! {:user "different-userid" :userattrs (json/generate-string {:userid "bad"})})
+      (cache/reset! rems.db.users/user-cache)
+      (is (= {:userid "bad"
+              :name nil
+              :email nil}
+             (rems.db.users/get-user "different-userid"))))


Maybe we should get rid of this behavior at some point. We could perhaps choose to have the master data in the blob, and the other column (e.g. user id) just be there for indexing / searching, but of secondary importance. Or just have the blob if it works fast enough (index and constraint functionally into blob)

Macroz · 2024-10-01T13:23:15Z

test/clj/rems/service/test_catalogue.clj

-  :each
-  reset-caches-fixture
-  rollback-db-fixture)
+(def ^:private owner "owner")


I guess we could just inline this.

Macroz · 2024-10-01T13:27:08Z

test/clj/rems/service/test_catalogue.clj

+      (let [response (archive-catalogue-item! false)]
+        (is (= {:success false
+                :errors [{:type :t.administration.errors/dependencies-archived
+                          :forms [{:form/id form-id
+                                   :form/external-title {:en "form" :fi "form" :sv "form"}
+                                   :form/internal-name "catalogue-item-enabled-archived-test-form"}]}]}
+               response)))


I suppose the assertion was simpler before so that the details of the response don't need to be tested here.

Macroz · 2024-10-01T13:28:38Z

test/clj/rems/service/test_catalogue.clj

+      (is (= []
+             (mapv :id (get-catalogue-items))
+             (mapv :id (get-catalogue-items {:archived false}))))


Maybe split into two asserts was better on the error diff?

Macroz · 2024-10-01T16:35:53Z

test/clj/rems/api/test_forms.clj

@@ -255,7 +256,7 @@
                      api-key user-id)
            (is (= {:success false
                    :errors [{:type "t.administration.errors/in-use-by"
-                              :catalogue-items [{:id cat-id :localizations {}}]}]}
+                              :catalogue-items [{:catalogue-item/id cat-id :localizations {}}]}]}


This is technically a change in the API? Perhaps worth a changelog entry.

Also remove tracking reset statistics, because they are not so interesting anymore.

Performance difference here is not very meaningful anymore, since dependency synchronization will always dominate overall execution time.

Cache dependencies can update at any given time. To ensure that caches work correctly, transactions test tries to run read/write operations concurrently, as fast as possible. Most of the time cache read/write operates in correct order, but certain race conditions existed (or may still exist). This change attempts to rectify those. Dependencies are loaded in separate threads, handled by a separate thread pool. Two count down latches are used: one to signal main loader that all sub-dependencies are ready, and one to indicate that sub-dependency thread can release cache locks. Each latch is awaited with timeout, so at maximum they can cause thread locking for duration of the timeout.

When an exception is thrown, for example in middleware, we didn't log anything in some cases. This makes it difficult to know in test what happened. Let's make sure we log the error message also when API-key is used (tests or external calls).

Re-frame and React seem to take a bit of time to update so we can't immediately check the results. We can sleep for a bit. Also let's switch the expected values to the right place.

- Try to simplify the locking strategy to make the code simpler and locking more safe. We want to allow a cache read to happen to stale data so we don't block reads. - Add also a simple performance test with synthetic scenario.

Macroz

Looks good to merge. Only localization issue really needs to be fixed before release. Others can be worked in the follow-up issues.

aatkin changed the title ~~Users cache 3~~ DB caches 3 Aug 16, 2024

aatkin force-pushed the users-cache-2 branch from 9818973 to 0295279 Compare August 16, 2024 14:07

aatkin changed the base branch from cache-optimizations to users-cache-1 August 16, 2024 14:07

aatkin force-pushed the users-cache-1 branch from 539100b to 81f6300 Compare August 16, 2024 14:10

aatkin force-pushed the users-cache-2 branch 3 times, most recently from 2c116a7 to 75b56ad Compare August 21, 2024 17:26

aatkin changed the base branch from users-cache-1 to cache-optimizations August 21, 2024 17:27

aatkin changed the title ~~DB caches 3~~ Users DB caches Aug 21, 2024

aatkin changed the title ~~Users DB caches~~ DB caches, part 2 Aug 21, 2024

aatkin force-pushed the cache-optimizations branch from 891b9d3 to 5dd4781 Compare August 23, 2024 08:09

aatkin force-pushed the users-cache-2 branch 2 times, most recently from e8c679a to f5436bb Compare August 27, 2024 14:55

aatkin changed the base branch from cache-optimizations to master August 27, 2024 14:57

aatkin mentioned this pull request Aug 27, 2024

DB caches, part 1 #3318

Closed

18 tasks

aatkin changed the title ~~DB caches, part 2~~ DB caches Aug 27, 2024

aatkin force-pushed the users-cache-2 branch from f5436bb to 1d2903f Compare August 28, 2024 14:58

aatkin added 2 commits September 12, 2024 15:32

feat: common cache protocol

98e04d6

feat: use common cache in application injections

1a5cf3c

aatkin force-pushed the users-cache-2 branch 3 times, most recently from 37a972a to c3d1689 Compare September 13, 2024 11:04

aatkin added 8 commits September 13, 2024 16:13

feat: workflow db cache

67c7246

test: reset caches more consistently

4f34068

feat: user settings db cache

2f3e3dc

feat: user mappings db cache

c74f817

feat: user db cache

f7f3f7d

feat: role db cache

5a3690f

feat: resource db cache

55b3414

feat: licenses db cache

14c133d

feat: use watch instead to reset dependent caches

7030002

Dependent cache reset should happen as soon as the cache is updated, but cache update algorithm is not strictly dependent on *when* it happens. add-watch seems like a convenient utility here.

aatkin force-pushed the users-cache-2 branch from 35aa47e to 7030002 Compare September 27, 2024 15:41

Macroz reviewed Sep 30, 2024

View reviewed changes

Macroz reviewed Oct 1, 2024

View reviewed changes

test/clj/rems/api/test_permissions.clj Show resolved Hide resolved

Macroz reviewed Oct 1, 2024

View reviewed changes

aatkin added 6 commits October 10, 2024 13:29

feat: change cache reset to uninitialization

e559968

Also remove tracking reset statistics, because they are not so interesting anymore.

feat: use atom instead of volatile

ad289e4

Performance difference here is not very meaningful anymore, since dependency synchronization will always dominate overall execution time.

refactor: extract random wait into function

8a6561b

refactor: remove unused functions and update doc

10f3597

fix: ensure cache thread pool is always started

11a8937

aatkin force-pushed the users-cache-2 branch from 5058078 to 11a8937 Compare October 10, 2024 10:29

aatkin and others added 9 commits October 10, 2024 14:46

fix: increase test timeout

b303050

wip: somewhat working cache transactions

7adb4de

wip: add logging to investigate failing test

c14d544

fix: shaky form editor test

6012ce9

Re-frame and React seem to take a bit of time to update so we can't immediately check the results. We can sleep for a bit. Also let's switch the expected values to the right place.

refactor: move range-1 to util

3aa27fd

feat: simplify cache locking and test

2e18aca

- Try to simplify the locking strategy to make the code simpler and locking more safe. We want to allow a cache read to happen to stale data so we don't block reads. - Add also a simple performance test with synthetic scenario.

test: cache evictions

e4f2203

chore: remove unused code

47e6e43

Macroz approved these changes Nov 4, 2024

View reviewed changes

Macroz merged commit 241d84b into master Nov 4, 2024
7 checks passed

Macroz deleted the users-cache-2 branch November 4, 2024 08:28

Macroz mentioned this pull request Nov 4, 2024

Refactoring layers and cache #3175

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DB caches #3320

DB caches #3320

aatkin commented Aug 16, 2024 •

edited

Loading

Macroz Sep 30, 2024

Macroz Nov 4, 2024

Macroz Oct 1, 2024

Macroz Oct 1, 2024

Macroz Oct 1, 2024

Macroz Oct 1, 2024

Macroz Oct 1, 2024

Macroz Oct 1, 2024

Macroz left a comment

DB caches #3320

DB caches #3320

Conversation

aatkin commented Aug 16, 2024 • edited Loading

Follow up

Possible follow ups

Other noteworthy changes

Implemented DB caches

Not yet implemented:

Existing cache implementations:

Checklist for author

Reviewability

Backwards compatibility

Documentation

Testing

Follow-up

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Macroz left a comment

Choose a reason for hiding this comment

aatkin commented Aug 16, 2024 •

edited

Loading