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

Documentation on Postgres setup #218

Closed
RoGryza opened this issue Mar 14, 2022 · 7 comments
Closed

Documentation on Postgres setup #218

RoGryza opened this issue Mar 14, 2022 · 7 comments

Comments

@RoGryza
Copy link
Contributor

RoGryza commented Mar 14, 2022

It took me a while to figure out I can setup linkding to use other databases than SQLite, the docker installation instructions don't touch on db engine choice and the backup docs section only mentions SQLite as well. I only realized that was even possible when bumping into this comment searching for postgres in the issues, then I went digging in Django's docs.

Would a PR for postgres setup instructions be welcome?
Also, Django's postgres engine uses psycopg2, so I can't use the official images from the project. I'm fine building my own image on top of yours, as it's just a pip install psycopg2, though I think official images (tagged *-postgres or something) would be seen as more trustworthy by other people. I'm happy to help here as well if it's something you're willing to do

Cheers, great project BTW!

@RoGryza
Copy link
Contributor Author

RoGryza commented Mar 15, 2022

Follow-up: it was a bit more involved than just installing psycopg2, postgres doesn't have the GROUP_CONCAT function. I've managed to make it run with the following patch:

diff --git a/bookmarks/queries.py b/bookmarks/queries.py
index 17a7a35..0fa35de 100644
--- a/bookmarks/queries.py
+++ b/bookmarks/queries.py
@@ -1,4 +1,5 @@
 from django.contrib.auth.models import User
+from django.contrib.postgres.aggregates import StringAgg
 from django.db.models import Q, Count, Aggregate, CharField, Value, BooleanField, QuerySet
 
 from bookmarks.models import Bookmark, Tag
@@ -31,7 +32,7 @@ def _base_bookmarks_query(user: User, query_string: str) -> QuerySet:
     # Add aggregated tag info to bookmark instances
     query_set = Bookmark.objects \
         .annotate(tag_count=Count('tags'),
-                  tag_string=Concat('tags__name'),
+                  tag_string=StringAgg('tags__name', delimiter=','),
                   tag_projection=Value(True, BooleanField()))
 
     # Filter for user

I don't know if there's a way to conditionally build queries dependant on the db engine in Django but I can look into this if you want to merge postgres support upstream, else I'll just maintain a fork without SQLite support.

@sissbruecker
Copy link
Owner

Thanks for looking into this. Yeah, this is unfortunately a bit of a bigger effort due to incompatible queries. Regarding supporting Postgres out of the box, it's hard for me to form an opinion on this regarding how much additional effort it would be for me to maintain that, especially since I don't use Postgres myself.

For "official" support I would expect:

  • to provide the driver
  • separate query implementations
  • ideally run test suite for both databases given that there would be different query implementations
    • which then needs infrastructure in CI
  • provide means to conveniently configure the database connection
  • provide documentation

That's already quite a bit of stuff to keep up to date. Maybe checking the alternative, what would be the practicalities of maintaining a fork from your perspective? There's always the option to keep Postgres support in a separate project, and then gauge interest. If there is high demand we can reconsider moving support into this repo.
Regarding separate Docker images, I'm not too concerned about this. We could add a note to the readme to link to the other project.

@RoGryza
Copy link
Contributor Author

RoGryza commented Apr 1, 2022

I wouldn't mind maintaining postgres support here, including all your expectations - though I'd stop if I stop using linkding, so I understand if you're not willing the risk to get stuck with this or having to deprecate it.
Maybe I don't even need to maintain a fork, I could have some package that builds on top and at worst do some nasty monkey patching. I think gauging interest that way is a great idea, I'll get it started and mention here when I have something to show, shouldn't take long

@sissbruecker
Copy link
Owner

I wouldn't mind maintaining postgres support here, including all your expectations - though I'd stop if I stop using linkding, so I understand if you're not willing the risk to get stuck with this or having to deprecate it.

Yeah, I wouldn't really want to be in this situation and keep it so that I can maintain everything myself.

Maybe I don't even need to maintain a fork, I could have some package that builds on top and at worst do some nasty monkey patching.

That would be an OK approach. The worst part is overriding queries.py. For changing the connection settings you can use an existing mechanism that allows you to add a custom settings file on top of the default one. Just create a new python file with the settings that you want to override and copy it to <linkding-root>/siteroot/settings/custom.py.

@sissbruecker
Copy link
Owner

Closing this for now, but feel free to share some results here if you come up with something.

@RoGryza
Copy link
Contributor Author

RoGryza commented Jun 5, 2022

In case anybody ends up in this issue: I'm working in this repo.

I ended up going for a normal fork. This PR has the changes I needed for postgres to work.

@sissbruecker I needed to change importer.py which makes it seem to me that might be a bug that isn't caught in the test suite with SQLite, using postgres the test test_should_show_respective_hints_if_not_all_bookmarks_were_imported_successfully raised an error due to trying to insert a NULL bookmark_id in bookmarks_bookmark_tags:

ERROR:  null value in column "bookmark_id" of relation "bookmarks_bookmark_tags" violates not-null constraint
DETAIL:  Failing row contains (4, null, 4).

@sissbruecker
Copy link
Owner

@RoGryza Thanks, it seems Sqlite silently ignores this error, so it wasn't a problem with the default setup. But it definitely makes sense to prevent this, so I added a commit that prevents associating tags if the bookmark was not imported: b618a8b

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

No branches or pull requests

2 participants