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

expire binlog #2350

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

expire binlog #2350

wants to merge 2 commits into from

Conversation

HPPinata
Copy link

@HPPinata HPPinata commented Dec 15, 2024

Enabeling binlog without any cleanup/purge policy is a horrible idea, since it causes binlog.XXXXXX files to accumulate indefinitely, unless the administrator manually runs a purge by connecting to the database.
I only noticed this due to the fact almost 50GB of binlog files had accumulated on my server over a period of around 7 months, even on my relatively small instance.

I'm unsure why the binlog is enabled in the first place, since none of these setups are using replication, but the ability to do a point in time recovery might be the original intent.

Whatever the reasoning for enabeling binlog, this change should both stop new deployments from running into the issue and resolve the problem on any existing setups once the compose.yaml is updated.
The only exceptions are database servers under souch heavy load that the binlog grows quickly enough to reach a problematic size in the 7 days until it is cleaned up. The administrators of those instances (if they aren't already supported by Nextcloud Enterprise) would have noticed this issue and should be equipped to independently evaluate the usage of binlog on their database system.

In any case (and even for systems under high load) automatically purging binlog entries older than a week should have a significant positive impact on the deployments storage usage without negatively affecting either replication (if configured) or point in time recovery.

--slave_connections_needed_for_purge=0 is required to make the binlog cleanup on databases without replication possible, which should apply to essentially all deployments based on these compose.yaml files.

Including a check for a binlog without any cleanup active in the Web Ui's "security & setup warnings" would probably also be a good idea, but that's beyond my ability and should probably be a separate Issue/PR on https://github.com/nextcloud/server.

References:
https://mariadb.com/kb/en/using-and-maintaining-the-binary-log/
https://mariadb.com/kb/en/replication-and-binary-log-system-variables/#expire_logs_days

@HPPinata HPPinata force-pushed the binlog-fix branch 3 times, most recently from 8f2cd07 to faf1d9b Compare December 15, 2024 10:57
expire binlog entries older than a week to avoid unbounded binlog growth over longer timespans

Signed-off-by: HPPinata <[email protected]>
@HPPinata
Copy link
Author

Is there anything else I need to do before this could proceed, or is this just a matter of internal prioritization?

@joshtrichards
Copy link
Member

joshtrichards commented Jan 21, 2025

Looks like this was an oversight from #1881 and #581.

Also there are differences between MySQL and MariaDB, and between versions (the former has bin-log on by default even if you don't specify it and also expires them automatically every 30 days by default). That probably added to the confusion.

We should perhaps try to clarify the upstream documentation too, as noted in that other issue, but on the other hand this is mostly a local environment/system admin matter rather than something Nextcloud Server itself cares about.

We've deviated from the Admin Manual here a bit (which we try not to do here for this image).

I don't think we should even have the binlog enabled by default in our example Compose files. So I propose we revert #581 and #1881 and drop binlog-format.

Since InnoDB is used, recovery and online backups are already covered. The binlog is only going to be used by people that know what they're doing - i.e. replication and/or point-in-time recovery requirements which already inherently require DBA level knowledge. The people operating these environments likely are already going to be competent enough to decide if they want binlog on, how they want expiration to be configured, etc. Having it on for everyone else, even with better expiration defaults, doesn't make much sense in hindsight.

@joshtrichards joshtrichards requested a review from J0WI January 21, 2025 16:12
@joshtrichards
Copy link
Member

joshtrichards commented Jan 21, 2025

Cc: @meonkeys, @tzerber (since you did some recent research in this area), @st3iny (due to #581)

tldr: I think we need to fix the Server docs to make it clearer that binlog isn't actually required by Nextcloud Server itself then instead of this PR as-is, in the docker image examples we'll drop all references to binlog entirely.

P.S. Thanks @HPPinata for following up on this.

@joshtrichards joshtrichards requested a review from st3iny January 21, 2025 16:20
@joshtrichards joshtrichards added examples Compose/Dockerfile/etc 3. to review labels Jan 21, 2025
@HPPinata
Copy link
Author

I don't think we should even have the binlog enabled by default in our example Compose files. So I propose we revert #1881 and drop binlog-format.

Since InnoDB is used, recovery and online backups are already covered. The binlog is only going to be used by people that know what they're doing - i.e. replication and/or point-in-time recovery requirements which already inherently require DBA level knowledge. The people operating these environments likely are already going to be competent enough to decide if they want binlog on, how they want expiration to be configured, etc. Having it on for everyone else, even with better expiration defaults, doesn't make much sense in hindsight.

That sounds reasonable. I wasn't sure if anything important relied on binlogs being enabled since at least ˋbinlog-formatˋ was part of the compose file for several years, but not having to worry about binlog at all is even better than fixing the symptom via cleanup.

@tzerber
Copy link
Contributor

tzerber commented Jan 21, 2025

When I was working on the examples on several occasions I got confused on should or shouldn't binlog being used. Most of my tests we're done on temporary fresh installs, and never had any issue, but it appears that the accumulation of binlog happens after a while. I'd say we drop it completely if it's not needed, and I also agree that anyone needing it has the sufficient DBA knowledge to handle it. I will check and update the docs and examples here if needed, and will try to update upstream docs as well.

remove all references and options regarding binlog from docker compose files

Signed-off-by: HPPinata <[email protected]>
@meonkeys
Copy link
Contributor

Agreed, looks like a great idea to disable the binlog by default. My binlogs were expiring (and I have a small Nextcloud instance) so I never noticed the issue!

I'm now using mariadb 10.11 with only --transaction-isolation=READ-COMMITTED and it appears to be working fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants