Paths

Table of Contentst

[SQLite] add version column to session table & rename to `target_device`
ClosedPublic
Actions

Authored by kamil on Apr 8 2024, 6:19 AM.

Details

Reviewers

bartek
marcin
tomek

Commits

rCOMMc2ec64740555: [SQLite] add version column to session table & rename to `target_device`

Summary

ENG-7285.
ENG-7034.

Depends on D11574

Test Plan

Tested query in SQLite.

Before:

Query:

After:

Tested migration on both web and native.
Tested creating DB from scratch on both web and native.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Apr 8 2024, 6:19 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptApr 8 2024, 6:19 AM

kamil edited the test plan for this revision. (Show Details)Apr 8 2024, 6:22 AM

fix identation

kamil added a child revision: D11580: [SQLite] update `OlmPersistSession` entity.Apr 8 2024, 6:32 AM

Harbormaster failed remote builds in B28047: Diff 38897!Apr 8 2024, 6:37 AM

Harbormaster failed remote builds in B28049: Diff 38899!Apr 8 2024, 6:43 AM

Tests are failing because of a not-yet updated entity - this is fixed in the next diff (D11580). I will land it together or merge those two diffs.

kamil published this revision for review.Apr 9 2024, 6:00 AM

I have for questions for @kamil:

What is the purpose of version column?
Isn't the counter value extractable from the session object itself?
When and how is it going to be updated? Is it possible that value in the version column will go out of sync with the counter value in the session object?
Finally, is that going to be a problem the we don't have such counterID tracking for notifs sessions - in case it is should we implement it for notifs as well?

In D11577#333755, @marcin wrote:

Answer to your questions is here but just for the record:

I have for questions for @kamil:

What is the purpose of version column?

To track the version of olm session between two clients helps discover race conditions when to create a new session or discard an old session request.

Isn't the counter value extractable from the session object itself?

If you're asking about olm session (not our definition of Session) - the answer is no.

When and how is it going to be updated?

When creating a new session/accepting a session request.

Is it possible that value in the version column will go out of sync with the counter value in the session object?

Those are two completely different things.

Finally, is that going to be a problem the we don't have such counterID tracking for notifs sessions - in case it is should we implement it for notifs as well?

We should implement it for notifs while working on DMs - now there is no need to I guess.

marcin accepted this revision.Apr 10 2024, 6:19 AM

marcin added inline comments.

native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp
634 ↗	(On Diff #38899)	Nit: I would rather name it as `add_version_column_to_olm_persist_sessions_table`. Current name is quite generic and actually any migration on this table could be named like that.

This revision is now accepted and ready to land.Apr 10 2024, 6:19 AM

We should implement it for notifs while working on DMs - now there is no need to I guess.

Is there a task for this?

In D11577#333862, @ashoat wrote:

We should implement it for notifs while working on DMs - now there is no need to I guess.

Is there a task for this?

I think it is https://linear.app/comm/issue/ENG-7658/add-olm-session-versioning-for-notifs - is that correct @kamil ?

rename to add_version_column_to_olm_persist_sessions_table

In D11577#334170, @marcin wrote:

In D11577#333862, @ashoat wrote:

We should implement it for notifs while working on DMs - now there is no need to I guess.

Is there a task for this?

I think it is https://linear.app/comm/issue/ENG-7658/add-olm-session-versioning-for-notifs - is that correct @kamil ?