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

safekeeper: remove .partial suffix on the last WAL file. #7882

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

Conversation

arssher
Copy link
Contributor

@arssher arssher commented May 24, 2024

Reasons:

  • it makes pg_waldump usage slightly more cumbersome, forcing to rename file.
  • it makes pull_timeline slightly more cumbersome because at any moment source file can be renamed from partial to full.

Leave ability to read .partial files for backward compatibility.

Fixes #4459

@arssher arssher requested a review from a team as a code owner May 24, 2024 19:28
@arssher arssher requested a review from petuhovskiy May 24, 2024 19:28
@arssher arssher force-pushed the sk-rm-partial branch 2 times, most recently from f7f4bb1 to f06f67c Compare May 24, 2024 19:54
Copy link

github-actions bot commented May 24, 2024

3162 tests run: 3029 passed, 0 failed, 133 skipped (full report)


Flaky tests (4)

Postgres 16

  • test_vm_bit_clear_on_heap_lock: debug

Postgres 15

  • test_pageserver_restarts_under_worload: release
  • test_peer_recovery: debug
  • test_vm_bit_clear_on_heap_lock: debug

Code coverage* (full report)

  • functions: 31.4% (6447 of 20541 functions)
  • lines: 48.3% (49949 of 103356 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
0a43b05 at 2024-05-27T14:11:22.819Z :recycle:

Reasons:
- it makes pg_waldump usage slightly more cumbersome, forcing to rename file.
- it makes pull_timeline slightly more cumbersome because at any
  moment source file can be renamed from partial to full.

Leave ability to read .partial files for backward compatibility.
Copy link
Member

@petuhovskiy petuhovskiy left a comment

Choose a reason for hiding this comment

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

Risky change, it changes on-disk storage format. I support the change, but we need to be very careful when releasing it.

AFAIU it's not forward compatible, because previous version of safekeepers will complain if they'll see unfinished file without .partial prefix, right? It's strange that test_forward_compatibility passes then.

It conflicts with #7887 in a few lines, will look at this PR again after 7887 merges.

@arssher
Copy link
Contributor Author

arssher commented May 31, 2024

AFAIU it's not forward compatible, because previous version of safekeepers will complain if they'll see unfinished file without .partial prefix, right?

truncate_wal does full -> .partial rename, most likely that's the explanation

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.

Remove .partial WAL segments suffixes in safekeeper
2 participants