Differential D6924
[keyserver] Introduce columns in messages table to support pinned messages Authored by rohan on Mar 1 2023, 7:28 AM. Tags None Referenced Files
Subscribers
Details Introduce two new columns to the messages table to handle messages being pinned.
I've also added a migration to update messages. Tested the ALTER TABLE` statements locally in TablePlus individually to confirm the columns gets created and no syntax errors are present. ALTER TABLE messages ADD COLUMN IF NOT EXISTS pinned tinyint(1) UNSIGNED NOT NULL DEFAULT 0, ADD COLUMN IF NOT EXISTS pin_time bigint(20) DEFAULT NULL, ADD INDEX IF NOT EXISTS pinned (pinned); Also checked the table after the migration was successful: 
Diff Detail
Event TimelineComment Actions While I agree that it might be better database design to create a separate table, it will complicate things on the SQLite side. We persist messageStore.messages and threadStore in SQLite on the native client, and it would probably make the most sense to store pinned_messages in the SQLite threads table instead of introducing a whole new table and set of “ops.” That would introduce inconsistency between the MariaDB and SQLite database… which isn’t the end of the world, but could be avoided if we stored pinned_messages in the threads table? Comment Actions We could store pinned_messages in the threads table if we really want to. In terms of storing the messages, I think each time we get a new pin for a thread, we'd run an UPDATE and CONCAT the existing pinned_messages with the new message ID. Essentially looking like a long CSV in the column 103438, 103928, 104948 etc. We could then query the column and split it into an array of IDs and reverse it to have it from newest to oldest pins. It definitely is less ideal, especially when having to remove a messageID from some index in the list when unpinning a message. I'm a little wary of the performance of it when having to find a messageID in a long list of pinned_messages and update the string accordingly with REPLACE If you feel like it's better to introduce a new column to the threads table though, I'm happy to change that Comment Actions
In which way does it affect the database on the keyserver side? On the other hand creating a new table also has some issues from the design's perspective. For example, thread column is a function of messageID because based on message id we can uniquely identify a thread. So this new table isn't e.g. a joining table but instead it depends on just messages table and could be replaced by new columns (is_pinned and pin_timestamp) on it. I'm not sure, at this point, what is the best design here, but I think that we should spent a little more time figuring it out.
Comment Actions Yeah, that's a valid point. An alternative approach, like you suggest, is to run a migration to update the messages table to include two new columns, either a boolean or 0/1 to signify is_pinned, and pin_time. thread is already indexed and it shouldn't be a problem to add an index onto is_pinned since we may be query'ing on it (not sure yet). I'm happy to go down this route as well, it prevents the creation of a new table and doesn't include the complexity of storing pinned messages data within the threads table.
Comment Actions How the pinning will be saved on client's side? Do we plan to have it included in a message info? If that's the case, do we plan to update all the message infos saved on all the clients?
Comment Actions I'm currently introducing a new message type and a message spec for pinned messages, so we'd have messageContentForClientDB. From my understanding, I can support both content for server and client, but feel free to correct me if I'm wrong.
Comment Actions It seems like I wasn't clear enough in my comment, so let me explain it differently. Correct me in any place where I'm wrong. So on the client side we would want to display three things:
As far as I understand, what you're describing handles only the 1st point. How do we plan to handle 2nd and 3rd?
Comment Actions Ah I understand now, sorry. Yeah you're right, those are the three things we want to display client side.
Comment Actions Create compound index, set my DB version back to 20 and re-run the migration to confirm no errors are present, and use EXPLAIN on the query in D7110 to confirm that the query uses the newly created thread_pinned index Comment Actions
Answering these is crucial because it affects how the API looks like. We probably should prefer simplifying the solution but it still requires being sure what the requirements are. Comment Actions
Comment Actions The current fetchPinnedMessageInfos query doesn't have the ability to paginate, but it can always be added in later. We could initially fetch all of the pinned messages at once from the server, but have a task to paginate the fetching. Separately, I'm a bit confused about how the client will determine eg. how many messages are pinned for a given chat. Is this info going to be fetched on-demand when the user opens the chat? (In which case it would appear "dynamically" after being fetched.) Or are we looking to "cache" this information for each thread, so that we correctly display it when opened? I think it would be good to avoid having to fetch anything "dynamically" when a chat is opened. Depending on what information we show by default (I don't recall the designs exactly but it might just be a number of pinned messages), we might want to consider modifying ThreadInfo to store this information. Comment Actions Yeah agreed, I've made a task to track: https://linear.app/comm/issue/ENG-3397/add-pagination-to-fetching-message-pins-for-a-thread
That's a good point, it probably would be good to store the number in ThreadInfo. I've similarly made a follow-up task: https://linear.app/comm/issue/ENG-3396/store-the-number-of-pinned-messages-in-threadinfo | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||