Skip to content

Commit

Permalink
Add sql::DatabaseOptions to encapsulate its configuration options
Browse files Browse the repository at this point in the history 
sql::Database uses internal state to manage its configuration options
like page size, journal mode etc. These options need to be set before
Database::Open() is called. Any change to this state has no effect if
done after the database has already been opened. There are also some
SQLite subtleties around different behaviour of this state depending on
whether the database is being opened for the first time or being
re-opened.

This change encapsulates all such state into a sql::DatabaseOptions
struct which is passed to the sql::Database constructor. This ensures
that the required options have been configured before calling Open().

We migrate sql/ to use the new constructor instead of the setters for
the state. In upcoming changes, we plan to remove the setters for this
state. This will make the Database API enforce that the options are set
only once and stay the same over the lifetime of the object.

Bug: 1126968
Change-Id: Id44b6b3883f7a695f3b5cd96ad071e15cb1dc44a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2427035
Commit-Queue: Shubham Aggarwal <shuagga@microsoft.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#817386}
  • Loading branch information
Shubham Aggarwal authored and Commit Bot committed Oct 15, 2020
1 parent afa39cc commit 7b60fe6
Show file tree
Hide file tree
Showing 7 changed files with 140 additions and 121 deletions.
37 changes: 19 additions & 18 deletions sql/database.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -233,19 +233,15 @@ void Database::StatementRef::Close(bool forced) {
was_valid_ = was_valid_ && forced;
}

static_assert(
Database::kDefaultPageSize == SQLITE_DEFAULT_PAGE_SIZE,
"Database::kDefaultPageSize must match the value configured into SQLite");
static_assert(DatabaseOptions::kDefaultPageSize == SQLITE_DEFAULT_PAGE_SIZE,
"DatabaseOptions::kDefaultPageSize must match the value "
"configured into SQLite");

constexpr int Database::kDefaultPageSize;
Database::Database() : Database({.exclusive_locking = false}) {}

Database::Database()
Database::Database(DatabaseOptions options)
: db_(nullptr),
page_size_(kDefaultPageSize),
cache_size_(0),
exclusive_locking_(false),
want_wal_mode_(
base::FeatureList::IsEnabled(features::kEnableWALModeByDefault)),
options_(options),
transaction_nesting_(0),
needs_rollback_(false),
in_memory_(false),
Expand All @@ -254,7 +250,12 @@ Database::Database()
mmap_disabled_(!enable_mmap_by_default_),
mmap_enabled_(false),
total_changes_at_last_release_(0),
stats_histogram_(nullptr) {}
stats_histogram_(nullptr) {
DCHECK_GE(options.page_size, 512);
DCHECK_LE(options.page_size, 65536);
DCHECK(!(options.page_size & (options.page_size - 1)))
<< "page_size must be a power of two";
}

