Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base #3268

tiegz · 2023-12-01T22:50:24Z

these changes take a cold update of npm/gatsby from 67 sec to 34 sec for me locally (down from 83 min originally, before #3267)

replace VersionUpdater with BulkVersionUpdater, which does all the version upserts in one query (accompanied by manual callbacks)
remove the uniqueness validation on Version for [:number, :project_id] because we have a unique index on those columns

app/models/version.rb

tiegz · 2023-12-02T17:43:57Z

app/models/package_manager/base/bulk_version_updater.rb

+              THEN versions.repository_sources
+            ELSE
+              (COALESCE(versions.repository_sources, '[]'::jsonb) || COALESCE(EXCLUDED.repository_sources, '[]'::jsonb))
+            END)


this took a while to get right, but it should simulate the logic we had before of appending repository_sources. See the spec for a table test of examples

tiegz · 2023-12-02T17:47:08Z

app/models/package_manager/base/bulk_version_updater.rb

+          .first
+          &.tap(&:update_repository_async)
+          &.tap(&:update_project_tags_async)
+        @db_project.update_column(:versions_count, @db_project.versions.count) # normally counter_culture does this


I'm tempted to wrap everything in here in a transaction, but we might want to wait and see how the upsert_all does in production.

If we do that, note that there's a new transaction-aware Sidekiq feature that we could use to defer the job-enqueuing here until after the transaction runs.

Since this is now a place where we need to keep potential new hooks in sync with this code, is there a way to limit the amount of knowledge about the Version class we have in this code? Can those hook calls be encapsulated elsewhere, closer to Version?

I've put a note in the Version class here: https://github.com/librariesio/libraries.io/pull/3268/files#diff-61977d248b654178763e77a2fddc09152a20b85b1aecfc8222ef4233564593fcR48

But I agree, it'd be nice to make it more explicit. I'll look into a bulk callback thingy

tiegz · 2023-12-02T21:17:58Z

app/models/package_manager/base/bulk_version_updater.rb

+          # handles merging any existing repository_sources with new repository_source (see specs for table tests)
+          # note that timestamps act slightly differently: we'll use the provided updated_at here if it exists, which 
+          # might update updated_at even if no attributes changed. Whereas normally with AR's dirty attributes, timestamps
+          # wouldn't change unless other attributes had changed during the save.


this is really the only big data change: before, we wouldn't update a Version if its attributes hadn't changed. But now, since we're passing all the Version attributes into one query, we'll save the updated_at if it's passed here (which it will be).

(this could be averted with a lot of SQL below, but I don't think it's worth it personally)

This wouldn't trigger extra Release changes when we sync, right?

nah, for releases that already exist, the on_duplicate logic below dictates how we update any info:

status: pick new over old

runtime_dependencies_count: pick new over old

original_license: pick new over old

published_at: pick new over old

created_at: use old, and if it isn't set then use new

updated_at: use new, and if it isn't set then use old

repository_sources: merge new into old, without duplicates or nulls

Updating the updated_at if nothing's changed doesn't seem great, esp since it's used for debugging data issues and is not standard object behavior :/ I would be expecting something like the "last checked at" timestamp to be bumped, but not the updated_at.

I'll see what I can do! Might not be too bad to preserve dirty state

johnbintz-tidelift · 2023-12-15T15:16:00Z

app/models/package_manager/maven/google.rb

        end

+        pp "added versions #{api_versions.map(&:version_number).join(', ')}"


before I remove this, do you remember if there was a reason for it? 349b6fb#diff-b3d7088ca715b858cc9b988967128f4f0fb6a117c0633b2e0bc7fb4926bda26fR108

Oh, right, this was for debugging since this is run from a Rake task. This can stay.

app/models/package_manager/maven/google.rb

johnbintz-tidelift · 2023-12-15T15:23:03Z

app/models/package_manager/base/bulk_version_updater.rb

+          .first
+          &.tap(&:update_repository_async)
+          &.tap(&:update_project_tags_async)
+        @db_project.update_column(:versions_count, @db_project.versions.count) # normally counter_culture does this


Since this is now a place where we need to keep potential new hooks in sync with this code, is there a way to limit the amount of knowledge about the Version class we have in this code? Can those hook calls be encapsulated elsewhere, closer to Version?

tiegz · 2023-12-18T19:19:18Z

app/models/package_manager/base/bulk_version_updater.rb

+        newly_inserted_versions = @db_project.versions.where.not(id: existing_version_ids)
+
+        # run callbacks manually since upsert_all doesn't run callbacks.
+        Version.bulk_after_create_commit(newly_inserted_versions, @db_project)


