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

Add docs for the deletion of incremental backups #987

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Conversation

gustabowill
Copy link
Contributor

Adds some explanation in the docs about how dependent Postgres incremental backups are going to be deleted along with their parent as they become useless for recovery in such cases.

References: BAR-251

@gustabowill gustabowill self-assigned this Aug 14, 2024
@gustabowill gustabowill marked this pull request as ready for review August 14, 2024 18:46
@gustabowill gustabowill requested a review from a team as a code owner August 14, 2024 18:46
descendants will also be deleted during this operation, as they become unusable for recovery
having a missing parent in its chain. This logic does not apply to Rsync incremental backups,
which are independent on their own.

Copy link
Contributor

Choose a reason for hiding this comment

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

Check the parts where there are NOTES in this doc file. It would be good to keep standards with notes, like below:

A note with capitalized letters starting with a > symbol and bold.
Example:

NOTE:

Also maybe a reword like the following:
if the specified backup is a postgres backup and has dependent incremental backups, this backup and all their following dependents will be deleted. This happens because a backup without its parent makes the backup chain invalid and a further recovery of any of the dependent backups would not be possible.
This only applies to backups with backup_method="postgres".

My thought is that we should be less wordy and try to use the same words like dependent OR descendant (not both). Also i think that part (i.e., backups taken with --incremental <backup_to_delete>) is not so important and might be confusing.

The reword is just a suggestion, you do not need to follow it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, tbh i don't like this part either "(i.e., backups taken with --incremental <backup_to_delete>)" hahahaha.

I have two points about this:

1 - Do you think the term "postgres backup" is well-understood by the users? I mean, at the end, everything is a postgres backup, right? I've been avoiding this as I think it is mostly used by us to differentiate it from rsync backups but i'm not sure if it is the correct term.

2 - I have the impression that notes are mostly used when you're explaining something "extra", like something that didn't fit in the context but that's still important to mention. In this case, i think explaining this behavior is just a continuation of the existing explanation. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

1 - good point! If this is misleading to users, we might need to use backup_method="postgres" instead of just "postgres backup". Something like this!
2 - Yeah, i guess i saw the Note in the beginning of the sentence and i thought it would be better to put as a big NOTE. But i guess that explanation could be an IMPORTANT flag or NOTE because in fact it is some kind of note or important information.

But of course, we can wait for more suggestion from the others!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll try a better version later today and ping you here to see what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

when adding a NOTE or an IMPORTANT section to the doc, use

**NOTE:**

or

**IMPORTANT:**

Not commenting on the content yet as seems that some changes are going to happen @gustabowill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andremagui @gcalacoci changed it and added it as important note. I'm using the term "block-level incremental backups" instead of "Postgres incremental backups" as I think is clearer and is also what I'm using to explain the --incremental option in the other doc PR.

Copy link
Contributor

@barthisrael barthisrael left a comment

Choose a reason for hiding this comment

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

The PR looks good, I only posted a fix for a typo.

doc/manual/43-backup-commands.en.md Outdated Show resolved Hide resolved
Adds some explanation in the docs about how dependent Postgres
incremental backups are going to be deleted along with their parent as
they become useless for recovery in such cases.

References: BAR-251

Signed-off-by: Gustavo William <[email protected]>
@gustabowill
Copy link
Contributor Author

The PR looks good, I only posted a fix for a typo.

Thanks, just updated the PR.

Copy link
Contributor

@barthisrael barthisrael 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!

Copy link
Contributor

@gcalacoci gcalacoci left a comment

Choose a reason for hiding this comment

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

LGTM

@barthisrael barthisrael merged commit 0e096fb into master Aug 20, 2024
7 of 8 checks passed
@barthisrael barthisrael deleted the dev/bar-251 branch August 20, 2024 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants