From 1c83f36ddf002ddb718ba3ec65c7acd163b28b64 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 10 Aug 2026 15:41:22 -0400 Subject: [PATCH] perf(persistence): make album list pages index-served The two album list requests reported in discussion #5929 (a 110k-album, two-library instance) were dominated by work that scaled with the whole library instead of the page: - Every native API list request also runs a count for the X-Total-Count header. album had no index covering library_id, so that count scanned the entire table on every request, dragging each row's JSON blobs through the page cache. count(distinct id) additionally built a temp b-tree over the full result set even when the query had no join that could duplicate rows. - A random sort cannot be served by an index, so SQLite evaluated SEEDEDRAND for every matching row and pushed each one, with its library and annotation joins and full column projection, through a sorter that kept only 36 rows. Add an album(id, library_id) index so library-filtered counts are served by a covering index scan, and make the shared count() helper use count(*) when the rendered query has no join, keeping count(distinct id) wherever a join could fan out rows. id leads the index deliberately: with library_id leading, the planner drives sorted list queries off it and sorts an entire library rather than walking a sort-satisfying index, and fresh statistics do not change that choice. Resolve random pages by id before hydrating them, so the sort pass runs on that covering index with no joins and only the page itself is materialized. Pages larger than 1000 keep the single-query path. The seeded shuffle is unchanged: the id query carries the caller's options, so SEEDEDRAND, explicit seeds, and the reseed-only-at-offset-0 rule all still apply, and the page is reordered in Go to preserve it. Also make the album name sort a total order by appending album.id, and widen album_order_album_name to (order_album_name, order_album_artist_name, id) to serve it. Without a tiebreaker, equal names made page order plan-dependent, which can skip or duplicate rows across paginated requests. --- ...60810171732_add_album_library_id_index.sql | 16 ++++ persistence/album_repository.go | 32 +++++++- persistence/album_repository_test.go | 78 +++++++++++++++++++ persistence/sql_base_repository.go | 15 +++- persistence/sql_base_repository_test.go | 16 ++++ 5 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 db/migrations/20260810171732_add_album_library_id_index.sql diff --git a/db/migrations/20260810171732_add_album_library_id_index.sql b/db/migrations/20260810171732_add_album_library_id_index.sql new file mode 100644 index 000000000..82c3f8a71 --- /dev/null +++ b/db/migrations/20260810171732_add_album_library_id_index.sql @@ -0,0 +1,16 @@ +-- +goose Up +-- id leads so library_id cannot drive a seek: the index covers library-filtered counts without +-- tempting the planner to sort a whole library instead of walking a sort-satisfying index. +create index if not exists album_id_library_id + on album(id, library_id); + +drop index if exists album_order_album_name; +create index album_order_album_name + on album(order_album_name, order_album_artist_name, id); + +-- +goose Down +drop index if exists album_id_library_id; + +drop index if exists album_order_album_name; +create index album_order_album_name + on album(order_album_name); diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 91e90127c..22e96b600 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -1,6 +1,7 @@ package persistence import ( + "cmp" "context" "encoding/json" "errors" @@ -109,7 +110,7 @@ func NewAlbumRepository(ctx context.Context, db dbx.Builder) model.AlbumReposito r.tableName = "album" r.registerModel(&model.Album{}, albumFilters()) r.setSortMappings(map[string]string{ - "name": "order_album_name, order_album_artist_name", + "name": "order_album_name, order_album_artist_name, album.id", "artist": "compilation, order_album_artist_name, order_album_name", "album_artist": "compilation, order_album_artist_name, order_album_name", // TODO Rename this to just year (or date) @@ -239,7 +240,14 @@ func (r *albumRepository) Get(id string) (*model.Album, error) { return &res[0], nil } +// Above this page size the id round-trip stops paying for itself and the IN list gets unwieldy. +const maxRandomPageIDs = 1000 + func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, error) { + if len(options) > 0 && options[0].Sort == "random" && + options[0].Max > 0 && options[0].Max <= maxRandomPageIDs { + return r.getRandomPage(options[0]) + } sq := r.selectAlbum(options...) var res dbAlbums err := r.queryAll(sq, &res) @@ -251,6 +259,28 @@ func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, e return albums, nil } +// No index can serve a random sort, so the sort pass visits every matching row. Resolving ids +// first keeps that pass on a covering index instead of dragging full rows and joins through it. +func (r *albumRepository) getRandomPage(options model.QueryOptions) (model.Albums, error) { + ids, err := r.GetAllIDs(options) + if err != nil { + return nil, err + } + if len(ids) == 0 { + return model.Albums{}, nil + } + albums, err := r.GetAll(model.QueryOptions{Filters: Eq{"album.id": ids}}) + if err != nil { + return nil, err + } + pos := make(map[string]int, len(ids)) + for i, albumID := range ids { + pos[albumID] = i + } + slices.SortFunc(albums, func(a, b model.Album) int { return cmp.Compare(pos[a.ID], pos[b.ID]) }) + return albums, nil +} + func (r *albumRepository) hydrateArtwork(albums model.Albums) { hydrateItems(r.ctx, r.db, model.KindAlbumArtwork, albums, func(a *model.Album) (string, *model.ItemImage) { return a.ID, &a.ItemImage }) diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 061083949..5ade21f90 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -109,6 +109,84 @@ var _ = Describe("AlbumRepository", func() { Expect(GetAll()).To(Equal(testAlbums)) }) + // The REST layer sends library_id as a string. SQLite only coerces it to the column's + // integer affinity while the term stays a plain column reference, so a filter built on an + // expression instead would silently match nothing (#5929). + DescribeTable("library_id filter from the REST layer", + func(sort string) { + res, err := albumRepo.ReadAll(rest.QueryOptions{ + Sort: sort, Max: 10, Filters: map[string]any{"library_id": "1"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(res.(model.Albums)).ToNot(BeEmpty()) + }, + Entry("name sort", "name"), + Entry("random sort", "random"), + Entry("no sort", ""), + ) + + Context("random sort", func() { + It("returns the page in the same order the id query produced", func() { + opts := model.QueryOptions{Sort: "random", Max: 4, Seed: "a-seed"} + ids, err := albumRepo.GetAllIDs(opts) + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(HaveLen(4)) + + albums, err := GetAll(opts) + Expect(err).ToNot(HaveOccurred()) + Expect(slice.Map(albums, func(a model.Album) string { return a.ID })).To(Equal(ids)) + }) + + It("keeps paging on one shuffle for a given seed", func() { + ids := func(albums model.Albums) []string { + return slice.Map(albums, func(a model.Album) string { return a.ID }) + } + firstTwoPages, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Seed: "s"}) + Expect(err).ToNot(HaveOccurred()) + secondPage, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Offset: 3, Seed: "s"}) + Expect(err).ToNot(HaveOccurred()) + whole, err := GetAll(model.QueryOptions{Sort: "random", Max: 6, Seed: "s"}) + Expect(err).ToNot(HaveOccurred()) + Expect(append(ids(firstTwoPages), ids(secondPage)...)).To(Equal(ids(whole))) + }) + + // Without an explicit seed, only Offset == 0 reseeds, so scrolling never repeats an album. + It("does not repeat albums across pages of one scroll", func() { + page1, err := GetAll(model.QueryOptions{Sort: "random", Max: 3}) + Expect(err).ToNot(HaveOccurred()) + page2, err := GetAll(model.QueryOptions{Sort: "random", Max: 3, Offset: 3}) + Expect(err).ToNot(HaveOccurred()) + Expect(page1).To(HaveLen(3)) + Expect(page2).To(HaveLen(3)) + for _, a := range page2 { + Expect(page1).ToNot(ContainElement(a)) + } + }) + + It("returns every record when no limit is given", func() { + albums, err := GetAll(model.QueryOptions{Sort: "random"}) + Expect(err).ToNot(HaveOccurred()) + Expect(albums).To(ConsistOf(testAlbums)) + }) + + It("applies filters", func() { + albums, err := GetAll(model.QueryOptions{ + Sort: "random", Max: 10, Filters: squirrel.Eq{"album.name": albumSgtPeppers.Name}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(albums).To(HaveLen(1)) + Expect(albums[0].ID).To(Equal(albumSgtPeppers.ID)) + }) + + It("hydrates the same fields as a non-random page", func() { + albums, err := GetAll(model.QueryOptions{ + Sort: "random", Max: 1, Filters: squirrel.Eq{"album.id": albumSgtPeppers.ID}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(albums).To(Equal(model.Albums{albumSgtPeppers})) + }) + }) + It("returns all records sorted", func() { Expect(GetAll(model.QueryOptions{Sort: "name"})).To(Equal(model.Albums{ albumAbbeyRoad, diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 33450fe9f..3085a4708 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -511,13 +511,26 @@ func (r sqlRepository) classifyOwnedWriteMiss(id string) error { return rest.ErrNotFound } +var joinRegex = regexp.MustCompile(`\bJOIN\b`) + +// countExpression returns count(*) for join-free queries; joins can fan out rows per id and +// need the much more expensive count(distinct id) (temp b-tree over the whole result set). +func (r sqlRepository) countExpression(query SelectBuilder) string { + sql, _, err := query.Columns("1").ToSql() + if err != nil || joinRegex.MatchString(sql) { + return "count(distinct " + r.tableName + ".id) as count" + } + return "count(*) as count" +} + func (r sqlRepository) count(countQuery SelectBuilder, options ...model.QueryOptions) (int64, error) { countQuery = countQuery. - RemoveColumns().Columns("count(distinct " + r.tableName + ".id) as count"). + RemoveColumns(). RemoveOffset().RemoveLimit(). OrderBy(r.tableName + ".id"). // To remove any ORDER BY clause that could slow down the query From(r.tableName) countQuery = r.applyFilters(countQuery, options...) + countQuery = countQuery.Columns(r.countExpression(countQuery)) var res struct{ Count int64 } err := r.queryOne(countQuery, &res) return res.Count, err diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index 9c6c6007f..c875092e9 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -18,6 +18,22 @@ var _ = Describe("sqlRepository", func() { r.tableName = "table" }) + Describe("countExpression", func() { + It("uses a plain count when the query has no joins", func() { + sq := squirrel.Select().From("table").Where(squirrel.Eq{"library_id": []int{1, 4}}) + Expect(r.countExpression(sq)).To(Equal("count(*) as count")) + }) + It("uses a distinct count when the query has a join", func() { + sq := squirrel.Select().From("table"). + LeftJoin("annotation on annotation.item_id = table.id") + Expect(r.countExpression(sq)).To(Equal("count(distinct table.id) as count")) + }) + It("is not fooled by parametrized values containing the word join", func() { + sq := squirrel.Select().From("table").Where(squirrel.Eq{"name": "the join band"}) + Expect(r.countExpression(sq)).To(Equal("count(*) as count")) + }) + }) + Describe("applyOptions", func() { var sq squirrel.SelectBuilder BeforeEach(func() {