Fix tag updates atomicity and deadlocks by MareStare · Pull Request #483 · philomena-dev/philomena

MareStare · 2025-03-30T19:42:40Z

Before you begin

I understand my contributions may be rejected for any reason
I understand my contributions are for the benefit of Derpibooru and/or the Philomena software
I understand my contributions are licensed under the GNU AGPLv3

I understand all of the above

I've noticed unique constraint violation errors in get_or_create_tags when working on #481. So I replaced the existing impl with two separate queries INSERT ON CONFLICT DO NOTHING and a subsequent SELECT.

But then I started seeing deadlocks in update_all queries for incrementing the image counts on tags. So I fixed it this way.

Deadlock demo diagram is below. This can only happen in bulk-update transactions.

The problem is that postgres doesn't guarantee the order of acquiring row-level locks during the update. So one transaction can acquire its part of the locks, and the other transaction may acquire another part of the locks, thus they both wait on each other's remaining part of the records to be released falling into a deadlock.

Tested it with the image seeding script in #483, that does ~50 image uploads concurrently.

lib/philomena/tags.ex

MareStare · 2025-03-30T19:51:27Z

lib/philomena/tags.ex

+          ok
+
+        {:error, err} ->
+          raise "get_or_create_tags failed: #{inspect(err)}\ntag_names: #{inspect(tag_names)}"


I don't like this error handling story, but this function never returned a Result in the first place, but it wasn't annotated with the ! suffix. In fact, it seems that all Repo.*() methods never return a Result. Is this expected? I suppose they use raise internally, but shouldn't they have a ! suffix?

MareStare · 2025-03-30T21:12:48Z

Found this error during seeding after recent change, trying to debug:

app-1  | [error] Task #PID<0.1927.0> started from #PID<0.1926.0> terminating
app-1  | ** (Ecto.NoResultsError) expected at least one result but got none in query:
app-1  | 
app-1  | from i0 in Philomena.Images.Image,
app-1  |   where: i0.id == ^30
app-1  | 
app-1  |     (ecto 3.12.5) lib/ecto/repo/queryable.ex:164: Ecto.Repo.Queryable.one!/3
app-1  |     (philomena 1.1.0) lib/philomena/images/thumbnailer.ex:77: Philomena.Images.Thumbnailer.generate_thumbnails/1
app-1  |     (philomena 1.1.0) lib/philomena/workers/thumbnail_worker.ex:6: Philomena.ThumbnailWorker.perform/1
app-1  |     (exq 0.19.0) lib/exq/worker/server.ex:175: anonymous fn/4 in Exq.Worker.Server.dispatch_work/3
app-1  |     (elixir 1.18.1) lib/task/supervised.ex:101: Task.Supervised.invoke_mfa/2
app-1  | Function: #Function<0.120145007/0 in Exq.Worker.Server.dispatch_work/3>
app-1  |     Args: []

MareStare · 2025-03-30T21:21:11Z

Hm, can't reproduce that error mode after docker compose down. I suppose I just had my DB messed up during intermediate iterations, where no images were uploaded. Reopening this again for review

…eturned as per docs

MareStare · 2025-05-02T18:52:36Z

@Meow I discussed this deadlock in PM with Liam. Note that postgres logs every deadlock occurrence in its sever logs like this:

postgres-1  | 2025-03-31 21:18:23.319 UTC [289] DETAIL:  Process 289 waits for ShareLock on transaction 149633; blocked by process 296.
postgres-1  |   Process 296 waits for ShareLock on transaction 149621; blocked by process 289.
postgres-1  |   Process 289: UPDATE "tags" AS t0 SET "images_count" = t0."images_count" + $1 WHERE (t0."id" IN (SELECT st0."id" FROM "tags" AS st0 WHERE (st0."id" = ANY($2)) ORDER BY st0."id" FOR UPDATE))
postgres-1  |   Process 296: UPDATE "tags" AS t0 SET "images_count" = t0."images_count" + $1 WHERE (t0."id" IN (SELECT st0."id" FROM "tags" AS st0 WHERE (st0."id" = ANY($2)) ORDER BY st0."id" FOR UPDATE))

Here, there is good enough context on which queries trapped into a deadlock so it becomes much easier to find the root cause. Postgres never actually hangs on a deadlock, it just detects them and cancels all participating transactions (there can be more than 2 transactions in the deadlock chain, and it happened for me during my testing of my dev seeds parallelization efforts), therefore it's technically not a critical error.

