Page MenuHomePhabricator

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

Authored by rohan on Jul 31 2023, 10:42 AM.
Tags
None
Referenced Files
F3516405: D8683.id29441.diff
Sun, Dec 22, 2:20 PM
Unknown Object (File)
Wed, Dec 18, 6:17 AM
Unknown Object (File)
Thu, Nov 28, 2:47 PM
Unknown Object (File)
Sat, Nov 23, 11:39 PM
Unknown Object (File)
Sat, Nov 23, 12:56 PM
Unknown Object (File)
Sat, Nov 23, 12:55 PM
Unknown Object (File)
Sat, Nov 23, 12:55 PM
Unknown Object (File)
Sat, Nov 23, 12:55 PM
Subscribers

Details

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

atul requested changes to this revision.Jul 31 2023, 12:08 PM

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

  1. Ensure correctness
  2. Reduce sequential roundtrips to DB
keyserver/src/updaters/thread-updaters.js
864–910

Could we group these together in a single transaction? It would

  1. Ensure correctness
  2. 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?

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

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

keyserver/src/updaters/thread-updaters.js
866–873 ↗(On Diff #29441)

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:

  1. Updates the messages.pinned and messages.pin_time based on the pin action
  2. 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.

atul requested changes to this revision.Aug 1 2023, 2:10 PM
atul added a reviewer: ashoat.

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:

e256bf.png (1×1 px, 233 KB)

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 ↗(On Diff #29441)

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 ↗(On Diff #29441)

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 ↗(On Diff #29441)

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 ↗(On Diff #29441)

I don't actually think so, I'll fix the formatting

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

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

keyserver/src/updaters/thread-updaters.js
892–905 ↗(On Diff #29441)

Ping on this feedback (starting with "Not exactly")

keyserver/src/updaters/thread-updaters.js
892–905 ↗(On Diff #29441)

My bad I didn't address it, but I'm working on that locally right now!

This revision is now accepted and ready to land.Aug 5 2023, 12:12 AM
keyserver/src/updaters/thread-updaters.js
868–869 ↗(On Diff #29544)

Nit: we usually align it this way

Fix indentation

This revision was landed with ongoing or failed builds.Aug 7 2023, 8:06 AM
This revision was automatically updated to reflect the committed changes.