From f0abba3eef76b95e17a887c2ed8ca4a3a7f71c92 Mon Sep 17 00:00:00 2001 From: Gusted Date: Fri, 25 Oct 2024 18:41:37 +0200 Subject: [PATCH] fix: use buffered iterate for debian searchpackages - The driver being used for PostgreSQL doesn't handle interleaved queries (you start a query, read some rows and start another query while you didn't finish that query yet), this is the case with using `.Iterate` from XORM. - Switch to a variant of what exist in the current codebase of `db.Iterate`, which is a simple buffered iteration and doesn't keep queries open, which allow other database operations to happen. - Unit test added. This doesn't cover that postgres does not error on this case, as this is not run with a postgres database. - Resolves #5696 (cherry picked from commit 459ab11a8aeb5a4d77cb27561c28eecb11ed0f94) --- models/packages/debian/search.go | 45 +++++++++---- models/packages/debian/search_test.go | 93 +++++++++++++++++++++++++++ 2 files changed, 124 insertions(+), 14 deletions(-) create mode 100644 models/packages/debian/search_test.go diff --git a/models/packages/debian/search.go b/models/packages/debian/search.go index 77c4a18462..abf23e42f6 100644 --- a/models/packages/debian/search.go +++ b/models/packages/debian/search.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/packages" debian_module "code.gitea.io/gitea/modules/packages/debian" + "code.gitea.io/gitea/modules/setting" "xorm.io/builder" ) @@ -76,25 +77,41 @@ func ExistPackages(ctx context.Context, opts *PackageSearchOptions) (bool, error // SearchPackages gets the packages matching the search options func SearchPackages(ctx context.Context, opts *PackageSearchOptions, iter func(*packages.PackageFileDescriptor)) error { - return db.GetEngine(ctx). - Table("package_file"). - Select("package_file.*"). - Join("INNER", "package_version", "package_version.id = package_file.version_id"). - Join("INNER", "package", "package.id = package_version.package_id"). - Where(opts.toCond()). - Asc("package.lower_name", "package_version.created_unix"). - Iterate(new(packages.PackageFile), func(_ int, bean any) error { - pf := bean.(*packages.PackageFile) + var start int + batchSize := setting.Database.IterateBufferSize + for { + select { + case <-ctx.Done(): + return ctx.Err() + default: + beans := make([]*packages.PackageFile, 0, batchSize) - pfd, err := packages.GetPackageFileDescriptor(ctx, pf) - if err != nil { + if err := db.GetEngine(ctx). + Table("package_file"). + Select("package_file.*"). + Join("INNER", "package_version", "package_version.id = package_file.version_id"). + Join("INNER", "package", "package.id = package_version.package_id"). + Where(opts.toCond()). + Asc("package.lower_name", "package_version.created_unix"). + Limit(batchSize, start). + Find(&beans); err != nil { return err } + if len(beans) == 0 { + return nil + } + start += len(beans) - iter(pfd) + for _, bean := range beans { + pfd, err := packages.GetPackageFileDescriptor(ctx, bean) + if err != nil { + return err + } - return nil - }) + iter(pfd) + } + } + } } // GetDistributions gets all available distributions diff --git a/models/packages/debian/search_test.go b/models/packages/debian/search_test.go new file mode 100644 index 0000000000..104a01498b --- /dev/null +++ b/models/packages/debian/search_test.go @@ -0,0 +1,93 @@ +// Copyright 2024 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package debian + +import ( + "strings" + "testing" + + "code.gitea.io/gitea/models/db" + packages_model "code.gitea.io/gitea/models/packages" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/packages" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + packages_service "code.gitea.io/gitea/services/packages" + + _ "code.gitea.io/gitea/models" + _ "code.gitea.io/gitea/models/actions" + _ "code.gitea.io/gitea/models/activities" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m) +} + +func preparePackage(t *testing.T, owner *user_model.User, name string) { + t.Helper() + + data, err := packages.CreateHashedBufferFromReader(strings.NewReader("data")) + require.NoError(t, err) + + _, _, err = packages_service.CreatePackageOrAddFileToExisting( + db.DefaultContext, + &packages_service.PackageCreationInfo{ + PackageInfo: packages_service.PackageInfo{ + Owner: owner, + PackageType: packages_model.TypeDebian, + Name: name, + }, + Creator: owner, + }, + &packages_service.PackageFileCreationInfo{ + PackageFileInfo: packages_service.PackageFileInfo{ + Filename: name, + }, + Data: data, + Creator: owner, + IsLead: true, + }, + ) + + require.NoError(t, err) +} + +func TestSearchPackages(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + defer test.MockVariableValue(&setting.Database.IterateBufferSize, 1)() + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + user3 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3}) + + preparePackage(t, user2, "debian-1") + preparePackage(t, user2, "debian-2") + preparePackage(t, user3, "debian-1") + + packageFiles := []string{} + require.NoError(t, SearchPackages(db.DefaultContext, &PackageSearchOptions{ + OwnerID: user2.ID, + }, func(pfd *packages_model.PackageFileDescriptor) { + assert.NotNil(t, pfd) + packageFiles = append(packageFiles, pfd.File.Name) + })) + + assert.Len(t, packageFiles, 2) + assert.Contains(t, packageFiles, "debian-1") + assert.Contains(t, packageFiles, "debian-2") + + packageFiles = []string{} + require.NoError(t, SearchPackages(db.DefaultContext, &PackageSearchOptions{ + OwnerID: user3.ID, + }, func(pfd *packages_model.PackageFileDescriptor) { + assert.NotNil(t, pfd) + packageFiles = append(packageFiles, pfd.File.Name) + })) + + assert.Len(t, packageFiles, 1) + assert.Contains(t, packageFiles, "debian-1") +}