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

Avoid hidden failures when dumping #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented Dec 31, 2024

I recently ran into an issue where we were getting silent failures despite pg_dump failing. In particular had a client/server mismatch (client had a version of 16, with the server being on 17) - resulting in us creating empty dumps on S3.

This PR sets pipefail when dumping to ensure capture failures in pg_dump. Currently none of the integration tests interact with S3, but if we want something I'm happy to add something mocking out the storage layer to simulate piping even when dumping locally.

@max-muoto max-muoto force-pushed the capture-any-non-zero-exit-code branch 2 times, most recently from 6a82b05 to 43c5008 Compare December 31, 2024 22:09
@max-muoto max-muoto marked this pull request as ready for review December 31, 2024 22:10
@max-muoto max-muoto marked this pull request as draft December 31, 2024 22:15
pgclone/run.py Outdated
@@ -8,11 +8,24 @@
from pgclone import exceptions, logging


def shell(cmd, ignore_errors=False, env=None):
def _is_pipefail_supported():
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set -o pipefail is supported in most POSSIX shell implementations at this point, it was added to the spec in 2022 - but the user could still be missing it

@max-muoto max-muoto changed the title Capture any exit code when dumping Avoid hidden failures when dumping Dec 31, 2024
@max-muoto max-muoto marked this pull request as ready for review December 31, 2024 23:06
@max-muoto max-muoto force-pushed the capture-any-non-zero-exit-code branch from 39b00f6 to 275923b Compare January 1, 2025 21:23
def _is_pipefail_supported() -> bool:
"""Check if the current shell supports pipefail."""
if sys.platform == "win32":
return False
Copy link
Member

Choose a reason for hiding this comment

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

you may need a pragma: no cover here for the coverage check

@wesleykendall
Copy link
Member

@max-muoto See this Stack Overflow thread for what I am talking about. pg_restore will return non-zero exit codes even though the restore is technically successful.

Any idea if this may be a problem for pg_dump too? If you can double check before I deploy this, that'd be nice. I think this is only a pg_restore issue and one of the reasons I didn't check the exit codes for it

@max-muoto
Copy link
Contributor Author

@max-muoto See this Stack Overflow thread for what I am talking about. pg_restore will return non-zero exit codes even though the restore is technically successful.

Any idea if this may be a problem for pg_dump too? If you can double check before I deploy this, that'd be nice. I think this is only a pg_restore issue and one of the reasons I didn't check the exit codes for it

Thanks for the callout, let me double check and get back to you here!

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.

2 participants