Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Two database connections and missing transaction isolation level for read operations #45003

Closed
4 of 8 tasks
ChristophWurst opened this issue Apr 24, 2024 · 4 comments · Fixed by #45013
Closed
4 of 8 tasks
Assignees
Labels
4. to release Ready to be released and/or waiting for tests to finish 29-feedback bug regression

Comments

@ChristophWurst
Copy link
Member

⚠️ This issue respects the following points: ⚠️

Bug description

I am capturing all queries of my MariaDB installation and noticed a strange query pattern for simple Nextcloud requests:

-- nc_admin@localhost on nextclouddev using Socket
SELECT `appid`, `configkey`, `configvalue`, `type` FROM `oc_appconfig` WHERE `lazy` = 0;
SELECT `uid`, `displayname`, `password` FROM `oc_users` WHERE `uid_lower` = 'admin'
SELECT `gu`.`gid`, `g`.`displayname` FROM `oc_group_user` `gu` LEFT JOIN `oc_groups` `g` ON `gu`.`gid` = `g`.`gid` WHERE `uid` = 'admin'
SELECT `provider_id`, `enabled` FROM `oc_twofactor_providers` WHERE `uid` = 'admin'
SELECT `appid`, `configkey`, `configvalue` FROM `oc_preferences` WHERE `userid` = 'admin'
-- nc_admin@localhost on nextclouddev using Socket
SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED
SET SESSION AUTOCOMMIT=1
START TRANSACTION
SELECT * FROM `oc_authtoken` WHERE (`uid` = 'admin') AND (`version` = 2) LIMIT 1000
COMMIT
...

Steps to reproduce

  1. Set up Nextcloud with a single node database
  2. Send requests

Expected behavior

Single connection and read queries use READ COMMITTED too, not the global database default.

Installation method

None

Nextcloud Server version

29

Operating system

None

PHP engine version

None

Web server

None

Database engine version

None

Is this bug present after an update or on a fresh install?

None

Are you using the Nextcloud Server Encryption module?

None

What user-backends are you using?

  • Default user-backend (database)
  • LDAP/ Active Directory
  • SSO - SAML
  • Other

Configuration report

No response

List of activated Apps

No response

Nextcloud Signing status

No response

Nextcloud Logs

No response

Additional info

Related to #41998.

@ChristophWurst ChristophWurst added bug 0. Needs triage Pending check for reproducibility or if it fits our roadmap 29-feedback regression labels Apr 24, 2024
@ChristophWurst
Copy link
Member Author

Stack traces

SetTransactionIsolationLevel.php:41, OC\DB\SetTransactionIsolationLevel->postConnect()
EventManager.php:43, Doctrine\Common\EventManager->dispatchEvent()
PrimaryReadReplicaConnection.php:211, Doctrine\DBAL\Connections\PrimaryReadReplicaConnection->performConnect()
PrimaryReadReplicaConnection.php:155, Doctrine\DBAL\Connections\PrimaryReadReplicaConnection->connect()
Connection.php:150, OC\DB\Connection->connect()
Connection.php:453, Doctrine\DBAL\Connection->getDatabasePlatformVersion()
Connection.php:411, Doctrine\DBAL\Connection->detectDatabasePlatform()
Connection.php:318, Doctrine\DBAL\Connection->getDatabasePlatform()
ConnectionAdapter.php:200, OC\DB\ConnectionAdapter->getDatabasePlatform()
QueryBuilder.php:121, OC\DB\QueryBuilder\QueryBuilder->expr()
AppConfig.php:1239, OC\AppConfig->loadConfig()
AppConfig.php:264, OC\AppConfig->searchValues()
AppConfig.php:1380, OC\AppConfig->getValues()
AppManager.php:131, OC\App\AppManager->getInstalledAppsValues()
AppManager.php:152, OC\App\AppManager->getInstalledApps()
OC_App.php:234, OC_App::getEnabledApps()
Coordinator.php:90, OC\AppFramework\Bootstrap\Coordinator->runInitialRegistration()
base.php:706, OC::init()
base.php:1181, require_once()
v1.php:31, {main}()


SetTransactionIsolationLevel.php:41, OC\DB\SetTransactionIsolationLevel->postConnect()
EventManager.php:43, Doctrine\Common\EventManager->dispatchEvent()
PrimaryReadReplicaConnection.php:211, Doctrine\DBAL\Connections\PrimaryReadReplicaConnection->performConnect()
PrimaryReadReplicaConnection.php:224, Doctrine\DBAL\Connections\PrimaryReadReplicaConnection->ensureConnectedToPrimary()
PrimaryReadReplicaConnection.php:290, Doctrine\DBAL\Connections\PrimaryReadReplicaConnection->executeStatement()
Connection.php:361, OC\DB\Connection->executeStatement()
QueryBuilder.php:393, Doctrine\DBAL\Query\QueryBuilder->execute()
QueryBuilder.php:280, OC\DB\QueryBuilder\QueryBuilder->execute()
QueryBuilder.php:326, OC\DB\QueryBuilder\QueryBuilder->executeStatement()
AllConfig.php:283, OC\AllConfig->setUserValue()
User.php:259, OC\User\User->updateLastLoginTimestamp()
Session.php:399, OC\User\Session->completeLogin()
Session.php:643, OC\User\Session->loginWithPassword()
Session.php:366, OC\User\Session->login()
Session.php:463, OC\User\Session->logClientIn()
Session.php:602, OC\User\Session->tryBasicAuthLogin()
base.php:1135, OC::handleLogin()
v1.php:63, {main}()

so it's the Doctrine primary/replica connection class that switches to the primary once we start a transaction. The class doesn't factor in that we only have primary and no replica and still opens a separate connection.

We set the primary parameters as the first replica if there is no replica config. Doctrine requires this for the Primary/Replica class, else it throws Uncaught InvalidArgumentException: You have to configure at least one replica. in nextcloud/3rdparty/doctrine/dbal/src/Connections/PrimaryReadReplicaConnection.php:117.

@ChristophWurst ChristophWurst self-assigned this Apr 24, 2024
@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Apr 24, 2024
@ChristophWurst
Copy link
Member Author

I think the best fix would be to only subclass PrimaryReadReplicaConnection when db replicas are actually available and use the simple Doctrine Connection instead.

@kesselb
Copy link
Contributor

kesselb commented Apr 24, 2024

Good finding 👍

@joshtrichards
Copy link
Member

joshtrichards commented May 16, 2024

We've got some other bugs in this area of code too:

Both of the above have the same underlying cause. #45257 is particularly nasty since it not only doesn't copy the source database, but then it clears out the (wrong) entire database schema (!).

We need to at a minimum clean up the parameter value merging for #45257 (or whatever is the appropriate approach with the new replica stuff now being in mainline). Ideally before next maintenance release (which admittedly is cutting it close and I'm not offering up a fix since I haven't had the time to look deep enough at that area of code so far and doubt I'll have the time to do in the next day or so).

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish 29-feedback bug regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants