From 9ce51cf575f698a93393516144e7ae0415f32751 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 23 Jul 2026 13:20:22 -0400 Subject: [PATCH] perf(artwork): fetch only IDs for backfill enumeration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backfill enumerated every album, artist, playlist and radio via GetAll and mapped out just the ID. GetAll materializes full entities (library joins, participant/stats/tags JSON, annotation, artwork hydration), so on a large library it loaded tens of thousands of heavy structs only to read one field each — spiking transient RSS to ~1GB during the one-time upgrade backfill, a memory risk on small NAS/Pi hardware. Add GetAllIDs to the album, artist, playlist and radio repositories: it reuses each repo's base row-set filter (library visibility, artist content join, playlist userFilter) but projects only id, skipping the heavy columns and post-processing. A per-repo parity test asserts GetAllIDs returns exactly the same id set as GetAll. Verified on a 727MB / 29k-artist production DB copy: peak RSS during backfill dropped from ~1012MB to ~89MB, file descriptors flat, same 36,138 items enqueued. --- core/artwork/housekeeping.go | 21 ++++----------------- model/album.go | 1 + model/artist.go | 1 + model/playlist.go | 1 + model/radio.go | 1 + persistence/album_repository.go | 9 +++++++++ persistence/album_repository_test.go | 15 +++++++++++++++ persistence/artist_repository.go | 9 +++++++++ persistence/artist_repository_test.go | 15 +++++++++++++++ persistence/playlist_repository.go | 10 ++++++++++ persistence/playlist_repository_test.go | 15 +++++++++++++++ persistence/radio_repository.go | 8 ++++++++ persistence/radio_repository_test.go | 15 +++++++++++++++ tests/mock_album_repo.go | 12 ++++++++++++ tests/mock_artist_repo.go | 12 ++++++++++++ tests/mock_playlist_repo.go | 12 ++++++++++++ tests/mock_radio_repository.go | 12 ++++++++++++ 17 files changed, 152 insertions(+), 17 deletions(-) diff --git a/core/artwork/housekeeping.go b/core/artwork/housekeeping.go index fa7196ae6..f45c62ab6 100644 --- a/core/artwork/housekeeping.go +++ b/core/artwork/housekeeping.go @@ -12,7 +12,6 @@ import ( "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" - "github.com/navidrome/navidrome/utils/slice" ) // FingerprintPropertyKey is the model.PropertyRepository key Backfill compares against @@ -54,22 +53,10 @@ func Backfill(ctx context.Context, ds model.DataStore) (bool, error) { kind string fetch func() ([]string, error) }{ - {"ar", func() ([]string, error) { - as, err := ds.Artist(ctx).GetAll() - return slice.Map(as, func(a model.Artist) string { return a.ID }), err - }}, - {"al", func() ([]string, error) { - as, err := ds.Album(ctx).GetAll() - return slice.Map(as, func(a model.Album) string { return a.ID }), err - }}, - {"pl", func() ([]string, error) { - ps, err := ds.Playlist(ctx).GetAll() - return slice.Map(ps, func(p model.Playlist) string { return p.ID }), err - }}, - {"ra", func() ([]string, error) { - rs, err := ds.Radio(ctx).GetAll() - return slice.Map(rs, func(r model.Radio) string { return r.ID }), err - }}, + {"ar", func() ([]string, error) { return ds.Artist(ctx).GetAllIDs() }}, + {"al", func() ([]string, error) { return ds.Album(ctx).GetAllIDs() }}, + {"pl", func() ([]string, error) { return ds.Playlist(ctx).GetAllIDs() }}, + {"ra", func() ([]string, error) { return ds.Radio(ctx).GetAllIDs() }}, } for _, k := range kinds { ids, err := k.fetch() diff --git a/model/album.go b/model/album.go index 55df8c63c..7c03ccf1e 100644 --- a/model/album.go +++ b/model/album.go @@ -142,6 +142,7 @@ type AlbumRepository interface { UpdateExternalInfo(*Album) error Get(id string) (*Album, error) GetAll(...QueryOptions) (Albums, error) + GetAllIDs(...QueryOptions) ([]string, error) GetCursor(...QueryOptions) (AlbumCursor, error) GetYears(libraryIDs ...int) ([]int, error) diff --git a/model/artist.go b/model/artist.go index f9c4bffd5..053f261ce 100644 --- a/model/artist.go +++ b/model/artist.go @@ -89,6 +89,7 @@ type ArtistRepository interface { UpdateExternalInfo(a *Artist) error Get(id string) (*Artist, error) GetAll(options ...QueryOptions) (Artists, error) + GetAllIDs(options ...QueryOptions) ([]string, error) GetCursor(options ...QueryOptions) (ArtistCursor, error) GetIndex(includeMissing bool, libraryIds []int, roles ...Role) (ArtistIndexes, error) diff --git a/model/playlist.go b/model/playlist.go index 185f6f942..b5726e4ff 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -143,6 +143,7 @@ type PlaylistRepository interface { Get(id string) (*Playlist, error) GetWithTracks(id string, refreshSmartPlaylist, includeMissing bool) (*Playlist, error) GetAll(options ...QueryOptions) (Playlists, error) + GetAllIDs(options ...QueryOptions) ([]string, error) GetCursor(options ...QueryOptions) (PlaylistCursor, error) FindByPath(path string) (*Playlist, error) Delete(id string) error diff --git a/model/radio.go b/model/radio.go index 86f27c24c..7f654b665 100644 --- a/model/radio.go +++ b/model/radio.go @@ -32,5 +32,6 @@ type RadioRepository interface { Delete(id string) error Get(id string) (*Radio, error) GetAll(options ...QueryOptions) (Radios, error) + GetAllIDs(options ...QueryOptions) ([]string, error) Put(u *Radio, colsToUpdate ...string) error } diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 2bb541003..e7b155e6f 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -254,6 +254,15 @@ func (r *albumRepository) GetAll(options ...model.QueryOptions) (model.Albums, e return res.toModels(), nil } +// GetAllIDs returns just the album IDs for the same row set as GetAll, skipping the +// heavy column projection and JSON post-processing. Used by bulk enumeration (artwork backfill). +func (r *albumRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + sq := r.applyLibraryFilter(r.newSelect(options...).Columns("album.id")) + ids := []string{} + err := r.queryAllSlice(sq, &ids) + return ids, err +} + func (r *albumRepository) GetCursor(options ...model.QueryOptions) (model.AlbumCursor, error) { sq := r.selectAlbum(options...) cursor, err := queryWithStableResults[dbAlbum](r.sqlRepository, sq) diff --git a/persistence/album_repository_test.go b/persistence/album_repository_test.go index 465617b10..ef18cf426 100644 --- a/persistence/album_repository_test.go +++ b/persistence/album_repository_test.go @@ -84,6 +84,21 @@ var _ = Describe("AlbumRepository", func() { }) }) + Describe("GetAllIDs", func() { + It("returns the same id set as GetAll", func() { + want, err := albumRepo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(want).ToNot(BeEmpty()) + wantIDs := make([]string, 0, len(want)) + for _, a := range want { + wantIDs = append(wantIDs, a.ID) + } + ids, err := albumRepo.GetAllIDs() + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(ConsistOf(wantIDs)) + }) + }) + Describe("GetAll", func() { var GetAll = func(opts ...model.QueryOptions) (model.Albums, error) { albums, err := albumRepo.GetAll(opts...) diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index b542dedb4..f26ade8ba 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -264,6 +264,15 @@ func (r *artistRepository) GetAll(options ...model.QueryOptions) (model.Artists, return res, err } +// GetAllIDs returns just the artist IDs for the same row set as GetAll, skipping the +// heavy stats/annotation columns and JSON post-processing. Used by bulk enumeration (artwork backfill). +func (r *artistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + sq := r.applyLibraryFilterToArtistQuery(r.newSelect(options...).Columns("artist.id")).GroupBy("artist.id") + ids := []string{} + err := r.queryAllSlice(sq, &ids) + return ids, err +} + func (r *artistRepository) GetCursor(options ...model.QueryOptions) (model.ArtistCursor, error) { sel := r.selectArtist(options...) cursor, err := queryWithStableResults[dbArtist](r.sqlRepository, sel) diff --git a/persistence/artist_repository_test.go b/persistence/artist_repository_test.go index dc11ede36..43a7f4928 100644 --- a/persistence/artist_repository_test.go +++ b/persistence/artist_repository_test.go @@ -284,6 +284,21 @@ var _ = Describe("ArtistRepository", func() { }) }) + Describe("GetAllIDs", func() { + It("returns the same id set as GetAll", func() { + want, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(want).ToNot(BeEmpty()) + wantIDs := make([]string, 0, len(want)) + for _, a := range want { + wantIDs = append(wantIDs, a.ID) + } + ids, err := repo.GetAllIDs() + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(ConsistOf(wantIDs)) + }) + }) + Describe("Basic Operations", func() { Describe("Count", func() { It("returns the number of artists in the DB", func() { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index c78e0df1e..0abf53d93 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -189,6 +189,16 @@ func (r *playlistRepository) GetAll(options ...model.QueryOptions) (model.Playli return playlists, err } +// GetAllIDs returns just the playlist IDs for the same row set as GetAll (honoring userFilter), +// skipping the owner-name join columns and annotation. Used by bulk enumeration (artwork backfill). +func (r *playlistRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + sq := r.newSelect(options...).Columns("playlist.id"). + Join("user on user.id = owner_id").Where(r.userFilter()) + ids := []string{} + err := r.queryAllSlice(sq, &ids) + return ids, err +} + func (r *playlistRepository) GetCursor(options ...model.QueryOptions) (model.PlaylistCursor, error) { // Same userFilter as GetAll: a cursor must not widen visibility beyond public/owned playlists. sel := r.selectPlaylist(options...).Where(r.userFilter()) diff --git a/persistence/playlist_repository_test.go b/persistence/playlist_repository_test.go index 00d4ff9f2..8dc86d887 100644 --- a/persistence/playlist_repository_test.go +++ b/persistence/playlist_repository_test.go @@ -37,6 +37,21 @@ var _ = Describe("PlaylistRepository", func() { }) }) + Describe("GetAllIDs", func() { + It("returns the same id set as GetAll", func() { + want, err := repo.GetAll() + Expect(err).ToNot(HaveOccurred()) + Expect(want).ToNot(BeEmpty()) + wantIDs := make([]string, 0, len(want)) + for _, p := range want { + wantIDs = append(wantIDs, p.ID) + } + ids, err := repo.GetAllIDs() + Expect(err).ToNot(HaveOccurred()) + Expect(ids).To(ConsistOf(wantIDs)) + }) + }) + Describe("Exists", func() { It("returns true for an existing playlist", func() { Expect(repo.Exists(plsCool.ID)).To(BeTrue()) diff --git a/persistence/radio_repository.go b/persistence/radio_repository.go index 20f82ee36..3ef900920 100644 --- a/persistence/radio_repository.go +++ b/persistence/radio_repository.go @@ -59,6 +59,14 @@ func (r *radioRepository) GetAll(options ...model.QueryOptions) (model.Radios, e return res, err } +// GetAllIDs returns just the radio IDs. Used by bulk enumeration (artwork backfill). +func (r *radioRepository) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + sel := r.newSelect(options...).Columns("id") + ids := []string{} + err := r.queryAllSlice(sel, &ids) + return ids, err +} + func (r *radioRepository) Put(radio *model.Radio, colsToUpdate ...string) error { if !r.isPermitted() { return rest.ErrPermissionDenied diff --git a/persistence/radio_repository_test.go b/persistence/radio_repository_test.go index 1d6cce4f8..4b243c142 100644 --- a/persistence/radio_repository_test.go +++ b/persistence/radio_repository_test.go @@ -78,6 +78,21 @@ var _ = Describe("RadioRepository", func() { }) }) + Describe("GetAllIDs", func() { + It("returns the same id set as GetAll", func() { + want, err := repo.GetAll() + Expect(err).To(BeNil()) + Expect(want).ToNot(BeEmpty()) + wantIDs := make([]string, 0, len(want)) + for _, r := range want { + wantIDs = append(wantIDs, r.ID) + } + ids, err := repo.GetAllIDs() + Expect(err).To(BeNil()) + Expect(ids).To(ConsistOf(wantIDs)) + }) + }) + Describe("Put", func() { It("successfully updates item", func() { err := repo.Put(&model.Radio{ diff --git a/tests/mock_album_repo.go b/tests/mock_album_repo.go index cc7d66b4d..17539255b 100644 --- a/tests/mock_album_repo.go +++ b/tests/mock_album_repo.go @@ -76,6 +76,18 @@ func (m *MockAlbumRepo) GetAll(qo ...model.QueryOptions) (model.Albums, error) { return m.All, nil } +func (m *MockAlbumRepo) GetAllIDs(qo ...model.QueryOptions) ([]string, error) { + all, err := m.GetAll(qo...) + if err != nil { + return nil, err + } + ids := make([]string, len(all)) + for i, a := range all { + ids[i] = a.ID + } + return ids, nil +} + func (m *MockAlbumRepo) GetCursor(qo ...model.QueryOptions) (model.AlbumCursor, error) { res, err := m.GetAll(qo...) if err != nil { diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index e6ea7aea4..4fdd198fe 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -113,6 +113,18 @@ func (m *MockArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, e return allArtists, nil } +func (m *MockArtistRepo) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + all, err := m.GetAll(options...) + if err != nil { + return nil, err + } + ids := make([]string, len(all)) + for i, a := range all { + ids[i] = a.ID + } + return ids, nil +} + func (m *MockArtistRepo) GetCursor(options ...model.QueryOptions) (model.ArtistCursor, error) { res, err := m.GetAll(options...) if err != nil { diff --git a/tests/mock_playlist_repo.go b/tests/mock_playlist_repo.go index 8f8842c8e..bb1fe3379 100644 --- a/tests/mock_playlist_repo.go +++ b/tests/mock_playlist_repo.go @@ -52,6 +52,18 @@ func (m *MockPlaylistRepo) GetAll(options ...model.QueryOptions) (model.Playlist return m.All, nil } +func (m *MockPlaylistRepo) GetAllIDs(options ...model.QueryOptions) ([]string, error) { + all, err := m.GetAll(options...) + if err != nil { + return nil, err + } + ids := make([]string, len(all)) + for i, p := range all { + ids[i] = p.ID + } + return ids, nil +} + func (m *MockPlaylistRepo) GetCursor(options ...model.QueryOptions) (model.PlaylistCursor, error) { res, err := m.GetAll(options...) if err != nil { diff --git a/tests/mock_radio_repository.go b/tests/mock_radio_repository.go index c50a529e5..6d1d45f6e 100644 --- a/tests/mock_radio_repository.go +++ b/tests/mock_radio_repository.go @@ -73,6 +73,18 @@ func (m *MockedRadioRepo) GetAll(qo ...model.QueryOptions) (model.Radios, error) return m.All, nil } +func (m *MockedRadioRepo) GetAllIDs(qo ...model.QueryOptions) ([]string, error) { + all, err := m.GetAll(qo...) + if err != nil { + return nil, err + } + ids := make([]string, len(all)) + for i, r := range all { + ids[i] = r.ID + } + return ids, nil +} + func (m *MockedRadioRepo) Put(radio *model.Radio, _ ...string) error { if m.Err { return errors.New("error")