Database::~Database() {
Close();
Expand Down Expand Up @@ -821,7 +822,7 @@ bool Database::Raze() {
}

const std::string page_size_sql =
base::StringPrintf("PRAGMA page_size=%d", page_size_);
base::StringPrintf("PRAGMA page_size=%d", options_.page_size);
if (!null_db.Execute(page_size_sql.c_str()))
return false;

Expand Down Expand Up @@ -1519,7 +1520,7 @@ bool Database::OpenInternal(const std::string& file_name,
// enabled.
//
// TODO(crbug.com/1120969): Remove support for non-exclusive mode.
if (!exclusive_locking_) {
if (!options_.exclusive_locking) {
err = ExecuteAndReturnErrorCode("PRAGMA locking_mode=NORMAL");
if (err != SQLITE_OK)
return false;
Expand Down Expand Up @@ -1562,7 +1563,7 @@ bool Database::OpenInternal(const std::string& file_name,
// Needs to happen before entering WAL mode. Will only work if this the first
// time the database is being opened in WAL mode.
const std::string page_size_sql =
base::StringPrintf("PRAGMA page_size=%d", page_size_);
base::StringPrintf("PRAGMA page_size=%d", options_.page_size);
ignore_result(ExecuteWithTimeout(page_size_sql.c_str(), kBusyTimeout));

// http://www.sqlite.org/pragma.html#pragma_journal_mode
Expand Down Expand Up @@ -1594,9 +1595,9 @@ bool Database::OpenInternal(const std::string& file_name,
ignore_result(Execute("PRAGMA journal_mode=TRUNCATE"));
}

if (cache_size_ != 0) {
if (options_.cache_size != 0) {
const std::string cache_size_sql =
base::StringPrintf("PRAGMA cache_size=%d", cache_size_);
base::StringPrintf("PRAGMA cache_size=%d", options_.cache_size);
ignore_result(ExecuteWithTimeout(cache_size_sql.c_str(), kBusyTimeout));
}

Expand Down Expand Up @@ -1822,9 +1823,9 @@ bool Database::UseWALMode() const {
// locking, because this case does not require shared memory support.
// At the time this was implemented (May 2020), Fuchsia's shared
// memory support was insufficient for SQLite's needs.
return want_wal_mode_ && exclusive_locking_;
return options_.wal_mode && options_.exclusive_locking;
#else
return want_wal_mode_;
return options_.wal_mode;
#endif // defined(OS_FUCHSIA)
}

Expand Down
75 changes: 57 additions & 18 deletions sql/database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
#include "base/compiler_specific.h"
#include "base/component_export.h"
#include "base/containers/flat_map.h"
#include "base/feature_list.h"
#include "base/gtest_prod_util.h"
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/optional.h"
#include "base/sequence_checker.h"
#include "base/threading/scoped_blocking_call.h"
#include "sql/internal_api_token.h"
#include "sql/sql_features.h"
#include "sql/statement_id.h"

struct sqlite3;
Expand All @@ -46,6 +48,46 @@ namespace test {
class ScopedErrorExpecter;
} // namespace test

struct COMPONENT_EXPORT(SQL) DatabaseOptions {
// Default page size for newly created databases.
//
// Guaranteed to match SQLITE_DEFAULT_PAGE_SIZE.
static constexpr int kDefaultPageSize = 4096;

// If true, the database can only be opened by one process at a time.
//
// Exclusive mode is strongly recommended. It reduces the I/O cost of setting
// up a transaction. It also removes the need of handling transaction failures
// due to lock contention.
bool exclusive_locking = true;

// If true, enables SQLite's Write-Ahead Logging (WAL).
//
// WAL integration is under development, and should not be used in shipping
// Chrome features yet. In particular, our custom database recovery code does
// not support the WAL log file.
//
// More details at https://www.sqlite.org/wal.html
bool wal_mode =
base::FeatureList::IsEnabled(sql::features::kEnableWALModeByDefault);

// Database page size.
//
// Larger page sizes result in shallower B-trees, because they allow an inner
// page to hold more keys. On the flip side, larger page sizes may result in
// more I/O when making small changes to existing records.
int page_size = kDefaultPageSize;

// The size of in-memory cache, in pages.
//
// SQLite's database cache will take up at most (`page_size` * `cache_size`)
// bytes of RAM.
//
// 0 invokes SQLite's default, which is currently to size up the cache to use
// exactly 2,048,000 bytes of RAM.
int cache_size = 0;
};

// Handle to an open SQLite database.
//
// Instances of this class are thread-unsafe and DCHECK that they are accessed
Expand All @@ -57,7 +99,13 @@ class COMPONENT_EXPORT(SQL) Database {
public:
// The database is opened by calling Open[InMemory](). Any uncommitted
// transactions will be rolled back when this object is deleted.
//
// This constructor is deprecated.
// TODO(crbug.com/1126968): Remove this constructor after migrating all
// uses to the explicit constructor below.
Database();
// |options| only affects newly created databases.
explicit Database(DatabaseOptions options);
~Database();

// Allows mmapping to be disabled globally by default in the calling process.
Expand All @@ -79,19 +127,19 @@ class COMPONENT_EXPORT(SQL) Database {
DCHECK(!(page_size & (page_size - 1)))
<< "page_size must be a power of two";

page_size_ = page_size;
options_.page_size = page_size;
}

// The page size that will be used when creating a new database.
int page_size() const { return page_size_; }
int page_size() const { return options_.page_size; }

// Sets the number of pages that will be cached in memory by sqlite. The
// total cache size in bytes will be page_size * cache_size. This must be
// called before Open() to have an effect.
void set_cache_size(int cache_size) {
DCHECK_GE(cache_size, 0);

cache_size_ = cache_size;
options_.cache_size = cache_size;
}

// Returns whether a database will be opened in WAL mode.
Expand All @@ -109,7 +157,7 @@ class COMPONENT_EXPORT(SQL) Database {
// no-ops.
//
// This must be called before Open() to have an effect.
void want_wal_mode(bool enabled) { want_wal_mode_ = enabled; }
void want_wal_mode(bool enabled) { options_.wal_mode = enabled; }

// Makes database accessible by only one process at a time.
//
Expand All @@ -130,7 +178,7 @@ class COMPONENT_EXPORT(SQL) Database {
//
// SQLite's locking protocol is summarized at
// https://www.sqlite.org/c3ref/io_methods.html
void set_exclusive_locking() { exclusive_locking_ = true; }
void set_exclusive_locking() { options_.exclusive_locking = true; }

// Call to use alternative status-tracking for mmap. Usually this is tracked
// in the meta table, but some databases have no meta table.
Expand Down Expand Up @@ -291,7 +339,7 @@ class COMPONENT_EXPORT(SQL) Database {
// these all return false, since it is unlikely that the caller
// could fix them.
//
// The database's page size is taken from |page_size_|. The
// The database's page size is taken from |options_.page_size|. The
// existing database's |auto_vacuum| setting is lost (the
// possibility of corruption makes it unreliable to pull it from the
// existing database). To re-enable on the empty database requires
Expand Down Expand Up @@ -530,11 +578,6 @@ class COMPONENT_EXPORT(SQL) Database {
// the existence of specific files.
static base::FilePath SharedMemoryFilePath(const base::FilePath& db_path);

// Default page size for newly created databases.
//
// Guaranteed to match SQLITE_DEFAULT_PAGE_SIZE.
static constexpr int kDefaultPageSize = 4096;

// Internal state accessed by other classes in //sql.
sqlite3* db(InternalApiToken) const { return db_; }
bool poisoned(InternalApiToken) const { return poisoned_; }
Expand Down Expand Up @@ -738,13 +781,9 @@ class COMPONENT_EXPORT(SQL) Database {
// Init resulted in an error.
sqlite3* db_;

// Parameters we'll configure in sqlite before doing anything else. Zero means
// use the default value.
int page_size_;
int cache_size_;

bool exclusive_locking_;
bool want_wal_mode_;
// TODO(shuagga@microsoft.com): Make `options_` const after removing all
// setters.
DatabaseOptions options_;

// Holds references to all cached statements so they remain active.
//
Expand Down
Loading

0 comments on commit 7b60fe6

Please sign in to comment.