Skip to content

Conversation

brianmay
Copy link
Collaborator

@brianmay brianmay commented May 6, 2025

The database instructions, while technically correct, are not idiot proof. There are so many different ways you can stuff up and end up losing all your data.

For example, the current docs say:

docker compose exec -T database pg_dump -U teslamate teslamate > ./teslamate.bck

If anything goes wrong with this command, it might overwrite any potentially good backup with a 0 byte file. And some of the users won't notice until after they have deleted the current database and tried to restore. By this stage it is too late to recover.

These instructions attempt to make the process more resilient by not deleting the current data until after it is confirmed that the restore is good.

The instructions also use a unique filename for every backup. So you are not overwriting the same backup file every time.

@brianmay brianmay requested a review from Copilot May 6, 2025 22:12
Copy link

netlify bot commented May 6, 2025

Deploy Preview for teslamate ready!

Name Link
🔨 Latest commit 5e73738
🔍 Latest deploy log https://app.netlify.com/projects/teslamate/deploys/6827ad72ed8e220008367b91
😎 Deploy Preview https://deploy-preview-4726--teslamate.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR revises the database upgrade and backup/restore procedures to make them more resilient and prevent data loss during PostgreSQL upgrades.

  • Updated backup commands to generate unique, timestamp-based filenames.
  • Revised instructions to delay deletion of previous volumes until after a successful restore.
  • Modified docker-compose configurations to reference the PostgreSQL 17 image and updated volume names.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
website/docs/maintenance/upgrading_postgres.md Updated upgrade instructions with new backup filename pattern and volume naming changes
website/docs/maintenance/backup_restore.md Revised restore instructions to use timestamp-based filenames and clearer guidance on file usage
website/docs/installation/docker.md Updated docker installation instructions with revised volume name for PostgreSQL 17

@brianmay
Copy link
Collaborator Author

brianmay commented May 6, 2025

I have marked this as draft because I have not tested these changes in any way shape or form.

@brianmay brianmay force-pushed the push-nspqmtsxrnvm branch from b1d5710 to 2dfcfa2 Compare May 6, 2025 22:26
@brianmay brianmay marked this pull request as draft May 6, 2025 22:27
@brianmay brianmay requested a review from Copilot May 6, 2025 22:28
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR revises the database backup and restore documentation to minimize data loss risks and to improve clarity during PostgreSQL upgrades.

  • Updates backup commands to generate unique filenames and prevent overwriting existing backups.
  • Adjusts upgrade instructions to safely switch to PostgreSQL 17 with a new volume configuration.
  • Revises the backup_restore and docker documentation to align with the new safe-restoration workflow.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
website/docs/maintenance/upgrading_postgres.md Updated upgrade steps with enhanced backup and restore instructions.
website/docs/maintenance/backup_restore.md Improved backup file creation and restoration commands.
website/docs/installation/docker.md Adjusted volume mapping to support PostgreSQL 17.

@brianmay
Copy link
Collaborator Author

brianmay commented May 6, 2025

@swiffer
Copy link
Collaborator

swiffer commented May 7, 2025

You may still want to add the command to delete the old volume after verified that the new setup is working?

@DrMichael
Copy link
Collaborator

Perhaps we should make 2. under Upgrade in bold face?

@brianmay
Copy link
Collaborator Author

brianmay commented May 7, 2025

You may still want to add the command to delete the old volume after verified that the new setup is working?

Good question. I deliberately left that out because I thought it might be risky. But it could be desirable all the same.

Perhaps we should make 2. under Upgrade in bold face?

Why? Because it is an important point that needs highlighting maybe?

Also I just noticed most of the step start with upper case letter, but one of the points under Rollback doesn't. Oops.

@DrMichael
Copy link
Collaborator

Perhaps we should make 2. under Upgrade in bold face?
Why? Because it is an important point that needs highlighting maybe?

Genau...

Also I just noticed most of the step start with upper case letter, but one of the points under Rollback doesn't. Oops.

