perf(db): skip annotation join in CountAll when unused (#5694)
Some checks are pending
Pipeline: Test, Lint, Build / Get version info (push) Waiting to run
Pipeline: Test, Lint, Build / Lint Go code (push) Waiting to run
Pipeline: Test, Lint, Build / Test Go code (push) Waiting to run
Pipeline: Test, Lint, Build / Test Go code (Windows) (push) Waiting to run
Pipeline: Test, Lint, Build / Test JS code (push) Waiting to run
Pipeline: Test, Lint, Build / Lint i18n files (push) Waiting to run
Pipeline: Test, Lint, Build / Check Docker configuration (push) Waiting to run
Pipeline: Test, Lint, Build / Build (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-1 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-2 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-3 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-4 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-5 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-6 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-7 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-8 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-9 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build-10 (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Push to GHCR (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Push to Docker Hub (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Cleanup digest artifacts (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Build Windows installers (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Package/Release (push) Blocked by required conditions
Pipeline: Test, Lint, Build / Upload Linux PKG (push) Blocked by required conditions

* perf(persistence): skip annotation join in CountAll when unused

The Native API list endpoints (/api/song, /api/album, /api/artist) issue a
pagination count on every request via rest.GetAll. CountAll unconditionally
added a LEFT JOIN on the annotation table to a count(distinct id) query. The
join's columns are stripped by count(), but the distinct-over-join forced
SQLite to stream and dedup every row, making the count dominate the request
time on large libraries (e.g. ~190ms cold for 95k songs, seconds for
non-admin users behind the library subquery).

Gate the annotation join: only add it when a filter actually references an
annotation column. The need is detected by rendering the query to SQL and
matching annotation column names as whole words, which covers both named
filters (starred, has_rating) and raw squirrel filters. The column set is
derived from model.Annotations so it tracks schema changes; average_rating is
excluded because it lives on the base table, and word-boundary matching keeps
it from matching the annotation column rating.

Counts are unchanged; only the query plan changes. Unfiltered song counts drop
from ~37ms to ~4ms (warm) on a 95k-song library.

* test(persistence): make annotation-join detection case-insensitive

Address review feedback: SQLite column names are case-insensitive, so a raw
filter using e.g. "RATING" would previously evade the case-sensitive column
regex and wrongly drop the annotation join. Add the (?i) flag (average_rating
stays excluded — the underscore still prevents a word boundary before rating)
and cover it with mixed-case tests. Also make the starred-count assertion an
exact value instead of a range.
This commit is contained in:
Deluan Quintão 2026-06-30 22:50:43 -04:00 committed by GitHub
parent 5f2b7ee105
commit 385e75e9a9
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
7 changed files with 198 additions and 3 deletions

View file

@ -186,8 +186,10 @@ func allRolesFilter(_ string, value any) Sqlizer {
func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) {
query := r.newSelect()
query = r.withAnnotation(query, "album.id")
query = r.applyLibraryFilter(query)
if filtersNeedAnnotation(r.applyFilters(query, options...)) {
query = r.withAnnotation(query, "album.id")
}
return r.count(query, options...)
}

View file

@ -201,7 +201,11 @@ func (r *artistRepository) selectArtist(options ...model.QueryOptions) SelectBui
func (r *artistRepository) CountAll(options ...model.QueryOptions) (int64, error) {
query := r.newSelect()
query = r.applyLibraryFilterToArtistQuery(query)
query = r.withAnnotation(query, "artist.id")
// Only the annotation join is gated; the library_artist join above (and its count(distinct))
// must stay, since an artist can span multiple libraries.
if filtersNeedAnnotation(r.applyFilters(query, options...)) {
query = r.withAnnotation(query, "artist.id")
}
return r.count(query, options...)
}

View file

@ -273,6 +273,23 @@ var _ = Describe("ArtistRepository", func() {
It("returns the number of artists in the DB", func() {
Expect(repo.CountAll()).To(Equal(int64(4)))
})
It("counts starred artists when an annotation filter is present", func() {
// The Beatles (id 3) is starred for the admin user in the seed data
count, err := repo.CountAll(model.QueryOptions{
Filters: annotationBoolFilter("starred")("starred", "true"),
})
Expect(err).ToNot(HaveOccurred())
Expect(count).To(Equal(int64(1)))
})
It("counts with has_rating=false without a 'no such column' error (join kept)", func() {
count, err := repo.CountAll(model.QueryOptions{
Filters: annotationBoolFilter("rating")("rating", "false"),
})
Expect(err).ToNot(HaveOccurred())
Expect(count).To(Equal(int64(4)))
})
})
Describe("Exists", func() {

View file

@ -124,8 +124,11 @@ func mediaFileRecentlyAddedSort() string {
func (r *mediaFileRepository) CountAll(options ...model.QueryOptions) (int64, error) {
query := r.newSelect()
query = r.withAnnotation(query, "media_file.id")
query = r.applyLibraryFilter(query)
// The annotation join is expensive with count(distinct) and pointless unless a filter uses it.
if filtersNeedAnnotation(r.applyFilters(query, options...)) {
query = r.withAnnotation(query, "media_file.id")
}
return r.count(query, options...)
}

View file

@ -45,6 +45,37 @@ var _ = Describe("MediaRepository", func() {
Expect(mr.CountAll()).To(Equal(int64(13)))
})
Describe("CountAll annotation-join gating", func() {
var adminRepo model.MediaFileRepository
BeforeEach(func() {
adminCtx := request.WithUser(log.NewContext(context.TODO()), model.User{ID: "userid", IsAdmin: true})
adminRepo = NewMediaFileRepository(adminCtx, GetDBXBuilder())
})
It("counts starred songs when an annotation filter is present", func() {
// Come Together (id 1002) is starred for the admin user in the seed data
count, err := adminRepo.CountAll(model.QueryOptions{
Filters: annotationBoolFilter("starred")("starred", "true"),
})
Expect(err).ToNot(HaveOccurred())
Expect(count).To(Equal(int64(1)))
})
It("counts with starred=false without a 'no such column' error (join kept)", func() {
count, err := adminRepo.CountAll(model.QueryOptions{
Filters: annotationBoolFilter("starred")("starred", "false"),
})
Expect(err).ToNot(HaveOccurred())
// All songs except the one starred one
Expect(count).To(Equal(int64(12)))
})
It("counts unfiltered with the join dropped", func() {
Expect(adminRepo.CountAll()).To(Equal(int64(13)))
})
})
Describe("CountBySuffix", func() {
var mp3File, flacFile1, flacFile2, flacUpperFile model.MediaFile

View file

@ -4,17 +4,61 @@ import (
"database/sql"
"errors"
"fmt"
"regexp"
"sort"
"strings"
"sync"
"time"
. "github.com/Masterminds/squirrel"
"github.com/fatih/structs"
"github.com/navidrome/navidrome/conf"
"github.com/navidrome/navidrome/consts"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
)
const annotationTable = "annotation"
// annotationColumns are the columns withAnnotation's LEFT JOIN contributes, derived from
// model.Annotations so the set tracks schema changes. average_rating is excluded: it lives on the
// base table, not the annotation join.
var annotationColumns = sync.OnceValue(func() map[string]struct{} {
cols := map[string]struct{}{}
for name := range structs.Map(model.Annotations{}) {
if name == "average_rating" {
continue
}
cols[name] = struct{}{}
}
return cols
})
// annotationColumnRE matches any annotation column as a whole word. The word boundaries keep the
// base-table column average_rating from matching the annotation column rating (Go's \b treats '_'
// as a word char). It is case-insensitive because SQLite column names are, so a raw filter using
// e.g. "RATING" must still be detected.
var annotationColumnRE = sync.OnceValue(func() *regexp.Regexp {
cols := make([]string, 0, len(annotationColumns()))
for col := range annotationColumns() {
cols = append(cols, regexp.QuoteMeta(col))
}
sort.Strings(cols) // map iteration is random; sort for a stable pattern
return regexp.MustCompile(`(?i)\b(?:` + strings.Join(cols, "|") + `)\b`)
})
// filtersNeedAnnotation reports whether the rendered query references an annotation column, i.e.
// whether the annotation LEFT JOIN must be kept. Scanning the rendered SQL catches every filter
// path. The placeholder column is needed because squirrel won't render a column-less SELECT; on a
// render error, keep the join to be safe.
func filtersNeedAnnotation(query SelectBuilder) bool {
sql, _, err := query.Columns("1").ToSql()
if err != nil {
return true
}
return annotationColumnRE().MatchString(sql)
}
func (r sqlRepository) withAnnotation(query SelectBuilder, idField string) SelectBuilder {
userID := loggedUser(r.ctx).ID
if userID == invalidUserId {

View file

@ -150,4 +150,98 @@ var _ = Describe("Annotation Filters", func() {
}
Expect(found).To(BeTrue(), "Item without annotation should be included when filter is ignored")
})
Describe("annotationColumns", func() {
It("derives the annotation join columns from model.Annotations, excluding average_rating", func() {
cols := annotationColumns()
Expect(cols).To(HaveKey("starred"))
Expect(cols).To(HaveKey("starred_at"))
Expect(cols).To(HaveKey("rating"))
Expect(cols).To(HaveKey("rated_at"))
Expect(cols).To(HaveKey("play_count"))
Expect(cols).To(HaveKey("play_date"))
Expect(cols).To(HaveLen(6), "expected exactly the 6 annotation-join columns")
Expect(cols).ToNot(HaveKey("average_rating"), "average_rating lives on the base table, not the annotation join")
})
})
Describe("filtersNeedAnnotation", func() {
It("is true when the query references an annotation column", func() {
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Eq{"starred": true})
Expect(filtersNeedAnnotation(q)).To(BeTrue())
})
It("is true for a raw expression referencing an annotation column", func() {
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("rating > 0"))
Expect(filtersNeedAnnotation(q)).To(BeTrue())
})
It("is false for a query that references no annotation column", func() {
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Eq{"missing": false})
Expect(filtersNeedAnnotation(q)).To(BeFalse())
})
It("is false for a filter on average_rating (base-table column, not the annotation rating)", func() {
// Regression: average_rating must not match the annotation column "rating".
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Gt{"average_rating": 3})
Expect(filtersNeedAnnotation(q)).To(BeFalse())
})
It("is true when both average_rating and a real annotation column are referenced", func() {
q := squirrel.Select("count(1)").From("media_file").
Where(squirrel.Gt{"average_rating": 3}).
Where(squirrel.Expr("COALESCE(rating, 0) > 0"))
Expect(filtersNeedAnnotation(q)).To(BeTrue())
})
It("is true for uppercase/mixed-case annotation columns (SQLite is case-insensitive)", func() {
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("RATING > 0"))
Expect(filtersNeedAnnotation(q)).To(BeTrue())
})
It("is false for uppercase average_rating (still excluded case-insensitively)", func() {
q := squirrel.Select("count(1)").From("media_file").Where(squirrel.Expr("AVERAGE_RATING > 3"))
Expect(filtersNeedAnnotation(q)).To(BeFalse())
})
})
Describe("CountAll annotation-join gating", func() {
It("counts all items unfiltered (join dropped)", func() {
total, err := albumRepo.CountAll()
Expect(err).ToNot(HaveOccurred())
Expect(total).To(BeNumerically(">=", int64(1)))
filtered, err := albumRepo.CountAll(model.QueryOptions{
Filters: squirrel.Eq{"album.id": albumWithoutAnnotation.ID},
})
Expect(err).ToNot(HaveOccurred())
Expect(filtered).To(Equal(int64(1)))
})
It("counts starred items correctly (named annotation filter keeps the join)", func() {
starredAlbum := model.Album{ID: "counted-starred-album", Name: "Counted Starred", LibraryID: 1}
Expect(albumRepo.Put(&starredAlbum)).To(Succeed())
Expect(albumRepo.SetStar(true, starredAlbum.ID)).To(Succeed())
defer func() {
_, _ = albumRepo.executeSQL(squirrel.Delete("annotation").Where(squirrel.Eq{"item_id": starredAlbum.ID}))
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": starredAlbum.ID}))
}()
// Exactly two albums are starred for this user: the one created above and
// albumRadioactivity (id 103) from the seed data.
count, err := albumRepo.CountAll(model.QueryOptions{
Filters: annotationBoolFilter("starred")("starred", "true"),
})
Expect(err).ToNot(HaveOccurred())
Expect(count).To(Equal(int64(2)))
})
It("counts via a raw annotation filter without a 'no such column' error", func() {
count, err := albumRepo.CountAll(model.QueryOptions{
Filters: squirrel.Expr("COALESCE(rating, 0) > 0"),
})
Expect(err).ToNot(HaveOccurred())
Expect(count).To(BeNumerically(">=", int64(0)))
})
})
})