diff options
-rw-r--r-- | config/config.sample.php | 17 | ||||
-rw-r--r-- | lib/private/DB/Connection.php | 101 | ||||
-rw-r--r-- | lib/private/DB/ConnectionFactory.php | 18 | ||||
-rw-r--r-- | lib/private/DB/SetTransactionIsolationLevel.php | 10 | ||||
-rw-r--r-- | lib/private/Server.php | 3 | ||||
-rw-r--r-- | lib/private/Setup/AbstractDatabase.php | 6 |
6 files changed, 139 insertions, 16 deletions
diff --git a/config/config.sample.php b/config/config.sample.php index 39cb0adea94..d999b1e39b0 100644 --- a/config/config.sample.php +++ b/config/config.sample.php @@ -152,6 +152,14 @@ $CONFIG = [ 'dbpersistent' => '', /** + * Specify read only replicas to be used by Nextcloud when querying the database + */ +'dbreplica' => [ + ['user' => 'nextcloud', 'password' => 'password1', 'host' => 'replica1', 'dbname' => ''], + ['user' => 'nextcloud', 'password' => 'password2', 'host' => 'replica2', 'dbname' => ''], +], + +/** * Indicates whether the Nextcloud instance was installed successfully; ``true`` * indicates a successful installation, and ``false`` indicates an unsuccessful * installation. @@ -987,6 +995,15 @@ $CONFIG = [ 'loglevel_frontend' => 2, /** + * Loglevel used by the dirty database query detection. Useful to identify + * potential database bugs in production. Set this to loglevel or higher to + * see dirty queries in the logs. + * + * Defaults to ``0`` (debug) + */ +'loglevel_dirty_database_queries' => 0, + +/** * If you maintain different instances and aggregate the logs, you may want * to distinguish between them. ``syslog_tag`` can be set per instance * with a unique id. Only available if ``log_type`` is set to ``syslog`` or diff --git a/lib/private/DB/Connection.php b/lib/private/DB/Connection.php index 6e2724ca5ab..9e6769aaafd 100644 --- a/lib/private/DB/Connection.php +++ b/lib/private/DB/Connection.php @@ -38,6 +38,7 @@ namespace OC\DB; use Doctrine\Common\EventManager; use Doctrine\DBAL\Cache\QueryCacheProfile; use Doctrine\DBAL\Configuration; +use Doctrine\DBAL\Connections\PrimaryReadReplicaConnection; use Doctrine\DBAL\Driver; use Doctrine\DBAL\Exception; use Doctrine\DBAL\Platforms\MySQLPlatform; @@ -53,9 +54,11 @@ use OCP\Diagnostics\IEventLogger; use OCP\IRequestId; use OCP\PreConditionNotMetException; use OCP\Profiler\IProfiler; +use Psr\Clock\ClockInterface; use Psr\Log\LoggerInterface; +use function in_array; -class Connection extends \Doctrine\DBAL\Connection { +class Connection extends PrimaryReadReplicaConnection { /** @var string */ protected $tablePrefix; @@ -65,6 +68,8 @@ class Connection extends \Doctrine\DBAL\Connection { /** @var SystemConfig */ private $systemConfig; + private ClockInterface $clock; + private LoggerInterface $logger; protected $lockedTable = null; @@ -78,6 +83,11 @@ class Connection extends \Doctrine\DBAL\Connection { /** @var DbDataCollector|null */ protected $dbDataCollector = null; + protected ?float $transactionActiveSince = null; + + /** @var array<string, int> */ + protected $tableDirtyWrites = []; + /** * Initializes a new instance of the Connection class. * @@ -103,6 +113,7 @@ class Connection extends \Doctrine\DBAL\Connection { $this->tablePrefix = $params['tablePrefix']; $this->systemConfig = \OC::$server->getSystemConfig(); + $this->clock = \OCP\Server::get(ClockInterface::class); $this->logger = \OC::$server->get(LoggerInterface::class); /** @var \OCP\Profiler\IProfiler */ @@ -119,7 +130,7 @@ class Connection extends \Doctrine\DBAL\Connection { /** * @throws Exception */ - public function connect() { + public function connect($connectionName = null) { try { if ($this->_conn) { /** @psalm-suppress InternalMethod */ @@ -254,6 +265,42 @@ class Connection extends \Doctrine\DBAL\Connection { * @throws \Doctrine\DBAL\Exception */ public function executeQuery(string $sql, array $params = [], $types = [], QueryCacheProfile $qcp = null): Result { + $tables = $this->getQueriedTables($sql); + $now = $this->clock->now()->getTimestamp(); + $dirtyTableWrites = []; + foreach ($tables as $table) { + $lastAccess = $this->tableDirtyWrites[$table] ?? 0; + // Only very recent writes are considered dirty + if ($lastAccess >= ($now - 3)) { + $dirtyTableWrites[] = $table; + } + } + if ($this->isTransactionActive()) { + // Transacted queries go to the primary. The consistency of the primary guarantees that we can not run + // into a dirty read. + } elseif (count($dirtyTableWrites) === 0) { + // No tables read that could have been written already in the same request and no transaction active + // so we can switch back to the replica for reading as long as no writes happen that switch back to the primary + // We cannot log here as this would log too early in the server boot process + $this->ensureConnectedToReplica(); + } else { + // Read to a table that has been written to previously + // While this might not necessarily mean that we did a read after write it is an indication for a code path to check + $this->logger->log( + (int) ($this->systemConfig->getValue('loglevel_dirty_database_queries', null) ?? 0), + 'dirty table reads: ' . $sql, + [ + 'tables' => array_keys($this->tableDirtyWrites), + 'reads' => $tables, + 'exception' => new \Exception(), + ], + ); + // To prevent a dirty read on a replica that is slightly out of sync, we + // switch back to the primary. This is detrimental for performance but + // safer for consistency. + $this->ensureConnectedToPrimary(); + } + $sql = $this->replaceTablePrefix($sql); $sql = $this->adapter->fixupStatement($sql); $this->queriesExecuted++; @@ -262,6 +309,16 @@ class Connection extends \Doctrine\DBAL\Connection { } /** + * Helper function to get the list of tables affected by a given query + * used to track dirty tables that received a write with the current request + */ + private function getQueriedTables(string $sql): array { + $re = '/(\*PREFIX\*\w+)/mi'; + preg_match_all($re, $sql, $matches); + return array_map([$this, 'replaceTablePrefix'], $matches[0] ?? []); + } + + /** * @throws Exception */ public function executeUpdate(string $sql, array $params = [], array $types = []): int { @@ -287,6 +344,10 @@ class Connection extends \Doctrine\DBAL\Connection { * @throws \Doctrine\DBAL\Exception */ public function executeStatement($sql, array $params = [], array $types = []): int { + $tables = $this->getQueriedTables($sql); + foreach ($tables as $table) { + $this->tableDirtyWrites[$table] = $this->clock->now()->getTimestamp(); + } $sql = $this->replaceTablePrefix($sql); $sql = $this->adapter->fixupStatement($sql); $this->queriesExecuted++; @@ -302,6 +363,11 @@ class Connection extends \Doctrine\DBAL\Connection { $prefix .= \OC::$server->get(IRequestId::class)->getId() . "\t"; } + // FIXME: Improve to log the actual target db host + $isPrimary = $this->connections['primary'] === $this->_conn; + $prefix .= ' ' . ($isPrimary === true ? 'primary' : 'replica') . ' '; + $prefix .= ' ' . $this->getTransactionNestingLevel() . ' '; + file_put_contents( $this->systemConfig->getValue('query_log_file', ''), $prefix . $sql . "\n", @@ -603,4 +669,35 @@ class Connection extends \Doctrine\DBAL\Connection { return new Migrator($this, $config, $dispatcher); } } + + public function beginTransaction() { + if (!$this->inTransaction()) { + $this->transactionActiveSince = microtime(true); + } + return parent::beginTransaction(); + } + + public function commit() { + $result = parent::commit(); + if ($this->getTransactionNestingLevel() === 0) { + $timeTook = microtime(true) - $this->transactionActiveSince; + $this->transactionActiveSince = null; + if ($timeTook > 1) { + $this->logger->warning('Transaction took ' . $timeTook . 's', ['exception' => new \Exception('Transaction took ' . $timeTook . 's')]); + } + } + return $result; + } + + public function rollBack() { + $result = parent::rollBack(); + if ($this->getTransactionNestingLevel() === 0) { + $timeTook = microtime(true) - $this->transactionActiveSince; + $this->transactionActiveSince = null; + if ($timeTook > 1) { + $this->logger->warning('Transaction rollback took longer than 1s: ' . $timeTook, ['exception' => new \Exception('Long running transaction rollback')]); + } + } + return $result; + } } diff --git a/lib/private/DB/ConnectionFactory.php b/lib/private/DB/ConnectionFactory.php index 4b286ff5442..e868f18ec34 100644 --- a/lib/private/DB/ConnectionFactory.php +++ b/lib/private/DB/ConnectionFactory.php @@ -32,7 +32,6 @@ use Doctrine\Common\EventManager; use Doctrine\DBAL\Configuration; use Doctrine\DBAL\DriverManager; use Doctrine\DBAL\Event\Listeners\OracleSessionInit; -use Doctrine\DBAL\Event\Listeners\SQLSessionInit; use OC\SystemConfig; /** @@ -127,11 +126,8 @@ class ConnectionFactory { $normalizedType = $this->normalizeType($type); $eventManager = new EventManager(); $eventManager->addEventSubscriber(new SetTransactionIsolationLevel()); + $additionalConnectionParams = array_merge($this->createConnectionParams(), $additionalConnectionParams); switch ($normalizedType) { - case 'mysql': - $eventManager->addEventSubscriber( - new SQLSessionInit("SET SESSION AUTOCOMMIT=1")); - break; case 'oci': $eventManager->addEventSubscriber(new OracleSessionInit); // the driverOptions are unused in dbal and need to be mapped to the parameters @@ -159,7 +155,7 @@ class ConnectionFactory { } /** @var Connection $connection */ $connection = DriverManager::getConnection( - array_merge($this->getDefaultConnectionParams($type), $additionalConnectionParams), + $additionalConnectionParams, new Configuration(), $eventManager ); @@ -195,10 +191,10 @@ class ConnectionFactory { public function createConnectionParams(string $configPrefix = '') { $type = $this->config->getValue('dbtype', 'sqlite'); - $connectionParams = [ + $connectionParams = array_merge($this->getDefaultConnectionParams($type), [ 'user' => $this->config->getValue($configPrefix . 'dbuser', $this->config->getValue('dbuser', '')), 'password' => $this->config->getValue($configPrefix . 'dbpassword', $this->config->getValue('dbpassword', '')), - ]; + ]); $name = $this->config->getValue($configPrefix . 'dbname', $this->config->getValue('dbname', self::DEFAULT_DBNAME)); if ($this->normalizeType($type) === 'sqlite3') { @@ -237,7 +233,11 @@ class ConnectionFactory { $connectionParams['persistent'] = true; } - return $connectionParams; + $replica = $this->config->getValue('dbreplica', []) ?: [$connectionParams]; + return array_merge($connectionParams, [ + 'primary' => $connectionParams, + 'replica' => $replica, + ]); } /** diff --git a/lib/private/DB/SetTransactionIsolationLevel.php b/lib/private/DB/SetTransactionIsolationLevel.php index b067edde441..9d9323664c8 100644 --- a/lib/private/DB/SetTransactionIsolationLevel.php +++ b/lib/private/DB/SetTransactionIsolationLevel.php @@ -26,8 +26,10 @@ declare(strict_types=1); namespace OC\DB; use Doctrine\Common\EventSubscriber; +use Doctrine\DBAL\Connections\PrimaryReadReplicaConnection; use Doctrine\DBAL\Event\ConnectionEventArgs; use Doctrine\DBAL\Events; +use Doctrine\DBAL\Platforms\MySQLPlatform; use Doctrine\DBAL\TransactionIsolationLevel; class SetTransactionIsolationLevel implements EventSubscriber { @@ -36,7 +38,13 @@ class SetTransactionIsolationLevel implements EventSubscriber { * @return void */ public function postConnect(ConnectionEventArgs $args) { - $args->getConnection()->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED); + $connection = $args->getConnection(); + if ($connection instanceof PrimaryReadReplicaConnection && $connection->isConnectedToPrimary()) { + $connection->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED); + if ($connection->getDatabasePlatform() instanceof MySQLPlatform) { + $connection->executeStatement('SET SESSION AUTOCOMMIT=1'); + } + } } public function getSubscribedEvents() { diff --git a/lib/private/Server.php b/lib/private/Server.php index d9bbc8625e9..acc66b9cb0a 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -843,8 +843,7 @@ class Server extends ServerContainer implements IServerContainer { if (!$factory->isValidType($type)) { throw new \OC\DatabaseException('Invalid database type'); } - $connectionParams = $factory->createConnectionParams(); - $connection = $factory->getConnection($type, $connectionParams); + $connection = $factory->getConnection($type, []); return $connection; }); /** @deprecated 19.0.0 */ diff --git a/lib/private/Setup/AbstractDatabase.php b/lib/private/Setup/AbstractDatabase.php index 6bef40338c9..82c00cb271a 100644 --- a/lib/private/Setup/AbstractDatabase.php +++ b/lib/private/Setup/AbstractDatabase.php @@ -140,10 +140,12 @@ abstract class AbstractDatabase { } $connectionParams['host'] = $host; } - $connectionParams = array_merge($connectionParams, $configOverwrite); + $connectionParams = array_merge($connectionParams, ['primary' => $connectionParams, 'replica' => [$connectionParams]]); $cf = new ConnectionFactory($this->config); - return $cf->getConnection($this->config->getValue('dbtype', 'sqlite'), $connectionParams); + $connection = $cf->getConnection($this->config->getValue('dbtype', 'sqlite'), $connectionParams); + $connection->ensureConnectedToPrimary(); + return $connection; } /** |