I recommend you to dump the logs from postgres in prod and grep them for ERROR: deadlock detected (not in log snippet higher, but it's also nearby it) to check if there are any real deadlocks happening in prod today.

Meow · 2025-05-12T08:20:42Z

lib/philomena/images.ex

-      tags = Tag |> where([t], t.id in ^tag_ids)

-      {count, nil} = repo.update_all(tags, inc: [images_count: 1])
+      count = Tags.update_images_counts(repo, +1, tag_ids)


can this not be written simply as 1 instead of +1?

Removed the +

Meow · 2025-05-12T08:36:29Z

lib/philomena/tags.ex

+  """
+  @spec update_images_counts(term(), integer(), [integer()]) :: integer()
+  def update_images_counts(repo, diff, tag_ids) do
+    case tag_ids do


could this case statement not be eliminated by using pattern matching in the function definition like def update_image_counts(repo, diff, tag_ids) when length(tag_ids) == 0, do: 0

i'd also rename the function to update_image_counts, instead of plural images

Meow · 2025-05-12T08:42:16Z

lib/philomena/tags.ex

+      |> Enum.sort()
+      |> Enum.dedup()
+      |> Enum.map(
+        &(%Tag{}


i'd not use this shorthand form here for clarity's sake, and instead would write Enum.map(fn tag_name -> ...

…ase` with a function-level match

Meow · 2025-05-12T08:44:46Z

lib/philomena/tags.ex

-        Tag
-        |> where([t], t.id in ^tag_ids)
-        |> Repo.update_all(inc: [images_count: 1])
+        update_images_counts(Repo, +1, tag_ids)


remove the + here as well

Done in 79c794c

Meow · 2025-05-12T08:51:57Z

lib/philomena/tags.ex


-      _ ->
-        locked_tags = vectorized_mutation_lock("FOR NO KEY UPDATE", tag_ids)
+  def update_image_counts(nil, _diff, []), do: 0


match repo to nil? what if repo isn't nil but tag_ids is []?

Right, I haven't noticed that nil. Copilot autocomplete betrayed me there 💀. FIxed

* Fix the tag updates atomicity and deadlocks * Replace `Map.drop()` with `Map.take()` as per Liam's feedback in DM * Fix error handling since there will be a tuple of more than 2 items returned as per docs * Replace `+1` with `1` for update_images_counts * Rename `update_images_counts` to `update_image_counts` and replace `case` with a function-level match * Replace the capture operator with an explicit `fn tag_name ->` * Fix remove repo nil match autocompleted by Github Copilot (bruh)

MareStare force-pushed the fix/get-or-create-tags-atomicity branch from a222e25 to a7acf06 Compare March 30, 2025 19:44

MareStare marked this pull request as draft March 30, 2025 19:56

MareStare force-pushed the fix/get-or-create-tags-atomicity branch from a7acf06 to ba2fcdc Compare March 30, 2025 20:13

Fix the tag updates atomicity and deadlocks

945b6a3

MareStare force-pushed the fix/get-or-create-tags-atomicity branch from ba2fcdc to 945b6a3 Compare March 30, 2025 20:37

MareStare marked this pull request as ready for review March 30, 2025 20:39

MareStare requested a review from liamwhite March 30, 2025 20:39

MareStare commented Mar 30, 2025

View reviewed changes

MareStare changed the title ~~Fix the tag updates atomicity and deadlocks~~ Fix tag updates atomicity and deadlocks Mar 30, 2025

Replace Map.drop() with Map.take() as per Liam's feedback in DM

e72f7db

MareStare force-pushed the fix/get-or-create-tags-atomicity branch from e120fae to e72f7db Compare March 30, 2025 21:06

MareStare marked this pull request as draft March 30, 2025 21:12

MareStare marked this pull request as ready for review March 30, 2025 21:21

Fix error handling since there will be a tuple of more than 2 items r…

a7f4105

…eturned as per docs

Meow force-pushed the master branch from de1ee3c to 08b31fa Compare April 27, 2025 08:53

Meow added this to the 1.2 milestone May 9, 2025

Meow reviewed May 12, 2025

View reviewed changes

Replace +1 with 1 for update_images_counts

79c794c

Meow reviewed May 12, 2025

View reviewed changes

Rename update_images_counts to update_image_counts and replace `c…

270825c

…ase` with a function-level match

Meow reviewed May 12, 2025

View reviewed changes

MareStare added 2 commits May 12, 2025 08:52

Replace the capture operator with an explicit fn tag_name ->

343b6e1

Fix remove repo nil match autocompleted by Github Copilot (bruh)

1a601a9

Meow merged commit 9331ad4 into philomena-dev:master May 12, 2025
6 checks passed

Uh oh!

Conversation

MareStare commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before you begin

Uh oh!

Uh oh!

MareStare Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MareStare commented Mar 30, 2025

Uh oh!

MareStare commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MareStare commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MareStare May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MareStare commented Mar 30, 2025 •

edited

Loading

MareStare Mar 30, 2025 •

edited

Loading

MareStare commented Mar 30, 2025 •

edited

Loading

MareStare commented May 2, 2025 •

edited

Loading

MareStare May 12, 2025 •

edited

Loading