From 063c3b83381341fbd149cc6df7a712b0e4bde5e4 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Sat, 28 Mar 2020 17:39:48 -0700 Subject: [PATCH 01/13] Make server_info report consistent with internal evaluations for validated ledger age. --- src/ripple/app/ledger/LedgerMaster.h | 4 ++++ src/ripple/app/ledger/impl/LedgerMaster.cpp | 2 +- src/ripple/app/misc/NetworkOPs.cpp | 26 ++++++++++++++------- 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/ripple/app/ledger/LedgerMaster.h b/src/ripple/app/ledger/LedgerMaster.h index b82fce0bd12..8b1b288a7e6 100644 --- a/src/ripple/app/ledger/LedgerMaster.h +++ b/src/ripple/app/ledger/LedgerMaster.h @@ -54,6 +54,10 @@ class Transaction; class LedgerMaster : public Stoppable, public AbstractFetchPackContainer { public: + // Age for last validated ledger if the process has yet to validate. + static constexpr std::chrono::seconds NO_VALIDATED_LEDGER_AGE = + std::chrono::hours{24 * 14}; + explicit LedgerMaster( Application& app, Stopwatch& stopwatch, diff --git a/src/ripple/app/ledger/impl/LedgerMaster.cpp b/src/ripple/app/ledger/impl/LedgerMaster.cpp index 9a8f7dfe382..6fb71a483f4 100644 --- a/src/ripple/app/ledger/impl/LedgerMaster.cpp +++ b/src/ripple/app/ledger/impl/LedgerMaster.cpp @@ -269,7 +269,7 @@ LedgerMaster::getValidatedLedgerAge() if (valClose == 0s) { JLOG(m_journal.debug()) << "No validated ledger"; - return weeks{2}; + return NO_VALIDATED_LEDGER_AGE; } std::chrono::seconds ret = app_.timeKeeper().closeTime().time_since_epoch(); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 66d01b791e0..5c3aadc0ec0 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2757,16 +2757,24 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) if (std::abs(closeOffset.count()) >= 60) l[jss::close_time_offset] = closeOffset.count(); - auto lCloseTime = lpClosed->info().closeTime; - auto closeTime = app_.timeKeeper().closeTime(); - if (lCloseTime <= closeTime) + constexpr std::chrono::seconds HIGH_AGE_THRESHOLD{1000000}; + if (m_ledgerMaster.haveValidated()) { - using namespace std::chrono_literals; - auto age = closeTime - lCloseTime; - if (age < 1000000s) - l[jss::age] = Json::UInt(age.count()); - else - l[jss::age] = 0; + auto const age = m_ledgerMaster.getValidatedLedgerAge(); + l[jss::age] = + Json::UInt(age < HIGH_AGE_THRESHOLD ? age.count() : 0); + } + else + { + auto lCloseTime = lpClosed->info().closeTime; + auto closeTime = app_.timeKeeper().closeTime(); + if (lCloseTime <= closeTime) + { + using namespace std::chrono_literals; + auto age = closeTime - lCloseTime; + l[jss::age] = + Json::UInt(age < HIGH_AGE_THRESHOLD ? age.count() : 0); + } } } From 6e9051e964eb648d7ce9d96f407f34b9024730f3 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Mon, 11 May 2020 16:48:34 -0400 Subject: [PATCH 02/13] Improve online_delete configuration and DB tuning: * Document delete_batch, back_off_milliseconds, age_threshold_seconds. * Convert those time values to chrono types. * Fix bug that ignored age_threshold_seconds. * Add a "recovery buffer" to the config that gives the node a chance to recover before aborting online delete. * Add begin/end log messages around the SQL queries. * Add a new configuration section: [sqlite] to allow tuning the sqlite database operations. Ignored on full/large history servers. * Update documentation of [node_db] and [sqlite] in the rippled-example.cfg file. * Resolves #3321 --- cfg/rippled-example.cfg | 149 ++++++++++++++--- src/ripple/app/consensus/RCLConsensus.cpp | 2 + src/ripple/app/ledger/Ledger.cpp | 4 +- src/ripple/app/main/Application.cpp | 5 +- src/ripple/app/main/DBInit.h | 33 ++-- src/ripple/app/main/Main.cpp | 6 +- src/ripple/app/misc/SHAMapStoreImp.cpp | 117 +++++++++---- src/ripple/app/misc/SHAMapStoreImp.h | 6 +- src/ripple/core/DatabaseCon.h | 20 ++- src/ripple/core/impl/Config.cpp | 4 +- src/ripple/core/impl/DatabaseCon.cpp | 94 +++++++++++ src/ripple/net/impl/DatabaseBody.ipp | 3 +- src/ripple/nodestore/impl/Shard.cpp | 9 +- src/test/app/Manifest_test.cpp | 1 + src/test/nodestore/Database_test.cpp | 193 ++++++++++++++++++++++ 15 files changed, 563 insertions(+), 83 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index c4917d65044..240bc32f950 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -869,18 +869,62 @@ # # These keys are possible for any type of backend: # +# earliest_seq The default is 32570 to match the XRP ledger +# network's earliest allowed sequence. Alternate +# networks may set this value. Minimum value of 1. +# If a [shard_db] section is defined, and this +# value is present either [node_db] or [shard_db], +# it must be defined with the same value in both +# sections. +# # online_delete Minimum value of 256. Enable automatic purging # of older ledger information. Maintain at least this # number of ledger records online. Must be greater # than or equal to ledger_history. # -# advisory_delete 0 for disabled, 1 for enabled. If set, then -# require administrative RPC call "can_delete" -# to enable online deletion of ledger records. +# These keys modify the behavior of online_delete, and thus are only +# relevant if online_delete is defined and non-zero: # -# earliest_seq The default is 32570 to match the XRP ledger -# network's earliest allowed sequence. Alternate -# networks may set this value. Minimum value of 1. +# advisory_delete 0 for disabled, 1 for enabled. If set, the +# administrative RPC call "can_delete" is required +# to enable online deletion of ledger records. +# Online deletion does not run automatically if +# non-zero and the last deletion was on a ledger +# greater than the current "can_delete" setting. +# Default is 0. +# +# delete_batch When automatically purging, SQLite database +# records are deleted in batches. This value +# controls the maximum size of each batch. Larger +# batches keep the databases locked for more time, +# which may cause other functions to fall behind, +# and thus cause the node to lose sync. +# Default is 100. +# +# back_off_milliseconds +# Number of milliseconds to wait between +# online_delete batches to allow other functions +# to catch up. +# Default is 100. +# +# age_threshold_seconds +# The online delete process will only run if the +# latest validated ledger is younger than this +# number of seconds. +# Default is 60. +# +# recovery_buffer_seconds +# The online delete process checks periodically +# that rippled is still in sync with the network, +# and that the validated ledger is less than +# *age_threshold_seconds* old. By default, if it +# is not the online delete process aborts and +# tries again later. If *recovery_buffer_seconds* +# is set, online delete will wait this number of +# seconds for rippled to recover before it aborts. +# Set this value if the node is otherwise staying +# in sync, or recovering quickly. +# Default is unset. # # Notes: # The 'node_db' entry configures the primary, persistent storage. @@ -892,6 +936,12 @@ # [import_db] Settings for performing a one-time import (optional) # [database_path] Path to the book-keeping databases. # +# There are 4 or 5 bookkeeping SQLite database that the server creates and +# maintains. If you omit this configuration setting, it will default to +# creating a directory called "db" located in the same place as your +# rippled.cfg file. Partial pathnames will be considered relative to +# the location of the rippled executable. +# # [shard_db] Settings for the Shard Database (optional) # # Format (without spaces): @@ -907,12 +957,64 @@ # # max_size_gb Maximum disk space the database will utilize (in gigabytes) # +# [sqlite] Tuning settings for the SQLite databases (optional) # -# There are 4 bookkeeping SQLite database that the server creates and -# maintains. If you omit this configuration setting, it will default to -# creating a directory called "db" located in the same place as your -# rippled.cfg file. Partial pathnames will be considered relative to -# the location of the rippled executable. +# Format (without spaces): +# One or more lines of case-insensitive key / value pairs: +# '=' +# ... +# +# Example: +# sync_level=low +# journal_mode=off +# +# WARNING: These settings can have significant effects on data integrity, +# particularly in failure scenarios. It is strongly recommended that they +# be left at their defaults unless the server is having performance issues +# during normal operation or during automatic purging (online_delete) +# operations. +# +# Optional keys: +# +# safety_level Valid values: high, low +# The default is "high", and tunes the SQLite +# databases in the most reliable mode. "low" +# is equivalent to +# journal_mode=memory +# synchronous=off +# temp_store=memory +# These settings trade speed and reduced I/O +# for a higher risk of data loss. See the +# individual settings below for more information. +# +# journal_mode Valid values: delete, truncate, persist, memory, wal, off +# The default is "wal", which uses a write-ahead +# log to implement database transactions. +# Alternately, "memory" saves disk I/O, but if +# rippled crashes during a transaction, the +# database is likely to be corrupted. +# See https://www.sqlite.org/pragma.html#pragma_journal_mode +# for more details about the available options. +# +# synchronous Valid values: off, normal, full, extra +# The default is "normal", which works well with +# the "wal" journal mode. Alternatively, "off" +# allows rippled to continue as soon as data is +# passed to the OS, which can significantly +# increase speed, but risks data corruption if +# the host computer crashes before writing that +# data to disk. +# See https://www.sqlite.org/pragma.html#pragma_synchronous +# for more details about the available options. +# +# temp_store Valid values: default, file, memory +# The default is "file", which will use files +# for temporary database tables and indices. +# Alternatively, "memory" may save I/O, but +# rippled does not currently use many, if any, +# of these temporary objects. +# See https://www.sqlite.org/pragma.html#pragma_temp_store +# for more details about the available options. # # # @@ -1212,23 +1314,24 @@ medium # This is primary persistent datastore for rippled. This includes transaction # metadata, account states, and ledger headers. Helpful information can be -# found here: https://ripple.com/wiki/NodeBackEnd -# delete old ledgers while maintaining at least 2000. Do not require an -# external administrative command to initiate deletion. +# found at https://xrpl.org/capacity-planning.html#node-db-type +# type=NuDB is recommended for non-validators with fast SSDs. Validators or +# slow / spinning disks should use RocksDB. +# online_delete=512 is recommended to delete old ledgers while maintaining at +# least 512. +# advisory_delete=0 allows the online delete process to run automatically +# when the node has approximately two times the "online_delete" value of +# ledgers. No external administrative command is required to initiate +# deletion. [node_db] -type=RocksDB -path=/var/lib/rippled/db/rocksdb -open_files=2000 -filter_bits=12 -cache_mb=256 -file_size_mb=8 -file_size_mult=2 -online_delete=2000 +type=NuDB +path=/var/lib/rippled/db/nudb +online_delete=512 advisory_delete=0 # This is the persistent datastore for shards. It is important for the health # of the ripple network that rippled operators shard as much as practical. -# NuDB requires SSD storage. Helpful information can be found here +# NuDB requires SSD storage. Helpful information can be found at # https://ripple.com/build/history-sharding #[shard_db] #path=/var/lib/rippled/db/shards/nudb diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index f05497a5332..aa4d966dedc 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -1001,7 +1001,9 @@ void RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const { if (!positions && app_.getOPs().isFull()) + { app_.getOPs().setMode(OperatingMode::CONNECTED); + } } void diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index 3a6c43376c5..86d0e33b284 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -228,14 +228,14 @@ Ledger::Ledger( !txMap_->fetchRoot(SHAMapHash{info_.txHash}, nullptr)) { loaded = false; - JLOG(j.warn()) << "Don't have TX root for ledger"; + JLOG(j.warn()) << "Don't have TX root for ledger" << info_.seq; } if (info_.accountHash.isNonZero() && !stateMap_->fetchRoot(SHAMapHash{info_.accountHash}, nullptr)) { loaded = false; - JLOG(j.warn()) << "Don't have AS root for ledger"; + JLOG(j.warn()) << "Don't have AS root for ledger" << info_.seq; } txMap_->setImmutable(); diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 3b37c736963..712962bde87 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1026,7 +1026,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp // transaction database mTxnDB = std::make_unique( - setup, TxDBName, TxDBPragma, TxDBInit); + setup, TxDBName, true, TxDBPragma, TxDBInit); mTxnDB->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config_->getValueFor(SizedItem::txnDBCache))); @@ -1065,7 +1065,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp // ledger database mLedgerDB = std::make_unique( - setup, LgrDBName, LgrDBPragma, LgrDBInit); + setup, LgrDBName, true, LgrDBPragma, LgrDBInit); mLedgerDB->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config_->getValueFor(SizedItem::lgrDBCache))); @@ -1075,6 +1075,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp mWalletDB = std::make_unique( setup, WalletDBName, + false, std::array(), WalletDBInit); } diff --git a/src/ripple/app/main/DBInit.h b/src/ripple/app/main/DBInit.h index 2aa183d4e70..8dd45cfe817 100644 --- a/src/ripple/app/main/DBInit.h +++ b/src/ripple/app/main/DBInit.h @@ -26,13 +26,23 @@ namespace ripple { //////////////////////////////////////////////////////////////////////////////// +// These pragmas are built at startup and applied to all database +// connections, unless otherwise noted. +inline constexpr char const* CommonDBPragmaJournal{"PRAGMA journal_mode=%s;"}; +inline constexpr char const* CommonDBPragmaSync{"PRAGMA synchronous=%s;"}; +inline constexpr char const* CommonDBPragmaTemp{"PRAGMA temp_store=%s;"}; +// Default values will always be used for the common pragmas if +// at least this much ledger history is configured. This includes +// full history nodes. This is because such a large amount of data will +// be more difficult to recover if a rare failure occurs, which are +// more likely with some of the other available tuning settings. +inline constexpr std::uint32_t SQLITE_TUNING_CUTOFF = 100'000'000; + // Ledger database holds ledgers and ledger confirmations inline constexpr auto LgrDBName{"ledger.db"}; -inline constexpr std::array LgrDBPragma{ - {"PRAGMA synchronous=NORMAL;", - "PRAGMA journal_mode=WAL;", - "PRAGMA journal_size_limit=1582080;"}}; +inline constexpr std::array LgrDBPragma{ + {"PRAGMA journal_size_limit=1582080;"}}; inline constexpr std::array LgrDBInit{ {"BEGIN TRANSACTION;", @@ -63,15 +73,14 @@ inline constexpr auto TxDBName{"transaction.db"}; inline constexpr #if (ULONG_MAX > UINT_MAX) && !defined(NO_SQLITE_MMAP) - std::array + std::array TxDBPragma { { #else - std::array TxDBPragma {{ + std::array TxDBPragma {{ #endif - "PRAGMA page_size=4096;", "PRAGMA synchronous=NORMAL;", - "PRAGMA journal_mode=WAL;", "PRAGMA journal_size_limit=1582080;", + "PRAGMA page_size=4096;", "PRAGMA journal_size_limit=1582080;", "PRAGMA max_page_count=2147483646;", #if (ULONG_MAX > UINT_MAX) && !defined(NO_SQLITE_MMAP) "PRAGMA mmap_size=17179869184;" @@ -115,10 +124,8 @@ inline constexpr std::array TxDBInit{ // Temporary database used with an incomplete shard that is being acquired inline constexpr auto AcquireShardDBName{"acquire.db"}; -inline constexpr std::array AcquireShardDBPragma{ - {"PRAGMA synchronous=NORMAL;", - "PRAGMA journal_mode=WAL;", - "PRAGMA journal_size_limit=1582080;"}}; +inline constexpr std::array AcquireShardDBPragma{ + {"PRAGMA journal_size_limit=1582080;"}}; inline constexpr std::array AcquireShardDBInit{ {"CREATE TABLE IF NOT EXISTS Shard ( \ @@ -130,6 +137,7 @@ inline constexpr std::array AcquireShardDBInit{ //////////////////////////////////////////////////////////////////////////////// // Pragma for Ledger and Transaction databases with complete shards +// These override the CommonDBPragma values defined above. inline constexpr std::array CompleteShardDBPragma{ {"PRAGMA synchronous=OFF;", "PRAGMA journal_mode=OFF;"}}; @@ -172,6 +180,7 @@ inline constexpr std::array WalletDBInit{ static constexpr auto stateDBName{"state.db"}; +// These override the CommonDBPragma values defined above. static constexpr std::array DownloaderDBPragma{ {"PRAGMA synchronous=FULL;", "PRAGMA journal_mode=DELETE;"}}; diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index ccc3f2c773e..8a4fdb11880 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -542,7 +542,7 @@ run(int argc, char** argv) } auto txnDB = std::make_unique( - dbSetup, TxDBName, TxDBPragma, TxDBInit); + dbSetup, TxDBName, false, TxDBPragma, TxDBInit); auto& session = txnDB->getSession(); std::uint32_t pageSize; @@ -555,7 +555,9 @@ run(int argc, char** argv) session << "PRAGMA temp_store_directory=\"" << tmpPath.string() << "\";"; session << "VACUUM;"; - session << "PRAGMA journal_mode=WAL;"; + assert(dbSetup.CommonPragma); + for (auto const& p : *dbSetup.CommonPragma) + session << p; session << "PRAGMA page_size;", soci::into(pageSize); std::cout << "VACUUM finished. page_size: " << pageSize diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 94deae5d276..460e81eb5bf 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -180,13 +180,24 @@ SHAMapStoreImp::SHAMapStoreImp( section.set("filter_bits", "10"); } - get_if_exists(section, "delete_batch", deleteBatch_); - get_if_exists(section, "backOff", backOff_); - get_if_exists(section, "age_threshold", ageThreshold_); get_if_exists(section, "online_delete", deleteInterval_); if (deleteInterval_) { + // Configuration that affects the behavior of online delete + get_if_exists(section, "delete_batch", deleteBatch_); + std::uint32_t temp; + if (get_if_exists(section, "back_off_milliseconds", temp) || + // Included for backward compaibility with an undocumented setting + get_if_exists(section, "backOff", temp)) + { + backOff_ = std::chrono::milliseconds{temp}; + } + if (get_if_exists(section, "age_threshold_seconds", temp)) + ageThreshold_ = std::chrono::seconds{temp}; + if (get_if_exists(section, "recovery_buffer_seconds", temp)) + recoveryBuffer_.emplace(std::chrono::seconds{temp}); + get_if_exists(section, "advisory_delete", advisoryDelete_); auto const minInterval = config.standalone() @@ -348,23 +359,14 @@ SHAMapStoreImp::run() // will delete up to (not including) lastRotated if (validatedSeq >= lastRotated + deleteInterval_ && - canDelete_ >= lastRotated - 1) + canDelete_ >= lastRotated - 1 && !health()) { JLOG(journal_.warn()) << "rotating validatedSeq " << validatedSeq << " lastRotated " << lastRotated << " deleteInterval " << deleteInterval_ - << " canDelete_ " << canDelete_; - - switch (health()) - { - case Health::stopping: - stopped(); - return; - case Health::unhealthy: - continue; - case Health::ok: - default:; - } + << " canDelete_ " << canDelete_ << " state " + << app_.getOPs().strOperatingMode(false) << " age " + << ledgerMaster_->getValidatedLedgerAge().count() << 's'; clearPrior(lastRotated); switch (health()) @@ -378,14 +380,13 @@ SHAMapStoreImp::run() default:; } + JLOG(journal_.trace()) << "copying ledger " << validatedSeq; std::uint64_t nodeCount = 0; validatedLedger->stateMap().snapShot(false)->visitNodes(std::bind( &SHAMapStoreImp::copyNode, this, std::ref(nodeCount), std::placeholders::_1)); - JLOG(journal_.debug()) << "copied ledger " << validatedSeq - << " nodecount " << nodeCount; switch (health()) { case Health::stopping: @@ -396,9 +397,12 @@ SHAMapStoreImp::run() case Health::ok: default:; } + // Only log if we completed without a "health" abort + JLOG(journal_.debug()) << "copied ledger " << validatedSeq + << " nodecount " << nodeCount; + JLOG(journal_.trace()) << "freshening caches"; freshenCaches(); - JLOG(journal_.debug()) << validatedSeq << " freshened caches"; switch (health()) { case Health::stopping: @@ -409,7 +413,10 @@ SHAMapStoreImp::run() case Health::ok: default:; } + // Only log if we completed without a "health" abort + JLOG(journal_.debug()) << validatedSeq << " freshened caches"; + JLOG(journal_.trace()) << "Making a new backend"; auto newBackend = makeBackendRotating(); JLOG(journal_.debug()) << validatedSeq << " new backend " << newBackend->getName(); @@ -566,12 +573,21 @@ SHAMapStoreImp::clearSql( std::string const& minQuery, std::string const& deleteQuery) { + assert(deleteInterval_); LedgerIndex min = std::numeric_limits::max(); { - auto db = database.checkoutDb(); boost::optional m; - *db << minQuery, soci::into(m); + JLOG(journal_.trace()) + << "Begin: Look up lowest value of: " << minQuery; + if (auto db = database.checkoutDb()) + *db << minQuery, soci::into(m); + else + { + assert(false); + return false; + } + JLOG(journal_.trace()) << "End: Look up lowest value of: " << minQuery; if (!m) return false; min = *m; @@ -579,6 +595,12 @@ SHAMapStoreImp::clearSql( if (min > lastRotated || health() != Health::ok) return false; + if (min == lastRotated) + { + // Micro-optimization mainly to clarify logs + JLOG(journal_.trace()) << "Nothing to delete from " << deleteQuery; + return true; + } boost::format formattedDeleteQuery(deleteQuery); @@ -587,14 +609,27 @@ SHAMapStoreImp::clearSql( while (min < lastRotated) { min = std::min(lastRotated, min + deleteBatch_); + JLOG(journal_.trace()) << "Begin: Delete up to " << deleteBatch_ + << " rows with LedgerSeq < " << min + << " using query: " << deleteQuery; + if (auto db = database.checkoutDb()) { - auto db = database.checkoutDb(); *db << boost::str(formattedDeleteQuery % min); } + else + { + assert(false); + return false; + } + JLOG(journal_.trace()) + << "End: Delete up to " << deleteBatch_ << " rows with LedgerSeq < " + << min << " using query: " << deleteQuery; if (health()) return true; if (min < lastRotated) - std::this_thread::sleep_for(std::chrono::milliseconds(backOff_)); + std::this_thread::sleep_for(backOff_); + if (health()) + return true; } JLOG(journal_.debug()) << "finished: " << deleteQuery; return true; @@ -627,7 +662,11 @@ SHAMapStoreImp::clearPrior(LedgerIndex lastRotated) // Do not allow ledgers to be acquired from the network // that are about to be deleted. minimumOnline_ = lastRotated + 1; + JLOG(journal_.trace()) << "Begin: Clear internal ledgers up to " + << lastRotated; ledgerMaster_->clearPriorLedgers(lastRotated); + JLOG(journal_.trace()) << "End: Clear internal ledgers up to " + << lastRotated; if (health()) return; @@ -666,16 +705,32 @@ SHAMapStoreImp::health() } if (!netOPs_) return Health::ok; + assert(deleteInterval_); - constexpr static std::chrono::seconds age_threshold(60); - auto age = ledgerMaster_->getValidatedLedgerAge(); - OperatingMode mode = netOPs_->getOperatingMode(); - if (mode != OperatingMode::FULL || age > age_threshold) + if (healthy_) { - JLOG(journal_.warn()) << "Not deleting. state: " - << app_.getOPs().strOperatingMode(mode, false) - << ". age " << age.count() << 's'; - healthy_ = false; + auto age = ledgerMaster_->getValidatedLedgerAge(); + OperatingMode mode = netOPs_->getOperatingMode(); + if (recoveryBuffer_ && mode == OperatingMode::SYNCING && + age < ageThreshold_) + { + JLOG(journal_.warn()) + << "Waiting " << recoveryBuffer_->count() + << "s for node to get back into sync with network. state: " + << app_.getOPs().strOperatingMode(mode, false) << ". age " + << age.count() << 's'; + std::this_thread::sleep_for(*recoveryBuffer_); + + age = ledgerMaster_->getValidatedLedgerAge(); + mode = netOPs_->getOperatingMode(); + } + if (mode != OperatingMode::FULL || age > ageThreshold_) + { + JLOG(journal_.warn()) << "Not deleting. state: " + << app_.getOPs().strOperatingMode(mode, false) + << ". age " << age.count() << 's'; + healthy_ = false; + } } if (healthy_) diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index 2fabf1a6996..a5f4b7ace89 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -106,8 +107,9 @@ class SHAMapStoreImp : public SHAMapStore std::uint32_t deleteInterval_ = 0; bool advisoryDelete_ = false; std::uint32_t deleteBatch_ = 100; - std::uint32_t backOff_ = 100; - std::int32_t ageThreshold_ = 60; + std::chrono::milliseconds backOff_{100}; + std::chrono::seconds ageThreshold_{60}; + boost::optional recoveryBuffer_{}; // these do not exist upon SHAMapStore creation, but do exist // as of onPrepare() or before diff --git a/src/ripple/core/DatabaseCon.h b/src/ripple/core/DatabaseCon.h index d79ecef2071..d077c40e95f 100644 --- a/src/ripple/core/DatabaseCon.h +++ b/src/ripple/core/DatabaseCon.h @@ -89,12 +89,17 @@ class DatabaseCon Config::StartUpType startUp = Config::NORMAL; bool standAlone = false; boost::filesystem::path dataDir; + static std::unique_ptr const> CommonPragma; + /// Shortcut used by the database connections that ignore the common + /// pragma strings + static const std::vector NoCommonPragma; }; template DatabaseCon( Setup const& setup, std::string const& DBName, + bool useCommonPragma, std::array const& pragma, std::array const& initSQL) { @@ -106,7 +111,12 @@ class DatabaseCon boost::filesystem::path pPath = useTempFiles ? "" : (setup.dataDir / DBName); - init(pPath, pragma, initSQL); + assert(!useCommonPragma || setup.CommonPragma); + init( + pPath, + useCommonPragma ? *setup.CommonPragma : setup.NoCommonPragma, + pragma, + initSQL); } template @@ -116,7 +126,7 @@ class DatabaseCon std::array const& pragma, std::array const& initSQL) { - init((dataDir / DBName), pragma, initSQL); + init((dataDir / DBName), {}, pragma, initSQL); } soci::session& @@ -139,11 +149,17 @@ class DatabaseCon void init( boost::filesystem::path const& pPath, + std::vector const& commonPragma, std::array const& pragma, std::array const& initSQL) { open(session_, "sqlite", pPath.string()); + for (auto const& p : commonPragma) + { + soci::statement st = session_.prepare << p; + st.execute(true); + } for (auto const& p : pragma) { soci::statement st = session_.prepare << p; diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index f12ba7dbcee..1897bdd9e41 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -442,7 +442,7 @@ Config::loadFromString(std::string const& fileContents) if (getSingleSection(secConfig, SECTION_LEDGER_HISTORY, strTemp, j_)) { if (boost::iequals(strTemp, "full")) - LEDGER_HISTORY = 1000000000u; + LEDGER_HISTORY = 1'000'000'000u; else if (boost::iequals(strTemp, "none")) LEDGER_HISTORY = 0; else @@ -454,7 +454,7 @@ Config::loadFromString(std::string const& fileContents) if (boost::iequals(strTemp, "none")) FETCH_DEPTH = 0; else if (boost::iequals(strTemp, "full")) - FETCH_DEPTH = 1000000000u; + FETCH_DEPTH = 1'000'000'000u; else FETCH_DEPTH = beast::lexicalCastThrow(strTemp); diff --git a/src/ripple/core/impl/DatabaseCon.cpp b/src/ripple/core/impl/DatabaseCon.cpp index 3a4489b2f94..14ceb05ebbf 100644 --- a/src/ripple/core/impl/DatabaseCon.cpp +++ b/src/ripple/core/impl/DatabaseCon.cpp @@ -21,6 +21,8 @@ #include #include #include +#include +#include #include namespace ripple { @@ -38,9 +40,101 @@ setup_DatabaseCon(Config const& c) Throw("database_path must be set."); } + if (!setup.CommonPragma) + { + setup.CommonPragma = [&c]() { + auto const& sqlite = c.section("sqlite"); + auto result = std::make_unique>(); + result->reserve(3); + + // defaults + std::string safety_level = "high"; + std::string journal_mode = "wal"; + std::string synchronous = "normal"; + std::string temp_store = "file"; + + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF && + set(safety_level, "safety_level", sqlite)) + { + if (boost::iequals(safety_level, "low")) + { + // low safety defaults + journal_mode = "memory"; + synchronous = "off"; + temp_store = "memory"; + } + else if (!boost::iequals(safety_level, "high")) + { + Throw( + "Invalid safety_level value: " + safety_level); + } + } + + // #journal_mode Valid values : delete, truncate, persist, memory, + // wal, off + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(journal_mode, "journal_mode", sqlite); + if (boost::iequals(journal_mode, "delete") || + boost::iequals(journal_mode, "truncate") || + boost::iequals(journal_mode, "persist") || + boost::iequals(journal_mode, "memory") || + boost::iequals(journal_mode, "wal") || + boost::iequals(journal_mode, "off")) + { + result->emplace_back(boost::str( + boost::format(CommonDBPragmaJournal) % journal_mode)); + } + else + { + Throw( + "Invalid journal_mode value: " + journal_mode); + } + + //#synchronous Valid values : off, normal, full, extra + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(synchronous, "synchronous", sqlite); + if (boost::iequals(synchronous, "off") || + boost::iequals(synchronous, "normal") || + boost::iequals(synchronous, "full") || + boost::iequals(synchronous, "extra")) + { + result->emplace_back(boost::str( + boost::format(CommonDBPragmaSync) % synchronous)); + } + else + { + Throw( + "Invalid synchronous value: " + synchronous); + } + + // #temp_store Valid values : default, file, memory + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(temp_store, "temp_store", sqlite); + if (boost::iequals(temp_store, "default") || + boost::iequals(temp_store, "file") || + boost::iequals(temp_store, "memory")) + { + result->emplace_back( + boost::str(boost::format(CommonDBPragmaTemp) % temp_store)); + } + else + { + Throw( + "Invalid temp_store value: " + temp_store); + } + + assert(result->size() == 3); + return result; + }(); + } + return setup; } +std::unique_ptr const> + DatabaseCon::Setup::CommonPragma; +const std::vector DatabaseCon::Setup::NoCommonPragma; + void DatabaseCon::setupCheckpointing(JobQueue* q, Logs& l) { diff --git a/src/ripple/net/impl/DatabaseBody.ipp b/src/ripple/net/impl/DatabaseBody.ipp index d6bae7b47f7..805bb6dfe8b 100644 --- a/src/ripple/net/impl/DatabaseBody.ipp +++ b/src/ripple/net/impl/DatabaseBody.ipp @@ -51,8 +51,9 @@ DatabaseBody::value_type::open( auto setup = setup_DatabaseCon(config); setup.dataDir = path.parent_path(); + // Downloader ignores the "CommonPragma" conn_ = std::make_unique( - setup, "Download", DownloaderDBPragma, DatabaseBodyDBInit); + setup, "Download", false, DownloaderDBPragma, DatabaseBodyDBInit); path_ = path; diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 1701206fe4d..01088f98365 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -128,6 +128,7 @@ Shard::open(Scheduler& scheduler, nudb::context& ctx) acquireInfo_->SQLiteDB = std::make_unique( setup, AcquireShardDBName, + true, AcquireShardDBPragma, AcquireShardDBInit); acquireInfo_->SQLiteDB->setupCheckpointing( @@ -684,14 +685,14 @@ Shard::initSQLite(std::lock_guard const&) if (backendComplete_) { lgrSQLiteDB_ = std::make_unique( - setup, LgrDBName, CompleteShardDBPragma, LgrDBInit); + setup, LgrDBName, false, CompleteShardDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes( config.getValueFor(SizedItem::lgrDBCache, boost::none))); txSQLiteDB_ = std::make_unique( - setup, TxDBName, CompleteShardDBPragma, TxDBInit); + setup, TxDBName, false, CompleteShardDBPragma, TxDBInit); txSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes( @@ -701,14 +702,14 @@ Shard::initSQLite(std::lock_guard const&) { // The incomplete shard uses a Write Ahead Log for performance lgrSQLiteDB_ = std::make_unique( - setup, LgrDBName, LgrDBPragma, LgrDBInit); + setup, LgrDBName, true, LgrDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config.getValueFor(SizedItem::lgrDBCache))); lgrSQLiteDB_->setupCheckpointing(&app_.getJobQueue(), app_.logs()); txSQLiteDB_ = std::make_unique( - setup, TxDBName, TxDBPragma, TxDBInit); + setup, TxDBName, true, TxDBPragma, TxDBInit); txSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config.getValueFor(SizedItem::txnDBCache))); diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 063b4383281..7d03b7d93ec 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -259,6 +259,7 @@ class Manifest_test : public beast::unit_test::suite DatabaseCon dbCon( setup, dbName.data(), + false, std::array(), WalletDBInit); diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index b1a88bea557..71bd2948468 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -18,8 +18,11 @@ //============================================================================== #include +#include #include #include +#include +#include #include #include @@ -35,6 +38,194 @@ class Database_test : public TestBase { } + void + testConfig() + { + testcase("Config"); + + using namespace ripple::test::jtx; + + { + // defaults + Env env(*this); + + auto const s = setup_DatabaseCon(env.app().config()); + + if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + { + BEAST_EXPECT( + s.CommonPragma->at(0) == "PRAGMA journal_mode=wal;"); + BEAST_EXPECT( + s.CommonPragma->at(1) == "PRAGMA synchronous=normal;"); + BEAST_EXPECT( + s.CommonPragma->at(2) == "PRAGMA temp_store=file;"); + } + } + { + // Low safety level + DatabaseCon::Setup::CommonPragma.reset(); + + Env env = [&]() { + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "low"); + } + + return Env(*this, std::move(p)); + }(); + + auto const s = setup_DatabaseCon(env.app().config()); + if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + { + BEAST_EXPECT( + s.CommonPragma->at(0) == "PRAGMA journal_mode=memory;"); + BEAST_EXPECT( + s.CommonPragma->at(1) == "PRAGMA synchronous=off;"); + BEAST_EXPECT( + s.CommonPragma->at(2) == "PRAGMA temp_store=memory;"); + } + } + { + // Override individual settings + DatabaseCon::Setup::CommonPragma.reset(); + + Env env = [&]() { + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("journal_mode", "off"); + section.set("synchronous", "extra"); + section.set("temp_store", "default"); + } + + return Env(*this, std::move(p)); + }(); + + auto const s = setup_DatabaseCon(env.app().config()); + if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + { + BEAST_EXPECT( + s.CommonPragma->at(0) == "PRAGMA journal_mode=off;"); + BEAST_EXPECT( + s.CommonPragma->at(1) == "PRAGMA synchronous=extra;"); + BEAST_EXPECT( + s.CommonPragma->at(2) == "PRAGMA temp_store=default;"); + } + } + { + // Override individual settings with low safety level + // (Low doesn't force the other settings) + DatabaseCon::Setup::CommonPragma.reset(); + + Env env = [&]() { + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "low"); + section.set("journal_mode", "off"); + section.set("synchronous", "extra"); + section.set("temp_store", "default"); + } + + return Env(*this, std::move(p)); + }(); + + auto const s = setup_DatabaseCon(env.app().config()); + if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + { + BEAST_EXPECT( + s.CommonPragma->at(0) == "PRAGMA journal_mode=off;"); + BEAST_EXPECT( + s.CommonPragma->at(1) == "PRAGMA synchronous=extra;"); + BEAST_EXPECT( + s.CommonPragma->at(2) == "PRAGMA temp_store=default;"); + } + } + { + // Errors + DatabaseCon::Setup::CommonPragma.reset(); + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "slow"); + } + + try + { + Env env(*this, std::move(p)); + fail(); + } + catch (std::exception const& e) + { + pass(); + } + } + { + // Errors + DatabaseCon::Setup::CommonPragma.reset(); + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("journal_mode", "fast"); + } + + try + { + Env env(*this, std::move(p)); + fail(); + } + catch (std::exception const& e) + { + pass(); + } + } + { + // Errors + DatabaseCon::Setup::CommonPragma.reset(); + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("synchronous", "instant"); + } + + try + { + Env env(*this, std::move(p)); + fail(); + } + catch (std::exception const& e) + { + pass(); + } + } + { + // Errors + DatabaseCon::Setup::CommonPragma.reset(); + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("temp_store", "network"); + } + + try + { + Env env(*this, std::move(p)); + fail(); + } + catch (std::exception const& e) + { + pass(); + } + } + } + + //-------------------------------------------------------------------------- + void testImport( std::string const& destBackendType, @@ -221,6 +412,8 @@ class Database_test : public TestBase { std::int64_t const seedValue = 50; + testConfig(); + testNodeStore("memory", false, seedValue); // Persistent backend tests From 4b408770e73887d0ea9286359717bbf573b1e9cf Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 3 Jun 2020 12:19:59 -0400 Subject: [PATCH 03/13] [FOLD] Small cleanup that I missed when preparing the PR --- src/ripple/app/consensus/RCLConsensus.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/ripple/app/consensus/RCLConsensus.cpp b/src/ripple/app/consensus/RCLConsensus.cpp index aa4d966dedc..f05497a5332 100644 --- a/src/ripple/app/consensus/RCLConsensus.cpp +++ b/src/ripple/app/consensus/RCLConsensus.cpp @@ -1001,9 +1001,7 @@ void RCLConsensus::Adaptor::updateOperatingMode(std::size_t const positions) const { if (!positions && app_.getOPs().isFull()) - { app_.getOPs().setMode(OperatingMode::CONNECTED); - } } void From e42c1ee497e0ebf3efcb50cf3df3447ac076852e Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 3 Jun 2020 12:25:38 -0400 Subject: [PATCH 04/13] [FOLD] Clarify the description of 'recovery_buffer_seconds' --- cfg/rippled-example.cfg | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 240bc32f950..ac6017e89e7 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -917,13 +917,16 @@ # The online delete process checks periodically # that rippled is still in sync with the network, # and that the validated ledger is less than -# *age_threshold_seconds* old. By default, if it +# 'age_threshold_seconds' old. By default, if it # is not the online delete process aborts and -# tries again later. If *recovery_buffer_seconds* -# is set, online delete will wait this number of -# seconds for rippled to recover before it aborts. +# tries again later. If 'recovery_buffer_seconds' +# is set and rippled is out of sync, but likely to +# recover quickly, then online delete will wait +# this number of seconds for rippled to get back +# into sync before it aborts. # Set this value if the node is otherwise staying -# in sync, or recovering quickly. +# in sync, or recovering quickly, but the online +# delete process is unable to finish. # Default is unset. # # Notes: From f10c335479450db8254ad505476c78f7c04c0e23 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Thu, 4 Jun 2020 13:52:48 -0400 Subject: [PATCH 05/13] [FOLD] Remove unused catch variable --- src/test/nodestore/Database_test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index 71bd2948468..61b84ba90ec 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -157,7 +157,7 @@ class Database_test : public TestBase Env env(*this, std::move(p)); fail(); } - catch (std::exception const& e) + catch (...) { pass(); } @@ -177,7 +177,7 @@ class Database_test : public TestBase Env env(*this, std::move(p)); fail(); } - catch (std::exception const& e) + catch (...) { pass(); } @@ -197,7 +197,7 @@ class Database_test : public TestBase Env env(*this, std::move(p)); fail(); } - catch (std::exception const& e) + catch (...) { pass(); } @@ -217,7 +217,7 @@ class Database_test : public TestBase Env env(*this, std::move(p)); fail(); } - catch (std::exception const& e) + catch (...) { pass(); } From 088d0afdc063d98cb90700a97c4aeabfb2703bae Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Thu, 4 Jun 2020 14:46:31 -0400 Subject: [PATCH 06/13] [FOLD] Review feedback from @nbougalis: (Include these items in the main commit when squashing.) * Update rippled URLs in documentation. * Full history and fetch depth use max() value instead of an arbitrary value of 1 billion. --- cfg/rippled-example.cfg | 9 +++++---- src/ripple/core/impl/Config.cpp | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index ac6017e89e7..aaa5ca8173d 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -36,7 +36,7 @@ # For more information on where the rippled server instance searches for the # file, visit: # -# https://developers.ripple.com/commandline-usage.html#generic-options +# https://xrpl.org/commandline-usage.html#generic-options # # This file should be named rippled.cfg. This file is UTF-8 with DOS, UNIX, # or Mac style end of lines. Blank lines and lines beginning with '#' are @@ -939,7 +939,7 @@ # [import_db] Settings for performing a one-time import (optional) # [database_path] Path to the book-keeping databases. # -# There are 4 or 5 bookkeeping SQLite database that the server creates and +# There are 4 or 5 bookkeeping SQLite databases that the server creates and # maintains. If you omit this configuration setting, it will default to # creating a directory called "db" located in the same place as your # rippled.cfg file. Partial pathnames will be considered relative to @@ -1335,7 +1335,7 @@ advisory_delete=0 # This is the persistent datastore for shards. It is important for the health # of the ripple network that rippled operators shard as much as practical. # NuDB requires SSD storage. Helpful information can be found at -# https://ripple.com/build/history-sharding +# https://xrpl.org/history-sharding.html #[shard_db] #path=/var/lib/rippled/db/shards/nudb #max_size_gb=500 @@ -1354,7 +1354,8 @@ time.apple.com time.nist.gov pool.ntp.org -# To use the XRP test network (see https://ripple.com/build/xrp-test-net/), +# To use the XRP test network +# (see https://xrpl.org/connect-your-rippled-to-the-xrp-test-net.html), # use the following [ips] section: # [ips] # r.altnet.rippletest.net 51235 diff --git a/src/ripple/core/impl/Config.cpp b/src/ripple/core/impl/Config.cpp index 1897bdd9e41..a7524574fc8 100644 --- a/src/ripple/core/impl/Config.cpp +++ b/src/ripple/core/impl/Config.cpp @@ -442,7 +442,8 @@ Config::loadFromString(std::string const& fileContents) if (getSingleSection(secConfig, SECTION_LEDGER_HISTORY, strTemp, j_)) { if (boost::iequals(strTemp, "full")) - LEDGER_HISTORY = 1'000'000'000u; + LEDGER_HISTORY = + std::numeric_limits::max(); else if (boost::iequals(strTemp, "none")) LEDGER_HISTORY = 0; else @@ -454,7 +455,7 @@ Config::loadFromString(std::string const& fileContents) if (boost::iequals(strTemp, "none")) FETCH_DEPTH = 0; else if (boost::iequals(strTemp, "full")) - FETCH_DEPTH = 1'000'000'000u; + FETCH_DEPTH = std::numeric_limits::max(); else FETCH_DEPTH = beast::lexicalCastThrow(strTemp); From 6b9687605bc1fcb2cebce8f3d69b4a9992dfcfa4 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Fri, 5 Jun 2020 18:58:50 -0400 Subject: [PATCH 07/13] [fold] review feeback from @nbougalis 2: * changed the sqlite ledger_history cutoff to log a warning instead of ignoring sqlite settings. updated documentation. * integrated the commonpragma back in to the setup struct and restored the interface of the databasecon ctors. --- cfg/rippled-example.cfg | 6 +- src/ripple/app/main/Application.cpp | 8 +- src/ripple/app/main/DBInit.h | 12 +-- src/ripple/app/main/Main.cpp | 7 +- src/ripple/core/DatabaseCon.h | 44 ++++++---- src/ripple/core/impl/DatabaseCon.cpp | 120 +++++++++++++++------------ src/ripple/net/impl/DatabaseBody.ipp | 3 +- src/ripple/nodestore/impl/Shard.cpp | 12 +-- src/test/app/Manifest_test.cpp | 2 +- src/test/nodestore/Database_test.cpp | 46 +++++----- 10 files changed, 149 insertions(+), 111 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index aaa5ca8173d..b572d5ea3c6 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -975,7 +975,11 @@ # particularly in failure scenarios. It is strongly recommended that they # be left at their defaults unless the server is having performance issues # during normal operation or during automatic purging (online_delete) -# operations. +# operations. A warning will be logged on startup if 'ledger_history' +# is configured to store more than 10,000,000 ledgers and any of these +# settings are less safe than the default. This is due to the inordinate +# amount of time and bandwidth it will take to safely rebuild a corrupted +# database from other peers. # # Optional keys: # diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 712962bde87..fabcdafe390 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1022,11 +1022,11 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp try { - auto const setup = setup_DatabaseCon(*config_); + auto setup = setup_DatabaseCon(*config_); // transaction database mTxnDB = std::make_unique( - setup, TxDBName, true, TxDBPragma, TxDBInit); + setup, TxDBName, TxDBPragma, TxDBInit); mTxnDB->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config_->getValueFor(SizedItem::txnDBCache))); @@ -1065,17 +1065,17 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp // ledger database mLedgerDB = std::make_unique( - setup, LgrDBName, true, LgrDBPragma, LgrDBInit); + setup, LgrDBName, LgrDBPragma, LgrDBInit); mLedgerDB->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config_->getValueFor(SizedItem::lgrDBCache))); mLedgerDB->setupCheckpointing(m_jobQueue.get(), logs()); // wallet database + setup.noPragma(); mWalletDB = std::make_unique( setup, WalletDBName, - false, std::array(), WalletDBInit); } diff --git a/src/ripple/app/main/DBInit.h b/src/ripple/app/main/DBInit.h index 8dd45cfe817..fc81b1fc551 100644 --- a/src/ripple/app/main/DBInit.h +++ b/src/ripple/app/main/DBInit.h @@ -31,12 +31,12 @@ namespace ripple { inline constexpr char const* CommonDBPragmaJournal{"PRAGMA journal_mode=%s;"}; inline constexpr char const* CommonDBPragmaSync{"PRAGMA synchronous=%s;"}; inline constexpr char const* CommonDBPragmaTemp{"PRAGMA temp_store=%s;"}; -// Default values will always be used for the common pragmas if -// at least this much ledger history is configured. This includes -// full history nodes. This is because such a large amount of data will -// be more difficult to recover if a rare failure occurs, which are -// more likely with some of the other available tuning settings. -inline constexpr std::uint32_t SQLITE_TUNING_CUTOFF = 100'000'000; +// A warning will be logged if any lower-safety sqlite tuning settings +// are used and at least this much ledger history is configured. This +// includes full history nodes. This is because such a large amount of +// data will be more difficult to recover if a rare failure occurs, +// which are more likely with some of the other available tuning settings. +inline constexpr std::uint32_t SQLITE_TUNING_CUTOFF = 10'000'000; // Ledger database holds ledgers and ledger confirmations inline constexpr auto LgrDBName{"ledger.db"}; diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index 8a4fdb11880..713c68b6851 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -541,8 +541,9 @@ run(int argc, char** argv) return -1; } + dbSetup.noPragma(); auto txnDB = std::make_unique( - dbSetup, TxDBName, false, TxDBPragma, TxDBInit); + dbSetup, TxDBName, TxDBPragma, TxDBInit); auto& session = txnDB->getSession(); std::uint32_t pageSize; @@ -555,8 +556,8 @@ run(int argc, char** argv) session << "PRAGMA temp_store_directory=\"" << tmpPath.string() << "\";"; session << "VACUUM;"; - assert(dbSetup.CommonPragma); - for (auto const& p : *dbSetup.CommonPragma) + assert(dbSetup.globalPragma); + for (auto const& p : *dbSetup.globalPragma) session << p; session << "PRAGMA page_size;", soci::into(pageSize); diff --git a/src/ripple/core/DatabaseCon.h b/src/ripple/core/DatabaseCon.h index d077c40e95f..debcdf2a400 100644 --- a/src/ripple/core/DatabaseCon.h +++ b/src/ripple/core/DatabaseCon.h @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -89,17 +90,28 @@ class DatabaseCon Config::StartUpType startUp = Config::NORMAL; bool standAlone = false; boost::filesystem::path dataDir; - static std::unique_ptr const> CommonPragma; - /// Shortcut used by the database connections that ignore the common - /// pragma strings - static const std::vector NoCommonPragma; + // If unseated, then the `globalPragma` are not used, + // otherwise should point to `globalPragma` + std::shared_ptr const> commonPragma; + void + noPragma() + { + commonPragma.reset(); + } + void + usePragma() + { + assert(globalPragma); + commonPragma = globalPragma; + } + + static std::shared_ptr const> globalPragma; }; template DatabaseCon( Setup const& setup, std::string const& DBName, - bool useCommonPragma, std::array const& pragma, std::array const& initSQL) { @@ -111,12 +123,7 @@ class DatabaseCon boost::filesystem::path pPath = useTempFiles ? "" : (setup.dataDir / DBName); - assert(!useCommonPragma || setup.CommonPragma); - init( - pPath, - useCommonPragma ? *setup.CommonPragma : setup.NoCommonPragma, - pragma, - initSQL); + init(pPath, setup.commonPragma, pragma, initSQL); } template @@ -149,16 +156,19 @@ class DatabaseCon void init( boost::filesystem::path const& pPath, - std::vector const& commonPragma, + std::shared_ptr const> const& commonPragma, std::array const& pragma, std::array const& initSQL) { open(session_, "sqlite", pPath.string()); - for (auto const& p : commonPragma) + if (commonPragma) { - soci::statement st = session_.prepare << p; - st.execute(true); + for (auto const& p : *commonPragma) + { + soci::statement st = session_.prepare << p; + st.execute(true); + } } for (auto const& p : pragma) { @@ -179,7 +189,9 @@ class DatabaseCon }; DatabaseCon::Setup -setup_DatabaseCon(Config const& c); +setup_DatabaseCon( + Config const& c, + boost::optional j = boost::none); } // namespace ripple diff --git a/src/ripple/core/impl/DatabaseCon.cpp b/src/ripple/core/impl/DatabaseCon.cpp index 14ceb05ebbf..431ee95b63a 100644 --- a/src/ripple/core/impl/DatabaseCon.cpp +++ b/src/ripple/core/impl/DatabaseCon.cpp @@ -28,7 +28,7 @@ namespace ripple { DatabaseCon::Setup -setup_DatabaseCon(Config const& c) +setup_DatabaseCon(Config const& c, boost::optional j) { DatabaseCon::Setup setup; @@ -40,9 +40,9 @@ setup_DatabaseCon(Config const& c) Throw("database_path must be set."); } - if (!setup.CommonPragma) + if (!setup.globalPragma) { - setup.CommonPragma = [&c]() { + setup.globalPragma = [&c, &j]() { auto const& sqlite = c.section("sqlite"); auto result = std::make_unique>(); result->reserve(3); @@ -53,8 +53,8 @@ setup_DatabaseCon(Config const& c) std::string synchronous = "normal"; std::string temp_store = "file"; - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF && - set(safety_level, "safety_level", sqlite)) + bool showWarning = false; + if (set(safety_level, "safety_level", sqlite)) { if (boost::iequals(safety_level, "low")) { @@ -62,6 +62,7 @@ setup_DatabaseCon(Config const& c) journal_mode = "memory"; synchronous = "off"; temp_store = "memory"; + showWarning = true; } else if (!boost::iequals(safety_level, "high")) { @@ -70,70 +71,87 @@ setup_DatabaseCon(Config const& c) } } - // #journal_mode Valid values : delete, truncate, persist, memory, - // wal, off - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(journal_mode, "journal_mode", sqlite); - if (boost::iequals(journal_mode, "delete") || - boost::iequals(journal_mode, "truncate") || - boost::iequals(journal_mode, "persist") || - boost::iequals(journal_mode, "memory") || - boost::iequals(journal_mode, "wal") || - boost::iequals(journal_mode, "off")) { - result->emplace_back(boost::str( - boost::format(CommonDBPragmaJournal) % journal_mode)); - } - else - { - Throw( - "Invalid journal_mode value: " + journal_mode); + // #journal_mode Valid values : delete, truncate, persist, + // memory, wal, off + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(journal_mode, "journal_mode", sqlite); + bool higherRisk = boost::iequals(journal_mode, "memory") || + boost::iequals(journal_mode, "off"); + showWarning = showWarning || higherRisk; + if (higherRisk || boost::iequals(journal_mode, "delete") || + boost::iequals(journal_mode, "truncate") || + boost::iequals(journal_mode, "persist") || + boost::iequals(journal_mode, "wal")) + { + result->emplace_back(boost::str( + boost::format(CommonDBPragmaJournal) % journal_mode)); + } + else + { + Throw( + "Invalid journal_mode value: " + journal_mode); + } } - //#synchronous Valid values : off, normal, full, extra - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(synchronous, "synchronous", sqlite); - if (boost::iequals(synchronous, "off") || - boost::iequals(synchronous, "normal") || - boost::iequals(synchronous, "full") || - boost::iequals(synchronous, "extra")) - { - result->emplace_back(boost::str( - boost::format(CommonDBPragmaSync) % synchronous)); - } - else { - Throw( - "Invalid synchronous value: " + synchronous); + //#synchronous Valid values : off, normal, full, extra + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(synchronous, "synchronous", sqlite); + bool higherRisk = boost::iequals(synchronous, "off"); + showWarning = showWarning || higherRisk; + if (higherRisk || boost::iequals(synchronous, "normal") || + boost::iequals(synchronous, "full") || + boost::iequals(synchronous, "extra")) + { + result->emplace_back(boost::str( + boost::format(CommonDBPragmaSync) % synchronous)); + } + else + { + Throw( + "Invalid synchronous value: " + synchronous); + } } - // #temp_store Valid values : default, file, memory - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(temp_store, "temp_store", sqlite); - if (boost::iequals(temp_store, "default") || - boost::iequals(temp_store, "file") || - boost::iequals(temp_store, "memory")) { - result->emplace_back( - boost::str(boost::format(CommonDBPragmaTemp) % temp_store)); + // #temp_store Valid values : default, file, memory + if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) + set(temp_store, "temp_store", sqlite); + bool higherRisk = boost::iequals(temp_store, "memory"); + showWarning = showWarning || higherRisk; + if (higherRisk || boost::iequals(temp_store, "default") || + boost::iequals(temp_store, "file")) + { + result->emplace_back(boost::str( + boost::format(CommonDBPragmaTemp) % temp_store)); + } + else + { + Throw( + "Invalid temp_store value: " + temp_store); + } } - else + + if (showWarning && j && c.LEDGER_HISTORY > SQLITE_TUNING_CUTOFF) { - Throw( - "Invalid temp_store value: " + temp_store); + JLOG(j->warn()) + << "reducing the data integrity guarantees from the " + "default [sqlite] behavior is not recommended for " + "nodes storing large amounts of history, because of the " + "difficulty inherent in rebuilding corrupted data."; } - assert(result->size() == 3); return result; }(); } + setup.commonPragma = setup.globalPragma; return setup; } -std::unique_ptr const> - DatabaseCon::Setup::CommonPragma; -const std::vector DatabaseCon::Setup::NoCommonPragma; +std::shared_ptr const> + DatabaseCon::Setup::globalPragma; void DatabaseCon::setupCheckpointing(JobQueue* q, Logs& l) diff --git a/src/ripple/net/impl/DatabaseBody.ipp b/src/ripple/net/impl/DatabaseBody.ipp index 805bb6dfe8b..659ab8a0f70 100644 --- a/src/ripple/net/impl/DatabaseBody.ipp +++ b/src/ripple/net/impl/DatabaseBody.ipp @@ -50,10 +50,11 @@ DatabaseBody::value_type::open( auto setup = setup_DatabaseCon(config); setup.dataDir = path.parent_path(); + setup.noPragma(); // Downloader ignores the "CommonPragma" conn_ = std::make_unique( - setup, "Download", false, DownloaderDBPragma, DatabaseBodyDBInit); + setup, "Download", DownloaderDBPragma, DatabaseBodyDBInit); path_ = path; diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index 01088f98365..e0bcfcef3c8 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -124,11 +124,11 @@ Shard::open(Scheduler& scheduler, nudb::context& ctx) setup.startUp = config.START_UP; setup.standAlone = config.standalone(); setup.dataDir = dir_; + setup.usePragma(); acquireInfo_->SQLiteDB = std::make_unique( setup, AcquireShardDBName, - true, AcquireShardDBPragma, AcquireShardDBInit); acquireInfo_->SQLiteDB->setupCheckpointing( @@ -684,15 +684,16 @@ Shard::initSQLite(std::lock_guard const&) if (backendComplete_) { + setup.noPragma(); lgrSQLiteDB_ = std::make_unique( - setup, LgrDBName, false, CompleteShardDBPragma, LgrDBInit); + setup, LgrDBName, CompleteShardDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes( config.getValueFor(SizedItem::lgrDBCache, boost::none))); txSQLiteDB_ = std::make_unique( - setup, TxDBName, false, CompleteShardDBPragma, TxDBInit); + setup, TxDBName, CompleteShardDBPragma, TxDBInit); txSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes( @@ -701,15 +702,16 @@ Shard::initSQLite(std::lock_guard const&) else { // The incomplete shard uses a Write Ahead Log for performance + setup.usePragma(); lgrSQLiteDB_ = std::make_unique( - setup, LgrDBName, true, LgrDBPragma, LgrDBInit); + setup, LgrDBName, LgrDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config.getValueFor(SizedItem::lgrDBCache))); lgrSQLiteDB_->setupCheckpointing(&app_.getJobQueue(), app_.logs()); txSQLiteDB_ = std::make_unique( - setup, TxDBName, true, TxDBPragma, TxDBInit); + setup, TxDBName, TxDBPragma, TxDBInit); txSQLiteDB_->getSession() << boost::str( boost::format("PRAGMA cache_size=-%d;") % kilobytes(config.getValueFor(SizedItem::txnDBCache))); diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index 7d03b7d93ec..c638fc436ee 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -256,10 +256,10 @@ class Manifest_test : public beast::unit_test::suite { DatabaseCon::Setup setup; setup.dataDir = getDatabasePath(); + BEAST_EXPECT(!setup.commonPragma); DatabaseCon dbCon( setup, dbName.data(), - false, std::array(), WalletDBInit); diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index 61b84ba90ec..89fb66ff99d 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -51,19 +51,19 @@ class Database_test : public TestBase auto const s = setup_DatabaseCon(env.app().config()); - if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + if (BEAST_EXPECT(s.globalPragma->size() == 3)) { BEAST_EXPECT( - s.CommonPragma->at(0) == "PRAGMA journal_mode=wal;"); + s.globalPragma->at(0) == "PRAGMA journal_mode=wal;"); BEAST_EXPECT( - s.CommonPragma->at(1) == "PRAGMA synchronous=normal;"); + s.globalPragma->at(1) == "PRAGMA synchronous=normal;"); BEAST_EXPECT( - s.CommonPragma->at(2) == "PRAGMA temp_store=file;"); + s.globalPragma->at(2) == "PRAGMA temp_store=file;"); } } { // Low safety level - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); Env env = [&]() { auto p = test::jtx::envconfig(); @@ -76,19 +76,19 @@ class Database_test : public TestBase }(); auto const s = setup_DatabaseCon(env.app().config()); - if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + if (BEAST_EXPECT(s.globalPragma->size() == 3)) { BEAST_EXPECT( - s.CommonPragma->at(0) == "PRAGMA journal_mode=memory;"); + s.globalPragma->at(0) == "PRAGMA journal_mode=memory;"); BEAST_EXPECT( - s.CommonPragma->at(1) == "PRAGMA synchronous=off;"); + s.globalPragma->at(1) == "PRAGMA synchronous=off;"); BEAST_EXPECT( - s.CommonPragma->at(2) == "PRAGMA temp_store=memory;"); + s.globalPragma->at(2) == "PRAGMA temp_store=memory;"); } } { // Override individual settings - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); Env env = [&]() { auto p = test::jtx::envconfig(); @@ -103,20 +103,20 @@ class Database_test : public TestBase }(); auto const s = setup_DatabaseCon(env.app().config()); - if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + if (BEAST_EXPECT(s.globalPragma->size() == 3)) { BEAST_EXPECT( - s.CommonPragma->at(0) == "PRAGMA journal_mode=off;"); + s.globalPragma->at(0) == "PRAGMA journal_mode=off;"); BEAST_EXPECT( - s.CommonPragma->at(1) == "PRAGMA synchronous=extra;"); + s.globalPragma->at(1) == "PRAGMA synchronous=extra;"); BEAST_EXPECT( - s.CommonPragma->at(2) == "PRAGMA temp_store=default;"); + s.globalPragma->at(2) == "PRAGMA temp_store=default;"); } } { // Override individual settings with low safety level // (Low doesn't force the other settings) - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); Env env = [&]() { auto p = test::jtx::envconfig(); @@ -132,19 +132,19 @@ class Database_test : public TestBase }(); auto const s = setup_DatabaseCon(env.app().config()); - if (BEAST_EXPECT(s.CommonPragma->size() == 3)) + if (BEAST_EXPECT(s.globalPragma->size() == 3)) { BEAST_EXPECT( - s.CommonPragma->at(0) == "PRAGMA journal_mode=off;"); + s.globalPragma->at(0) == "PRAGMA journal_mode=off;"); BEAST_EXPECT( - s.CommonPragma->at(1) == "PRAGMA synchronous=extra;"); + s.globalPragma->at(1) == "PRAGMA synchronous=extra;"); BEAST_EXPECT( - s.CommonPragma->at(2) == "PRAGMA temp_store=default;"); + s.globalPragma->at(2) == "PRAGMA temp_store=default;"); } } { // Errors - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); auto p = test::jtx::envconfig(); { @@ -164,7 +164,7 @@ class Database_test : public TestBase } { // Errors - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); auto p = test::jtx::envconfig(); { @@ -184,7 +184,7 @@ class Database_test : public TestBase } { // Errors - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); auto p = test::jtx::envconfig(); { @@ -204,7 +204,7 @@ class Database_test : public TestBase } { // Errors - DatabaseCon::Setup::CommonPragma.reset(); + DatabaseCon::Setup::globalPragma.reset(); auto p = test::jtx::envconfig(); { From 18f5d07973099afc1405268cd16c9992706a6d4c Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Mon, 8 Jun 2020 16:27:23 -0400 Subject: [PATCH 08/13] [fold] add unit tests for log messages added last commit * involved moving some test logging subclasses around to be more easily accessible, and some minor refactoring of Env. --- src/ripple/app/main/Application.cpp | 2 +- src/test/app/LedgerHistory_test.cpp | 51 +----------------- src/test/jtx/CaptureLogs.h | 80 ++++++++++++++++++++++++++++ src/test/jtx/CheckMessageLogs.h | 74 +++++++++++++++++++++++++ src/test/jtx/Env.h | 16 ++++-- src/test/jtx/impl/Env.cpp | 5 +- src/test/nodestore/Database_test.cpp | 24 ++++++++- src/test/server/Server_test.cpp | 55 +------------------ 8 files changed, 193 insertions(+), 114 deletions(-) create mode 100644 src/test/jtx/CaptureLogs.h create mode 100644 src/test/jtx/CheckMessageLogs.h diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index fabcdafe390..47e740dd249 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1022,7 +1022,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp try { - auto setup = setup_DatabaseCon(*config_); + auto setup = setup_DatabaseCon(*config_, m_journal); // transaction database mTxnDB = std::make_unique( diff --git a/src/test/app/LedgerHistory_test.cpp b/src/test/app/LedgerHistory_test.cpp index ac2dcda61b2..f1149a0454c 100644 --- a/src/test/app/LedgerHistory_test.cpp +++ b/src/test/app/LedgerHistory_test.cpp @@ -27,6 +27,7 @@ #include #include #include +#include namespace ripple { namespace test { @@ -34,56 +35,6 @@ namespace test { class LedgerHistory_test : public beast::unit_test::suite { public: - /** Log manager that searches for a specific message substring - */ - class CheckMessageLogs : public Logs - { - std::string msg_; - bool& found_; - - class CheckMessageSink : public beast::Journal::Sink - { - CheckMessageLogs& owner_; - - public: - CheckMessageSink( - beast::severities::Severity threshold, - CheckMessageLogs& owner) - : beast::Journal::Sink(threshold, false), owner_(owner) - { - } - - void - write(beast::severities::Severity level, std::string const& text) - override - { - if (text.find(owner_.msg_) != std::string::npos) - owner_.found_ = true; - } - }; - - public: - /** Constructor - - @param msg The message string to search for - @param found The variable to set to true if the message is found - */ - CheckMessageLogs(std::string msg, bool& found) - : Logs{beast::severities::kDebug} - , msg_{std::move(msg)} - , found_{found} - { - } - - std::unique_ptr - makeSink( - std::string const& partition, - beast::severities::Severity threshold) override - { - return std::make_unique(threshold, *this); - } - }; - /** Generate a new ledger by hand, applying a specific close time offset and optionally inserting a transaction. diff --git a/src/test/jtx/CaptureLogs.h b/src/test/jtx/CaptureLogs.h new file mode 100644 index 00000000000..4a6bb2a7567 --- /dev/null +++ b/src/test/jtx/CaptureLogs.h @@ -0,0 +1,80 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2020 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include + +namespace ripple { +namespace test { + +/** + * @brief Log manager for CaptureSinks. This class holds the stream + * instance that is written to by the sinks. Upon destruction, all + * contents of the stream are assigned to the string specified in the + * ctor + */ +class CaptureLogs : public Logs +{ + std::stringstream strm_; + std::string& result_; + + /** + * @brief sink for writing all log messages to a stringstream + */ + class CaptureSink : public beast::Journal::Sink + { + std::stringstream& strm_; + + public: + CaptureSink( + beast::severities::Severity threshold, + std::stringstream& strm) + : beast::Journal::Sink(threshold, false), strm_(strm) + { + } + + void + write(beast::severities::Severity level, std::string const& text) + override + { + strm_ << text; + } + }; + +public: + explicit CaptureLogs(std::string& result) + : Logs(beast::severities::kInfo), result_(result) + { + } + + ~CaptureLogs() override + { + result_ = strm_.str(); + } + + std::unique_ptr + makeSink( + std::string const& partition, + beast::severities::Severity threshold) override + { + return std::make_unique(threshold, strm_); + } +}; + +} // namespace test +} // namespace ripple diff --git a/src/test/jtx/CheckMessageLogs.h b/src/test/jtx/CheckMessageLogs.h new file mode 100644 index 00000000000..e168bc4fbbd --- /dev/null +++ b/src/test/jtx/CheckMessageLogs.h @@ -0,0 +1,74 @@ +//------------------------------------------------------------------------------ +/* + This file is part of rippled: https://github.com/ripple/rippled + Copyright (c) 2020 Ripple Labs Inc. + + Permission to use, copy, modify, and/or distribute this software for any + purpose with or without fee is hereby granted, provided that the above + copyright notice and this permission notice appear in all copies. + + THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR + ANY SPECIAL , DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +*/ +//============================================================================== + +#include + +namespace ripple { +namespace test { + +/** Log manager that searches for a specific message substring + */ +class CheckMessageLogs : public Logs +{ + std::string msg_; + bool& found_; + + class CheckMessageSink : public beast::Journal::Sink + { + CheckMessageLogs& owner_; + + public: + CheckMessageSink( + beast::severities::Severity threshold, + CheckMessageLogs& owner) + : beast::Journal::Sink(threshold, false), owner_(owner) + { + } + + void + write(beast::severities::Severity level, std::string const& text) + override + { + if (text.find(owner_.msg_) != std::string::npos) + owner_.found_ = true; + } + }; + +public: + /** Constructor + + @param msg The message string to search for + @param found The variable to set to true if the message is found + */ + CheckMessageLogs(std::string msg, bool& found) + : Logs{beast::severities::kDebug}, msg_{std::move(msg)}, found_{found} + { + } + + std::unique_ptr + makeSink( + std::string const& partition, + beast::severities::Severity threshold) override + { + return std::make_unique(threshold, *this); + } +}; + +} // namespace test +} // namespace ripple diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index f06cfbf7a9c..ff34e67a6d1 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -27,6 +27,7 @@ #include #include #include // +#include #include #include #include @@ -131,7 +132,8 @@ class Env AppBundle( beast::unit_test::suite& suite, std::unique_ptr config, - std::unique_ptr logs); + std::unique_ptr logs, + beast::severities::Severity thresh); ~AppBundle(); }; @@ -163,12 +165,14 @@ class Env Env(beast::unit_test::suite& suite_, std::unique_ptr config, FeatureBitset features, - std::unique_ptr logs = nullptr) + std::unique_ptr logs = nullptr, + beast::severities::Severity thresh = beast::severities::kError) : test(suite_) , bundle_( suite_, std::move(config), - logs ? std::move(logs) : std::make_unique(suite_)) + logs ? std::move(logs) : std::make_unique(suite_), + thresh) , journal{bundle_.app->journal("Env")} { memoize(Account::master); @@ -211,11 +215,13 @@ class Env */ Env(beast::unit_test::suite& suite_, std::unique_ptr config, - std::unique_ptr logs = nullptr) + std::unique_ptr logs = nullptr, + beast::severities::Severity thresh = beast::severities::kError) : Env(suite_, std::move(config), supported_amendments(), - std::move(logs)) + std::move(logs), + thresh) { } diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index a9b7c3430ff..1a940c5bd75 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -59,7 +59,8 @@ namespace jtx { Env::AppBundle::AppBundle( beast::unit_test::suite& suite, std::unique_ptr config, - std::unique_ptr logs) + std::unique_ptr logs, + beast::severities::Severity thresh) : AppBundle() { using namespace beast::severities; @@ -72,7 +73,7 @@ Env::AppBundle::AppBundle( owned = make_Application( std::move(config), std::move(logs), std::move(timeKeeper_)); app = owned.get(); - app->logs().threshold(kError); + app->logs().threshold(thresh); if (!app->setup()) Throw("Env::AppBundle: setup failed"); timeKeeper->set(app->getLedgerMaster().getClosedLedger()->info().closeTime); diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index 89fb66ff99d..f4c787b2b4d 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -43,8 +44,14 @@ class Database_test : public TestBase { testcase("Config"); + using namespace ripple::test; using namespace ripple::test::jtx; + auto const integrityWarning = + "reducing the data integrity guarantees from the " + "default [sqlite] behavior is not recommended for " + "nodes storing large amounts of history, because of the " + "difficulty inherent in rebuilding corrupted data."; { // defaults Env env(*this); @@ -65,16 +72,23 @@ class Database_test : public TestBase // Low safety level DatabaseCon::Setup::globalPragma.reset(); + bool found = false; Env env = [&]() { auto p = test::jtx::envconfig(); { auto& section = p->section("sqlite"); section.set("safety_level", "low"); } + p->LEDGER_HISTORY = 100'000'000; - return Env(*this, std::move(p)); + return Env( + *this, + std::move(p), + std::make_unique(integrityWarning, found), + beast::severities::kWarning); }(); + BEAST_EXPECT(found); auto const s = setup_DatabaseCon(env.app().config()); if (BEAST_EXPECT(s.globalPragma->size() == 3)) { @@ -90,6 +104,7 @@ class Database_test : public TestBase // Override individual settings DatabaseCon::Setup::globalPragma.reset(); + bool found = false; Env env = [&]() { auto p = test::jtx::envconfig(); { @@ -99,9 +114,14 @@ class Database_test : public TestBase section.set("temp_store", "default"); } - return Env(*this, std::move(p)); + return Env( + *this, + std::move(p), + std::make_unique(integrityWarning, found), + beast::severities::kWarning); }(); + BEAST_EXPECT(!found); auto const s = setup_DatabaseCon(env.app().config()); if (BEAST_EXPECT(s.globalPragma->size() == 3)) { diff --git a/src/test/server/Server_test.cpp b/src/test/server/Server_test.cpp index ef132d2eb0c..bbe2dcc021b 100644 --- a/src/test/server/Server_test.cpp +++ b/src/test/server/Server_test.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include #include @@ -375,60 +376,6 @@ class Server_test : public beast::unit_test::suite pass(); } - /** - * @brief sink for writing all log messages to a stringstream - */ - class CaptureSink : public beast::Journal::Sink - { - std::stringstream& strm_; - - public: - CaptureSink( - beast::severities::Severity threshold, - std::stringstream& strm) - : beast::Journal::Sink(threshold, false), strm_(strm) - { - } - - void - write(beast::severities::Severity level, std::string const& text) - override - { - strm_ << text; - } - }; - - /** - * @brief Log manager for CaptureSinks. This class holds the stream - * instance that is written to by the sinks. Upon destruction, all - * contents of the stream are assigned to the string specified in the - * ctor - */ - class CaptureLogs : public Logs - { - std::stringstream strm_; - std::string& result_; - - public: - explicit CaptureLogs(std::string& result) - : Logs(beast::severities::kInfo), result_(result) - { - } - - ~CaptureLogs() override - { - result_ = strm_.str(); - } - - std::unique_ptr - makeSink( - std::string const& partition, - beast::severities::Severity threshold) override - { - return std::make_unique(threshold, strm_); - } - }; - void testBadConfig() { From 9173a6a7d3a08be4a7e8345bd763802693a8dcdb Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Fri, 12 Jun 2020 18:06:35 -0400 Subject: [PATCH 09/13] [FOLD] Review feedback from @mtravis, part 1: * Make `safety_level` and other tuning options mutually exclusive * Clean up VACUUM functionality and tuning to use settings. * Fix bugs in the config setup where it still ignored tuning settings if full history. Add tests to catch that mistake. * Fix some bugs in the unit tests, and make them more robustly check error messages. --- cfg/rippled-example.cfg | 9 ++ src/ripple/app/main/Application.cpp | 2 +- src/ripple/app/main/Main.cpp | 30 ++-- src/ripple/core/impl/DatabaseCon.cpp | 43 +++-- src/test/jtx/Env.h | 6 +- src/test/jtx/impl/Env.cpp | 13 +- src/test/nodestore/Database_test.cpp | 224 +++++++++++++++++++++++++-- 7 files changed, 271 insertions(+), 56 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index b572d5ea3c6..21320b0a672 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -993,6 +993,9 @@ # These settings trade speed and reduced I/O # for a higher risk of data loss. See the # individual settings below for more information. +# This setting may not be combined with any of the +# other tuning settings: "journal_mode", +# "synchronous", or "temp_store". # # journal_mode Valid values: delete, truncate, persist, memory, wal, off # The default is "wal", which uses a write-ahead @@ -1002,6 +1005,8 @@ # database is likely to be corrupted. # See https://www.sqlite.org/pragma.html#pragma_journal_mode # for more details about the available options. +# This setting may not be combined with the +# "safety_level" setting. # # synchronous Valid values: off, normal, full, extra # The default is "normal", which works well with @@ -1013,6 +1018,8 @@ # data to disk. # See https://www.sqlite.org/pragma.html#pragma_synchronous # for more details about the available options. +# This setting may not be combined with the +# "safety_level" setting. # # temp_store Valid values: default, file, memory # The default is "file", which will use files @@ -1022,6 +1029,8 @@ # of these temporary objects. # See https://www.sqlite.org/pragma.html#pragma_temp_store # for more details about the available options. +# This setting may not be combined with the +# "safety_level" setting. # # # diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index 47e740dd249..c1125935eb7 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1364,7 +1364,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp JLOG(m_journal.fatal()) << "Free SQLite space for transaction db is less than " "512MB. To fix this, rippled must be executed with the " - "vacuum parameter before restarting. " + "\"--vacuum\" parameter before restarting. " "Note that this activity can take multiple days, " "depending on database size."; signalStop(); diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index 713c68b6851..c73dbda9cc9 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -354,10 +354,7 @@ run(int argc, char** argv) "nodetoshard", "Import node store into shards")( "replay", "Replay a ledger close.")( "start", "Start from a fresh Ledger.")( - "vacuum", - po::value(), - "VACUUM the transaction db. Mandatory string argument specifies " - "temporary directory path.")( + "vacuum", "VACUUM the transaction db.")( "valid", "Consider the initial ledger a valid network ledger.")( "validateShards", shardsText.c_str()); @@ -522,26 +519,26 @@ run(int argc, char** argv) using namespace boost::filesystem; DatabaseCon::Setup dbSetup = setup_DatabaseCon(*config); path dbPath = dbSetup.dataDir / TxDBName; - path tmpPath = vm["vacuum"].as(); try { uintmax_t const dbSize = file_size(dbPath); assert(dbSize != static_cast(-1)); - if (space(tmpPath).available < dbSize) { - std::cerr << "A valid directory for vacuuming must be " - "specified on a filesystem with at least " - "as much free space as the size of " - << dbPath.string() << ", which is " << dbSize - << " bytes. The filesystem for " << tmpPath.string() - << " only has " << space(tmpPath).available - << " bytes.\n"; - return -1; + auto available = space(dbPath.parent_path()).available; + if (available < dbSize) + { + std::cerr + << "The database filesystem must have at least as " + "much free space as the size of " + << dbPath.string() << ", which is " << dbSize + << " bytes. Only " << available + << " bytes are available.\n"; + return -1; + } } - dbSetup.noPragma(); auto txnDB = std::make_unique( dbSetup, TxDBName, TxDBPragma, TxDBInit); auto& session = txnDB->getSession(); @@ -552,9 +549,6 @@ run(int argc, char** argv) std::cout << "VACUUM beginning. page_size: " << pageSize << std::endl; - session << "PRAGMA journal_mode=OFF;"; - session << "PRAGMA temp_store_directory=\"" << tmpPath.string() - << "\";"; session << "VACUUM;"; assert(dbSetup.globalPragma); for (auto const& p : *dbSetup.globalPragma) diff --git a/src/ripple/core/impl/DatabaseCon.cpp b/src/ripple/core/impl/DatabaseCon.cpp index 431ee95b63a..2ace8b2177e 100644 --- a/src/ripple/core/impl/DatabaseCon.cpp +++ b/src/ripple/core/impl/DatabaseCon.cpp @@ -48,21 +48,21 @@ setup_DatabaseCon(Config const& c, boost::optional j) result->reserve(3); // defaults - std::string safety_level = "high"; + std::string safety_level; std::string journal_mode = "wal"; std::string synchronous = "normal"; std::string temp_store = "file"; + bool showRiskWarning = false; - bool showWarning = false; if (set(safety_level, "safety_level", sqlite)) { - if (boost::iequals(safety_level, "low")) + showRiskWarning = boost::iequals(safety_level, "low"); + if (showRiskWarning) { // low safety defaults journal_mode = "memory"; synchronous = "off"; temp_store = "memory"; - showWarning = true; } else if (!boost::iequals(safety_level, "high")) { @@ -74,11 +74,16 @@ setup_DatabaseCon(Config const& c, boost::optional j) { // #journal_mode Valid values : delete, truncate, persist, // memory, wal, off - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(journal_mode, "journal_mode", sqlite); + if (set(journal_mode, "journal_mode", sqlite) && + !safety_level.empty()) + { + Throw( + "Configuration file may not define both " + "\"safety_level\" and \"journal_mode\""); + } bool higherRisk = boost::iequals(journal_mode, "memory") || boost::iequals(journal_mode, "off"); - showWarning = showWarning || higherRisk; + showRiskWarning = showRiskWarning || higherRisk; if (higherRisk || boost::iequals(journal_mode, "delete") || boost::iequals(journal_mode, "truncate") || boost::iequals(journal_mode, "persist") || @@ -96,10 +101,15 @@ setup_DatabaseCon(Config const& c, boost::optional j) { //#synchronous Valid values : off, normal, full, extra - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(synchronous, "synchronous", sqlite); + if (set(synchronous, "synchronous", sqlite) && + !safety_level.empty()) + { + Throw( + "Configuration file may not define both " + "\"safety_level\" and \"synchronous\""); + } bool higherRisk = boost::iequals(synchronous, "off"); - showWarning = showWarning || higherRisk; + showRiskWarning = showRiskWarning || higherRisk; if (higherRisk || boost::iequals(synchronous, "normal") || boost::iequals(synchronous, "full") || boost::iequals(synchronous, "extra")) @@ -116,10 +126,15 @@ setup_DatabaseCon(Config const& c, boost::optional j) { // #temp_store Valid values : default, file, memory - if (c.LEDGER_HISTORY < SQLITE_TUNING_CUTOFF) - set(temp_store, "temp_store", sqlite); + if (set(temp_store, "temp_store", sqlite) && + !safety_level.empty()) + { + Throw( + "Configuration file may not define both " + "\"safety_level\" and \"temp_store\""); + } bool higherRisk = boost::iequals(temp_store, "memory"); - showWarning = showWarning || higherRisk; + showRiskWarning = showRiskWarning || higherRisk; if (higherRisk || boost::iequals(temp_store, "default") || boost::iequals(temp_store, "file")) { @@ -133,7 +148,7 @@ setup_DatabaseCon(Config const& c, boost::optional j) } } - if (showWarning && j && c.LEDGER_HISTORY > SQLITE_TUNING_CUTOFF) + if (showRiskWarning && j && c.LEDGER_HISTORY > SQLITE_TUNING_CUTOFF) { JLOG(j->warn()) << "reducing the data integrity guarantees from the " diff --git a/src/test/jtx/Env.h b/src/test/jtx/Env.h index ff34e67a6d1..f2934bb5002 100644 --- a/src/test/jtx/Env.h +++ b/src/test/jtx/Env.h @@ -168,11 +168,7 @@ class Env std::unique_ptr logs = nullptr, beast::severities::Severity thresh = beast::severities::kError) : test(suite_) - , bundle_( - suite_, - std::move(config), - logs ? std::move(logs) : std::make_unique(suite_), - thresh) + , bundle_(suite_, std::move(config), std::move(logs), thresh) , journal{bundle_.app->journal("Env")} { memoize(Account::master); diff --git a/src/test/jtx/impl/Env.cpp b/src/test/jtx/impl/Env.cpp index 1a940c5bd75..855dfe7bbf0 100644 --- a/src/test/jtx/impl/Env.cpp +++ b/src/test/jtx/impl/Env.cpp @@ -64,8 +64,17 @@ Env::AppBundle::AppBundle( : AppBundle() { using namespace beast::severities; - // Use kFatal threshold to reduce noise from STObject. - setDebugLogSink(std::make_unique("Debug", kFatal, suite)); + if (logs) + { + setDebugLogSink(logs->makeSink("Debug", kFatal)); + } + else + { + logs = std::make_unique(suite); + // Use kFatal threshold to reduce noise from STObject. + setDebugLogSink( + std::make_unique("Debug", kFatal, suite)); + } auto timeKeeper_ = std::make_unique(); timeKeeper = timeKeeper_.get(); // Hack so we don't have to call Config::setup diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index f4c787b2b4d..1c7921ae130 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -68,6 +68,38 @@ class Database_test : public TestBase s.globalPragma->at(2) == "PRAGMA temp_store=file;"); } } + { + // High safety level + DatabaseCon::Setup::globalPragma.reset(); + + bool found = false; + Env env = [&]() { + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "high"); + } + p->LEDGER_HISTORY = 100'000'000; + + return Env( + *this, + std::move(p), + std::make_unique(integrityWarning, found), + beast::severities::kWarning); + }(); + + BEAST_EXPECT(!found); + auto const s = setup_DatabaseCon(env.app().config()); + if (BEAST_EXPECT(s.globalPragma->size() == 3)) + { + BEAST_EXPECT( + s.globalPragma->at(0) == "PRAGMA journal_mode=wal;"); + BEAST_EXPECT( + s.globalPragma->at(1) == "PRAGMA synchronous=normal;"); + BEAST_EXPECT( + s.globalPragma->at(2) == "PRAGMA temp_store=file;"); + } + } { // Low safety level DatabaseCon::Setup::globalPragma.reset(); @@ -121,6 +153,8 @@ class Database_test : public TestBase beast::severities::kWarning); }(); + // No warning, even though higher risk settings were used because + // LEDGER_HISTORY is small BEAST_EXPECT(!found); auto const s = setup_DatabaseCon(env.app().config()); if (BEAST_EXPECT(s.globalPragma->size() == 3)) @@ -134,23 +168,30 @@ class Database_test : public TestBase } } { - // Override individual settings with low safety level - // (Low doesn't force the other settings) + // Override individual settings with large history DatabaseCon::Setup::globalPragma.reset(); + bool found = false; Env env = [&]() { auto p = test::jtx::envconfig(); { auto& section = p->section("sqlite"); - section.set("safety_level", "low"); section.set("journal_mode", "off"); section.set("synchronous", "extra"); section.set("temp_store", "default"); } + p->LEDGER_HISTORY = 50'000'000; - return Env(*this, std::move(p)); + return Env( + *this, + std::move(p), + std::make_unique(integrityWarning, found), + beast::severities::kWarning); }(); + // No warning, even though higher risk settings were used because + // LEDGER_HISTORY is small + BEAST_EXPECT(found); auto const s = setup_DatabaseCon(env.app().config()); if (BEAST_EXPECT(s.globalPragma->size() == 3)) { @@ -163,8 +204,131 @@ class Database_test : public TestBase } } { - // Errors + // Error: Mix safety_level and individual settings + DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: " + "Configuration file may not define both \"safety_level\" and " + "\"journal_mode\""; + bool found = false; + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "low"); + section.set("journal_mode", "off"); + section.set("synchronous", "extra"); + section.set("temp_store", "default"); + } + + try + { + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); + fail(); + } + catch (...) + { + BEAST_EXPECT(found); + } + } + { + // Error: Mix safety_level and one setting (gotta catch 'em all) + DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Configuration file may " + "not define both \"safety_level\" and \"journal_mode\""; + bool found = false; + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "high"); + section.set("journal_mode", "off"); + } + + try + { + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); + fail(); + } + catch (...) + { + BEAST_EXPECT(found); + } + } + { + // Error: Mix safety_level and one setting (gotta catch 'em all) + DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Configuration file may " + "not define both \"safety_level\" and \"synchronous\""; + bool found = false; + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "low"); + section.set("synchronous", "extra"); + } + + try + { + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); + fail(); + } + catch (...) + { + BEAST_EXPECT(found); + } + } + { + // Error: Mix safety_level and one setting (gotta catch 'em all) DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Configuration file may " + "not define both \"safety_level\" and \"temp_store\""; + bool found = false; + + auto p = test::jtx::envconfig(); + { + auto& section = p->section("sqlite"); + section.set("safety_level", "high"); + section.set("temp_store", "default"); + } + + try + { + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); + fail(); + } + catch (...) + { + BEAST_EXPECT(found); + } + } + { + // Error: Invalid value + DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Invalid safety_level " + "value: slow"; + bool found = false; auto p = test::jtx::envconfig(); { @@ -174,17 +338,25 @@ class Database_test : public TestBase try { - Env env(*this, std::move(p)); + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); fail(); } catch (...) { - pass(); + BEAST_EXPECT(found); } } { - // Errors + // Error: Invalid value DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Invalid journal_mode " + "value: fast"; + bool found = false; auto p = test::jtx::envconfig(); { @@ -194,17 +366,25 @@ class Database_test : public TestBase try { - Env env(*this, std::move(p)); + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); fail(); } catch (...) { - pass(); + BEAST_EXPECT(found); } } { - // Errors + // Error: Invalid value DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Invalid synchronous " + "value: instant"; + bool found = false; auto p = test::jtx::envconfig(); { @@ -214,17 +394,25 @@ class Database_test : public TestBase try { - Env env(*this, std::move(p)); + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); fail(); } catch (...) { - pass(); + BEAST_EXPECT(found); } } { - // Errors + // Error: Invalid value DatabaseCon::Setup::globalPragma.reset(); + auto const expected = + "Failed to initialize SQLite databases: Invalid temp_store " + "value: network"; + bool found = false; auto p = test::jtx::envconfig(); { @@ -234,12 +422,16 @@ class Database_test : public TestBase try { - Env env(*this, std::move(p)); + Env env( + *this, + std::move(p), + std::make_unique(expected, found), + beast::severities::kWarning); fail(); } catch (...) { - pass(); + BEAST_EXPECT(found); } } } From 10e02e6b1acdab97808760fb933ae2f8a045458a Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Tue, 16 Jun 2020 14:46:12 -0400 Subject: [PATCH 10/13] [FOLD] Review feedback from @mtravis, part 2: * Change some log message levels. * clearSql no longer returns a bool that every caller ignores. * Restructure a few unnecessary validity checks --- src/ripple/app/misc/SHAMapStoreImp.cpp | 32 ++++++++++---------------- src/ripple/app/misc/SHAMapStoreImp.h | 2 +- 2 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index 460e81eb5bf..afbd713e6c6 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -380,7 +380,7 @@ SHAMapStoreImp::run() default:; } - JLOG(journal_.trace()) << "copying ledger " << validatedSeq; + JLOG(journal_.debug()) << "copying ledger " << validatedSeq; std::uint64_t nodeCount = 0; validatedLedger->stateMap().snapShot(false)->visitNodes(std::bind( &SHAMapStoreImp::copyNode, @@ -401,7 +401,7 @@ SHAMapStoreImp::run() JLOG(journal_.debug()) << "copied ledger " << validatedSeq << " nodecount " << nodeCount; - JLOG(journal_.trace()) << "freshening caches"; + JLOG(journal_.debug()) << "freshening caches"; freshenCaches(); switch (health()) { @@ -566,7 +566,7 @@ SHAMapStoreImp::makeBackendRotating(std::string path) return backend; } -bool +void SHAMapStoreImp::clearSql( DatabaseCon& database, LedgerIndex lastRotated, @@ -580,26 +580,23 @@ SHAMapStoreImp::clearSql( boost::optional m; JLOG(journal_.trace()) << "Begin: Look up lowest value of: " << minQuery; - if (auto db = database.checkoutDb()) - *db << minQuery, soci::into(m); - else { - assert(false); - return false; + auto db = database.checkoutDb(); + *db << minQuery, soci::into(m); } JLOG(journal_.trace()) << "End: Look up lowest value of: " << minQuery; if (!m) - return false; + return; min = *m; } if (min > lastRotated || health() != Health::ok) - return false; + return; if (min == lastRotated) { // Micro-optimization mainly to clarify logs JLOG(journal_.trace()) << "Nothing to delete from " << deleteQuery; - return true; + return; } boost::format formattedDeleteQuery(deleteQuery); @@ -612,27 +609,22 @@ SHAMapStoreImp::clearSql( JLOG(journal_.trace()) << "Begin: Delete up to " << deleteBatch_ << " rows with LedgerSeq < " << min << " using query: " << deleteQuery; - if (auto db = database.checkoutDb()) { + auto db = database.checkoutDb(); *db << boost::str(formattedDeleteQuery % min); } - else - { - assert(false); - return false; - } JLOG(journal_.trace()) << "End: Delete up to " << deleteBatch_ << " rows with LedgerSeq < " << min << " using query: " << deleteQuery; if (health()) - return true; + return; if (min < lastRotated) std::this_thread::sleep_for(backOff_); if (health()) - return true; + return; } JLOG(journal_.debug()) << "finished: " << deleteQuery; - return true; + return; } void diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index a5f4b7ace89..e1e75fb10f8 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -220,7 +220,7 @@ class SHAMapStoreImp : public SHAMapStore * @return true if any deletable rows were found (though not * necessarily deleted. */ - bool + void clearSql( DatabaseCon& database, LedgerIndex lastRotated, From e060249e68c38fcdcc4841c1e99f052fa120ee06 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Tue, 16 Jun 2020 18:39:13 -0400 Subject: [PATCH 11/13] [FOLD] Remove an unnecessary health check --- src/ripple/app/misc/SHAMapStoreImp.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index afbd713e6c6..d5022e1c428 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -648,9 +648,6 @@ SHAMapStoreImp::freshenCaches() void SHAMapStoreImp::clearPrior(LedgerIndex lastRotated) { - if (health()) - return; - // Do not allow ledgers to be acquired from the network // that are about to be deleted. minimumOnline_ = lastRotated + 1; From c1fff2e8113321fdca9bd8dfbae678e61841e76b Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Tue, 23 Jun 2020 18:30:09 -0400 Subject: [PATCH 12/13] [FOLD] Review feedback from @seelabs and @mtrippled: * Rename retry_buffer_seconds to retry_wait_seconds. * Improve documentation and examples in example.cfg file. * Simplify the DatabaseCon::Setup pragma handling. * Other misc cleanups --- cfg/rippled-example.cfg | 37 ++++++++++------- src/ripple/app/main/Application.cpp | 2 +- src/ripple/app/main/DBInit.h | 16 ++------ src/ripple/app/main/Main.cpp | 25 ++++++------ src/ripple/app/misc/NetworkOPs.cpp | 6 +-- src/ripple/app/misc/SHAMapStoreImp.cpp | 11 +++--- src/ripple/app/misc/SHAMapStoreImp.h | 18 ++++++--- src/ripple/core/DatabaseCon.h | 55 ++++++++++++++------------ src/ripple/core/impl/DatabaseCon.cpp | 8 ++-- src/ripple/net/impl/DatabaseBody.ipp | 2 +- src/ripple/nodestore/impl/Shard.cpp | 16 ++++---- src/test/app/LedgerHistory_test.cpp | 8 ++-- src/test/app/Manifest_test.cpp | 2 +- src/test/jtx/CaptureLogs.h | 8 ++-- src/test/jtx/CheckMessageLogs.h | 11 +++--- src/test/nodestore/Database_test.cpp | 28 +++++++------ src/test/server/Server_test.cpp | 12 +++--- 17 files changed, 140 insertions(+), 125 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 21320b0a672..2809bb6b32d 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -913,13 +913,13 @@ # number of seconds. # Default is 60. # -# recovery_buffer_seconds +# recovery_wait_seconds # The online delete process checks periodically # that rippled is still in sync with the network, # and that the validated ledger is less than # 'age_threshold_seconds' old. By default, if it # is not the online delete process aborts and -# tries again later. If 'recovery_buffer_seconds' +# tries again later. If 'recovery_wait_seconds' # is set and rippled is out of sync, but likely to # recover quickly, then online delete will wait # this number of seconds for rippled to get back @@ -967,30 +967,37 @@ # '=' # ... # -# Example: +# Example 1: # sync_level=low +# +# Example 2: # journal_mode=off +# synchronous=off # # WARNING: These settings can have significant effects on data integrity, -# particularly in failure scenarios. It is strongly recommended that they -# be left at their defaults unless the server is having performance issues -# during normal operation or during automatic purging (online_delete) -# operations. A warning will be logged on startup if 'ledger_history' -# is configured to store more than 10,000,000 ledgers and any of these -# settings are less safe than the default. This is due to the inordinate -# amount of time and bandwidth it will take to safely rebuild a corrupted -# database from other peers. +# particularly in systemic failure scenarios. It is strongly recommended +# that they be left at their defaults unless the server is having +# performance issues during normal operation or during automatic purging +# (online_delete) operations. A warning will be logged on startup if +# 'ledger_history' is configured to store more than 10,000,000 ledgers and +# any of these settings are less safe than the default. This is due to the +# inordinate amount of time and bandwidth it will take to safely rebuild a +# corrupted database of that size from other peers. # # Optional keys: # # safety_level Valid values: high, low -# The default is "high", and tunes the SQLite -# databases in the most reliable mode. "low" -# is equivalent to +# The default is "high", which tunes the SQLite +# databases in the most reliable mode, and is +# equivalent to: +# journal_mode=wal +# synchronous=normal +# temp_store=file +# "low" is equivalent to: # journal_mode=memory # synchronous=off # temp_store=memory -# These settings trade speed and reduced I/O +# These "low" settings trade speed and reduced I/O # for a higher risk of data loss. See the # individual settings below for more information. # This setting may not be combined with any of the diff --git a/src/ripple/app/main/Application.cpp b/src/ripple/app/main/Application.cpp index c1125935eb7..0db9051f5e6 100644 --- a/src/ripple/app/main/Application.cpp +++ b/src/ripple/app/main/Application.cpp @@ -1072,7 +1072,7 @@ class ApplicationImp : public Application, public RootStoppable, public BasicApp mLedgerDB->setupCheckpointing(m_jobQueue.get(), logs()); // wallet database - setup.noPragma(); + setup.useGlobalPragma = false; mWalletDB = std::make_unique( setup, WalletDBName, diff --git a/src/ripple/app/main/DBInit.h b/src/ripple/app/main/DBInit.h index fc81b1fc551..0a561be8834 100644 --- a/src/ripple/app/main/DBInit.h +++ b/src/ripple/app/main/DBInit.h @@ -71,21 +71,13 @@ inline constexpr std::array LgrDBInit{ // Transaction database holds transactions and public keys inline constexpr auto TxDBName{"transaction.db"}; -inline constexpr -#if (ULONG_MAX > UINT_MAX) && !defined(NO_SQLITE_MMAP) - std::array - TxDBPragma +inline constexpr std::array TxDBPragma { - { -#else - std::array TxDBPragma {{ -#endif - "PRAGMA page_size=4096;", "PRAGMA journal_size_limit=1582080;", - "PRAGMA max_page_count=2147483646;", + "PRAGMA page_size=4096;", "PRAGMA journal_size_limit=1582080;", + "PRAGMA max_page_count=2147483646;", #if (ULONG_MAX > UINT_MAX) && !defined(NO_SQLITE_MMAP) - "PRAGMA mmap_size=17179869184;" + "PRAGMA mmap_size=17179869184;" #endif - } }; inline constexpr std::array TxDBInit{ diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index c73dbda9cc9..e8ed917587f 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -517,7 +517,7 @@ run(int argc, char** argv) } using namespace boost::filesystem; - DatabaseCon::Setup dbSetup = setup_DatabaseCon(*config); + DatabaseCon::Setup const dbSetup = setup_DatabaseCon(*config); path dbPath = dbSetup.dataDir / TxDBName; try @@ -525,18 +525,15 @@ run(int argc, char** argv) uintmax_t const dbSize = file_size(dbPath); assert(dbSize != static_cast(-1)); + if (auto available = space(dbPath.parent_path()).available; + available < dbSize) { - auto available = space(dbPath.parent_path()).available; - if (available < dbSize) - { - std::cerr - << "The database filesystem must have at least as " - "much free space as the size of " - << dbPath.string() << ", which is " << dbSize - << " bytes. Only " << available - << " bytes are available.\n"; - return -1; - } + std::cerr << "The database filesystem must have at least as " + "much free space as the size of " + << dbPath.string() << ", which is " << dbSize + << " bytes. Only " << available + << " bytes are available.\n"; + return -1; } auto txnDB = std::make_unique( @@ -544,6 +541,10 @@ run(int argc, char** argv) auto& session = txnDB->getSession(); std::uint32_t pageSize; + // Only the most trivial databases will fit in memory on typical + // (recommended) software. Force temp files to be written to disk + // regardless of the config settings. + session << boost::format(CommonDBPragmaTemp) % "file"; session << "PRAGMA page_size;", soci::into(pageSize); std::cout << "VACUUM beginning. page_size: " << pageSize diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index 5c3aadc0ec0..0dc0771d244 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2757,12 +2757,12 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) if (std::abs(closeOffset.count()) >= 60) l[jss::close_time_offset] = closeOffset.count(); - constexpr std::chrono::seconds HIGH_AGE_THRESHOLD{1000000}; + constexpr std::chrono::seconds highAgeThreshold{1000000}; if (m_ledgerMaster.haveValidated()) { auto const age = m_ledgerMaster.getValidatedLedgerAge(); l[jss::age] = - Json::UInt(age < HIGH_AGE_THRESHOLD ? age.count() : 0); + Json::UInt(age < highAgeThreshold ? age.count() : 0); } else { @@ -2773,7 +2773,7 @@ NetworkOPsImp::getServerInfo(bool human, bool admin, bool counters) using namespace std::chrono_literals; auto age = closeTime - lCloseTime; l[jss::age] = - Json::UInt(age < HIGH_AGE_THRESHOLD ? age.count() : 0); + Json::UInt(age < highAgeThreshold ? age.count() : 0); } } } diff --git a/src/ripple/app/misc/SHAMapStoreImp.cpp b/src/ripple/app/misc/SHAMapStoreImp.cpp index d5022e1c428..590decf1924 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.cpp +++ b/src/ripple/app/misc/SHAMapStoreImp.cpp @@ -195,8 +195,8 @@ SHAMapStoreImp::SHAMapStoreImp( } if (get_if_exists(section, "age_threshold_seconds", temp)) ageThreshold_ = std::chrono::seconds{temp}; - if (get_if_exists(section, "recovery_buffer_seconds", temp)) - recoveryBuffer_.emplace(std::chrono::seconds{temp}); + if (get_if_exists(section, "recovery_wait_seconds", temp)) + recoveryWaitTime_.emplace(std::chrono::seconds{temp}); get_if_exists(section, "advisory_delete", advisoryDelete_); @@ -624,7 +624,6 @@ SHAMapStoreImp::clearSql( return; } JLOG(journal_.debug()) << "finished: " << deleteQuery; - return; } void @@ -700,15 +699,15 @@ SHAMapStoreImp::health() { auto age = ledgerMaster_->getValidatedLedgerAge(); OperatingMode mode = netOPs_->getOperatingMode(); - if (recoveryBuffer_ && mode == OperatingMode::SYNCING && + if (recoveryWaitTime_ && mode == OperatingMode::SYNCING && age < ageThreshold_) { JLOG(journal_.warn()) - << "Waiting " << recoveryBuffer_->count() + << "Waiting " << recoveryWaitTime_->count() << "s for node to get back into sync with network. state: " << app_.getOPs().strOperatingMode(mode, false) << ". age " << age.count() << 's'; - std::this_thread::sleep_for(*recoveryBuffer_); + std::this_thread::sleep_for(*recoveryWaitTime_); age = ledgerMaster_->getValidatedLedgerAge(); mode = netOPs_->getOperatingMode(); diff --git a/src/ripple/app/misc/SHAMapStoreImp.h b/src/ripple/app/misc/SHAMapStoreImp.h index e1e75fb10f8..6145cb48dfd 100644 --- a/src/ripple/app/misc/SHAMapStoreImp.h +++ b/src/ripple/app/misc/SHAMapStoreImp.h @@ -109,7 +109,12 @@ class SHAMapStoreImp : public SHAMapStore std::uint32_t deleteBatch_ = 100; std::chrono::milliseconds backOff_{100}; std::chrono::seconds ageThreshold_{60}; - boost::optional recoveryBuffer_{}; + /// If set, and the node is out of sync during an + /// online_delete health check, sleep the thread + /// for this time and check again so the node can + /// recover. + /// See also: "recovery_wait_seconds" in rippled-example.cfg + boost::optional recoveryWaitTime_; // these do not exist upon SHAMapStore creation, but do exist // as of onPrepare() or before @@ -214,11 +219,9 @@ class SHAMapStoreImp : public SHAMapStore return false; } - /** delete from sqlite table in batches to not lock the db excessively - * pause briefly to extend access time to other users - * call with mutex object unlocked - * @return true if any deletable rows were found (though not - * necessarily deleted. + /** delete from sqlite table in batches to not lock the db excessively. + * Pause briefly to extend access time to other users. + * Call with mutex object unlocked. */ void clearSql( @@ -238,6 +241,9 @@ class SHAMapStoreImp : public SHAMapStore // Assume that, once unhealthy, a necessary step has been // aborted, so the online-delete process needs to restart // at next ledger. + // If recoveryWaitTime_ is set, this may sleep to give rippled + // time to recover, so never call it from any thread other than + // the main "run()". Health health(); // diff --git a/src/ripple/core/DatabaseCon.h b/src/ripple/core/DatabaseCon.h index debcdf2a400..c6efc55aa54 100644 --- a/src/ripple/core/DatabaseCon.h +++ b/src/ripple/core/DatabaseCon.h @@ -90,22 +90,19 @@ class DatabaseCon Config::StartUpType startUp = Config::NORMAL; bool standAlone = false; boost::filesystem::path dataDir; - // If unseated, then the `globalPragma` are not used, - // otherwise should point to `globalPragma` - std::shared_ptr const> commonPragma; - void - noPragma() - { - commonPragma.reset(); - } - void - usePragma() + // Indicates whether or not to return the `globalPragma` + // from commonPragma() + bool useGlobalPragma = false; + + std::vector const* + commonPragma() const { - assert(globalPragma); - commonPragma = globalPragma; + assert(!useGlobalPragma || globalPragma); + return useGlobalPragma && globalPragma ? globalPragma.get() + : nullptr; } - static std::shared_ptr const> globalPragma; + static std::unique_ptr const> globalPragma; }; template @@ -114,16 +111,18 @@ class DatabaseCon std::string const& DBName, std::array const& pragma, std::array const& initSQL) - { // Use temporary files or regular DB files? - auto const useTempFiles = setup.standAlone && - setup.startUp != Config::LOAD && - setup.startUp != Config::LOAD_FILE && - setup.startUp != Config::REPLAY; - boost::filesystem::path pPath = - useTempFiles ? "" : (setup.dataDir / DBName); - - init(pPath, setup.commonPragma, pragma, initSQL); + : DatabaseCon( + {}, + setup.standAlone && setup.startUp != Config::LOAD && + setup.startUp != Config::LOAD_FILE && + setup.startUp != Config::REPLAY + ? "" + : (setup.dataDir / DBName), + setup.commonPragma(), + pragma, + initSQL) + { } template @@ -132,8 +131,8 @@ class DatabaseCon std::string const& DBName, std::array const& pragma, std::array const& initSQL) + : DatabaseCon({}, dataDir / DBName, {}, pragma, initSQL) { - init((dataDir / DBName), {}, pragma, initSQL); } soci::session& @@ -152,11 +151,15 @@ class DatabaseCon setupCheckpointing(JobQueue*, Logs&); private: + class Base + { + }; + template - void - init( + DatabaseCon( + Base, boost::filesystem::path const& pPath, - std::shared_ptr const> const& commonPragma, + std::vector const* commonPragma, std::array const& pragma, std::array const& initSQL) { diff --git a/src/ripple/core/impl/DatabaseCon.cpp b/src/ripple/core/impl/DatabaseCon.cpp index 2ace8b2177e..89c4ee1f291 100644 --- a/src/ripple/core/impl/DatabaseCon.cpp +++ b/src/ripple/core/impl/DatabaseCon.cpp @@ -56,13 +56,13 @@ setup_DatabaseCon(Config const& c, boost::optional j) if (set(safety_level, "safety_level", sqlite)) { - showRiskWarning = boost::iequals(safety_level, "low"); - if (showRiskWarning) + if (boost::iequals(safety_level, "low")) { // low safety defaults journal_mode = "memory"; synchronous = "off"; temp_store = "memory"; + showRiskWarning = true; } else if (!boost::iequals(safety_level, "high")) { @@ -160,12 +160,12 @@ setup_DatabaseCon(Config const& c, boost::optional j) return result; }(); } - setup.commonPragma = setup.globalPragma; + setup.useGlobalPragma = true; return setup; } -std::shared_ptr const> +std::unique_ptr const> DatabaseCon::Setup::globalPragma; void diff --git a/src/ripple/net/impl/DatabaseBody.ipp b/src/ripple/net/impl/DatabaseBody.ipp index 659ab8a0f70..5a1bd7e6185 100644 --- a/src/ripple/net/impl/DatabaseBody.ipp +++ b/src/ripple/net/impl/DatabaseBody.ipp @@ -50,7 +50,7 @@ DatabaseBody::value_type::open( auto setup = setup_DatabaseCon(config); setup.dataDir = path.parent_path(); - setup.noPragma(); + setup.useGlobalPragma = false; // Downloader ignores the "CommonPragma" conn_ = std::make_unique( diff --git a/src/ripple/nodestore/impl/Shard.cpp b/src/ripple/nodestore/impl/Shard.cpp index e0bcfcef3c8..f8799ff3d27 100644 --- a/src/ripple/nodestore/impl/Shard.cpp +++ b/src/ripple/nodestore/impl/Shard.cpp @@ -124,7 +124,7 @@ Shard::open(Scheduler& scheduler, nudb::context& ctx) setup.startUp = config.START_UP; setup.standAlone = config.standalone(); setup.dataDir = dir_; - setup.usePragma(); + setup.useGlobalPragma = true; acquireInfo_->SQLiteDB = std::make_unique( setup, @@ -669,10 +669,14 @@ bool Shard::initSQLite(std::lock_guard const&) { Config const& config{app_.config()}; - DatabaseCon::Setup setup; - setup.startUp = config.START_UP; - setup.standAlone = config.standalone(); - setup.dataDir = dir_; + DatabaseCon::Setup const setup = [&]() { + DatabaseCon::Setup result; + result.startUp = config.START_UP; + result.standAlone = config.standalone(); + result.dataDir = dir_; + result.useGlobalPragma = !backendComplete_; + return result; + }(); try { @@ -684,7 +688,6 @@ Shard::initSQLite(std::lock_guard const&) if (backendComplete_) { - setup.noPragma(); lgrSQLiteDB_ = std::make_unique( setup, LgrDBName, CompleteShardDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( @@ -702,7 +705,6 @@ Shard::initSQLite(std::lock_guard const&) else { // The incomplete shard uses a Write Ahead Log for performance - setup.usePragma(); lgrSQLiteDB_ = std::make_unique( setup, LgrDBName, LgrDBPragma, LgrDBInit); lgrSQLiteDB_->getSession() << boost::str( diff --git a/src/test/app/LedgerHistory_test.cpp b/src/test/app/LedgerHistory_test.cpp index f1149a0454c..cbc9c95b325 100644 --- a/src/test/app/LedgerHistory_test.cpp +++ b/src/test/app/LedgerHistory_test.cpp @@ -100,7 +100,7 @@ class LedgerHistory_test : public beast::unit_test::suite Env env{ *this, envconfig(), - std::make_unique("MISMATCH ", found)}; + std::make_unique("MISMATCH ", &found)}; LedgerHistory lh{beast::insight::NullCollector::New(), env.app()}; auto const genesis = makeLedger({}, env, lh, 0s); uint256 const dummyTxHash{1}; @@ -117,7 +117,7 @@ class LedgerHistory_test : public beast::unit_test::suite *this, envconfig(), std::make_unique( - "MISMATCH on close time", found)}; + "MISMATCH on close time", &found)}; LedgerHistory lh{beast::insight::NullCollector::New(), env.app()}; auto const genesis = makeLedger({}, env, lh, 0s); auto const ledgerA = makeLedger(genesis, env, lh, 4s); @@ -137,7 +137,7 @@ class LedgerHistory_test : public beast::unit_test::suite *this, envconfig(), std::make_unique( - "MISMATCH on prior ledger", found)}; + "MISMATCH on prior ledger", &found)}; LedgerHistory lh{beast::insight::NullCollector::New(), env.app()}; auto const genesis = makeLedger({}, env, lh, 0s); auto const ledgerA = makeLedger(genesis, env, lh, 4s); @@ -163,7 +163,7 @@ class LedgerHistory_test : public beast::unit_test::suite Env env{ *this, envconfig(), - std::make_unique(msg, found)}; + std::make_unique(msg, &found)}; LedgerHistory lh{beast::insight::NullCollector::New(), env.app()}; Account alice{"A1"}; diff --git a/src/test/app/Manifest_test.cpp b/src/test/app/Manifest_test.cpp index c638fc436ee..18460ce3689 100644 --- a/src/test/app/Manifest_test.cpp +++ b/src/test/app/Manifest_test.cpp @@ -256,7 +256,7 @@ class Manifest_test : public beast::unit_test::suite { DatabaseCon::Setup setup; setup.dataDir = getDatabasePath(); - BEAST_EXPECT(!setup.commonPragma); + BEAST_EXPECT(!setup.useGlobalPragma); DatabaseCon dbCon( setup, dbName.data(), diff --git a/src/test/jtx/CaptureLogs.h b/src/test/jtx/CaptureLogs.h index 4a6bb2a7567..30a562e99d0 100644 --- a/src/test/jtx/CaptureLogs.h +++ b/src/test/jtx/CaptureLogs.h @@ -31,7 +31,7 @@ namespace test { class CaptureLogs : public Logs { std::stringstream strm_; - std::string& result_; + std::string* pResult_; /** * @brief sink for writing all log messages to a stringstream @@ -57,14 +57,14 @@ class CaptureLogs : public Logs }; public: - explicit CaptureLogs(std::string& result) - : Logs(beast::severities::kInfo), result_(result) + explicit CaptureLogs(std::string* pResult) + : Logs(beast::severities::kInfo), pResult_(pResult) { } ~CaptureLogs() override { - result_ = strm_.str(); + *pResult_ = strm_.str(); } std::unique_ptr diff --git a/src/test/jtx/CheckMessageLogs.h b/src/test/jtx/CheckMessageLogs.h index e168bc4fbbd..66f5f7e106c 100644 --- a/src/test/jtx/CheckMessageLogs.h +++ b/src/test/jtx/CheckMessageLogs.h @@ -27,7 +27,7 @@ namespace test { class CheckMessageLogs : public Logs { std::string msg_; - bool& found_; + bool* pFound_; class CheckMessageSink : public beast::Journal::Sink { @@ -46,7 +46,7 @@ class CheckMessageLogs : public Logs override { if (text.find(owner_.msg_) != std::string::npos) - owner_.found_ = true; + *owner_.pFound_ = true; } }; @@ -54,10 +54,11 @@ class CheckMessageLogs : public Logs /** Constructor @param msg The message string to search for - @param found The variable to set to true if the message is found + @param pFound Pointer to the variable to set to true if the message is + found */ - CheckMessageLogs(std::string msg, bool& found) - : Logs{beast::severities::kDebug}, msg_{std::move(msg)}, found_{found} + CheckMessageLogs(std::string msg, bool* pFound) + : Logs{beast::severities::kDebug}, msg_{std::move(msg)}, pFound_{pFound} { } diff --git a/src/test/nodestore/Database_test.cpp b/src/test/nodestore/Database_test.cpp index 1c7921ae130..826f5ccf5bf 100644 --- a/src/test/nodestore/Database_test.cpp +++ b/src/test/nodestore/Database_test.cpp @@ -84,7 +84,8 @@ class Database_test : public TestBase return Env( *this, std::move(p), - std::make_unique(integrityWarning, found), + std::make_unique( + integrityWarning, &found), beast::severities::kWarning); }(); @@ -116,7 +117,8 @@ class Database_test : public TestBase return Env( *this, std::move(p), - std::make_unique(integrityWarning, found), + std::make_unique( + integrityWarning, &found), beast::severities::kWarning); }(); @@ -149,7 +151,8 @@ class Database_test : public TestBase return Env( *this, std::move(p), - std::make_unique(integrityWarning, found), + std::make_unique( + integrityWarning, &found), beast::severities::kWarning); }(); @@ -185,7 +188,8 @@ class Database_test : public TestBase return Env( *this, std::move(p), - std::make_unique(integrityWarning, found), + std::make_unique( + integrityWarning, &found), beast::severities::kWarning); }(); @@ -226,7 +230,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -255,7 +259,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -284,7 +288,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -313,7 +317,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -341,7 +345,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -369,7 +373,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -397,7 +401,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } @@ -425,7 +429,7 @@ class Database_test : public TestBase Env env( *this, std::move(p), - std::make_unique(expected, found), + std::make_unique(expected, &found), beast::severities::kWarning); fail(); } diff --git a/src/test/server/Server_test.cpp b/src/test/server/Server_test.cpp index bbe2dcc021b..521661c5895 100644 --- a/src/test/server/Server_test.cpp +++ b/src/test/server/Server_test.cpp @@ -391,7 +391,7 @@ class Server_test : public beast::unit_test::suite (*cfg).deprecatedClearSection("port_rpc"); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Missing 'ip' in [port_rpc]") != std::string::npos); @@ -404,7 +404,7 @@ class Server_test : public beast::unit_test::suite (*cfg)["port_rpc"].set("ip", getEnvLocalhostAddr()); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Missing 'port' in [port_rpc]") != std::string::npos); @@ -418,7 +418,7 @@ class Server_test : public beast::unit_test::suite (*cfg)["port_rpc"].set("port", "0"); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Invalid value '0' for key 'port' in [port_rpc]") != @@ -434,7 +434,7 @@ class Server_test : public beast::unit_test::suite (*cfg)["port_rpc"].set("protocol", ""); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Missing 'protocol' in [port_rpc]") != @@ -469,7 +469,7 @@ class Server_test : public beast::unit_test::suite (*cfg)["port_ws"].set("admin", getEnvLocalhostAddr()); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Required section [server] is missing") != @@ -495,7 +495,7 @@ class Server_test : public beast::unit_test::suite (*cfg)["server"].append("port_ws"); return cfg; }), - std::make_unique(messages)}; + std::make_unique(&messages)}; }); BEAST_EXPECT( messages.find("Missing section: [port_peer]") != std::string::npos); From 2b01158d05f48c28e0425c544c24ff60114deb41 Mon Sep 17 00:00:00 2001 From: Edward Hennis Date: Wed, 24 Jun 2020 13:59:50 -0400 Subject: [PATCH 13/13] [FOLD] Review feedback from @mduo13 and @seelabs: * Improved some of the documentation and log messages * Removed the unnecessary `DatabaseCon::Base` ctor helper class --- cfg/rippled-example.cfg | 14 ++++++++------ src/ripple/app/ledger/Ledger.cpp | 4 ++-- src/ripple/core/DatabaseCon.h | 8 +------- 3 files changed, 11 insertions(+), 15 deletions(-) diff --git a/cfg/rippled-example.cfg b/cfg/rippled-example.cfg index 2809bb6b32d..b2b3c03f346 100644 --- a/cfg/rippled-example.cfg +++ b/cfg/rippled-example.cfg @@ -939,11 +939,11 @@ # [import_db] Settings for performing a one-time import (optional) # [database_path] Path to the book-keeping databases. # -# There are 4 or 5 bookkeeping SQLite databases that the server creates and -# maintains. If you omit this configuration setting, it will default to -# creating a directory called "db" located in the same place as your -# rippled.cfg file. Partial pathnames will be considered relative to -# the location of the rippled executable. +# The server creates and maintains 4 to 5 bookkeeping SQLite databases in +# the 'database_path' location. If you omit this configuration setting, +# the server creates a directory called "db" located in the same place as +# your rippled.cfg file. +# Partial pathnames are relative to the location of the rippled executable. # # [shard_db] Settings for the Shard Database (optional) # @@ -1339,7 +1339,9 @@ medium # metadata, account states, and ledger headers. Helpful information can be # found at https://xrpl.org/capacity-planning.html#node-db-type # type=NuDB is recommended for non-validators with fast SSDs. Validators or -# slow / spinning disks should use RocksDB. +# slow / spinning disks should use RocksDB. Caution: Spinning disks are +# not recommended. They do not perform well enough to consistently remain +# synced to the network. # online_delete=512 is recommended to delete old ledgers while maintaining at # least 512. # advisory_delete=0 allows the online delete process to run automatically diff --git a/src/ripple/app/ledger/Ledger.cpp b/src/ripple/app/ledger/Ledger.cpp index 86d0e33b284..b583b540633 100644 --- a/src/ripple/app/ledger/Ledger.cpp +++ b/src/ripple/app/ledger/Ledger.cpp @@ -228,14 +228,14 @@ Ledger::Ledger( !txMap_->fetchRoot(SHAMapHash{info_.txHash}, nullptr)) { loaded = false; - JLOG(j.warn()) << "Don't have TX root for ledger" << info_.seq; + JLOG(j.warn()) << "Don't have transaction root for ledger" << info_.seq; } if (info_.accountHash.isNonZero() && !stateMap_->fetchRoot(SHAMapHash{info_.accountHash}, nullptr)) { loaded = false; - JLOG(j.warn()) << "Don't have AS root for ledger" << info_.seq; + JLOG(j.warn()) << "Don't have state data root for ledger" << info_.seq; } txMap_->setImmutable(); diff --git a/src/ripple/core/DatabaseCon.h b/src/ripple/core/DatabaseCon.h index c6efc55aa54..5cdabb08f08 100644 --- a/src/ripple/core/DatabaseCon.h +++ b/src/ripple/core/DatabaseCon.h @@ -113,7 +113,6 @@ class DatabaseCon std::array const& initSQL) // Use temporary files or regular DB files? : DatabaseCon( - {}, setup.standAlone && setup.startUp != Config::LOAD && setup.startUp != Config::LOAD_FILE && setup.startUp != Config::REPLAY @@ -131,7 +130,7 @@ class DatabaseCon std::string const& DBName, std::array const& pragma, std::array const& initSQL) - : DatabaseCon({}, dataDir / DBName, {}, pragma, initSQL) + : DatabaseCon(dataDir / DBName, nullptr, pragma, initSQL) { } @@ -151,13 +150,8 @@ class DatabaseCon setupCheckpointing(JobQueue*, Logs&); private: - class Base - { - }; - template DatabaseCon( - Base, boost::filesystem::path const& pPath, std::vector const* commonPragma, std::array const& pragma,