From e510f3460426fc8d7e913d92b1f935ac1d5ee8e3 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 26 Sep 2026 21:08:02 -0400 Subject: [PATCH] perf(playlists): evaluate smart playlist criteria before taking the write lock Smart playlists were refreshed with a single INSERT ... SELECT, so SQLite held the write lock while the criteria query ran. On a Raspberry Pi 4 with 300k songs and a rule matching 200k of them, that blocked every other writer for the whole 6 to 8 s evaluation, pushing the worst writer wait past the 15 s busy timeout at the end of a scan. The criteria query now runs first as a plain read, which WAL lets run alongside writers. The matching ids are then written in one short transaction: delete the old tracks, insert the new ones in order through json_each, and update the counters and evaluated_at. On the same Pi the evaluation no longer raises the worst writer wait above the scan's own baseline; the write itself holds the lock for about 3 s instead of the full evaluation. This also applies when a playlist is refreshed on read, and the scanner no longer wraps Evaluate in a transaction, so the read runs outside any lock. --- persistence/playlist_repository.go | 11 ++++ persistence/smart_playlist_repository.go | 73 ++++++++++++++---------- scanner/scanner.go | 5 +- 3 files changed, 55 insertions(+), 34 deletions(-) diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 41b75266c..47e43236e 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -260,6 +260,17 @@ func (r *playlistRepository) selectPlaylist(ctx context.Context, options ...mode return r.withAnnotation(ctx, sel, r.tableName+".id") } +// inTx runs fn in a transaction, joining the caller's if one is already open. +func (r *playlistRepository) inTx(fn func(tx *playlistRepository) error) error { + conn, ok := r.db.(*dbx.DB) + if !ok { + return fn(r) + } + return conn.Transactional(func(tx *dbx.Tx) error { + return fn(NewPlaylistRepository(tx).(*playlistRepository)) + }) +} + func (r *playlistRepository) updateTracks(ctx context.Context, id string, tracks model.MediaFiles) error { ids := make([]string, len(tracks)) for i := range tracks { diff --git a/persistence/smart_playlist_repository.go b/persistence/smart_playlist_repository.go index 602533442..fc78dcc50 100644 --- a/persistence/smart_playlist_repository.go +++ b/persistence/smart_playlist_repository.go @@ -2,6 +2,8 @@ package persistence import ( "context" + "encoding/json" + "errors" "fmt" "slices" "time" @@ -57,12 +59,6 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * log.Debug(ctx, "Refreshing smart playlist", "playlist", pls.Name, "id", pls.ID) start := time.Now() - del := Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID}) - if _, err := r.executeSQL(ctx, del); err != nil { - log.Error(ctx, "Error deleting old smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) - return false - } - rulesSQL := newSmartPlaylistCriteria(*pls.NormalizedRules(), withSmartPlaylistOwner(*usr)) if !r.refreshChildPlaylists(ctx, pls, rulesSQL, visited) { @@ -73,37 +69,56 @@ func (r *playlistRepository) refreshSmartPlaylistTree(ctx context.Context, pls * return false } - sq := r.buildSmartPlaylistQuery(ctx, pls, rulesSQL, usr.ID) - sq, err := r.addCriteria(sq, rulesSQL) + sq, err := r.addCriteria(r.buildSmartPlaylistQuery(ctx, rulesSQL, usr.ID), rulesSQL) if err != nil { log.Error(ctx, "Error building smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) return false } - insSql := Insert("playlist_tracks").Columns("id", "playlist_id", "media_file_id").Select(sq) - if _, err = r.executeSQL(ctx, insSql); err != nil { + // Evaluate the criteria before writing, so the write lock is only held for the short replace below + var ids []string + if err = r.queryAllSlice(ctx, sq, &ids); err != nil && !errors.Is(err, model.ErrNotFound) { + log.Error(ctx, "Error evaluating smart playlist criteria", "playlist", pls.Name, "id", pls.ID, err) + return false + } + + err = r.inTx(func(tx *playlistRepository) error { return tx.replaceSmartPlaylistTracks(ctx, pls, ids) }) + if err != nil { log.Error(ctx, "Error refreshing smart playlist tracks", "playlist", pls.Name, "id", pls.ID, err) return false } - if err = r.refreshCounters(ctx, pls); err != nil { - log.Error(ctx, "Error updating smart playlist stats", "playlist", pls.Name, "id", pls.ID, err) - return false - } - - // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree - now := pls.UpdatedAt - updSql := Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID}) - if _, err = r.executeSQL(ctx, updSql); err != nil { - log.Error(ctx, "Error updating smart playlist", "playlist", pls.Name, "id", pls.ID, err) - return false - } - pls.EvaluatedAt = &now - log.Debug(ctx, "Refreshed playlist", "playlist", pls.Name, "id", pls.ID, "numTracks", pls.SongCount, "elapsed", time.Since(start)) return true } +func (r *playlistRepository) replaceSmartPlaylistTracks(ctx context.Context, pls *model.Playlist, ids []string) error { + if _, err := r.executeSQL(ctx, Delete("playlist_tracks").Where(Eq{"playlist_id": pls.ID})); err != nil { + return err + } + if len(ids) > 0 { + idsJSON, err := json.Marshal(ids) + if err != nil { + return err + } + ins := Expr("INSERT INTO playlist_tracks (id, playlist_id, media_file_id) SELECT key + 1, ?, value FROM json_each(?)", + pls.ID, string(idsJSON)) + if _, err = r.executeSQL(ctx, ins); err != nil { + return err + } + } + if err := r.refreshCounters(ctx, pls); err != nil { + return err + } + // Reuse the stamp refreshCounters just wrote, so evaluated_at and updated_at agree + now := pls.UpdatedAt + if _, err := r.executeSQL(ctx, Update(r.tableName).Set("evaluated_at", now).Where(Eq{"id": pls.ID})); err != nil { + return err + } + pls.EvaluatedAt = &now + return nil +} + // shouldRefreshSmartPlaylist determines if a smart playlist needs to be refreshed based on its type, last evaluated // time, and ownership. func (r *playlistRepository) shouldRefreshSmartPlaylist(ctx context.Context, pls *model.Playlist, usr *model.User) bool { @@ -194,12 +209,10 @@ func (r *playlistRepository) resolvePercentageLimit(ctx context.Context, pls *mo return nil } -// buildSmartPlaylistQuery constructs the SQL query to select media files matching the smart playlist criteria, -// including the joins its fields require and library filtering. -func (r *playlistRepository) buildSmartPlaylistQuery(ctx context.Context, pls *model.Playlist, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { - orderBy := rulesSQL.orderBy() - sq := Select("row_number() over (order by "+orderBy+") as id", "'"+pls.ID+"' as playlist_id", "media_file.id as media_file_id"). - From("media_file") +// buildSmartPlaylistQuery constructs the SQL query to select the ids of media files matching the smart playlist +// criteria, including the joins its fields require and library filtering. +func (r *playlistRepository) buildSmartPlaylistQuery(ctx context.Context, rulesSQL smartPlaylistCriteria, userID string) SelectBuilder { + sq := Select("media_file.id").From("media_file") sq = rulesSQL.applyRequiredJoins(sq, userID) sq = r.applyLibraryFilter(ctx, sq, "media_file") return sq diff --git a/scanner/scanner.go b/scanner/scanner.go index 02a66dff8..fc1556b65 100644 --- a/scanner/scanner.go +++ b/scanner/scanner.go @@ -356,10 +356,7 @@ func (s *scannerImpl) runEvaluateSmartPlaylists(ctx context.Context, state *scan return func() error { for _, id := range state.smartPlaylistsToEvaluate() { start := time.Now() - err := s.ds.WithTxRetry(ctx, func(ctx context.Context, tx model.DataStore) error { - return tx.Playlist().Evaluate(ctx, id) - }, "scanner: evaluate smart playlist") - if err != nil { + if err := s.ds.Playlist().Evaluate(ctx, id); err != nil { log.Warn(ctx, "Scanner: Could not evaluate smart playlist", "id", id, err) continue }