[sql-21] sessions: SQL schemas & queries #994
Conversation
ellemouton
added
no-changelog
ellemouton
force-pushed
the
sql21Sessions13
branch
2 times, most recently
from
fc95724 to
f06e75c
Compare
ellemouton
changed the title
ellemouton
force-pushed
the
sql21Sessions13
branch
from
f06e75c to
afc5cca
Compare
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
LGTM 🚀! Leaving a few suggestions that I think will be useful.
| -- ID to use for the group ID. | ||
| group_id BIGINT REFERENCES sessions(id) ON DELETE CASCADE |
There was a problem hiding this comment.
Should we create an index for this as there's a queries using this as the identifier?
There was a problem hiding this comment.
ah, interesting - I thought foreign keys where automatically indexed. Turns out that is incorrect - so yes, will add 👍
There was a problem hiding this comment.
so yeah great catch - i totally had the wrong assumption - It also means i need to add indices for the tables below on any foreign key
|
|
||
| -- The session_macaroon_permissions table contains the macaroon permissions | ||
| -- that are associated with a session. | ||
| CREATE TABLE IF NOT EXISTS session_macaroon_permissions ( |
There was a problem hiding this comment.
Hmm I think it makes sense to include an id field for this table, session_feature_configs & session_privacy_flags as it'll likely be useful in the future?
There was a problem hiding this comment.
ok so i think it makes sense to add the id for mac perms and caveats since those are lists and not necessarily unique but think for feature configs and priv flags, it does not make sense to add id since those will always have a unique foreign-key:one-other-field pair.
sound good?
ellemouton
force-pushed
the
sql21Sessions13
branch
2 times, most recently
from
f855adb to
873e0f5
Compare
ellemouton
left a comment
ellemouton
left a comment
There was a problem hiding this comment.
@ViktorTigerstrom - could you take one last look at the last push (second last compare) 🙏
ViktorT-11
left a comment
ViktorT-11
left a comment
There was a problem hiding this comment.
LGTM after addressing @bitromortac last comment :) 🚀!
This commit adds all the schema definitions we require for the sessions store.
ellemouton
force-pushed
the
sql21Sessions13
branch
from
873e0f5 to
2016766
Compare
3 participants
This PR just defines the SQL Schemas and queries. The following diagram shows the new tables and the relations between them along with the optional link between the sessions and accounts table.
Basically today all of this data is stored in a single serialised session but in SQL land, we want the data to be completely normalised.
This PR is just 1 commit & an ACK here is more about understanding & agreeing with the defined schema.
If you'd like, it may make sense to review while glancing at the follow up which actually adds the CRUD to interact
with the new schema. But if we end up making minor changes to queries in the follow up after this is merged, it is no big deal as nothing activates this code in this PR.