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.

Sign up for GitHub

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

Jump to bottom

Add option dbfilesize to control LevelDB target ("max") file size #30059

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −0
Open

Add option dbfilesize to control LevelDB target ("max") file size #30059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/dbwrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ CDBWrapper::CDBWrapper(const DBParams& params)
DBContext().iteroptions.fill_cache = false;
DBContext().syncoptions.sync = true;
DBContext().options = GetOptions(params.cache_bytes);
DBContext().options.max_file_size = params.options.max_file_size;
DBContext().options.create_if_missing = true;
if (params.memory_only) {
DBContext().penv = leveldb::NewMemEnv(leveldb::Env::Default());
Expand Down
4 changes: 4 additions & 0 deletions src/dbwrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,14 @@
static const size_t DBWRAPPER_PREALLOC_KEY_SIZE = 64;
static const size_t DBWRAPPER_PREALLOC_VALUE_SIZE = 1024;

static constexpr size_t DEFAULT_DB_FILE_SIZE{2};

//! User-controlled performance and debug options.
struct DBOptions {
//! Compact database on startup.
bool force_compact = false;
//! Target size of files.
size_t max_file_size{DEFAULT_DB_FILE_SIZE << 20};
};

//! Application-specific storage settings.
Expand Down
6 changes: 6 additions & 0 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <common/args.h>
#include <common/system.h>
#include <consensus/amount.h>
#include <dbwrapper.h>
#include <deploymentstatus.h>
#include <hash.h>
#include <httprpc.h>
Expand Down Expand Up @@ -448,6 +449,11 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-dbfilesize",
strprintf("Target size of files within databases, in MiB (%u to %u, default: %u).",
Copy link
Contributor

Choose a reason for hiding this comment

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

The text states a range of 1MiB to 1024MiB, but these values didn't seem to be enforced (e.g. 0.5, -3, and 1025 were not rejected). Is the intent here to provide guidance for the user (i.e. tell the user to choose 1 to 1024 MiB) rather than enforce a specific range of values?

Copy link
Member Author

Choose a reason for hiding this comment

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

LevelDB itself enforces it. Would it make sense to check it redundantly on our end to control error behaviour? (But then we have to remember to keep it in sync with LevelDB... but maybe just having the docs necessitates that)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, redundant checking of range (beyond LevelDB) seems a bit much for a debug option (which is used by a more niche userbase). The range in the help message seems to be more of an example of a usable range rather than a recommendation of range or a strictly enforced range. The default value would be the defacto recommendation.

If we're set on continuing to have LevelDB handle the range rather than Core, it seems like it would make sense to inform the user in the help message that the range is an example. This could help prevent a developer from wasting time on a PR that misses the intent (e.g. could envision a PR starting with: ...dbfilesize specifies a range of allowable values, but doesn't enforce this range. This PR fixes this...).

It might also help to clarify to the user which files this option impacts.

This is a bit of a nit of a nit, so feel free to disregard it. Example:

diff --git a/src/init.cpp b/src/init.cpp
index 5cc513e18c9..f4be033f1c1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -450,7 +450,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
     argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-dbfilesize",
-                   strprintf("Target size of files within databases, in MiB (%u to %u, default: %u).",
+                   strprintf("Target size of files within databases (chainstate, indexes), in MiB (example: %u to %u, default: %u).",
                              1, 1024,
                              DEFAULT_DB_FILE_SIZE),
                    ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);

1, 1024,
DEFAULT_DB_FILE_SIZE),
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
argsman.AddArg("-includeconf=<file>", "Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-loadblock=<file>", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
Expand Down
1 change: 1 addition & 0 deletions src/node/database_args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@ void ReadDatabaseArgs(const ArgsManager& args, DBOptions& options)
// databases), but it'd be easy to parse database-specific options by adding
// a database_type string or enum parameter to this function.
if (auto value = args.GetBoolArg("-forcecompactdb")) options.force_compact = *value;
if (auto value = args.GetIntArg("-dbfilesize")) options.max_file_size = (*value) << 20;
}
} // namespace node