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: wrong regex for wal retention #1026

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nickmansrob
Copy link

@nickmansrob nickmansrob commented Sep 16, 2024

Fixes #1015

Credits to @andrewfung729 for providing the fix.

Created a PR out of this because I also encountered this issue and needed a fix relatively quickly.

For the maintainers, could you give an estimate on when spilo gets released and when the postgres operator is also bumped? If this could take a while, I may create my own monkey patch until this is released.

Thanks!

@nickmansrob
Copy link
Author

I also was wondering why the script does not use following command: wal-g delete retain FULL $DAYS_TO_RETAIN
This is probably an easier way instead of calculating the $BEFORE variable

Copy link

@emrahbecer emrahbecer left a comment

Choose a reason for hiding this comment

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

The new version of wal-g uses the header name "backup_name" instead of "name". The regex script should be modified to adapt to this

@bo0ts
Copy link

bo0ts commented Oct 21, 2024

Isn't this going to break wal-e because it still prints name in version 1.1.1?

@nickmansrob
Copy link
Author

nickmansrob commented Oct 26, 2024

@bo0ts @emrah83 I updated my regex to be backwards compatible with wal-e. The regex script already has been updated, I think, given that you are referring to this script.

@fgalind1
Copy link

fgalind1 commented Nov 9, 2024

any chance this would get approved and merged, we're facing also the same issue and tested that this fixes the issue

@Yingrjimsch
Copy link

If this is already touched, can we discuss if there is an option to have BACKUP_NUM_TO_RETAIN really be the number of backups and not the DAYS_TO_RETAIN this is confusing if I have multiple backups a day and then there are still more backups available then set by me via BACKUP_NUM_TO_RETAIN

@nickmansrob
Copy link
Author

@Yingrjimsch I'd advise you to create a separate issue for this :)

@nickmansrob
Copy link
Author

I'd like to see this merged asap, because I think it will take some time to get the version of spilo bumped on the operator, might want to use a custom build for the time being.

Copy link

@emrahbecer emrahbecer left a comment

Choose a reason for hiding this comment

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

Looks good to me.
This change has better be merged asap because we cannot tolerate backup files to accumulate infinitely.

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.

wal-g backup retention feature not working
6 participants