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() {