This differential adds new table to MariaDB database on the keyserver to store olm sessions for encrypted notifications.
Details
Details
To test migrations-config.js restart your keyserver terminal. Ensure DB version: 31 log line is visible.
To test setup-db.js create new database and update secrets folder so that it points to new database instance. Start your keyserver.
In both cases using a database connection tool of choice (I prefer to use mariadb terminal command), ensure olm_notifications_sessions table is present and its structure matches SQL statement used to create it.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-3026
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Set charset and collation for olm_notifications_session table both in setup-db.js and migration-config.js.
Comment Actions
- Make the table for olm sessions generic.
- Update primary key to be a pair of cookie and boolean indicating whether olm session was established using primary identity associated with cookie.
Comment Actions
Looks correct at a high level. One concern I had was the choice of ascii CHARSET. That isn't a CHARSET that we use elsewhere in create-db or migration-config, so curious as to the reasoning.
I don't know much about character sets, but my understanding is that ASCII is fairly limited? Maybe that's fine with what we're storing? Is there any downside to going with eg utf8mb4?
Adding @ashoat as blocking since he has more context on MySQL/MariaDB and the character sets we've chosen in the past.
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 336–337 ↗ | (On Diff #25218) | Could you explain choice of charset here? |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 336–337 ↗ | (On Diff #25218) | Pickling an olm session returns b64 string which is restricted to ASCII charset, so we should be safe using this charset here. The UTF8 dataset is suitable here as well, however we are somehow more efficient here using ASCII. |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 333 ↗ | (On Diff #25218) | Can you indent this to match how other queries are formatted? I think this might be Prettier's fault, but it should be possible to align it correctly |
| 336–337 ↗ | (On Diff #25218) | What's the difference between the latin1 charset we use elsewhere in the database and the ascii charset? Can you explain why you chose ascii instead of latin1? |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 336–337 ↗ | (On Diff #25218) | Basically latin1 is a superset of ascii that is still smaller than utf8: https://stackoverflow.com/questions/30750526/determining-iso-8859-1-vs-us-ascii-charset. ascii uses 7 bits to store a character, while latin1 uses 8, so the difference is negligible. Therefore I am open to replace ascii with latin1 if you are worried that ascii can be problematic, since we haven't use it in the project yet. |
| keyserver/src/database/setup-db.js | ||
|---|---|---|
| 255 ↗ | (On Diff #25492) | Do we not need the pickling key here? If not, where will it be stored? |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 333 ↗ | (On Diff #25218) | Sure. Just want to note that migration that creates message_search table is similarly malformatted. |
| keyserver/src/database/setup-db.js | ||
| 255 ↗ | (On Diff #25492) | The answer depends on our approach. On native CryptoModule uses the same pickling key for account and its related sessions. My idea was to do the same here. We will us primary account pickling key for primary sessions and notifications pickling key for notifications sessions. Since we cant instantiate a session without an account relevant pickling key is guaranteed to exist.
OR
I can't see any advantages of 1., while there is indeed an advantage of 2 in terms of simpler SQL queries when updating session persistence. However keeping a copy of uuid.v4 for every session row of the database will be significant memory overhead. For now I would proceed with my initial approach. |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 336 ↗ | (On Diff #25590) | Why is it possible for this to be NULL? |
Comment Actions
Not clear why pickled_olm_session can be NULL... I think we should switch it to NOT NULL before landing
Comment Actions
I have repeatedly given you feedback about line length. Please make sure to address it BEFORE putting up diffs in the future
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 360 ↗ | (On Diff #25997) | This is still over 80 chars long! Please pay attention to this feedback |
Comment Actions
Rebase before landing. Add version column to the olm_sessions table as it is clear we will need it for synchronization in future
| keyserver/src/database/setup-db.js | ||
|---|---|---|
| 268 ↗ | (On Diff #26277) | Character set should be latin1 here |