cc @johnbintz-tidelift

What happens if the callbacks fail for version #40 out of the 100 to be updated? Do we need to roll things back? Are the other 60 going to lose their callbacks forever?

In that case the PackageManagerDownloadWorker job would retry, and it would re-upsert all the versions and run their callbacks, which is fine because the job should be idempotent.. The callbacks are basically just hitting redis (to enqueue Sidekiq jobs), so if that's happening constantly we'll have a bigger problem on our hands.

tiegz · 2023-12-18T19:19:57Z

app/models/version.rb

  validates :status, inclusion: { in: STATUSES, allow_blank: true }

  belongs_to :project
  counter_culture :project
  has_many :dependencies, dependent: :delete_all
  has_many :runtime_dependencies, -> { where kind: %w[runtime normal] }, class_name: "Dependency"

+  ##############################################################################################
+  # NB: callbacks are run manually in bulk_after_create_commit, so keep that class in sync with these.
+  ##############################################################################################


I made this comment a little more prominent, cc @johnbintz-tidelift

johnbintz-tidelift · 2023-12-20T19:02:38Z

app/models/version.rb

+
+    # normally counter_culture does this
+    project.update_column(:versions_count, project.versions.count)
+    @db_project


Is this return value needed? If so, it's the wrong return value:

Suggested change

@db_project

project

…hange.

jhan217 · 2024-01-31T19:20:39Z

app/models/package_manager/base/bulk_version_updater.rb

+
+        # create synthetic versions without saving them, so we can get their attributes
+        attrs = @api_versions
+          .map { |api_version| Version.new(api_version.to_version_model_attributes.merge(project: @db_project)) }


Thoughts about doing a bulk find_or_create on the versions here instead? One concern is that while the synthetic objects might be valid when constructed here from scratch, but they might not be valid when you do the upsert due to existing state.

For example, if an existing row has an invalid status, I think the from-scratch version would pass validation here (no changes to status), but after the upsert it would still have an invalid status, so you'd end up with a Version that doesn't pass validation (if the intention here is to only save valid Version objects).

the intention here is just to make sure we run validations and callbacks on each Version beforehand, as if they were inserted individually. There's some logic on line 43 to prefer the new data if possible (status = EXCLUDED.status,) so that'll squash the invalid status case.

jhan217 · 2024-01-31T19:28:43Z

app/models/package_manager/base/bulk_version_updater.rb

+            # from Version#before_save
+            v.update_spdx_expression
+            # upsert_all doesn't do validation, so ensure they're valid here.
+            v.validate!


What happens if there are two identical versions for the same project id in @api_versions? +/- differences in data? Or if two of these were running for the same project at the same time?

that should raise a unique index error in Bugsnag, which'll be desirable because we'll want to know what kind of weird data we're getting (I personally haven't ever seen a duplicate version coming back from an API, FWIW).

jhan217 · 2024-01-31T19:31:12Z

app/models/package_manager/base/bulk_version_updater.rb

+        newly_inserted_versions = @db_project.versions.where.not(id: existing_version_ids)
+
+        # run callbacks manually since upsert_all doesn't run callbacks.
+        Version.bulk_after_create_commit(newly_inserted_versions, @db_project)


What happens if the callbacks fail for version #40 out of the 100 to be updated? Do we need to roll things back? Are the other 60 going to lose their callbacks forever?

jhan217 · 2024-01-31T19:37:26Z

app/models/package_manager/base/bulk_version_updater.rb

+          # handles merging any existing repository_sources with new repository_source (see specs for table tests)
+          # note that timestamps act slightly differently: we'll use the provided updated_at here if it exists, which 
+          # might update updated_at even if no attributes changed. Whereas normally with AR's dirty attributes, timestamps
+          # wouldn't change unless other attributes had changed during the save.


Updating the updated_at if nothing's changed doesn't seem great, esp since it's used for debugging data issues and is not standard object behavior :/ I would be expecting something like the "last checked at" timestamp to be bumped, but not the updated_at.

mpace965 · 2024-01-31T20:41:42Z

app/models/package_manager/base.rb

+          repository_source_name: self::HAS_MULTIPLE_REPO_SOURCES ? [self::REPOSITORY_SOURCE_NAME] : nil
+        ).run!
+
+        remove_missing_versions(db_project, api_versions.map(&:version_number)) if sync_version == :all


Should this use the version numbers from the existing and newly inserted Versions from BulkVersionUpdater?

I think this would potentially allow for extra API versions to be kept compared to what we actually found/inserted. Not sure how common that would be in practice, or if that matters