Bummer. :-)

@JakobLichterfeld JakobLichterfeld added the kind:documentation Adds or improves documentation label May 7, 2025
Copy link
Member

@JakobLichterfeld JakobLichterfeld left a comment

Choose a reason for hiding this comment

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

Thank you for your efforts!

I agree that the process has its pitfalls and we can improve it.

From my point of view, we need to find the sweet spot between easy copy-paste and robustness.
Given the number of tickets and other feedback channels, I do not feel that users are left without data.
I also want to remain OS independent.


```bash
docker volume rm "$(basename "$PWD")_teslamate-db"
cp docker-compose.yml docker-compose-16.yml
Copy link
Member

Choose a reason for hiding this comment

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

cp is not OS independent

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any suggestions? Maybe just have text "Copy file docker-compose-16.yml over docker-compose.yml?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think cp should work on all OS we care about though, right?

@JakobLichterfeld
Copy link
Member

I would love to get your perspective on DB backup and restore, @parkr

@brianmay brianmay force-pushed the push-nspqmtsxrnvm branch from 9aa4549 to b8a4eeb Compare May 7, 2025 21:31
@brianmay brianmay force-pushed the push-nspqmtsxrnvm branch 2 times, most recently from 9d58c46 to 2f70008 Compare May 15, 2025 21:52

```bash
docker compose exec -T database pg_dump -U teslamate teslamate > ./teslamate.bck
docker compose exec -T database pg_dump -U teslamate teslamate > teslamate_database_$(date +%F_%H-%M-%S).sql
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about using only a date (2025-05-16 here) - that way copy and paste of commands should still work (as users are most likely updating on the same day).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, not entirely sure what you are asking for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

going with $(date +%Y-%m-%d) and not using %H-%M-%S will allow the restore command to be a copy / paste without users manually searching for the exact filename taken earlier (assuming that most upgrades happen at the same day).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question.

Was trying to avoid the situation here where you might have a good backup, but then you accidentally run the above command again after the database is shutdown/replaced and replace the good backup with a bad backup. And not notice the glaring error message saying it failed.

Yes, would like to think this wouldn't happen, but we did get a report from somebody who had a 0 byte backup file, and this is one way that could occur.

At least with the above, there is some increased chance you might still have the good backup file.


