Skip to content
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

Jump to bottom

x/pkgsite: use moduleID in getPackagesInUnit #45854

Closed
julieqiu opened this issue Apr 29, 2021 · 5 comments
Closed

x/pkgsite: use moduleID in getPackagesInUnit #45854

julieqiu opened this issue Apr 29, 2021 · 5 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. pkgsite Suggested Issues that may be good for new contributors looking for work to do.
Milestone
pkgsite/unplanned

Comments

@julieqiu
Copy link
Member

getPackagesInUnit is used by the frontend via getUnitWithAllFields.

Since we can easily get the moduleID from the first query in that function, we should consider passing in the moduleID to getPackagesInUnit instead of (modulePath, version). This could increase performance since we no longer need to JOIN on the modules table and can filter by u.module_id.
@gopherbot gopherbot added this to the Unreleased milestone Apr 29, 2021
@julieqiu julieqiu added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 29, 2021
@julieqiu julieqiu added help wanted Suggested Issues that may be good for new contributors looking for work to do. labels Apr 29, 2021
@junjunjunk
Copy link

Hi. I'll try this issue!

@junjunjunk
Copy link

junjunjunk commented Apr 29, 2021 •
edited

May I ask you one question?
getPackagesInUnit is also used by ReInsertLatestVersion.

And this method doesn't use module_id. So I can not replace the argument of getPackagesInUnit to use module_id.
Should I implement db's new method getPackagesInUnitUsingModuleId or get module_id info in ReInsertLatestVersion?

@julieqiu
Copy link
Member Author

Thanks @junjunjunk!

Should I implement db's new method getPackagesInUnitUsingModuleId or get module_id info in ReInsertLatestVersion?

Yup! I would add a new function.

You can share a lot of the same logic between the two functions, so it might be helpful to know that we use https://pkg.go.dev/github.com/Masterminds/squirrel to create shared queries in our codebase.

https://github.com/golang/pkgsite/blob/dfa0f1976413309fde70a10909a49793da16725b/internal/postgres/symbol_history.go#L62 is an example of a place where squirrel is used.

@junjunjunk
Copy link

Thank you for your thoughtful response!
I'll work on it with the resources you provided.

junjunjunk added a commit to junjunjunk/pkgsite that referenced this issue Apr 30, 2021
@junjunjunk
internal/postgres: use moduleID in getPackagesInUnit
19f8cc0 
Add argument moduleID to getPackagesInUnit for more efficiency.

Fixes golang/go#45854
@junjunjunk junjunjunk mentioned this issue Apr 30, 2021
Closed
junjunjunk added a commit to junjunjunk/pkgsite that referenced this issue Apr 30, 2021
@junjunjunk
internal/postgres: use moduleID in getPackagesInUnit
f800906 
Add argument moduleID to getPackagesInUnit for more efficiency.

Fixes golang/go#45854
@gopherbot
Copy link

Change https://golang.org/cl/315489 mentions this issue: internal/postgres: use moduleID in getPackagesInUnit

@golang golang locked and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. pkgsite Suggested Issues that may be good for new contributors looking for work to do.
Projects
None yet
Milestone
pkgsite/unplanned
Development

Successfully merging a pull request may close this issue.

internal/postgres: use moduleID in getPackagesInUnit junjunjunk/pkgsite
3 participants
@julieqiu @gopherbot @junjunjunk