Probably unnecessary MariaDB global configuration change requirement

[pboguslawski]

How to use GitHub

• Please use the 👍 reaction to show that you are interested into the same feature.
• Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
• Subscribe to receive notifications on status change and new comments.

Is your feature request related to a problem? Please describe.

According to

https://docs.nextcloud.com/server/stable/admin_manual/configuration_database/linux_database_configuration.html#configuring-a-mysql-or-mariadb-database

Nextcloud requires non default MariaDB settings

transaction_isolation = READ-COMMITTED
binlog_format = ROW

Accoring to

https://help.nextcloud.com/t/transaction-isolation-level-of-nextcloud-database/88858/4

Nextcloud already sets transaction_isolation for its sessions.

Accroding to

https://mariadb.com/kb/en/binary-log-formats/#configuring-the-binary-log-format

binary log format may be configured per session too but I didn't find such solution in Nextcloud source.

Describe the solution you'd like

Please verify and consider removing MariaDB global configuration change requirement for transaction_isolation and binlog_format if not absolutely required (Nextcloud should configure own sessions only); don't not force admin to globally change default db server settings if not required as it may interfere with other applications using other databases on same server.

[MichaIng]

Committed here, sadly without further explanation or a PR: nextcloud/documentation@c32114a
Ah, seems here it was proposed: #10159

The additional text at the top of the page was committed here, pulled in from ownCloud docs in 2016: nextcloud/documentation#56

For the transaction isolation there is a setup warning implemented, but I've never seen or heard of this being triggered: https://github.com/nextcloud/server/blob/19d0708/apps/settings/lib/Controller/CheckSetupController.php#L523-L535

What I can say is that both settings have other defaults and I run a Nextcloud instance for several years + we (DietPi) offer a fully configured Nextcloud setup with MariaDB for years without touching any of the two settings, and at least we didn't get any issue reports that could be narrowed down to these missing, and never faced someone with this warning. So indeed Nextcloud seems to handle this internally. But not sure about the binary logging format.

@nickvergessen @MorrisJobke
As you were involved merging the documentation sections, do you know whether this is still right, and if so, how to test negative impact when not setting these globally for the database server? Generally I agree that the documentation should not contain obsolete or non-required settings, to keep them simple and readable, leaving the choice for a global default up to the database implementation maintainers or the admin.

[csware]

You can set the binlog format to mixed. Then MariaDB can decide which type to use.

[pboguslawski]

You can set the binlog format to mixed. Then MariaDB can decide which type to use.

As mentioned above: don't not force admin to globally change default db server settings if not required as it may interfere with other applications using other databases on same server. NC should configure only own sessions and not require global sql server changes.

[csware]

I suppose this is a non issue for the binlog as the default value in MariaDB changed to mixed anyways (cf. https://mariadb.com/kb/en/replication-and-binary-log-system-variables/#binlog_format).

[csware]

The transaction isolation level seems to be set on a per session basis (cf.

server/lib/private/DB/SetTransactionIsolationLevel.php

Line 39 in bd7ab89

$args->getConnection()->setTransactionIsolation(TransactionIsolationLevel::READ_COMMITTED);

; starting from release 19.x) as I've never modified it.

[csware]

So, the docs should be updated.

[pboguslawski]

as the default value in MariaDB changed to mixed anyways

SQL server admin is allowed to set whatever they like for server globally and NC should not assume that SQL server has required setting. SQL server may be shared between many apps with different requirements. NC should setup only own sessions if required (and not done already).

[csware]

Even setting the session variable for the biglog format requires super privileges which you don't want the nextcloud user to have in production.

[pboguslawski]

Seems super is required only for SET GLOBAL binlog_format not for SET SESSION binlog_format...

https://mariadb.com/kb/en/binary-log-formats/
https://dev.mysql.com/doc/refman/8.0/en/binary-log-setting.html

...but should be verified by devs if required.

[csware]

...but should be verified by devs if required.

Please look at the output of my PR, then you'll see that it is not allowed: https://drone.nextcloud.com/nextcloud/server/22239/11/5

[pboguslawski]

Just checked in MariaDB/Debian 11:

mysql --host=test --user=test --password=test --execute "SHOW SESSION VARIABLES LIKE 'binlog_format'; SET SESSION binlog_format='MIXED'; SHOW SESSION VARIABLES LIKE 'binlog_format';" test
+---------------+-------+
| Variable_name | Value |
+---------------+-------+
| binlog_format | ROW   |
+---------------+-------+
ERROR 1227 (42000) at line 1: Access denied; you need (at least one of) the SUPER, BINLOG ADMIN privilege(s) for this operation

so you're right - special privileges are required and nextcloud user problably should not have such privileges for security reasons.