database/crates: Add version columns #4106

Turbo87 · 2021-10-29T15:37:35Z

A crate on our JSON API has the fields max_version, newest_version, and max_stable_version. These fields are currently calculated dynamically at the time of each request, and require us to load the list of all crate versions from the database.

The reverse dependencies query does something roughly like SELECT * FROM versions ORDER BY to_semver_no_prerelease(num) LIMIT 1, which ends up being essentially the same as max_stable_version, but this subquery is costing us quite a bit of performance.

This PR proposes to add three new columns to the crates table, one for each of these versions above. Instead of calculating the values on every request we will recalculate them whenever the list of versions changes. This is currently limited to three operations:

publishing a new version
yanking an existing version
unyanking an existing version

This PR also adds a fill-top-versions admin command that iterates through all the crates in the database and fills the columns with the correct values.

Since backfilling of the data is a requirement to adjust the API endpoints to the new columns, this part is not yet included in this PR.

database/versions: Add new semver_no_prerelease column #4022 (This PR would likely be obsolete if we move forward with the version columns here)

pietroalbini

This is also missing updating the latest version on crate deletion.

pietroalbini · 2021-10-30T12:46:23Z

migrations/2021-10-28-135931_top-version-columns/up.sql

+ALTER TABLE crates ADD COLUMN highest_version VARCHAR;
+ALTER TABLE crates ADD COLUMN highest_stable_version VARCHAR;


Why is this a VARCHAR? Aren't we storing the versions.id?

we could also store the id, but the API returns the version number, not the ID, so we'd need another SELECT num FROM versions WHERE id = xxx query for the endpoints. it seemed easier to store the num directly and use that as a reference to the versions table.

I think it'd be better to store the ID for consistency.

I'm not sure why that would be more consistent, to be honest. we already have a uniqueness constraint on crate_id, num in the versions table, and as mentioned on Discord we can add a foreign key constraint to these new columns too, which would ensure that the referenced versions actually exist.

We're already using version IDs as foreign keys in all the tables... I don't think an extra indexed query in the frontend is going to have much of an impact on performance.

pietroalbini · 2021-10-30T12:47:24Z

migrations/2021-10-28-135931_top-version-columns/up.sql

@@ -0,0 +1,7 @@
+ALTER TABLE crates ADD COLUMN highest_version VARCHAR;
+ALTER TABLE crates ADD COLUMN highest_stable_version VARCHAR;
+ALTER TABLE crates ADD COLUMN newest_version VARCHAR;


Is there a reason why we're storing this? It should be possible to easily determine it with a query (while you can't do that with semver-related columns).

basically just to avoid the unnecessary versions queries for the endpoints that return a crate. it's currently somewhat tightly coupled to the highest version fields through our use of the TopVersions struct.

pietroalbini · 2021-10-30T12:57:47Z

src/admin/fill_top_versions.rs

+            diesel::sql_query("ALTER TABLE crates DISABLE TRIGGER trigger_crates_set_updated_at")
+                .execute(&conn)
+                .context("Failed to disable the `set_updated_at` trigger")?;


Hmm, is this disabling the trigger globally or just for this transaction?

that is a good question. I was assuming that it would only disable it for the transaction, but I'm not sure. can't find anything in the docs about this.

pietroalbini · 2021-10-30T13:02:12Z

src/admin/fill_top_versions.rs

+        let crates: Vec<Crate> = Crate::all()
+            .order(crates::id.asc())
+            .limit(page_size)
+            .offset(page_size * (page - 1))
+            .load(&conn)
+            .context("Failed to load crates")?;


It's probably easier and more performant here to do offset-pagination (WHERE id > last_id), but it shouldn't matter that much.

yeah, guess that could work too. it's running pretty fast already though, so probably not needed with our current number of crates.

pietroalbini · 2021-10-30T13:02:37Z

src/admin/fill_top_versions.rs

+                krate
+                    .update_top_versions(&conn, &top_versions)
+                    .context("Failed to update top versions in the database")?;


Can we change this to perform the UPDATE only if the update is actually needed?

yeah, I guess that should be possible

bors · 2021-10-30T17:25:15Z

☔ The latest upstream changes (presumably #4108) made this pull request unmergeable. Please resolve the merge conflicts.

…version is published

… unyanked

…sion is created

pietroalbini · 2021-11-07T15:22:08Z

Was that just a rebase or was feedback addressed @Turbo87?

Turbo87 · 2021-11-07T15:33:06Z

just rebases so far

bors · 2021-11-13T15:58:18Z

☔ The latest upstream changes (presumably #4120) made this pull request unmergeable. Please resolve the merge conflicts.

Turbo87 · 2024-04-18T09:59:11Z

closing in favor of #8484

Turbo87 requested review from pietroalbini, JohnTitor and jtgeibel October 29, 2021 15:37

Turbo87 added A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear labels Oct 29, 2021

Turbo87 mentioned this pull request Oct 29, 2021

database/versions: Add new semver_no_prerelease column #4022

Closed

pietroalbini requested changes Oct 30, 2021

View reviewed changes

Turbo87 force-pushed the top-versions branch 3 times, most recently from 8d5c1d9 to 4c716f4 Compare November 2, 2021 21:14

Turbo87 added 6 commits November 3, 2021 09:16

database/crates: Add version columns

60ee8bc

models::Crate: Implement update_top_versions() method

6f121c0

controllers::krate::publish: Call update_top_versions() when a new …

511ef9c

…version is published

worker::git: Call update_top_versions() when a version is yanked or…

51505fc

… unyanked

tests::builders::version: Call update_top_versions() when a new ver…

1993b1f

…sion is created

admin: Implement fill-top-versions command

dd39365

Turbo87 force-pushed the top-versions branch from 4c716f4 to dd39365 Compare November 3, 2021 08:16

Turbo87 marked this pull request as draft November 16, 2021 08:44

Turbo87 closed this Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

database/crates: Add version columns #4106

database/crates: Add version columns #4106

Turbo87 commented Oct 29, 2021

pietroalbini left a comment

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

pietroalbini Nov 7, 2021

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

pietroalbini Oct 30, 2021

Turbo87 Oct 30, 2021

bors commented Oct 30, 2021

pietroalbini commented Nov 7, 2021

Turbo87 commented Nov 7, 2021

bors commented Nov 13, 2021

Turbo87 commented Apr 18, 2024

		ALTER TABLE crates ADD COLUMN highest_version VARCHAR;
		ALTER TABLE crates ADD COLUMN highest_stable_version VARCHAR;

database/crates: Add version columns #4106

database/crates: Add version columns #4106

Conversation

Turbo87 commented Oct 29, 2021

pietroalbini left a 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

bors commented Oct 30, 2021

pietroalbini commented Nov 7, 2021

Turbo87 commented Nov 7, 2021

bors commented Nov 13, 2021

Turbo87 commented Apr 18, 2024