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.

Sign up for GitHub

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

Jump to bottom

WIP wal-restore #977

Merged
merged 20 commits into from Feb 4, 2022
Merged

WIP wal-restore #977

merged 20 commits into from Feb 4, 2022

Conversation

Xaspy
Copy link
Contributor

@Xaspy Xaspy commented May 15, 2021 •
edited

That what I think:

  1. I'm scary what I don't handle this case: we need wal segments that names already exists in local wal directory and need to rewrite them (Can this happen?). Because in current logic if we have wal segment with some name we can't download this wal file from external storage.

  2. This right that I will download missed segments instantly in local wal directory? I think this dangerous :)

  3. And I want to know how in one directory named 'internal' we have two packages: 'internal' and 'internal_test'.

  4. And yes, my code should be covered by tests. But before this I want know what I have right architecture of the utility.

UPD:

  1. I think I should add checks that two clusters have one parent: check history wal files and then, if I find difference get all wal segments on external storage after this difference.

UPD2:

  1. Should I do some checks which performs in pg_rewind?

@Xaspy
* Add wal-restore command
ecb54c8 
* First version of finding missing wal segments logic
@x4m
Copy link
Collaborator

x4m commented May 16, 2021 •
edited

Hi! Many WAL-G developers, like me and you, speak the Russian language. But we do not want knowledge of Russian to be a prerequisite to participate in WAL-G discussions. In fact, the opinion of every community member matters. Please, use English in Github discussions.

@Xaspy
Copy link
Contributor Author

Xaspy commented May 19, 2021

Hello! I updated my first comment.

@Xaspy
update logic:
8a28009 
* firstly we find common ancestor

* then gets all WAL segments after this common ancestor segment

* and download founded files
@Xaspy
Copy link
Contributor Author

Xaspy commented May 19, 2021

And question:

  1. Should I delete existing files with the same name with missed files? I think yes, because these files can be not for external cluster. But if user later will don't pg_rewind -> existing WAL files will be in inconsistent state.

@usernamedt
Copy link
Member

usernamedt commented May 24, 2021 •
edited

That what I think:

  1. I'm scary what I don't handle this case: we need wal segments that names already exists in local wal directory and need to rewrite them (Can this happen?). Because in current logic if we have wal segment with some name we can't download this wal file from external storage.

I think that we just can download the WAL files that are missing on the pg_rewind target up to the segment number of divergence.

  1. This right that I will download missed segments instantly in local wal directory? I think this dangerous :)

If you don't overwrite any existing files, why not? Also, if you do overwrite some WAL files, I don't think this is dangerous since we overwrite them with the version from storage. And I think we should prefer the storage WAL files to the local ones.

  1. And I want to know how in one directory named 'internal' we have two packages: 'internal' and 'internal_test'.

*_test packages contain tests, for example in internal_test we have tests for the internal package.

  1. I think I should add checks that two clusters have one parent: check history wal files and then, if I find difference get all wal segments on external storage after this difference.

I think comparing the system identifier of the clusters should be enough

@Xaspy Xaspy requested a review from a team as a code owner July 27, 2021 18:02
@usernamedt usernamedt self-requested a review July 28, 2021 09:46
usernamedt
usernamedt requested changes Aug 18, 2021
View reviewed changes
cmd/pg/wal_restore.go Outdated Show resolved Hide resolved
usernamedt
usernamedt requested changes Oct 5, 2021
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
docker/pg_tests/scripts/tests/restore_test.sh Outdated Show resolved Hide resolved
docker/pg_tests/scripts/tests/restore_test.sh Outdated Show resolved Hide resolved
internal/databases/postgres/wal_restore_handler.go Outdated Show resolved Hide resolved
Xaspy and others added 4 commits October 24, 2021 19:54
@Xaspy
#wal-restore:
a8af894 
* undo changing from target to source
* fixed docker test
* renamed docker test into docker-compose.yml to less confusing
@Xaspy
wal-restore:
7fce2a4 
* refactoring
@Xaspy
Merge branch 'master' into wal-restore
e788c4a
@Xaspy
wal-restore:
0a78fa9 
* fix problem after merge
@Xaspy Xaspy requested a review from usernamedt February 1, 2022 18:54
usernamedt
usernamedt approved these changes Feb 4, 2022
View reviewed changes
Copy link
Member

@usernamedt usernamedt left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

One more thing: Can you please add some information regarding this feature to the documentation?

@Xaspy
wal-restore:
6817468 
* fixed pg control data bug with size
* added unittests for this case
* added description about wal-restore in docs/PostgreSQL.md
* fixed description of wal-restore
@Xaspy
Copy link
Contributor Author

Xaspy commented Feb 4, 2022

I think we can close #850.

@usernamedt usernamedt merged commit 96e9375 into wal-g:master Feb 4, 2022
@usernamedt usernamedt mentioned this pull request Feb 4, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@usernamedt usernamedt usernamedt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Xaspy @x4m @usernamedt