Skip to content

snapshot: Ensure all snapshot files are durable#4891

Open
kim wants to merge 2 commits intomasterfrom
kim/snapshot/fsync
Open

snapshot: Ensure all snapshot files are durable#4891
kim wants to merge 2 commits intomasterfrom
kim/snapshot/fsync

Conversation

@kim
Copy link
Copy Markdown
Contributor

@kim kim commented Apr 24, 2026

When creating or compressing a snapshot, fsync all files and directories, so as to ensure that the snapshot is durable on the local disk.

This obviously amounts to a large number of fsync calls, which may negatively impact performance of taking a snapshot -- since we hold a transaction lock while taking a snapshot, this is not to be taken lightly.

Expected complexity level and risk

3 -- performance impact

Testing

I haven't quantified the performance impact.

When creating or compressing a snapshot, `fsync` all files and
directories, so as to ensure that the snapshot is durable on the local
disk.

This obviously amounts to a large number of `fsync` calls, which may
negatively impact performance of taking a snapshot -- since we hold a
transaction lock while taking a snapshot, this is not to be taken
lightly.
@kim kim requested review from gefjon and joshua-spacetime April 24, 2026 11:28
@gefjon
Copy link
Copy Markdown
Contributor

gefjon commented Apr 27, 2026

Is it necessary for replication to fsync before dropping the transaction lock? We were originally treating snapshots as an unreliable optimization, but I gather that may no longer be the case. The reason I ask is to suggest that we might write the files with the transaction lock held, then release it, and only then call fsync.

@kim
Copy link
Copy Markdown
Contributor Author

kim commented Apr 27, 2026

to suggest that we might write the files with the transaction lock held, then release it, and only then call fsync.

Yeah, could try that.

Copy link
Copy Markdown
Contributor

@gefjon gefjon 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. If you'd like to try the version that fsyncs after releasing the lock, it might be worth trying, but if you just wanna merge this version, that's fine by me.

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