Skip to content

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

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 2 commits into from
Nov 1, 2023

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Oct 19, 2023

Resolves #3882

This PR is basically reviving #3886, since we now have a generated and stored semver_no_prerelease column in the versions table, which should fix the performance concerns that led to #3886 being initially rejected.

@Turbo87 Turbo87 added C-bug 🐞 Category: unintended, undesired behavior A-backend ⚙️ labels Oct 19, 2023
@Turbo87
Copy link
Member Author

Turbo87 commented Oct 19, 2023

@paolobarbolini since @LawnGnome is on vacation and you've proven that you know a lot about Postgres, any chance you could take a look at this? :)

I'm most concerned about two things:

  • performance impact on the reverse dependencies query, which I assume should be zero since we have the stored column now
  • impact of running the create or replace function migration. it looks like drop + create is fast, but requires us to add the generated column again afterwards, which is slow. create or replace function is also fast but appears to keep the generated column, though potentially with old values.

@Turbo87 Turbo87 force-pushed the semver-sql-function branch 2 times, most recently from 2cae43f to a221dee Compare October 19, 2023 10:39
Copy link
Member

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@0xPoe 0xPoe left a comment

Choose a reason for hiding this comment

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

Looks good to me. I forgot this is called to_semver_no_prerelease, so I deleted my previous review comments:(

Tested locally and it worked well.

Copy link
Contributor

@LawnGnome LawnGnome left a comment

Choose a reason for hiding this comment

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

Makes sense to me, and the tests cover the two variations that are important (dash before plus, and vice versa). 👍

@Turbo87 Turbo87 force-pushed the semver-sql-function branch from a221dee to fcfda15 Compare November 1, 2023 14:23
@paolobarbolini
Copy link
Contributor

@paolobarbolini since @LawnGnome is on vacation and you've proven that you know a lot about Postgres, any chance you could take a look at this? :)

I'm most concerned about two things:

* performance impact on the reverse dependencies query, which I assume should be zero since we have the stored column now

* impact of running the `create or replace function` migration. it looks like `drop` + `create` is fast, but requires us to add the generated column again afterwards, which is slow. `create or replace function` is also fast but appears to keep the generated column, though potentially with old values.

I missed the notification 😅.

  • No impact
  • You should be able to replace the function. The GENERATED column won't be updated though. What you can do is write an UPDATE which assigns the raw version string to itself in order to force the GENERATED column to refresh. Should be even faster if you can filter it to only the rows which may actually need to be regenerated, although at your size it should be possible to YOLO it and just refresh the entire table and VACUUM.

@Turbo87 Turbo87 merged commit e8494e5 into rust-lang:main Nov 1, 2023
@Turbo87 Turbo87 deleted the semver-sql-function branch November 1, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ 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
4 participants