Skip to content

Fix reverse_dependencies #604

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

Merged
merged 1 commit into from
Mar 8, 2017

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Mar 8, 2017

So in #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 #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 #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.
@alexcrichton
Copy link
Member

You are far more of a sql wizard than I, so sounds great to me :)

IIRC it was something like this why I was so hesitatnt to expose reverse deps, it was just hard to do in the db...

@alexcrichton alexcrichton merged commit 9e5ccf0 into rust-lang:master Mar 8, 2017
@sgrif sgrif deleted the sg-fix-reverse-dependencies branch March 10, 2017 12:46
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.

2 participants