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

docs: default to sed pg_catalog for Linux, document restore/reset for PG bind mount #9021

Merged
merged 14 commits into from
Apr 22, 2024

Conversation

mmomjian
Copy link
Contributor

@mmomjian mmomjian commented Apr 22, 2024

  • Add sed as default for restore - multiple issues of people trying to restore recently and having geocoding issues or errors.
    • I do not know the equivalent command for Windows, would welcome someone adding it
  • Collapse some code blocks in the FAQ
  • Wrap the library-change instructions in a transaction
  • Update docs to explain how to reset the DB now that Docker volumes are no longer used

@mmomjian mmomjian added the documentation Improvements or additions to documentation label Apr 22, 2024
@danieldietzler
Copy link
Member

Regarding Windows I'd personally be fine with just leaving that out lmao. Alternatively we could also run that in the database container...? Not sure if that comes with sed preinstalled but I'd assume so.

@bo0tzz
Copy link
Member

bo0tzz commented Apr 22, 2024

Why is this pg_catalog line wrong in the first place? Is that something we can fix further up?

@mmomjian mmomjian changed the title docs: default to sed pg_catalog for Linux docs: default to sed pg_catalog for Linux, document restore/reset for PG bind mount Apr 22, 2024
@mmomjian
Copy link
Contributor Author

mmomjian commented Apr 22, 2024

Why is this pg_catalog line wrong in the first place? Is that something we can fix further up?

I think this is an upstream issue, not something we have control over. I could be wrong. link

It seems like there are some workarounds but seems like it require a bigger refactor of the way we implement the extension.

@mertalev

@mmomjian mmomjian marked this pull request as ready for review April 22, 2024 18:52
@mertalev
Copy link
Contributor

It could theoretically be fixed for future users if we changed CREATE EXTENSION cube to CREATE EXTENSION cube SCHEMA pg_catalog in the migration (also for earthdistance). But I'm not sure if this is allowed for non-superusers.

@mmomjian
Copy link
Contributor Author

mmomjian commented Apr 22, 2024

It could theoretically be fixed for future users if we changed CREATE EXTENSION cube to CREATE EXTENSION cube SCHEMA pg_catalog in the migration (also for earthdistance). But I'm not sure if this is allowed for non-superusers.

Immich has superuser in the default deployment, yes? I have no qualms about adding an extra step for people (like myself) who run an external PG instance - we know what we signed up for. My suggestion would be to have some kind of permission check +log message that brings the container down if this extension change is unsuccessful, to alert the admin that a change need to be made.

Adding these extensions already requires SU to begin with and is part of the prep instructions for a non-SU instance.

@mertalev
Copy link
Contributor

Oh right, I forgot that these aren't trusted extensions so they need admin intervention anyway.

I think we can update the instructions to include SCHEMA pg_catalog and change the migration as well. This will solve this issue for new instances without breaking anything for existing ones.

@mmomjian
Copy link
Contributor Author

mmomjian commented Apr 22, 2024

Will installing the extension into pg_catalog cause any issues for users or companies that run multiple instances of Immich in the same DB or alongside other services?

I think an alternate option would be to run the migration and if it fails, simply log an error and expect that the DB admin can manage their own pg_restore with a search path change if needed. The data is all there, this just makes the restore process a little cleaner. Of course if it's no issue to have it in pg_catalog that doesn't hurt either.

Requiring pg_catalog may provide a hindrance to certain deployments that may use managed DBs as well.

@mertalev
Copy link
Contributor

Each database has its own pg_catalog schema, so it shouldn't affect anything else. That being said, there is a risk that it could negatively impact managed deployments with potentially exotic setups.

For your suggestion, we can't just catch an exception and move on if a migration fails. All migrations are in the same transaction that will be aborted if anything throws an error. But it could be handled as a startup check outside of the migrations instead.

@mmomjian
Copy link
Contributor Author

Do you want me to remove the sed changes from this PR and open a new issue instead to track this fix? Let me know what you think is best.

@mertalev
Copy link
Contributor

mertalev commented Apr 22, 2024

The sed command is fine. I think we can add that now and maybe remove it at a later time when the code change is in and most people have updated.

docs/docs/administration/backup-and-restore.md Outdated Show resolved Hide resolved
docs/docs/FAQ.mdx Outdated Show resolved Hide resolved
mmomjian and others added 2 commits April 22, 2024 14:54
Co-authored-by: Mert <101130780+mertalev@users.noreply.github.com>
@mertalev mertalev merged commit a91fd77 into immich-app:main Apr 22, 2024
22 checks passed
@mmomjian mmomjian deleted the update-sed-pg branch April 28, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants