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

sqlite: disable WAL mode #17937

Closed
wants to merge 1 commit into from
Closed

Conversation

vrothberg
Copy link
Member

Disable the write-ahead log [1] for sqlite. Performance testing suggests that WAL mode has a negative impact on Podman. While most docs suggest that WAL mode improves performance, it does not seem to apply to Podman which tends to have very short-lived processes.

What I found most curious is that commands with many writes are faster without WAL mode, for instance, rm, stop, start, etc.

Hence, let's move to as-close-to-default config of sqlite and enable options only after thorough testing.

[NO NEW TESTS NEEDED] as it's a non-functional change.

[1] https://www.sqlite.org/wal.html

Does this PR introduce a user-facing change?

None

Disable the write-ahead log [1] for sqlite.  Performance testing
suggests that WAL mode has a negative impact on Podman.  While most docs
suggest that WAL mode improves performance, it does not seem to apply to
Podman which tends to have very short-lived processes.

What I found most curious is that commands with many writes are faster
without WAL mode, for instance, `rm`, `stop`, `start`, etc.

Hence, let's move to as-close-to-default config of sqlite and enable
options only after thorough testing.

[NO NEW TESTS NEEDED] as it's a non-functional change.

[1] https://www.sqlite.org/wal.html

Signed-off-by: Valentin Rothberg <[email protected]>
@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 27, 2023
@vrothberg vrothberg marked this pull request as ready for review March 27, 2023 12:09
@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Mar 27, 2023
@vrothberg
Copy link
Member Author

@mheon @giuseppe @baude @edsantiago PTAL

@baude
Copy link
Member

baude commented Mar 27, 2023

code is fine but i do not want to remove wal mode ... if you want to remove the sync journal, im onboard.

@vrothberg
Copy link
Member Author

code is fine but i do not want to remove wal mode ... if you want to remove the sync journal, im onboard.

Please elaborate. I tested WAL without full-sync which didn't improve things.

As written in the commit message, I think that WAL has a negative impact. Podman's DB is very small and connections are short-lived. I am unsure WAL has been tuned for that scenario.

@baude
Copy link
Member

baude commented Mar 27, 2023

I understand. I would like the opportunity to run tests with your PR if that is OK

@vrothberg
Copy link
Member Author

Absolutely, please all test. Different machines yield quiet different behaviors in the two configurations.

@edsantiago
Copy link
Member

None of the usual lock/unique strings[1] appear in the CI logs

[1] 'is locked|unique constraint|constraint fail|config row|adding db|dbconfig.id'

@vrothberg
Copy link
Member Author

/hold

To give everybody time to try out and compare.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2023
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vrothberg
Copy link
Member Author

@giuseppe did you run the benchmarks on your machine? If so, which observations did you make?

@edsantiago
Copy link
Member

ping, we now have sqlite testing in system tests, and the "stop" flake seems to be gone.

If this is still desired, please rebase and repush and reevaluate/review.

@vrothberg
Copy link
Member Author

Still waiting on feedback from others on performance impact. My machine enjoys running without WAL mode based on running the scripts in hack/perf.

@mheon
Copy link
Member

mheon commented Apr 13, 2023

I see fairly substantial benefits to WAL mode / normal sync (instead of full sync) and around equal performance for WAL mode with full sync, but there's enough variability involved that it's very hard to say. We need a more consistent testing environment to be sure.

@giuseppe
Copy link
Member

I think that until we have better numbers and that WAL mode is clearly better, it would be better to keep it simple and closer to the default configuration. IMO, we need to add custom tweaks to the default configuration only when it is clear that it is an improvement

@vrothberg vrothberg closed this Apr 19, 2023
@vrothberg vrothberg deleted the sqlite-no-wal branch April 19, 2023 08:51
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Aug 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants