-
Notifications
You must be signed in to change notification settings - Fork 621
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
Remove the max_version
column from Crates
#592
Remove the max_version
column from Crates
#592
Conversation
8c4276c
to
6ee457e
Compare
@alexcrichton I found this commit that adds the max_version column, and this commit that adds the tracking of the max_version, but I don't see any explanation in there of why this was needed.... were there performance problems? complex queries? |
@carols10cents I think it was a "too clever by half" attempt to make these operations fast:
crates.io has always operated blazingly fast though so I don't think we really need to be optimizing sql queries :) Looks great to me! |
src/bin/delete-crate.rs
Outdated
@@ -7,7 +7,6 @@ | |||
|
|||
#![deny(warnings)] | |||
|
|||
#[macro_use] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It literally doesn't compile if I don't remove it!
Ok, this looks good, and I'm all for it. The irreversible migration makes me nervous though, and I know this is basically just a cached column and we're not really losing any data blah blah blah but it would make me feel better if I deployed the query change without the migration at first to just make sure. And I want to cherry-pick the tests from #582 and run them with this code. And then deploy when I'm awake and alert and can watch the stats and be ready to roll back, so tomorrow morning perhaps. I know I'm probably being overdramatic, I miss the adrenaline of running a Big Production App ok??? |
So should I remove the migration? Or do you want me to leave it in? |
6ee457e
to
6b3d53d
Compare
I've rebased at least so the |
Oh it's no problem for me to split it up... which I'll have to do again since I did it right before you rebased ;) ;) ;) <3 |
Lol sorry. I was going to cherry-pick the tests for you, too. |
Don't you have a 👶 to take care of or something? :) <3 |
She's sleeping. :P |
Actually, ok, so I think the commits that add tests we should keep from #582 are: Then there's this commit: 575829e That PR changes max_version to be an |
I chose 0.0.0 because it was the default value in the database. We could certainly change it to be I pulled in the tests from that commit (I can't pull that commit directly, but I credited @yodaldevoid as the author in the commit which pulled the tests) |
Yeah ok so with your change, this is what happens on a crate page when all versions have been yanked: #582 doesn't fix #502, but at least it doesn't have an error because of 575829e. With 575829e and another tweak that I just pushed, your change doesn't show the flash error either. Wdyt? Both PRs fix #510. |
oops hm not sure where I pushed it but it's not here |
Sorry that was probably me. I think I got travis green at least. I went ahead and wrote an almost correct down migration as well since that was a concern. |
there we go. no i pushed it to a new branch instead of to your repo. |
Yeah, your fix looks good. I think I might have to futz with the line endings of that file one more time but we'll see what travis says. |
Ok i'm going to try deploying this to staging while we're waiting on travis |
Ok I have no clue how to get that file in a format that travis likes. Let me see if I can cherry pick it from that branch and not have git "fix" the line endings |
lol oops merge conflict |
70df378
to
217c019
Compare
Updating columns like this tends to be pretty brittle unless it's done via a trigger. We can't update this one from a trigger, since PG doesn't know about semver's comparison rules. Either way, we don't really need to cache this unless it turns out to be a performance bottleneck. We can instead calculate the value on the fly. This does introduce some N+1 queries, and those may end up being a hotspot. We can definitely get rid of them, but it's a pain in the ass to do manually and Diesel has support for it built in, so I'd rather just fix them as we move those endpoints over.
This version of the down function won't necessariliy 100% replicate the data that was dropped, but it'll get pretty darn close and probably close enough for our purposes if we end up actually having to roll this back.
217c019
to
f44f544
Compare
Ok rebased, squashed the line ending commits, and cherry picked the files in exactly the form they were in from #582 so let's see if travis is happy now |
Now I must feed 👶 |
uhhhhh soooooo the deploy to staging went ok, the 95th percentile response time is a bit higher than prod but still way lower than the median of most rails apps, buuuut i tried out the down migration and it is not "pretty darn close".... it picks the first version number it sees out of all crates and sets all crates' max versions to that... look i'm just going to take out the migration and we'll do it in a separate deploy |
Oh I forgot to make the order |
uhhh and join crates to versions on crate.id |
Oh yeah that too |
WE'VE GOT GREEN GO GO GO |
So in rust-lang#592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert rust-lang#592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes rust-lang#602.
So in rust-lang#592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert rust-lang#592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes rust-lang#602.
So in rust-lang#592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert rust-lang#592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes rust-lang#602.
So in rust-lang#592, I accidentally changed the meaning of the `reverse_dependencies` query from "select 10 crates where the max version of that crate depends on this one" to "select 10 crates where that crate had a version with the same number as this one's max version, which depended on this crate" which is nonsense and wrong. There's actually no way that we can truly replicate the old behavior without breaking pagination or loading all the versions of every crate which has ever depended on another into memory and doing the limiting locally... which would defeat the purpose of pagination. The only real alternative here is to revert rust-lang#592 and go back to updating the `max_version` column. I still think this approach is the right one, even with the added complexity to this column, as updating `max_version` will always be bug-prone unless we can do it in SQL itself. It's too bad we can't enable arbitrary extensions on heroku PG, as we could actually create a true semver type that links to the rust crate if we could. The main difference in behavior between this and the `max_version` column is that if a crate had *only* prerelease versions, and only some of those prerelease versions depended on another crate, it's effectively random whether it would appear in this list or not. It's a very niche edge case and one that I'm not terribly concerned about. I'd need to play around with indexes to see if this query could be optimized further, but the performance should be reasonable, as the window function will only require a single table scan. Fixes rust-lang#602.
…-ember-7.7.2, r=Turbo87 Bump eslint-plugin-ember from 7.0.0 to 7.7.2 Bumps [eslint-plugin-ember](https://github.com/ember-cli/eslint-plugin-ember) from 7.0.0 to 7.7.2. <details> <summary>Release notes</summary> *Sourced from [eslint-plugin-ember's releases](https://github.com/ember-cli/eslint-plugin-ember/releases).* > ## v7.7.2 > #### 🐛 Bug Fix > * [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/621) Fix false positive with `ignoreNonThisExpressions` option in `use-ember-get-and-set` rule ([@​Exelord](https://github.com/Exelord)) > > #### 📝 Documentation > * [#620](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/620) Use consistent prefixes for rule descriptions ([@​bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/625) Add eslint-plugin-jest internally and enable rules ([@​bmish](https://github.com/bmish)) > * [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/624) Add eslint-plugin-unicorn internally and enable recommended rules ([@​bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@​bmish](https://github.com/bmish)) > - Maciej Kwaśniak ([@​Exelord](https://github.com/Exelord)) > > ## v7.7.1 > #### 🐛 Bug Fix > * [#615](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/615) Fix issue causing assert to fire in `getSourceModuleName` util function ([@​patocallaghan](https://github.com/patocallaghan)) > > #### Committers: 1 > - Pat O'Callaghan ([@​patocallaghan](https://github.com/patocallaghan)) > > ## v7.7.0 > #### 🚀 Enhancement > * [#592](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/592) Update `no-classic-classes` rule to catch classic Ember Data model classes ([@​patocallaghan](https://github.com/patocallaghan)) > > #### 🐛 Bug Fix > * [#610](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/610) Fix invalid `no-get` rule autofix caused by invalid JS variable name ([@​bmish](https://github.com/bmish)) > * [#607](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/607) Fix spread property bug in `require-super-in-init` rule ([@​bmish](https://github.com/bmish)) > * [#600](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/600) Add missing schema validation for options on many rules ([@​bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#611](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/611) Add many missing tests for lines without test coverage ([@​bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@​bmish](https://github.com/bmish)) > - Pat O'Callaghan ([@​patocallaghan](https://github.com/patocallaghan)) > > ## v7.6.0 > #### 🚀 Enhancement > * [#594](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/594) Add new rule `no-get-with-default` ([@​steventsao](https://github.com/steventsao)) > > #### Committers: 1 > - Steven Tsao ([@​steventsao](https://github.com/steventsao)) > > ## v7.5.0 > #### 🚀 Enhancement > * [#583](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/583) Update `no-observers` rule to handle decorators ([@​bmish](https://github.com/bmish)) > * [#577](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/577) Add autofixer to `no-get` rule ([@​bmish](https://github.com/bmish)) > ></tr></table> ... (truncated) </details> <details> <summary>Changelog</summary> *Sourced from [eslint-plugin-ember's changelog](https://github.com/ember-cli/eslint-plugin-ember/blob/master/CHANGELOG.md).* > ## v7.7.2 (2019-12-12) > > #### 🐛 Bug Fix > * [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/621) Fix false positive with `ignoreNonThisExpressions` option in `use-ember-get-and-set` rule ([@​Exelord](https://github.com/Exelord)) > > #### 📝 Documentation > * [#620](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/620) Use consistent prefixes for rule descriptions ([@​bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/625) Add eslint-plugin-jest internally and enable rules ([@​bmish](https://github.com/bmish)) > * [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/624) Add eslint-plugin-unicorn internally and enable recommended rules ([@​bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@​bmish](https://github.com/bmish)) > - Maciej Kwaśniak ([@​Exelord](https://github.com/Exelord)) > > ## v7.7.1 (2019-11-29) > > #### 🐛 Bug Fix > * [#615](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/615) Fix issue causing assert to fire in `getSourceModuleName` util function ([@​patocallaghan](https://github.com/patocallaghan)) > > #### Committers: 1 > - Pat O'Callaghan ([@​patocallaghan](https://github.com/patocallaghan)) > > ## v7.7.0 (2019-11-29) > > #### 🚀 Enhancement > * [#592](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/592) Update `no-classic-classes` rule to catch classic Ember Data model classes ([@​patocallaghan](https://github.com/patocallaghan)) > > #### 🐛 Bug Fix > * [#610](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/610) Fix invalid `no-get` rule autofix caused by invalid JS variable name ([@​bmish](https://github.com/bmish)) > * [#607](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/607) Fix spread property bug in `require-super-in-init` rule ([@​bmish](https://github.com/bmish)) > * [#600](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/600) Add missing schema validation for options on many rules ([@​bmish](https://github.com/bmish)) > > #### 🏠 Internal > * [#611](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/611) Add many missing tests for lines without test coverage ([@​bmish](https://github.com/bmish)) > > #### Committers: 2 > - Bryan Mishkin ([@​bmish](https://github.com/bmish)) > - Pat O'Callaghan ([@​patocallaghan](https://github.com/patocallaghan)) > > ## v7.6.0 (2019-11-19) > > #### 🚀 Enhancement > * [#594](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/pull/594) Add new rule `no-get-with-default` ([@​steventsao](https://github.com/steventsao)) > > #### Committers: 1 > - Steven Tsao ([@​steventsao](https://github.com/steventsao)) > > ## v7.5.0 (2019-11-11) ></tr></table> ... (truncated) </details> <details> <summary>Commits</summary> - [`1805883`](ember-cli/eslint-plugin-ember@1805883) v7.7.2 - [`39539bc`](ember-cli/eslint-plugin-ember@39539bc) Update CHANGELOG - [`9943b7e`](ember-cli/eslint-plugin-ember@9943b7e) Merge pull request [#625](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/625) from bmish/eslint-plugin-jest - [`cb4824d`](ember-cli/eslint-plugin-ember@cb4824d) chore(lint): add eslint-plugin-jest internally and enable rules - [`dc149b5`](ember-cli/eslint-plugin-ember@dc149b5) Merge pull request [#624](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/624) from bmish/eslint-plugin-unicorn - [`9a6a4be`](ember-cli/eslint-plugin-ember@9a6a4be) chore(lint): add eslint-plugin-unicorn internally and autofix recommended rules - [`c93198c`](ember-cli/eslint-plugin-ember@c93198c) build(deps-dev): bump eslint-plugin-import from 2.18.2 to 2.19.1 ([#623](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/623)) - [`e2c5235`](ember-cli/eslint-plugin-ember@e2c5235) build(deps): bump snake-case from 3.0.1 to 3.0.2 ([#622](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/622)) - [`4c6aedd`](ember-cli/eslint-plugin-ember@4c6aedd) Merge pull request [#621](https://github-redirect.dependabot.com/ember-cli/eslint-plugin-ember/issues/621) from Exelord/fix/ignore-this-expressions - [`ed6301a`](ember-cli/eslint-plugin-ember@ed6301a) chore: fix lint violation - Additional commits viewable in [compare view](ember-cli/eslint-plugin-ember@v7.0.0...v7.7.2) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=eslint-plugin-ember&package-manager=npm_and_yarn&previous-version=7.0.0&new-version=7.7.2)](https://dependabot.com/compatibility-score.html?dependency-name=eslint-plugin-ember&package-manager=npm_and_yarn&previous-version=7.0.0&new-version=7.7.2) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- **Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit. You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com). <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Automerge options (never/patch/minor, and dev/runtime dependencies) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details>
Updating columns like this tends to be pretty brittle unless it's done
via a trigger. We can't update this one from a trigger, since PG doesn't
know about semver's comparison rules. Either way, we don't really need
to cache this unless it turns out to be a performance bottleneck. We can
instead calculate the value on the fly.
This does introduce some N+1 queries, and those may end up being a
hotspot. We can definitely get rid of them, but it's a pain in the ass
to do manually and Diesel has support for it built in, so I'd rather
just fix them as we move those endpoints over.