Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(persistence): Postgres support - 2820 #2821

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

jgkawell
Copy link

Closes: #2820

Description:

Adds support to optionally use postgres as the database instead of sqlite. This change is designed to be backwards compatible. If you want to run navidrome just as before, no changes are required as the defaults will run sqlite. If you wish to use postgres, simply set the DBPATH config value to the URI of your postgres instance and navidrome will adjust to using the postgres (pgx) driver instead of sqlite.

The changes to types and functions with the db and persistence packages shouldn't negatively affect any existing functionality built on sqlite. I tried to swap only for more generic functions and types so that things are one-to-one. Where I couldn't get away with that, I split functionality using switches on the database driver so that backwards compatibility was preserved. On the subject of changed data types: these changes are only in the migrations and won't affect anyone who has run these migrations in the past. New users will get the new types but shouldn't have any knowledge that they changed at all.

I've been running this change for the past month (using the postgres driver) and haven't run into any issues and when running navidrome locally using sqlite I don't see any obvious issue there either. This is a fairly large change though so please test it out yourself and suggest any way to improve it. I'm happy to do further testing or make future changes.

Changes:

  • Adjusted the SQL within the db and persistence packages to be generic to work with both sqlite and postgres (pgx) drivers. This entailed some type changes (e.g. datetime -> timestamp, string -> text) as well as changes to some functions (e.g. group_concat -> string_agg). Where simple swaps for more generic functions wouldn't work, I added switch statements to split the functionality between the two drivers.
  • Updated the configuration behavior to check the DBPATH config value and sets the driver based off of it.

Related issues:

Copy link

github-actions bot commented Jan 27, 2024

Download the artifacts for this pull request:

@jgkawell
Copy link
Author

jgkawell commented Mar 2, 2024

@deluan Are you at all interested in getting this functionality into Navidrome? I would understand if not as it's a big change that would entail extra effort to maintain. I'm asking because if you're not interested in incorporating this change I'll stop keeping this PR up to date and just maintain my own fork with these changes going forward.

@deluan
Copy link
Member

deluan commented Mar 2, 2024

Hey @jgkawell, sorry for the late reply. Thanks for working on this. Yes, this is something I want to add support for, some point in the future.

To properly support multiples DBs, we need to write integration tests to validate the results of all/most queries in the persistence layer, to be sure they behave the same with all supported DBs. This, and maintaining multiple sets of DB migrations are the hardest part.

I'm currently working on the new scanner. Once it is done, there will be multiple changes to the DB schema. I'd like to solve those issues I mentioned above, before adding this support in Navidrome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants