mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
perf(persistence): use *_artists join tables for artist participant filters (#5930)
* perf(persistence): use *_artists join tables for artist participant filters The artist_id/artists_id, role_<role>_id and role_total_id filters, plus the AlbumsByArtistID/AlbumsByContributingArtistID/SongsByArtistID helpers, scanned every album or media_file row through json_tree(participants, ...), which no index can serve — the cause of multi-second artist pages on large libraries (discussion #5929). Rewrite them to semi-join the album_artists and media_file_artists tables via a shared ParticipantIDFilter helper. The join tables are written in the same transaction as the participants JSON, so results are unchanged. album_artists' unique constraint led with album_id, so artist-driven lookups had no usable index. Rebuild the table with the constraint reordered to (artist_id, album_id, role, sub_role), mirroring media_file_artists, instead of adding a fourth index: measured within 4% of a dedicated covering index (geomean -92.5% vs json_tree on a 96k-track production copy) while saving ~7MiB and per-scan write amplification. Album-side consumers (participant rewrites, FK cascades, markMissing) keep using album_artists_album_id, and updateParticipants' ON CONFLICT target already names artist_id first. The rebuild is linear work: 1.1s on a 113k-row production copy. * chore(gitignore): add temp benchmark files to ignore list * fix(persistence): clear album_artists when an album is saved without participants albumRepository.Put skipped updateParticipants when the Participants map was empty, so a hypothetical save with no participants would write {} to the JSON column but leave stale album_artists rows behind, now visible through the semi-join filters. No current caller can hit this (albums built by MediaFiles.ToAlbum always have participants), but make Put unconditional anyway, matching mediaFileRepository.Put, so the join table always moves with the JSON. Raised by Codex review on #5930.
This commit is contained in:
parent
944ca3100f
commit
7993fb9158
3
.gitignore
vendored
3
.gitignore
vendored
@ -41,3 +41,6 @@ openspec/
|
||||
go.work*
|
||||
.worktrees/
|
||||
.playwright-mcp/
|
||||
|
||||
# Temp benchmark files
|
||||
zz_*_test.go
|
||||
@ -0,0 +1,59 @@
|
||||
-- +goose Up
|
||||
-- +goose StatementBegin
|
||||
-- Lead the unique constraint with artist_id (mirroring media_file_artists) so artist-driven
|
||||
-- filters and the artist-delete cascade get an index seek; album_id lookups keep album_artists_album_id.
|
||||
CREATE TABLE album_artists_tmp
|
||||
(
|
||||
album_id varchar not null
|
||||
REFERENCES album (id)
|
||||
ON DELETE CASCADE,
|
||||
artist_id varchar not null
|
||||
REFERENCES artist (id)
|
||||
ON DELETE CASCADE,
|
||||
role varchar default '' not null,
|
||||
sub_role varchar default '' not null,
|
||||
CONSTRAINT album_artists
|
||||
UNIQUE (artist_id, album_id, role, sub_role)
|
||||
);
|
||||
|
||||
INSERT INTO album_artists_tmp(album_id, artist_id, role, sub_role)
|
||||
SELECT album_id, artist_id, role, sub_role
|
||||
FROM album_artists;
|
||||
|
||||
DROP TABLE album_artists;
|
||||
ALTER TABLE album_artists_tmp RENAME TO album_artists;
|
||||
|
||||
CREATE INDEX album_artists_album_id
|
||||
ON album_artists (album_id);
|
||||
CREATE INDEX album_artists_role
|
||||
ON album_artists (role);
|
||||
-- +goose StatementEnd
|
||||
|
||||
-- +goose Down
|
||||
-- +goose StatementBegin
|
||||
CREATE TABLE album_artists_tmp
|
||||
(
|
||||
album_id varchar not null
|
||||
REFERENCES album (id)
|
||||
ON DELETE CASCADE,
|
||||
artist_id varchar not null
|
||||
REFERENCES artist (id)
|
||||
ON DELETE CASCADE,
|
||||
role varchar default '' not null,
|
||||
sub_role varchar default '' not null,
|
||||
CONSTRAINT album_artists
|
||||
UNIQUE (album_id, artist_id, role, sub_role)
|
||||
);
|
||||
|
||||
INSERT INTO album_artists_tmp(album_id, artist_id, role, sub_role)
|
||||
SELECT album_id, artist_id, role, sub_role
|
||||
FROM album_artists;
|
||||
|
||||
DROP TABLE album_artists;
|
||||
ALTER TABLE album_artists_tmp RENAME TO album_artists;
|
||||
|
||||
CREATE INDEX album_artists_album_id
|
||||
ON album_artists (album_id);
|
||||
CREATE INDEX album_artists_role
|
||||
ON album_artists (role);
|
||||
-- +goose StatementEnd
|
||||
@ -172,24 +172,22 @@ func yearFilter(_ string, value any) Sqlizer {
|
||||
}
|
||||
|
||||
func artistFilter(_ string, value any) Sqlizer {
|
||||
return Or{
|
||||
Exists("json_tree(participants, '$.albumartist')", Eq{"value": value}),
|
||||
Exists("json_tree(participants, '$.artist')", Eq{"value": value}),
|
||||
}
|
||||
return ParticipantIDFilter("album", value, model.RoleAlbumArtist, model.RoleArtist)
|
||||
}
|
||||
|
||||
func artistRoleFilter(name string, value any) Sqlizer {
|
||||
roleName := strings.TrimSuffix(strings.TrimPrefix(name, "role_"), "_id")
|
||||
|
||||
// Check if the role name is valid. If not, return an invalid filter
|
||||
if _, ok := model.AllRoles[roleName]; !ok {
|
||||
role, ok := model.AllRoles[roleName]
|
||||
if !ok {
|
||||
return Gt{"": nil}
|
||||
}
|
||||
return Exists(fmt.Sprintf("json_tree(participants, '$.%s')", roleName), Eq{"value": value})
|
||||
return ParticipantIDFilter("album", value, role)
|
||||
}
|
||||
|
||||
func allRolesFilter(_ string, value any) Sqlizer {
|
||||
return Like{"participants": fmt.Sprintf(`%%"%s"%%`, value)}
|
||||
return ParticipantIDFilter("album", value)
|
||||
}
|
||||
|
||||
func (r *albumRepository) CountAll(options ...model.QueryOptions) (int64, error) {
|
||||
@ -214,12 +212,7 @@ func (r *albumRepository) Put(al *model.Album) error {
|
||||
return err
|
||||
}
|
||||
al.ID = id
|
||||
if len(al.Participants) > 0 {
|
||||
if err = r.updateParticipants(al.ID, al.Participants); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
return r.updateParticipants(al.ID, al.Participants)
|
||||
}
|
||||
|
||||
// TODO Move external metadata to a separated table
|
||||
|
||||
@ -527,19 +527,16 @@ var _ = Describe("AlbumRepository", func() {
|
||||
|
||||
Describe("artistRoleFilter", func() {
|
||||
DescribeTable("creates correct SQL expressions for artist roles",
|
||||
func(filterName, artistID, expectedSQL string) {
|
||||
func(filterName, artistID, expectedRole string) {
|
||||
sqlizer := artistRoleFilter(filterName, artistID)
|
||||
sql, args, err := sqlizer.ToSql()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(sql).To(Equal(expectedSQL))
|
||||
Expect(args).To(Equal([]any{artistID}))
|
||||
Expect(sql).To(Equal("album.id IN (SELECT album_id FROM album_artists WHERE artist_id = ? AND role IN (?))"))
|
||||
Expect(args).To(Equal([]any{artistID, expectedRole}))
|
||||
},
|
||||
Entry("artist role", "role_artist_id", "123",
|
||||
"exists (select 1 from json_tree(participants, '$.artist') where value = ?)"),
|
||||
Entry("albumartist role", "role_albumartist_id", "456",
|
||||
"exists (select 1 from json_tree(participants, '$.albumartist') where value = ?)"),
|
||||
Entry("composer role", "role_composer_id", "789",
|
||||
"exists (select 1 from json_tree(participants, '$.composer') where value = ?)"),
|
||||
Entry("artist role", "role_artist_id", "123", "artist"),
|
||||
Entry("albumartist role", "role_albumartist_id", "456", "albumartist"),
|
||||
Entry("composer role", "role_composer_id", "789", "composer"),
|
||||
)
|
||||
|
||||
It("works with the actual filter map", func() {
|
||||
@ -553,8 +550,8 @@ var _ = Describe("AlbumRepository", func() {
|
||||
sqlizer := filterFunc(filterName, "test-id")
|
||||
sql, args, err := sqlizer.ToSql()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(sql).To(Equal(fmt.Sprintf("exists (select 1 from json_tree(participants, '$.%s') where value = ?)", roleName)))
|
||||
Expect(args).To(Equal([]any{"test-id"}))
|
||||
Expect(sql).To(Equal("album.id IN (SELECT album_id FROM album_artists WHERE artist_id = ? AND role IN (?))"))
|
||||
Expect(args).To(Equal([]any{"test-id", roleName}))
|
||||
}
|
||||
})
|
||||
|
||||
@ -644,6 +641,74 @@ var _ = Describe("AlbumRepository", func() {
|
||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID}))
|
||||
})
|
||||
|
||||
It("finds albums through the participant-based filters", func() {
|
||||
artist := &model.Artist{ID: "filter-artist-1", Name: "Filter Artist", OrderArtistName: "filter artist"}
|
||||
Expect(createArtistWithLibrary(artistRepo, artist, 1)).To(Succeed())
|
||||
|
||||
album := &model.Album{
|
||||
LibraryID: 1,
|
||||
ID: "filter-album-1",
|
||||
Name: "Filter Album",
|
||||
AlbumArtistID: artist.ID,
|
||||
AlbumArtist: artist.Name,
|
||||
Participants: model.Participants{
|
||||
model.RoleAlbumArtist: {{Artist: model.Artist{ID: artist.ID, Name: artist.Name}}},
|
||||
model.RoleComposer: {{Artist: model.Artist{ID: artist.ID, Name: artist.Name}}},
|
||||
},
|
||||
}
|
||||
Expect(albumRepo.Put(album)).To(Succeed())
|
||||
|
||||
byArtist, err := albumRepo.GetAll(model.QueryOptions{Filters: artistFilter("artist_id", artist.ID)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(byArtist).To(HaveLen(1))
|
||||
Expect(byArtist[0].ID).To(Equal(album.ID))
|
||||
|
||||
byComposer, err := albumRepo.GetAll(model.QueryOptions{Filters: artistRoleFilter("role_composer_id", artist.ID)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(byComposer).To(HaveLen(1))
|
||||
|
||||
byLyricist, err := albumRepo.GetAll(model.QueryOptions{Filters: artistRoleFilter("role_lyricist_id", artist.ID)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(byLyricist).To(BeEmpty())
|
||||
|
||||
byAnyRole, err := albumRepo.GetAll(model.QueryOptions{Filters: allRolesFilter("role_total_id", artist.ID)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(byAnyRole).To(HaveLen(1))
|
||||
|
||||
count, err := albumRepo.CountAll(model.QueryOptions{Filters: artistFilter("artist_id", artist.ID)})
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(count).To(Equal(int64(1)))
|
||||
|
||||
_, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artist.ID}))
|
||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID}))
|
||||
})
|
||||
|
||||
It("clears album_artists rows when saved with empty participants", func() {
|
||||
artist := &model.Artist{ID: "clear-artist-1", Name: "Clear Artist", OrderArtistName: "clear artist"}
|
||||
Expect(createArtistWithLibrary(artistRepo, artist, 1)).To(Succeed())
|
||||
|
||||
album := &model.Album{
|
||||
LibraryID: 1,
|
||||
ID: "clear-album-1",
|
||||
Name: "Clear Album",
|
||||
AlbumArtistID: artist.ID,
|
||||
AlbumArtist: artist.Name,
|
||||
Participants: model.Participants{
|
||||
model.RoleAlbumArtist: {{Artist: model.Artist{ID: artist.ID, Name: artist.Name}}},
|
||||
},
|
||||
}
|
||||
DeferCleanup(func() {
|
||||
_, _ = artistRepo.executeSQL(squirrel.Delete("artist").Where(squirrel.Eq{"id": artist.ID}))
|
||||
_, _ = albumRepo.executeSQL(squirrel.Delete("album").Where(squirrel.Eq{"id": album.ID}))
|
||||
})
|
||||
Expect(albumRepo.Put(album)).To(Succeed())
|
||||
verifyAlbumArtists(album.ID, []albumArtistRecord{{ArtistID: artist.ID, Role: "albumartist", SubRole: ""}})
|
||||
|
||||
album.Participants = model.Participants{}
|
||||
Expect(albumRepo.Put(album)).To(Succeed())
|
||||
verifyAlbumArtists(album.ID, []albumArtistRecord{})
|
||||
})
|
||||
|
||||
It("filters out invalid artist IDs leaving only valid participants in database", func() {
|
||||
// Create two real artists in the database
|
||||
artist1 := &model.Artist{
|
||||
|
||||
@ -113,7 +113,7 @@ var mediaFileFilter = sync.OnceValue(func() map[string]filterFunc {
|
||||
"has_rating": annotationBoolFilter("rating"),
|
||||
"genre_id": tagIDFilter,
|
||||
"missing": booleanFilter,
|
||||
"artists_id": artistFilter,
|
||||
"artists_id": mediaFileArtistFilter,
|
||||
"library_id": libraryIdFilter,
|
||||
"path": startsWithFilter("media_file.path"),
|
||||
}
|
||||
@ -126,6 +126,10 @@ var mediaFileFilter = sync.OnceValue(func() map[string]filterFunc {
|
||||
return filters
|
||||
})
|
||||
|
||||
func mediaFileArtistFilter(_ string, value any) Sqlizer {
|
||||
return ParticipantIDFilter("media_file", value, model.RoleAlbumArtist, model.RoleArtist)
|
||||
}
|
||||
|
||||
func mediaFileRecentlyAddedSort() string {
|
||||
if conf.Server.RecentlyAddedByModTime {
|
||||
return "media_file.updated_at, media_file.id"
|
||||
|
||||
@ -22,6 +22,30 @@ type flatParticipant struct {
|
||||
SubRole string `json:"sub_role,omitempty"`
|
||||
}
|
||||
|
||||
// ParticipantIDFilter matches rows of table where the artist participates in any of the given roles
|
||||
// (any role when empty). Semi-joins <table>_artists; json_tree over the JSON is far slower at scale.
|
||||
func ParticipantIDFilter(table string, artistID any, roles ...model.Role) Sqlizer {
|
||||
return participantIDFilter(table, artistID, false, roles)
|
||||
}
|
||||
|
||||
// NotParticipantIDFilter is the negation of ParticipantIDFilter.
|
||||
func NotParticipantIDFilter(table string, artistID any, roles ...model.Role) Sqlizer {
|
||||
return participantIDFilter(table, artistID, true, roles)
|
||||
}
|
||||
|
||||
func participantIDFilter(table string, artistID any, negate bool, roles []model.Role) Sqlizer {
|
||||
sel := Select(table + "_id").From(table + "_artists").Where(Eq{"artist_id": artistID})
|
||||
if len(roles) > 0 {
|
||||
sel = sel.Where(Eq{"role": slice.Map(roles, func(r model.Role) string { return r.String() })})
|
||||
}
|
||||
sql, args, _ := sel.ToSql()
|
||||
op := " IN ("
|
||||
if negate {
|
||||
op = " NOT IN ("
|
||||
}
|
||||
return Expr(table+".id"+op+sql+")", args...)
|
||||
}
|
||||
|
||||
func marshalParticipants(participants model.Participants) string {
|
||||
dbParticipants := make(map[model.Role][]participant)
|
||||
for role, artists := range participants {
|
||||
|
||||
@ -47,17 +47,13 @@ func AlbumsByArtist() Options {
|
||||
}
|
||||
|
||||
func AlbumsByArtistID(artistId string) Options {
|
||||
filters := []Sqlizer{
|
||||
persistence.Exists("json_tree(participants, '$.albumartist')", Eq{"value": artistId}),
|
||||
}
|
||||
roles := []model.Role{model.RoleAlbumArtist}
|
||||
if conf.Server.Subsonic.ArtistParticipations {
|
||||
filters = append(filters,
|
||||
persistence.Exists("json_tree(participants, '$.artist')", Eq{"value": artistId}),
|
||||
)
|
||||
roles = append(roles, model.RoleArtist)
|
||||
}
|
||||
return addDefaultFilters(Options{
|
||||
Sort: "max_year",
|
||||
Filters: Or(filters),
|
||||
Filters: persistence.ParticipantIDFilter("album", artistId, roles...),
|
||||
})
|
||||
}
|
||||
|
||||
@ -68,8 +64,8 @@ func AlbumsByContributingArtistID(artistId string) Options {
|
||||
return addDefaultFilters(Options{
|
||||
Sort: "max_year",
|
||||
Filters: And{
|
||||
persistence.Exists("json_tree(participants, '$.artist')", Eq{"value": artistId}),
|
||||
persistence.NotExists("json_tree(participants, '$.albumartist')", Eq{"value": artistId}),
|
||||
persistence.ParticipantIDFilter("album", artistId, model.RoleArtist),
|
||||
persistence.NotParticipantIDFilter("album", artistId, model.RoleAlbumArtist),
|
||||
},
|
||||
})
|
||||
}
|
||||
@ -104,13 +100,11 @@ func SongsByAlbum(albumId string) Options {
|
||||
}
|
||||
|
||||
// SongsByArtistID matches media files where the artist participates as album or track artist, in
|
||||
// album order. Semi-joins media_file_artists; scanning the participants JSON is ~10x slower at scale.
|
||||
// album order.
|
||||
func SongsByArtistID(artistId string) Options {
|
||||
return addDefaultFilters(Options{
|
||||
Sort: "album",
|
||||
Filters: Expr(
|
||||
"media_file.id IN (SELECT media_file_id FROM media_file_artists WHERE artist_id = ? AND role IN (?, ?))",
|
||||
artistId, model.RoleArtist.String(), model.RoleAlbumArtist.String()),
|
||||
Sort: "album",
|
||||
Filters: persistence.ParticipantIDFilter("media_file", artistId, model.RoleArtist, model.RoleAlbumArtist),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@ -219,7 +219,7 @@ var _ = Describe("Items", func() {
|
||||
albumRepo := ds.Album(context.Background()).(*tests.MockAlbumRepo)
|
||||
sql, _, err := albumRepo.Options.Filters.ToSql()
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
Expect(sql).To(ContainSubstring("json_tree"))
|
||||
Expect(sql).To(ContainSubstring("album_artists"))
|
||||
})
|
||||
|
||||
It("lists artists when IncludeItemTypes=MusicArtist", func() {
|
||||
@ -708,7 +708,7 @@ var _ = Describe("Items", func() {
|
||||
Expect(w.Code).To(Equal(http.StatusOK))
|
||||
sql, args, err := albumRepo.Options.Filters.ToSql()
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
Expect(sql).NotTo(ContainSubstring("json_tree")) // not treated as an artist-parent filter
|
||||
Expect(sql).NotTo(ContainSubstring("album_artists")) // not treated as an artist-parent filter
|
||||
Expect(sql).To(ContainSubstring("library_id"))
|
||||
Expect(args).To(ContainElement(2))
|
||||
})
|
||||
@ -724,7 +724,7 @@ var _ = Describe("Items", func() {
|
||||
sql, args, err := albumRepo.Options.Filters.ToSql()
|
||||
Expect(err).NotTo(HaveOccurred())
|
||||
// Falls back to treating "99" as an (empty-matching) artist-parent id...
|
||||
Expect(sql).To(ContainSubstring("json_tree"))
|
||||
Expect(sql).To(ContainSubstring("album_artists"))
|
||||
// ...while still scoping to the user's own accessible libraries.
|
||||
Expect(sql).To(ContainSubstring("library_id"))
|
||||
Expect(args).To(ContainElement(1))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user