Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bonus links and transactions sketch #3648

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

jcushman
Copy link
Contributor

@jcushman jcushman commented Nov 1, 2024

This is a quick sketch to try to reduce opportunities for deadlocks in saving or moving links.

  • Don't use a transaction in user.save() unless needed. I'm not sure if that's relevant, but why not.
  • Add an update_bonus_links() helper that does bonus_links updating in the database instead of on the instance, and then marks the instance's bonus_links value as outdated. This means in principle you can do lots of these in parallel without needing a lock on the user -- with the exception that you could give someone an extra link or two if they came in at just the right time.
  • Be a notch more selective in when to update bonus_links in Folder.save() -- I'm not sure how often this matters.
  • For clarity, don't let bonus_links be null.

Sketch:

  • update_bonus_links is mostly AI implemented, I didn't test, but I think it's about right.
  • I didn't remove many saves or select_for_updates here -- not sure yet if this approach itself helps, or just lets us potentially remove some locks elsewhere, or misses the problem.

@jcushman
Copy link
Contributor Author

jcushman commented Nov 1, 2024

Be a notch more selective in when to update bonus_links in Folder.save() -- I'm not sure how often this matters.

Oh, I suspect this would have made the difference here! The deadlock is on this line:

image

And the deadlocked call was "UPDATE "perma_linkuser" SET "bonus_links" = NULL WHERE "perma_linkuser"."id" = %s", which suggests bonus_links weren't in play. So moving the save() up into the if block above would have prevented it entirely.


I think the other important question is, is this select_for_update still doing anything?

image

If that was just to protect the bonus_links count I think we can lose it, but maybe it's for something else too, in which case we could make it more specific.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant