Skip to content

Commit

Permalink
Remove config setters from sql/database.h
Browse files Browse the repository at this point in the history 
Most of the setters for configuration options for a SQLite database only
worked before the database was opened and were no-ops otherwise. This
led to confusing semantics and needing to support unnatural behaviour in
other parts of sql/ code.

This change removes the setters for the database config now encapsulated
in the DatabaseOptions struct. This struct is only set once as part of a
new constructor.

All feature code has already been migrated to use the new constructor
instead of the setter methods.

Bug: 1126968
Change-Id: I13474fbb87c7f2da386e83fb8d1fe3d8c44ab963
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2627941
Commit-Queue: Shubham Aggarwal <shuagga@microsoft.com>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#848097}
  • Loading branch information
Shubham Aggarwal authored and Chromium LUCI CQ committed Jan 28, 2021
1 parent c80c885 commit b30a0ce
Showing 1 changed file with 22 additions and 58 deletions.
80 changes: 22 additions & 58 deletions sql/database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,19 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {

// If true, the database can only be opened by one process at a time.
//
// SQLite supports a locking protocol that allows multiple processes to safely
// operate on the same database at the same time. The locking protocol is used
// on every transaction, and comes with a small performance penalty.
//
// Setting this to true causes the locking protocol to be used once, when the
// database is opened. No other process will be able to access the database at
// the same time.
//
// More details at https://www.sqlite.org/pragma.html#pragma_locking_mode
//
// SQLite's locking protocol is summarized at
// https://www.sqlite.org/c3ref/io_methods.html
//
// 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.
Expand All @@ -67,6 +80,13 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
// Chrome features yet. In particular, our custom database recovery code does
// not support the WAL log file.
//
// WAL mode is currently not fully supported on FuchsiaOS. It will only be
// turned on if the database is also using exclusive locking mode.
// (https://crbug.com/1082059)
//
// Note: Changing page size is not supported when in WAL mode. So running
// 'PRAGMA page_size = <new-size>' will result in no-ops.
//
// More details at https://www.sqlite.org/wal.html
bool wal_mode =
base::FeatureList::IsEnabled(sql::features::kEnableWALModeByDefault);
Expand All @@ -76,6 +96,8 @@ struct COMPONENT_EXPORT(SQL) DatabaseOptions {
// 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.
//
// Must be a power of two between 512 and 65536 inclusive.
int page_size = kDefaultPageSize;

// The size of in-memory cache, in pages.
Expand Down Expand Up @@ -116,70 +138,12 @@ class COMPONENT_EXPORT(SQL) Database {

// Pre-init configuration ----------------------------------------------------

// Sets the page size that will be used when creating a new database. This
// must be called before Init(), and will only have an effect on new
// databases.
//
// The page size must be a power of two between 512 and 65536 inclusive.
void set_page_size(int page_size) {
DCHECK_GE(page_size, 512);
DCHECK_LE(page_size, 65536);
DCHECK(!(page_size & (page_size - 1)))
<< "page_size must be a power of two";

options_.page_size = page_size;
}

// The page size that will be used when creating a new database.
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);

options_.cache_size = cache_size;
}

// Returns whether a database will be opened in WAL mode.
bool UseWALMode() const;

// Enables/disables WAL mode (https://www.sqlite.org/wal.html) when
// opening a new database.
//
// WAL mode is currently not fully supported on FuchsiaOS. It will only be
// turned on if the database is also using exclusive locking mode.
// (https://crbug.com/1082059)
//
// Note: Changing page size is not supported when in WAL mode. So running
// 'PRAGMA page_size = <new-size>' or using set_page_size will result in
// no-ops.
//
// This must be called before Open() to have an effect.
void want_wal_mode(bool enabled) { options_.wal_mode = enabled; }

// Makes database accessible by only one process at a time.
//
// TODO(https://crbug.com/1120969): This should be the default mode. The
// "NORMAL" mode should be opt-in.
//
// SQLite supports a locking protocol that allows multiple processes to safely
// operate on the same database at the same time. The locking protocol is used
// on every transaction, and comes with a small performance penalty.
//
// Calling this method causes the locking protocol to be used once, when the
// database is opened. No other process will be able to access the database at
// the same time.
//
// This method must be called before Open() to have an effect.
//
// More details at https://www.sqlite.org/pragma.html#pragma_locking_mode
//
// SQLite's locking protocol is summarized at
// https://www.sqlite.org/c3ref/io_methods.html
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.
// TODO(shess): Maybe just have all databases use the alt option?
Expand Down

0 comments on commit b30a0ce

Please sign in to comment.