mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
refactor: reduce code duplication when accessing DefaultMaxInSize (#11999)
`DefaultMaxInSize` is an internal parameter for limiting the size of `field IN (...)` clauses in DB queries, which is a reasonable thing to do -- in addition to the errors noted when [originally introduced](https://github.com/go-gitea/gitea/pull/4594), there are technical limits that apply to each of PostgreSQL, MySQL, and SQLite which would prevent an unbounded size for a query like this. However: the size is incredibly small at 50, and, the implementation of `DefaultMaxInSize` is really wasteful with copy-and-paste coding. This PR: - introduces `GetByIDs` which fetches a `map[int64]*Model` from the database for an array of ID values, while respecting `IN` clause size limits - introduces `GetByFieldIn` which fetches a `map[int64][]*Model` from the database for an array of field values, while respecting `IN` clause size limits - uses `slices.Chunk` for other locations where queries are too complex for these implementations - bumps the `DefaultMaxInSize` parameter from 50 to 500, a conservative increase well under known limits, but 10x the current value: - PostgreSQL supports up to 1GB query text size with 65,535 parameters, but I've experienced performance degradation at high value counts - MySQL supports 64MB query text size without known limits of parameter count - SQLite supports 32,766 parameters in a query ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [x] in their respective `*_test.go` for unit tests. - Refactored functions are assumed to be covered by existing tests to some extent; that assumption is probably wrong but the changes here are relatively easily reviewed for correctness as well. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/11999 Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org> Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net> Co-committed-by: Mathieu Fenniak <mathieu@fenniak.net>
This commit is contained in:
Mathieu Fenniak
Mathieu Fenniak
parent
6a879e79df
commit
9abc1b0144
7 changed files with 215 additions and 378 deletions
|
|
@ -210,31 +210,9 @@ func (nl NotificationList) LoadRepos(ctx context.Context) (repo_model.Repository
|
|||
}
|
||||
|
||||
repoIDs := nl.getPendingRepoIDs()
|
||||
repos := make(map[int64]*repo_model.Repository, len(repoIDs))
|
||||
left := len(repoIDs)
|
||||
for left > 0 {
|
||||
limit := min(left, db.DefaultMaxInSize)
|
||||
rows, err := db.GetEngine(ctx).
|
||||
In("id", repoIDs[:limit]).
|
||||
Rows(new(repo_model.Repository))
|
||||
if err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
|
||||
for rows.Next() {
|
||||
var repo repo_model.Repository
|
||||
err = rows.Scan(&repo)
|
||||
if err != nil {
|
||||
rows.Close()
|
||||
return nil, nil, err
|
||||
}
|
||||
|
||||
repos[repo.ID] = &repo
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
left -= limit
|
||||
repoIDs = repoIDs[limit:]
|
||||
repos, err := db.GetByIDs(ctx, "id", repoIDs, &repo_model.Repository{})
|
||||
if err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
|
||||
failed := []int{}
|
||||
|
|
@ -281,31 +259,9 @@ func (nl NotificationList) LoadIssues(ctx context.Context) ([]int, error) {
|
|||
}
|
||||
|
||||
issueIDs := nl.getPendingIssueIDs()
|
||||
issues := make(map[int64]*issues_model.Issue, len(issueIDs))
|
||||
left := len(issueIDs)
|
||||
for left > 0 {
|
||||
limit := min(left, db.DefaultMaxInSize)
|
||||
rows, err := db.GetEngine(ctx).
|
||||
In("id", issueIDs[:limit]).
|
||||
Rows(new(issues_model.Issue))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
for rows.Next() {
|
||||
var issue issues_model.Issue
|
||||
err = rows.Scan(&issue)
|
||||
if err != nil {
|
||||
rows.Close()
|
||||
return nil, err
|
||||
}
|
||||
|
||||
issues[issue.ID] = &issue
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
left -= limit
|
||||
issueIDs = issueIDs[limit:]
|
||||
issues, err := db.GetByIDs(ctx, "id", issueIDs, &issues_model.Issue{})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
failures := []int{}
|
||||
|
|
@ -373,31 +329,9 @@ func (nl NotificationList) LoadUsers(ctx context.Context) ([]int, error) {
|
|||
}
|
||||
|
||||
userIDs := nl.getUserIDs()
|
||||
users := make(map[int64]*user_model.User, len(userIDs))
|
||||
left := len(userIDs)
|
||||
for left > 0 {
|
||||
limit := min(left, db.DefaultMaxInSize)
|
||||
rows, err := db.GetEngine(ctx).
|
||||
In("id", userIDs[:limit]).
|
||||
Rows(new(user_model.User))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
for rows.Next() {
|
||||
var user user_model.User
|
||||
err = rows.Scan(&user)
|
||||
if err != nil {
|
||||
rows.Close()
|
||||
return nil, err
|
||||
}
|
||||
|
||||
users[user.ID] = &user
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
left -= limit
|
||||
userIDs = userIDs[limit:]
|
||||
users, err := db.GetByIDs(ctx, "id", userIDs, &user_model.User{})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
failures := []int{}
|
||||
|
|
@ -421,31 +355,9 @@ func (nl NotificationList) LoadComments(ctx context.Context) ([]int, error) {
|
|||
}
|
||||
|
||||
commentIDs := nl.getPendingCommentIDs()
|
||||
comments := make(map[int64]*issues_model.Comment, len(commentIDs))
|
||||
left := len(commentIDs)
|
||||
for left > 0 {
|
||||
limit := min(left, db.DefaultMaxInSize)
|
||||
rows, err := db.GetEngine(ctx).
|
||||
In("id", commentIDs[:limit]).
|
||||
Rows(new(issues_model.Comment))
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
for rows.Next() {
|
||||
var comment issues_model.Comment
|
||||
err = rows.Scan(&comment)
|
||||
if err != nil {
|
||||
rows.Close()
|
||||
return nil, err
|
||||
}
|
||||
|
||||
comments[comment.ID] = &comment
|
||||
}
|
||||
_ = rows.Close()
|
||||
|
||||
left -= limit
|
||||
commentIDs = commentIDs[limit:]
|
||||
comments, err := db.GetByIDs(ctx, "id", commentIDs, &issues_model.Comment{})
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
failures := []int{}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue