Details

Reviewers

ginsu
ashoat
tomek
atul

Commits

rCOMM65e4193c4b28: [keyserver] Fix the pinned count MariaDB out of range issue

Summary

Context is in ENG-4399. Currently, if the client sends the unpin or pin action twice in a row, we don't
validate it on the keyserver, and instead allow it. We definitely want to prevent two back to back pin actions for the same message, but even more so for two unpin actions since that throws an error on
the keyserver.

THe best way I found to address both cases is to check the status of the message (is it already pinned / unpinned), and compare the requested action. If they are the same, we know we should not duplicate the
pin/unpin action, and return early. Otherwise, if the pin status is being changed, we can just continue as normal.

Test Plan

Confirmed that by sending two back to back unpin or pin actions for the same message, only the first one causes a change. The second one is just returned early

Diff Detail

Repository

rCOMM Comm

Branch

toggle_pin_error

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.Jul 31 2023, 10:42 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 31 2023, 10:42 AM

Harbormaster completed remote builds in B21385: Diff 29297.Jul 31 2023, 11:57 AM

rohan requested review of this revision.Jul 31 2023, 11:57 AM

We definitely want to prevent two back to back pin actions for the same message, but even more so for two unpin actions since that throws an error on the keyserver.

If this is the goal, I think it makes most sense to put both the "check" and UPDATE in the same DB transaction so they occur "atomically."

This will

Ensure correctness
Reduce sequential roundtrips to DB

keyserver/src/updaters/thread-updaters.js
864–910 ↗	(On Diff #29297)	Could we group these together in a single transaction? It would Ensure correctness Reduce sequential roundtrips to DB (We can determine whether rows were updated using `results.affectedRows`)

This revision now requires changes to proceed.Jul 31 2023, 12:08 PM

There's still a chance that the DB will get updated between select and update. One solution, as @atul suggested, might be to use transactions - which will work but might affect the performance. But there's another option: an update to multiple tables at once https://stackoverflow.com/questions/4361774/mysql-update-multiple-tables-with-one-query - can we use it?

Address feedback

In D8683#256553, @tomek wrote:

There's still a chance that the DB will get updated between select and update. One solution, as @atul suggested, might be to use transactions - which will work but might affect the performance. But there's another option: an update to multiple tables at once https://stackoverflow.com/questions/4361774/mysql-update-multiple-tables-with-one-query - can we use it?

Ah I wasn't aware that I could do an update to two tables without a transaction. Thanks for the link, I'll attempt an implementation of this first!

keyserver/src/updaters/thread-updaters.js
864–910 ↗	(On Diff #29297)	Yeah I can group these all, if I go with @tomek's suggestion I can still also use `results.affectedRows` as a way to know whether a pin action actually occurred

rohan added inline comments.Aug 1 2023, 8:24 AM

keyserver/src/updaters/thread-updaters.js
866–873	I've combined all of the queries into one as per @atul's suggestion, I think it's a lot cleaner. Also used the link @tomek provided for a multi-update query Ultimately, this now does two things: Updates the `messages.pinned` and `messages.pin_time` based on the pin action Update the `thread.pinned_count` based on the pin action The `WHERE` clause helps with the validation and preventing the original issue: `WHERE m.id = ${messageID}` --> we only cause the update on the provided message `WHERE m.thread = ${threadID}` --> we only cause the update on the provided message in the thread `WHERE t.id = ${threadID}` --> we only cause the update on the provided thread `WHERE m.pinned <> ${pinnedValue}` --> this solves the original issue, where we use the MariaDB not equal operator in order to ensure we only update if the pin action is different from the current pin value. If it is, we will go through with the updates and `result.affectedRows > 0`, otherwise it will not affect any rows.

Harbormaster completed remote builds in B21465: Diff 29441.Aug 1 2023, 9:29 AM

Looks good at a high level, adding @ashoat as blocking to make sure someone w/ more context on roles takes a look before landing.

...
AND m.pinned <> ${pinnedValue}
...

So it looks like <> is equivalent to !=, and might even be preferred in some instances for portability:

But... still think we should stick to != for consistency.

This revision now requires changes to proceed.Aug 1 2023, 2:10 PM

Makes sense to me, but I'd like @tomek to give a final approval since he suggested the multi-update query

keyserver/src/updaters/thread-updaters.js
892–905	Not exactly relevant to this diff, but it feels like the message creation can happen in parallel with the update creations. If we just wrapped lines 878-849 in one async IIFE, and lines 892-905 in another one, we could pass both promies into a single `Promise.all`. Would be best to do that in a separate diff though
897–905	Feels like we should skip the updates too if `result.affectedRows === 0`, no?

Should we also create a migration that fixes pinned_count for threads affected by the fixed bug?

keyserver/src/updaters/thread-updaters.js
867–868	Do we align equal signs in other places?

In D8683#257047, @tomek wrote:

Should we also create a migration that fixes pinned_count for threads affected by the fixed bug?

Yeah it could be a good safeguard to update the thread pinned_count to match the number of pinned messages in each thread.

keyserver/src/updaters/thread-updaters.js
867–868	I don't actually think so, I'll fix the formatting

Fix formatting
Use != instead of <>
Early return if result.affectedRows === 0 to skip updates as well

(Migration will come as a follow-up in this stack before landing this)

Harbormaster completed remote builds in B21536: Diff 29544.Aug 4 2023, 8:29 AM

rohan added a child revision: D8732: [keyserver] Run migration to update thread pinned_counts.Aug 4 2023, 8:34 AM

ashoat added inline comments.Aug 4 2023, 8:35 AM

keyserver/src/updaters/thread-updaters.js
892–905	Ping on this feedback (starting with "Not exactly")

rohan added inline comments.Aug 4 2023, 8:41 AM

keyserver/src/updaters/thread-updaters.js
892–905	My bad I didn't address it, but I'm working on that locally right now!

atul resigned from this revision.Aug 5 2023, 12:12 AM

This revision is now accepted and ready to land.Aug 5 2023, 12:12 AM

ashoat added inline comments.Aug 5 2023, 9:33 AM

keyserver/src/updaters/thread-updaters.js
868–869 ↗	(On Diff #29544)	Nit: we usually align it this way

ashoat accepted this revision.Aug 5 2023, 9:33 AM

Fix indentation

This revision was landed with ongoing or failed builds.Aug 7 2023, 8:06 AM

Closed by commit rCOMM65e4193c4b28: [keyserver] Fix the pinned count MariaDB out of range issue (authored by rohan). · Explain Why

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM65e4193c4b28: [keyserver] Fix the pinned count MariaDB out of range issue.

Harbormaster completed remote builds in B21566: Diff 29587.Aug 7 2023, 8:22 AM

[keyserver] Fix the pinned count MariaDB out of range issue
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 29441

keyserver/src/updaters/thread-updaters.js

[keyserver] Fix the pinned count MariaDB out of range issueClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 29441

keyserver/src/updaters/thread-updaters.js

[keyserver] Fix the pinned count MariaDB out of range issue
ClosedPublic
Actions

Revision Contents
Changeset List