-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(db): Prevent two connections for single node databases #45013
fix(db): Prevent two connections for single node databases #45013
Conversation
The downside of this approach is that the query logging will speak the truth for single database node setups and prefix queries with "primary" even if they would go to a replica on clustered setups. |
/backport to stable29 |
This feels tricky to achieve without breaking too much. The current approach works for single node and clustered databases. A unit test was added to ensure this doesn't happen again. |
I have reverted the second part of setting the transaction isolation level for read replicas. This PR now only fixes having two connections for one request connecting to a single node database. |
Let's try to rewrite this code sometime and make our connection class a decorator for the doctrine classes rather than extending it. |
b735389
to
fe94824
Compare
Squashed |
'host' => 'test', | ||
]; | ||
$adapter = $this->createMock(Adapter::class); | ||
$driver = $this->createMock(Driver::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You apparently need to mock detectDatabasePlatform
https://github.com/nextcloud/server/actions/runs/9265386150/job/25487375754?pr=45013
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by passing an actual platform. Thanks!
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
fe94824
to
3bfba20
Compare
Hi, is this fixing the problem #45257? |
I don't think so |
Summary
Two connections to one database node
Since #41998 our connection always extends Doctrine's PrimaryReadReplicaConnection. This baseclass requires a primary and at least one replica and always does the read-write split.
For simpler Nextcloud installations we only have one database node and no replica. We do not want two database connections.
This overwrites the
\Doctrine\DBAL\Connections\PrimaryReadReplicaConnection::performConnect
method so we always connect to the primary if there is no replica. If there is one, we do the Doctrine read-write split.Missing transaction isolation level for replicaNextcloud uses READ COMMITTED as transaction isolation level. Setting it for the session was missing for the replica connections.Reverted because it implicitly causes a connection to the primary just to set the session variable.How to test - Single node
I.
'log_query' => true',
II.
'query_log_file' => '/var/www/nextcloud/data/query.log',
master: you see replica and primary connections prefixes for the logged queries. That indicates that Nextcloud opens two connections per request.
here: you only see primary connections. That indicates that Nextcloud only opens one connection per request.
How to test - Cluster
I.
'log_query' => true',
II.
'query_log_file' => '/var/www/nextcloud/data/query.log',
master and here: Nextcloud opens two connections.
Checklist