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

perf(postgres)!: set fsync=off by default #276

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

luca-iachini
Copy link

This PR updates the Postgres container to set fsync=off.
Disabling fsync enhances test performance by minimizing disk I/O, as data durability guarantees are typically unnecessary during tests.

Copy link
Contributor

@DDtKey DDtKey left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution 🙏

I don't mind at all, just one suggestion to preserve compatibility and to make it explicit

src/postgres/mod.rs Outdated Show resolved Hide resolved
@DDtKey
Copy link
Contributor

DDtKey commented Jan 23, 2025

Btw, don't worry about failed gitea tests - they're flaky right now. It's known issue, but not a blocker for your PR

@DDtKey DDtKey changed the title chore(postgres): set fsync=off perf(postgres): set fsync=off Jan 23, 2025
@DDtKey DDtKey changed the title perf(postgres): set fsync=off perf(postgres)!: set fsync=off Jan 24, 2025
@DDtKey DDtKey changed the title perf(postgres)!: set fsync=off perf(postgres)!: set fsync=off by default Jan 24, 2025
@@ -91,6 +92,12 @@ impl Postgres {
.push(CopyToContainer::new(init_sql.into(), target));
self
}

/// Enables fsync for the Postgres instance.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might consider copying the postgres docs on this feature ^^
(feel free to shorten/ignore this input)

Suggested change
/// Enables fsync for the Postgres instance.
/// Enables `fsync` for the Postgres instance.
///
/// If this option is on, the PostgreSQL server will try to make sure that updates are physically written to disk, by issuing `fsync` system calls or various equivalent methods (see [wal_sync_method](https://www.postgresql.org/docs/8.1/runtime-config-wal.html#GUC-WAL-SYNC-METHOD)).
/// This ensures that the database cluster can recover to a consistent state after an operating system or hardware crash.
///
/// However, using `fsync` results in a performance penalty:
/// When a transaction is committed, PostgreSQL must wait for the operating system to flush the write-ahead log to disk.
/// When `fsync` is disabled, the operating system is allowed to do its best in buffering, ordering, and delaying writes.
/// This can result in significantly improved performance.
/// However, if the system crashes, the results of the last few committed transactions may be lost in part or whole.
/// In the worst case, unrecoverable data corruption may occur.
/// Crashes of the database software itself are not a risk factor here.
/// Only an operating-system-level crash creates a risk of corruption.
///
/// Due to the risks involved, there is no universally correct setting for `fsync`.
/// Some administrators always disable `fsync`, while others only turn it off during initial bulk data loads, where there is a clear restart point if something goes wrong.
/// Others always leave `fsync` enabled.
/// The default is to enable `fsync`, for maximum reliability.
/// If you trust your operating system, your hardware, and your utility company (or your battery backup), you can consider disabling `fsync`.

Copy link
Contributor

@DDtKey DDtKey Jan 25, 2025

Choose a reason for hiding this comment

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

I think it's excessive, we can provide link to the documentation instead (and users are responsible to ensure they read the documentation in accordance with their version of postgres)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Enables fsync for the Postgres instance.
/// Enables fsync for the Postgres instance.
///
/// See <https://www.postgresql.org/docs/8.1/runtime-config-wal.html> for further context

or alternative

Suggested change
/// Enables fsync for the Postgres instance.
/// Enables [the fsync-setting](https://www.postgresql.org/docs/8.1/runtime-config-wal.html) for the Postgres instance.

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.

3 participants