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

add parameter for limiting maximum AOF files size on disk #1425

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

Conversation

kronwerk
Copy link
Contributor

one can optionally want to limit AOF disk usage not with disk real size, but with some tailored value. this PR makes it possible.
possible use cases:

  1. filling disk up with AOF to the real limit may break something in user's environment (logs, watchdogs, anything) - setting lower value prevents that completely;
  2. setting this new limit to some low value may allow automatic solving of filling disk problem for those users who don't care too much about disk cost:
  • intense writes may overcome automatic AOF rewrite with some user's settings at some moment;
  • AOF reach lower than full disk limit (e.g. 50-60% of disk);
  • eventually automatic AOF rewrite comes in once more and finally fixes that.
    while on 100% filled disk (like nowadays) that automatic solution is impossible due to insufficient space for tmp AOF rewrite files (0 space, actually) - one should size up disk under running database which makes some people a bit nervous, as I noticed, and in some cases that's not always convenient or even possible.

Fixes #540

Signed-off-by: kronwerk <[email protected]>

improved aof-max-size tests

Signed-off-by: kronwerk <[email protected]>
@kronwerk kronwerk marked this pull request as draft December 11, 2024 14:02
Signed-off-by: kronwerk <[email protected]>
Copy link

codecov bot commented Dec 11, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.90%. Comparing base (1acf7f7) to head (b57409c).
Report is 17 commits behind head on unstable.

Files with missing lines Patch % Lines
src/aof.c 85.71% 1 Missing ⚠️
src/server.c 83.33% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1425      +/-   ##
============================================
+ Coverage     70.71%   70.90%   +0.19%     
============================================
  Files           119      119              
  Lines         64652    64625      -27     
============================================
+ Hits          45717    45822     +105     
+ Misses        18935    18803     -132     
Files with missing lines Coverage Δ
src/config.c 77.59% <ø> (-0.06%) ⬇️
src/server.h 100.00% <ø> (ø)
src/aof.c 81.25% <85.71%> (+1.12%) ⬆️
src/server.c 87.48% <83.33%> (-0.09%) ⬇️

... and 43 files with indirect coverage changes

@kronwerk kronwerk marked this pull request as ready for review December 11, 2024 17:57
ssize_t nwritten = 0, totwritten = 0, nonewritten = -1;

if (aof_max_size && (unsigned long long)aof_current_size >= aof_max_size) {
errno = ENOSPC;
Copy link
Member

Choose a reason for hiding this comment

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

The error message:
Error writing to the AOF file: <ENOSPC>
is misleading if the issue is not actual disk space but hitting the configured max file size. Consider logging a more explicit message, and using EFBIG instead of ENOSPC for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about smth like that, but haven't found a better error code than ENOSPC. the one you offered is (https://man7.org/linux/man-pages/man2/open.2.html):
EOVERFLOW pathname refers to a regular file that is too large to be opened. The usual scenario here is that an application compiled on a 32-bit platform without -D_FILE_OFFSET_BITS=64 tried to open a file whose size exceeds (1<<31)-1 bytes; see also O_LARGEFILE above. This is the error specified by POSIX.1; before Linux 2.6.24, Linux gave the error EFBIG for this case.

seems that it's more misleading (imho) - we're not opening a file, we're writing to it, and we're exactly out of space (though the limit is not a real disk limit).

we can try to define our own errno type, a new one, to differ it from the real disk limit and form a more descriptive message based on that upper in code stack - but shouldn't that be an overkill for such an issue?

Copy link
Member

Choose a reason for hiding this comment

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

POSIX error codes aren't tied to specific system/library calls and can be applied broadly. In the example above, it was in the context of open. Applications can use their own interpretations, this is an example:
https://github.com/valkey-io/valkey/blob/unstable/src/module.c#L5427

I'm fine with using either error codes, but there should be a custom error message to avoid misleading users into thinking it's a real disk space issue if ENOSPC is used.

@madolson madolson requested a review from soloestoy December 17, 2024 18:06
@madolson madolson added the major-decision-pending Major decision pending by TSC team label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NEW] Limit maximum size on disk of AOF files. Avoid disk full, long load times.
3 participants