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

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

Comments

@julieqiu
Copy link
Contributor

@julieqiu julieqiu commented Apr 29, 2021

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.

@junjunjunk
Copy link

@junjunjunk junjunjunk commented Apr 29, 2021

Hi. I'll try this issue!

@junjunjunk
Copy link

@junjunjunk junjunjunk commented Apr 29, 2021

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
Contributor Author

@julieqiu julieqiu commented Apr 29, 2021

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

@junjunjunk junjunjunk commented Apr 29, 2021

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

junjunjunk pushed a commit to junjunjunk/pkgsite that referenced this issue Apr 30, 2021
Add argument moduleID to getPackagesInUnit for more efficiency.

Fixes golang/go#45854
junjunjunk added a commit to junjunjunk/pkgsite that referenced this issue Apr 30, 2021
Add argument moduleID to getPackagesInUnit for more efficiency.

Fixes golang/go#45854
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 30, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants