Skip to content

Conversation

Qstick
Copy link
Member

@Qstick Qstick commented Apr 15, 2023

Database Migration

YES - 188 (Timezone adjustments to DateTime column types)

Description

This adds Postgres Support via config file parameters. Based on Radarr etc.. implementation

Todos

  • PG Tests on TC if wanted
  • Wiki Updates

Copy link
Member

@markus101 markus101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Untested, mostly would just like to see some consistency in the SQL strings.

@mynameisbogdan
Copy link
Contributor

Fixes #5617 too, due to changes to ProcessProvider. :)

@markus101
Copy link
Member

Fixing that shouldn't be contingent on landing this larger, unrelated PR though.

@Qstick
Copy link
Member Author

Qstick commented May 13, 2023

Rebased and comments addressed

@markus101 markus101 force-pushed the pgsql branch 2 times, most recently from ef41cc9 to f78144d Compare May 23, 2023 03:37
@Qstick Qstick changed the title PostgresSQL Support PostgreSQL Support Jun 12, 2023
@buroa

This comment was marked as off-topic.

@martimarkov
Copy link

Is there anything blocking this PR going into the develop branch?

@Emalton
Copy link

Emalton commented Jul 2, 2023

This would be a great addition for those of us using Kubernetes!

@politeauthority
Copy link

politeauthority commented Jul 2, 2023

I can't contribute here as it's out of my knowledge space. However I'm working on k8s implementations of the servarr stack, and this contribution is paramount (to my current knowledge) to making this work.
Excited to see this development!

@onedr0p
Copy link
Contributor

onedr0p commented Jul 3, 2023

@politeauthority As someone who's been using Kubernetes for the past 5 years and knows it pretty well, I don't see how this feature would be paramount to get it working in Kubernetes. Sonarr & SQLite works just fine with block storage like rook-ceph, openebs or longhorn and has been working fine like that for years for me.

This is just a nice to have in my opinion because then you can treat the configuration volume as ephemeral to a point. If the config volume is lost and you have the config.xml mounted from a secret (or do envsubt across it) you can just have Sonarr pull down the metadata again. It's not like you can scale Sonarr if this feature lands, it's a monolithic app.

@patrickjmcd
Copy link

@onedr0p humbly, I disagree. rook, ceph, and longhorn are all excellent products, but are a jackhammer when looking for a screwdriver.

Many of us use a nfs provider for config for many services, but when services depend heavily on any database, it's really nice to be able to build (and manage) a Postgres database and have your applications use it. Having an advanced storage system (a la ceph, rook, etc) is an advanced k8s concept where many people are following tutorials that lead them to even more simplistic NFS connections.

I totally understand the position of "we built this to use SQLite and the community offers lots of options for HA even with SQLite", but the real HA truly is a PostgresDB-compliant interface where that db can be self-hosted as well.

I hope this isn't coming off as attacking or rude, but as someone who's in the "more advanced at k8s" crew, but looking for the simplest solution for everything, I think that this is super valuable to the community.

@patrickjmcd
Copy link

And not to pile on more, but @Qstick made it totally optional to take this path, but made it available to anyone who wants to use it. What's the harm in that?

@onedr0p
Copy link
Contributor

onedr0p commented Jul 3, 2023

I never said it wasn't valuable and in fact I already use Postgres for Radarr and Prowlarr. I would use it since I already have cloudnative-pg up and running in my cluster. I just don't think there's a need to keep commenting on this PR asking for a status or to show some sort of urgency that it's needed now.

@patrickjmcd
Copy link

Ah ok, I'm sorry @onedr0p! Just trying to put my $0.02 behind this PR

Copy link
Member

@markus101 markus101 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the fixes and CI is happy (and ready to test future builds). Do we need to do anything else before landing?

@Qstick
Copy link
Member Author

Qstick commented Jul 3, 2023

Yea, let me rebase on latest just to ensure there are no lingering queries or migrations that need to be fixed.

@varac
Copy link

varac commented Jul 13, 2023

@Qstick Please rebase and push, would be great to have this merged soon. Thanks for this PR!

@Qstick
Copy link
Member Author

Qstick commented Jul 13, 2023

Please don't comment with spam that adds no value to the PR.

@Qstick
Copy link
Member Author

Qstick commented Jul 15, 2023

There's an issue here with Episode Missing Search and Episode Cutoff unmet search that results in an ambiguous order by column (Id) error when making the call to PG, SQLite just seems to make some assumptions and go with it. This needs sorting before this goes in.

@kharenis
Copy link
Contributor

kharenis commented Jul 26, 2023

@Qstick Hi, I've created a PR that resolves the Episode Missing Search and Episode Cutoff unmet search issues.

@nwithan8
Copy link

For those that aren't super technical, can we get a 30,000-foot overview of the performance improvements we can expect with this change to the backend database? Radarr already uses Postgres and I'm unable to immediately tell a difference between Sonarr and Radarr performance...

@VonLatvala
Copy link

For those that aren't super technical, can we get a 30,000-foot overview of the performance improvements we can expect with this change to the backend database? Radarr already uses Postgres and I'm unable to immediately tell a difference between Sonarr and Radarr performance...

For example, running this using sqlite on a container host having its PVCs (persistent volumes) mounted over NFS, the database is locked to an almost unusable level. Using postgres over the network works like a breeze. This is just from on the top of my head, someone else might have more rigorous numbers. Cheers!

@Qstick
Copy link
Member Author

Qstick commented Jul 30, 2023

@Qstick Hi, I've created a PR that resolves the Episode Missing Search and Episode Cutoff unmet search issues.

The table we are querying is not necessarily the sort key table... thus this fix causes other issues.

@Qstick
Copy link
Member Author

Qstick commented Aug 11, 2023

@markus101 Believe we are ready, good with this pushing in after a squash now?

@Qstick
Copy link
Member Author

Qstick commented Aug 11, 2023

For those that aren't super technical, can we get a 30,000-foot overview of the performance improvements we can expect with this change to the backend database? Radarr already uses Postgres and I'm unable to immediately tell a difference between Sonarr and Radarr performance...

99% of Sonarr or Radarr datasets are not what most people would consider huge datasets. Postgres gains a tad and obviously has other advantages, but it’s no magic speed bullet. How we query and how those queries are setup have much more impact to speed, and we’ve done a lot of work around that in Sonarr v4. Radarr uses Sqlite by default, you have to manually configure postgres via your config file in either app.

@markus101
Copy link
Member

@Qstick looks like CI is failing on a number of tests due to migration 192.

@Qstick
Copy link
Member Author

Qstick commented Aug 12, 2023

Thanks, I looked at the CI a bit to quick, it hadn't finished last build.

I'll fix

@Qstick
Copy link
Member Author

Qstick commented Aug 12, 2023

Should be better now, Although I don't like that fluentmigrator doesn't handle that PG column alter cleanly

@markus101
Copy link
Member

Yeah, that's annoying, looks good now though, squash it up and ship it!

RobinDadswell and others added 2 commits August 12, 2023 00:43
Co-Authored-By: Qstick <376117+Qstick@users.noreply.github.com>
Co-Authored-By: Richard <1252123+kharenis@users.noreply.github.com>
@Qstick Qstick merged commit c57ceac into develop Aug 12, 2023
@Qstick Qstick deleted the pgsql branch August 12, 2023 05:45
@martimarkov
Copy link

I'm sorry for the spam but amazing job!!! I've been waiting for this feature for years now!!!

Thanks @Qstick ❤️❤️❤️

@buroa
Copy link

buroa commented Aug 12, 2023

Thanks so much for this @Qstick; buroa/k8s-gitops@005efd5 :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.