Skip to content

SQL: Fix metadata handling in to_semver_no_prerelease() function #3886

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

Closed
wants to merge 3 commits into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Sep 3, 2021

Resolves #3882

@Turbo87 Turbo87 added C-bug 🐞 Category: unintended, undesired behavior A-backend ⚙️ labels Sep 3, 2021
@Turbo87 Turbo87 force-pushed the semver-sql-function branch from e0aaa6c to 0893b94 Compare September 3, 2021 14:30
@@ -88,8 +88,10 @@ tracing-subscriber = "0.2"
url = "2.1"

[dev-dependencies]
bigdecimal = { version = ">= 0.0.10, < 0.2.0" }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately diesel does not reexport the version that they depend on, so we have to add this here ourselves

claim = "0.5"
conduit-test = "0.9.0-alpha.4"
diesel = { version = "1.4.0", features = ["numeric"] }
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the numeric feature is necessary because the semver_triple returned by to_semver_no_prerelease() is based on the Numeric SQL type.

@Turbo87 Turbo87 force-pushed the semver-sql-function branch from 0893b94 to b812fe7 Compare September 3, 2021 14:44
@pietroalbini
Copy link
Member

While the changes here look correct, my main worry is the performance of the already slow reverse dependencies endpoint. Trying to run the old and the new function in a local database dump shows a 16% slowdown:

cratesio-prod=# explain (analyze, buffers) select num from versions;
                                                   QUERY PLAN                                                   
----------------------------------------------------------------------------------------------------------------
 Seq Scan on versions  (cost=0.00..9824.83 rows=367883 width=6) (actual time=0.053..51.207 rows=367883 loops=1)
   Buffers: shared hit=128 read=6018
 Planning time: 0.434 ms
 Execution time: 67.512 ms
(4 rows)

cratesio-prod=# explain (analyze, buffers) select to_semver_no_prerelease(num) from versions;
                                                     QUERY PLAN                                                      
---------------------------------------------------------------------------------------------------------------------
 Seq Scan on versions  (cost=0.00..101795.58 rows=367883 width=32) (actual time=0.241..1983.644 rows=367883 loops=1)
   Buffers: shared hit=166 read=5986
 Planning time: 0.419 ms
 Execution time: 2000.023 ms
(4 rows)

cratesio-prod=# explain (analyze, buffers) select to_semver_no_prerelease_new(num) from versions;
                                                     QUERY PLAN                                                      
---------------------------------------------------------------------------------------------------------------------
 Seq Scan on versions  (cost=0.00..101795.58 rows=367883 width=32) (actual time=0.247..2382.449 rows=367883 loops=1)
   Buffers: shared hit=192 read=5954
 Planning time: 0.198 ms
 Execution time: 2399.200 ms
(4 rows)

cratesio-prod=# 

I'd like to hear @jtgeibel's opinion here too.

@Turbo87
Copy link
Member Author

Turbo87 commented Sep 6, 2021

we could consider using https://www.postgresql.org/docs/current/ddl-generated-columns.html to have a generated (and stored) version_no_prerelease column in the versions table that the rev deps query can use. that way we only run the function when we insert a new version, but not all the time in the rev deps query.

the only downside of this is that generated columns were only introduced in Postgres 12 and apparently we're still using v11 in production.

I think that a 16% slowdown for a correctness bug is worth it though, and with the suggestion above we should have a viable way forward to make it much faster than before too.

@Turbo87
Copy link
Member Author

Turbo87 commented Sep 10, 2021

in our weekly team meeting today we decided to delay this bugfix until the performance concerns are addressed, which likely requires us to resolve #3480 first.

@bors
Copy link
Contributor

bors commented Sep 30, 2021

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

@Turbo87
Copy link
Member Author

Turbo87 commented Dec 10, 2021

closing in favor of #4022

@Turbo87 Turbo87 closed this Dec 10, 2021
@Turbo87 Turbo87 deleted the semver-sql-function branch October 17, 2022 17:35
@Turbo87 Turbo87 restored the semver-sql-function branch October 19, 2023 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ blocked C-bug 🐞 Category: unintended, undesired behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

to_semver_no_prerelease doesn't handle build metadata with hyphens correctly
5 participants