that's a great idea, altho could take a bit of code surgery. I'm tempted to introduce the least changes possible in this PR, so I'll just add a note about that for later.

johnbintz-tidelift · 2024-04-01T17:30:59Z

Is this still needed?

tiegz · 2024-04-01T17:37:09Z

I've not had time to figure out how to keep the timestamps updated properly. I'm closing it for now, but if we see performance issues we can always return to it. RIP

tiegz commented Dec 2, 2023

View reviewed changes

tiegz changed the title ~~Fix Version read/write N+1s in PackageManager::Base~~ Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base Dec 2, 2023

tiegz marked this pull request as ready for review December 2, 2023 18:19

tiegz requested review from johnbintz-tidelift, macowie, jhan217, kjmahalingam and tradiff December 2, 2023 18:38

tiegz commented Dec 2, 2023

View reviewed changes

tiegz force-pushed the tz/fix-version-n-plus-ones-in-package-manager-base branch from 8da5988 to d1faffa Compare December 2, 2023 21:21

tiegz mentioned this pull request Dec 14, 2023

Disable single version updates for NPM #3278

Merged

johnbintz-tidelift reviewed Dec 15, 2023

View reviewed changes

tiegz commented Dec 18, 2023

View reviewed changes

tiegz requested review from johnbintz-tidelift and havocp December 18, 2023 19:20

johnbintz-tidelift reviewed Dec 20, 2023

View reviewed changes

tiegz added 3 commits January 30, 2024 07:14

Replace VersionUpdater with a bulk upsert_all to avoid N+1s.

b64759d

We can remove Version uniq validation bc a uniq index exists.

c77b558

Insert callbacks that would have been called on each version.

594b28e

tiegz force-pushed the tz/fix-version-n-plus-ones-in-package-manager-base branch from feef05d to fa73f07 Compare January 30, 2024 14:31

tiegz added 3 commits January 30, 2024 11:28

Extract upsert_all code into a BulkVersionUpdater and start specs.

f632e26

Ensure we update proper attributes

cb6e7dd

Note that we are touching updated_at on records even if they didnt' c…

6250006

…hange.

tiegz force-pushed the tz/fix-version-n-plus-ones-in-package-manager-base branch from fa73f07 to cb86ee2 Compare January 30, 2024 18:29

Google updater fixes after rebase from main.

5c13a48

tiegz force-pushed the tz/fix-version-n-plus-ones-in-package-manager-base branch from cb86ee2 to 5c13a48 Compare January 30, 2024 18:51

tiegz requested a review from johnbintz-tidelift January 30, 2024 19:15

jhan217 reviewed Jan 31, 2024

View reviewed changes

mpace965 reviewed Jan 31, 2024

View reviewed changes

tiegz added 2 commits February 1, 2024 12:49

Add a note about a possible optimization.

db1d872

Better name for update_spdx_expression = set_spdx_expression.

8eea0c3

tiegz closed this Apr 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base #3268

Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base #3268

tiegz commented Dec 1, 2023 •

edited

tiegz Dec 2, 2023

tiegz Dec 2, 2023

johnbintz-tidelift Dec 15, 2023

tiegz Dec 18, 2023

tiegz Dec 2, 2023 •

edited

macowie Dec 4, 2023

tiegz Dec 4, 2023

jhan217 Jan 31, 2024

tiegz Feb 1, 2024

johnbintz-tidelift Dec 15, 2023

tiegz Dec 20, 2023

johnbintz-tidelift Feb 13, 2024

johnbintz-tidelift Dec 15, 2023

tiegz Dec 18, 2023

jhan217 Jan 31, 2024

tiegz Feb 1, 2024

tiegz Dec 18, 2023

johnbintz-tidelift Dec 20, 2023

jhan217 Jan 31, 2024

tiegz Feb 1, 2024

jhan217 Jan 31, 2024

tiegz Feb 1, 2024

jhan217 Jan 31, 2024

jhan217 Jan 31, 2024

mpace965 Jan 31, 2024

tiegz Feb 1, 2024

johnbintz-tidelift commented Apr 1, 2024

tiegz commented Apr 1, 2024

		end

		pp "added versions #{api_versions.map(&:version_number).join(', ')}"

Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base #3268

Introduces BulkVersionUpdater to fix Version read/write N+1s in PackageManager::Base #3268

Conversation

tiegz commented Dec 1, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiegz Dec 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnbintz-tidelift commented Apr 1, 2024

tiegz commented Apr 1, 2024

tiegz commented Dec 1, 2023 •

edited

tiegz Dec 2, 2023 •

edited