Support u64 version parts in SQL function semver_no_prerelease #1846
Conversation
|
r? @jtgeibel (rust_highfive has picked a reviewer for you, use r? to override) |
carols10cents
force-pushed
the
integer-sizes-lolsob
branch
2 times, most recently
from
509b1d3 to
27e9619
Compare
|
Do you have some perf numbers on how this affects the query on a large crate like |
|
The changes in this PR add 20ms to the query to get serde's reverse dependencies; Query used for analysis (reverse dependencies of serde on a recent production snapshot locally)SELECT *, COUNT(*) OVER () as total FROM (
SELECT DISTINCT ON (crate_downloads, crate_name)
dependencies.*,
crates.downloads AS crate_downloads,
crates.name AS crate_name
FROM dependencies
INNER JOIN (
SELECT versions.*,
row_number() OVER (
PARTITION BY crate_id
ORDER BY to_semver_no_prerelease(num) DESC NULLS LAST
) rn
FROM versions
WHERE NOT yanked
AND crate_id = ANY(
SELECT versions.crate_id
FROM versions
INNER JOIN dependencies
ON dependencies.version_id = versions.id
WHERE dependencies.crate_id = 463
)
) versions
ON versions.id = dependencies.version_id
INNER JOIN crates
ON crates.id = versions.crate_id
WHERE dependencies.crate_id = 463
AND rn = 1
ORDER BY crate_downloads DESC
) t
OFFSET 0
LIMIT 10
;EXPLAIN ANALYZE output before this change (Execution Time: 720.728 ms)Limit (cost=141754.79..141755.09 rows=10 width=90) (actual time=714.269..714.280 rows=10 loops=1) EXPLAIN ANALYZE output after this change (Execution Time: 739.230 ms) |
|
This looks good to me. @bors r+ |
|
📌 Commit 5b42015 has been approved by |
Support u64 version parts in SQL function semver_no_prerelease Fixes #1839. So the reverse dependencies API request for crc32fast started failing because the susu crate depends on crc32fast and has [published versions](https://crates.io/crates/susu/versions) like 0.1.20190509190436. In the logs for the request that were responding with 500, I saw a message that said `value "20190409163015" is out of range for type integer` 😰 So I went digging, and after isolating which query was the problem and then which part of the query was the problem, I figured out it was [this call to `to_semver_no_prerelease`](https://github.com/rust-lang/crates.io/blob/12c64bc716a4a7d1fd25f2a658f80cbdcbdcbf08/src/models/krate_reverse_dependencies.sql#L15). That function was defined in [this migration](https://github.com/rust-lang/crates.io/blob/a27c704faa2982ddd75a3dc564da85c0217b950e/migrations/20170308140537_create_to_semver_no_prerelease/up.sql), which defines a type `semver_triple` as PostgreSQL type `int4`, which corresponds to Rust type `i32`. And 20,190,409,163,015 > 2,147,483,647 😰 The SemVer spec [doesn't actually specify](https://semver.org/#does-semver-have-a-size-limit-on-the-version-string) the size of integers that should be used for the version parts. The `semver` crate, that we use in the Rust structures, [uses `u64`](https://github.com/steveklabnik/semver/blob/15189a1d97911abc7e22fb2e7959df12d5d6f462/src/version.rs#L115-L121). Cargo uses the `semver` crate as well, and `cargo build` doesn't work if you give the current crate a version number with one part set to the value of `std::u64::MAX` + 1. [PostgreSQL's integer types](https://www.postgresql.org/docs/9.1/datatype-numeric.html) include an `int8` type, but it's a signed integer, so that wouldn't support values between `i64::MAX` and `u64::MAX` that we need to support. So we have to use the `numeric` type. [Here's a travis build showing the test failing](https://travis-ci.com/rust-lang/crates.io/jobs/237375256#L833)
|
☀️ Test successful - checks-travis |
|
I'll deploy this in the morning
…On Wed, Oct 2, 2019 at 5:36 PM bors ***@***.***> wrote:
Merged #1846 <#1846> into
master.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1846?email_source=notifications&email_token=AALVMKY4TL3BDP3MNXETKHTQMUV6ZA5CNFSM4IYPVOPKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOT7QYUTQ#event-2682358350>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALVMKZYF47S2QGCS2B5ZQ3QMUV6ZANCNFSM4IYPVOPA>
.
--
Thanks,
Sean Griffin
|
Fixes #1839.
So the reverse dependencies API request for crc32fast started failing because the susu crate depends on crc32fast and has published versions like 0.1.20190509190436.
In the logs for the request that were responding with 500, I saw a message that said
value "20190409163015" is out of range for type integer😰So I went digging, and after isolating which query was the problem and then which part of the query was the problem, I figured out it was this call to
to_semver_no_prerelease.That function was defined in this migration, which defines a type
semver_tripleas PostgreSQL typeint4, which corresponds to Rust typei32. And 20,190,409,163,015 > 2,147,483,647 😰The SemVer spec doesn't actually specify the size of integers that should be used for the version parts.
The
semvercrate, that we use in the Rust structures, usesu64. Cargo uses thesemvercrate as well, andcargo builddoesn't work if you give the current crate a version number with one part set to the value ofstd::u64::MAX+ 1.PostgreSQL's integer types include an
int8type, but it's a signed integer, so that wouldn't support values betweeni64::MAXandu64::MAXthat we need to support. So we have to use thenumerictype.Here's a travis build showing the test failing