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

Vacuum the sqlite rpmdb if necessary #3452

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

ffesti
Copy link
Contributor

@ffesti ffesti commented Nov 18, 2024

Check if there is 20MB in free pages and then execute a VACUUM command. The threshold can be controlled by the _sqlite_vacuum macro. Don't add this to the macros file on purpose as we don't want people to get involved with such details. Here it is mainly used for testing.

Using a 20 MB threshold should prevent the vacuuming to happend too often while still triggering after large transactions. As we install new headers first and then remove the old ones transactions leave behind large amounts of free pages.

We do not use PRAGMA auto_vacuum here as it does not defrag the database and only frees empty pages. So it still requires running VACUUM from time to time. Freeing the empty pages would get rid of the condition we use here for running VACUUM.

Using pure C for the macro expansion on purpopse in case we want to back port this.

Resolves: #3309

@ffesti ffesti requested a review from a team as a code owner November 18, 2024 08:30
@ffesti ffesti requested review from dmnks and removed request for a team November 18, 2024 08:30
tests/rpmdb.at Outdated
],
[0],
[D: VACUUM: 0
D: Rpmdb Sqlite backend VACUUM maxfree: 1024, free: 397312 -> 0
Copy link
Member

@pmatilai pmatilai Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/usr/lib/sysimage/rpm is a "live" rpmdb, so these values can and will change over time, breaking the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's kinda on purpose. I wanted a real database for testing. But may be the test suite can do without.

int max_size = rpmExpandNumeric("%{?_sqlite_vacuum}");
if (max_size <= 0)
max_size = 20*1024*1024;
long free_space = sqlite_free_space(sdb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 32bit architectures, long is a mere 32bit integer. Better be safe and use explicit int64_t for the size since that's you're getting it from sqlite to begin with.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have been pondering about int vs long vs int64_t here.

rpmExpandNumeric also only returns an int. I wonder if we should at least move the C++ variant to 64 bits. Not that we need it here urgently but adding a 32 bit API seems kinda wrong.

lib/backend/sqlite.cc Outdated Show resolved Hide resolved
lib/backend/sqlite.cc Outdated Show resolved Hide resolved
lib/backend/sqlite.cc Outdated Show resolved Hide resolved
@pmatilai
Copy link
Member

Minor nits in what's not even my review 😆 aside, this is certainly a good thing to have 👍

I remember initially skipping this because I was worried about reliability and stuff for such a major live surgery, clearly not yet having quite the unwavering trust towards sqlite that we have now. I've been intending to look at this but I guess I got a bit burned out on the sqlite front back then and just haven't found the will to dig into it.

@ffesti
Copy link
Contributor Author

ffesti commented Nov 19, 2024

Nit picks addressed.

Yeah, that's why I did this in C. This looks a lot like something we want to have - even in older releases.

int max_size = rpmExpandNumeric("%{?_sqlite_vacuum_kb}");
if (max_size <= 0)
max_size = 20*1024;
int64_t free_space = sqlite_free_space(sdb) >> 10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int64_t here doesn't help when it's long in sqlite_free_space(). Which should return the same kind of data as the macro really so this calculation belongs to that function, more like. Doing *1024 but then >> 10 looks super confusing, lets just stick to basic maths for doing math, because to me a bitshift suggests there's something more special going on.


if (free_space > max_size) {
sqlexec(sdb, "VACUUM");
rpmlog(RPMLOG_DEBUG, "rpmdb sqlite backend VACUUM maxfree: %ikB, free: %" PRIu64 "kB -> %" PRIu64 "kB\n", max_size, free_space, sqlite_free_space(sdb));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and just spotted this now: please keep lines < 80 chacters to keep things readable in 80x terminal.

Check if there is 20MB in free pages and then execute a VACUUM command.
The threshold can be controlled by the _sqlite_vacuum macro. Don't add
this to the macros file on purpose as we don't want people to get
involved with such details. Here it is mainly used for testing.

Using a 20 MB threshold should prevent the vacuuming to happend too often
while still triggering after large transactions. As we install new
headers first and then remove the old ones transactions leave behind large
amounts of free pages.

We do not use PRAGMA auto_vacuum here as it does not defrag the
database and only frees empty pages. So it still requires running VACUUM
from time to time. Freeing the empty pages would get rid of the condition we
use here for running VACUUM.

Using pure C for the macro expansion on purpopse in case we want to back
port this.

Resolves: rpm-software-management#3309
@pmatilai pmatilai merged commit 5d3a6c4 into rpm-software-management:master Nov 19, 2024
1 check passed
@ffesti ffesti deleted the vaccuum branch November 19, 2024 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFE: auto-vacuum the sqlite rpmdb
2 participants