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

Fix compression in pg_dump and make it optional #132

Closed
wants to merge 2 commits into from

Conversation

JohnTheNerd
Copy link

@JohnTheNerd JohnTheNerd commented Apr 10, 2023

  • The pg_dump call never actually compressed the data, only pg_dumpall did. This PR fixes that.

  • Made compression optional, as there are a few reasons to not want to compress the backups (such as external backup solutions implementing deduplication for you). This is done via an environment variable named COMPRESS_BACKUPS which defaults to compress, and is documented in the README.

- The `pg_dump` call never actually compressed the data, only `pg_dumpall` did. This commit fixes that.

- Made compression optional, as there are a few to not want to compress the backups.
@prodrigestivill
Copy link
Owner

prodrigestivill commented Apr 11, 2023

Thanks for your time.
It looks as the tests fails when performing backups into separated files (directories).

Also I want to add that currently pg_dump backups can be uncompressed by changing the POSTGRESS_EXTRA_OPTS, the default options are so that pg_dump is the one performing the actual compression. But it is true that there is no option to disable it with pg_dumpall because in that case the compression is performed externally with gzip.

As a note, I would like to link this PR to #33 and #45, but not sure how would it affect them.

@JohnTheNerd
Copy link
Author

I see, I think what tripped me up is that I enabled POSTGRES_CLUSTER and never removed the -Z6 from POSTGRES_EXTRA_OPTS, which made the script fail. Then I removed that option without re-adding the compress option, which of course created a .sql.gz file that was not gzipped, making me believe it is broken.

Nevertheless, I feel like separating out compression from POSTGRES_EXTRA_OPTS could be good because:

  • It would allow uncompressed pg_dumpall backups.
  • It would allow setting the compression level on pg_dumpall backups.
  • I can see others being confused by something similar, but admittedly this is very subjective. Seeing it's documented right in the README, maybe that is more about my lack of attention than anyone else's confusion 😄

Would you like me to change the PR accordingly?

I don't know why the tests fail, I will look at that, but if I'm committing changes anyway I may aswell do that first.

@fritz-net
Copy link

It would be nice to also enable usage of other compression like tar to match pgadmin?
So maybe the env var should not just enable it but also include the used command?

COMPRESS_BACKUPS="" does nothing
COMPRESS_BACKUPS="gzip" as its currently
COMPRESS_BACKUPS="tar cvzf" creates .tar
COMPRESS_BACKUPS="zip -r" creates .zip

@prodrigestivill
Copy link
Owner

@fritz-net tar backups are already supported.
zip backups aren't currently, if you really requiere zip backups please open a new issue.

@fritz-net
Copy link

@prodrigestivill I did not find anything about the tar thing yet. :( please could u give me a hint? search did only show me that tar is linked within debian docker

As far is I am aware the following pipe to gzip would mess it up anyway?

pg_dumpall -l "${DB}" ${POSTGRES_EXTRA_OPTS} | gzip > "${FILE}"

zip was just an example

@prodrigestivill
Copy link
Owner

@fritz-net for gzipped tarball you should not use POSTGRES_CLUSTER mode please refer to #138 from Q/A, if this don't solve your case, please respond there or open another discussion in Q/A.

@JohnTheNerd I close this PR to avoid further noise. I open an issue #139 with your request and I will try to implement it whenever I have time. Thanks for all the effort.

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.

3 participants