```bash
docker volume rm "$(basename "$PWD")_teslamate-db"
cp docker-compose.yml docker-compose-16.yml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why should users backup the compose file. We could say - in case of error please revert your changes to the compose file to get back to a working version (that should be fine to ask for).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On one hand, I note that there is a comment that some environments may automatically delete any unexpected files, such as this backup. Oops.

On the other hand, not really convinced our users are good at following written instructions... Think spelling it out might be required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

those would also delete the postgres backup, right?

as long as the data is still accessible everything is alright. if someone isn't able to follow a "revert changes made to docker-compose.yaml" file they still can open up a discussion / ask in forums for help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These will delete the updated docker-compose.yml for postgres. The postgres data itself should remain intact.

At least that is the theory. Possibly some platform might decide that since the data is no longer referenced it can be deleted. Hoping that doesn't actually happen...

The database instructions, while technically correct, are not idiot
proof. There are so many different ways you can stuff up and end up
losing all your data.

For example, the current docs say:

docker compose exec -T database pg_dump -U teslamate teslamate > ./teslamate.bck

If anything goes wrong with this command, it might overwrite any
potentially good backup with a 0 byte file. And some of the users won't notice
until after they have deleted the current database and tried to restore. By
this stage it is too late to recover.

These instructions attempt to make the process more resilient by not
deleting the current data until after it is confirmed that the restore
is good.

The instructions also use a unique filename for every backup. So you are
not overwriting the same backup file every time.
@brianmay brianmay force-pushed the push-nspqmtsxrnvm branch from 2f70008 to 5e73738 Compare May 16, 2025 21:26
@swiffer
Copy link
Collaborator

swiffer commented May 18, 2025

Completely different idea:

What about using https://github.com/pgautoupgrade/docker-pgautoupgrade in one shot mode instead for the upgrade (Or as the postgres image)?

@JakobLichterfeld
Copy link
Member

JakobLichterfeld commented May 18, 2025

I do think about including backup and restore script in our repo, which will be used via docker-compose service or as nix idiomatic overlay. Already trying something this morning, including man page.

While setting up my new NixOS server, I stumbled about @scottbot95 s idea here: https://github.com/scottbot95/nixos-config/blob/58fa26c76980a983e4835e0e4cb6ded3194a9e51/machines/teslamate/backup.nix (which is outdated as he uses our flake and the script uses podman).
I then thought about how to implement it not only for nix but for docker compose installs as well. As a script in the container cannot stop other containers, the solution seems to be a docker compose service.

This will eliminate copy and paste errors, improve UX and maintainability without taking responsibility for the user's host.
And our doc will only say: docker compose run --rm backup or on nix teslamate-backup.

Will open an issue and PR for it if you agree.

@brianmay
Copy link
Collaborator Author

What about using https://github.com/pgautoupgrade/docker-pgautoupgrade in one shot mode instead for the upgrade (Or as the postgres image)?

That does seem like an interesting idea. But can you make a full procedure that uses it and is idiot proof?

I then thought about how to implement it not only for nix but for docker compose installs as well. As a script in the container cannot stop other containers, the solution seems to be a docker compose service.

I think these two cases probably will end up being totally different. Unfortunately.

For reference, here is the official nix upgrade procedure: https://nixos.org/manual/nixos/stable/#module-services-postgres-upgrading - we could include such a script in our nixos module. Possibly we might need some additional parameters, not sure. I guess we can assume the user wants to upgrade to the latest postgres in nix stable, but we might need details for the current database.

The Docker case may be complicated because we don't supply a docker-compose file, only give a sample file, so everyone is likely to have differences. In fact that is known to be the case already, some people have the database service called "db", where as the docs say it should be "database".

@JakobLichterfeld
Copy link
Member

I think these two cases probably will end up being totally different. Unfortunately.

Agree, that's why I only want a backup and restore script, maintained in our repo which will be called via docker-compose service or via nix.

some people have the database service called "db", where as the docs say it should be "database".

In ancient versions of the doc, it was called "db".

@JakobLichterfeld
Copy link
Member

While setting up my new NixOS server, I stumbled about @scottbot95 s idea here: https://github.com/scottbot95/nixos-config/blob/58fa26c76980a983e4835e0e4cb6ded3194a9e51/machines/teslamate/backup.nix (which is outdated as he uses our flake and the script uses podman).

just for reference, my working nix solution: https://github.com/JakobLichterfeld/nix-config/blob/98ac655f7473c2dcd28d3431c83ac89bd018f965/homelab/services/teslamate/backup_and_restore.nix

@swiffer
Copy link
Collaborator

swiffer commented May 20, 2025

That does seem like an interesting idea. But can you make a full procedure that uses it and is idiot proof?

it still would be

  1. create a backup (as is)
  2. docker compose down (as is)
  3. upgrade postgres (see below)
  4. modify docker-compose image (as is)
  5. docker compose up -d

current step 3 (deletion of volume) and 5 (restore) would no longer be neccessary

upgrade postgres would be:

docker run --name pgauto -it \
	--mount type=volume,src="$(basename "$PWD")_teslamate-db",dst=/var/lib/postgresql/data \
	-e POSTGRES_PASSWORD=password \
	-e PGAUTO_ONESHOT=yes \
	pgautoupgrade/pgautoupgrade:17-bookworm

@swiffer
Copy link
Collaborator

swiffer commented Jun 7, 2025

The next major upgrade might be different (requiring changes to docker compose) and in addition will open new possibilties (see docs):

https://github.com/docker-library/docs/pull/2582/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:documentation Adds or improves documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants