Prune old events from sqlite database #4737

azeemba · 2023-05-17T16:45:13Z

Adds a new config dbHistoryDays which defaults to undefined.

At start up, each database handler reads the config. If the value is set, then a reoccuring task is scheduled to clean up old events. Events older than dbHistoryDays are targeted but only a few thousand events are cleaned up per iteration to avoid freezing the server.

Adds unit tests to validate the cleanup logic as well.

Fixes #2822

Adds a new config `dbHistoryDays` which defaults to undefined. At start up, each database handler reads the config. If the value is set, then a reoccuring task is scheduled to clean up old events. Events older than `dbHistoryDays` are targeted but only a few thousand events are cleaned up per iteration to avoid freezing the server. Adds unit tests to validate the cleanup logic as well. Fixes thelounge#2822

brunnre8

Thanks a lot for taking this on.
Now, I've some general comments as to how I think this could work, plus a few minor comments in the code directly.

I think this is a bit too limited as is.
For starters, I want the history retention to be configurable by the user, not just the admin.
While in many cases that's the same, it isn't in all cases and not having to edit the server config for something like that is better.

What that means is that we need this to be a hierarchical configuration. Admins sets global limits as they see fit, user can lower that limit as they feel like.

We should also put that configuration into its own top level config object so that we can add more related settings in the future, for instance I would like to add the ability to only delete unimportant status messages (leave, part, nick change, away, back etc) while retaining the actual useful stuff indefinitely (privmsg, notice).

Does this make sense?

Also note, you should probably rebase on top of #4718 as I'll merge that soon (after a bugfix release for 4.4.0)

brunnre8 · 2023-05-18T15:06:33Z

defaults/config.js

+	// Undefined/-1/0 is treated an unlimited.
+	// The limit is seen as a soft target but not an exact goal. Only a few
+	// thousand rows are pruned at a time to avoid slowing down the service.
+	dbHistoryDays: undefined,


This needs to be moved into its own top level object, else we end up with a bunch of similar names which are all very long for no real reason.

brunnre8 · 2023-05-18T15:29:10Z

server/plugins/messageStorage/sqlite.ts

+		const deleteBefore = Date.now() - keepNdays * millisecondsInDay;
+		await this.serialize_run(
+			`DELETE FROM messages WHERE rowid in (
+				SELECT rowid FROM messages WHERE time < ? ORDER BY time ASC LIMIT ?


why are you enforcing an ordering here?
While I get the intention behind it, usually the messages in question are not visible to the client.

You could just let the DB do it in whatever order it sees fit, generally you'd assume that you can keep up within a single invocation right?
We have an index on the time, so it's probably relatively easy to order, but still.

Since we do delete in batches of only 1000 events, I felt wrong creating a "hole" in history. Since the index is already there, I think there is no harm in doing it this way right?

fnutt · 2023-05-19T08:08:24Z

I think this is a bit too limited as is. For starters, I want the history retention to be configurable by the user, not just the admin. While in many cases that's the same, it isn't in all cases and not having to edit the server config for something like that is better.

What that means is that we need this to be a hierarchical configuration. Admins sets global limits as they see fit, user can lower that limit as they feel like.

I'm not sure why a user would set it lower than the admin global limit. What is the usecase for this? Would any users want less history to search/scroll? If they want to remove history, they can do this per channel already?

brunnre8 · 2023-05-19T11:02:36Z

Admin says, yeah well, storage isn't an issue.
User says, out of privacy issues I'm uncomfortable to have history there for longer than a month.

Easy enough to imagine, no?

Similarly, User A is unlimited as that's my main account, User B is a test account for dev purposes and I just want some history, without actually using much space

brunnre8

Thanks for the update.
In my opinion this should still be restructured a bit though, tell me if you disagree with my comments and I try to explain my reasoning a bit better.

brunnre8 · 2023-07-15T09:37:59Z

server/plugins/messageStorage/sqlite.ts

+		this.mergedConfig = this.mergeClientConfig(clientStorageConfig);
+	}
+
+	mergeClientConfig(clientStorageConfig: SqliteConfig) {


This isn't the right place for that kind of logic.
The config needs to be merged by the core code, not the sqlite specific storage backend.

Yes, currently sqlite is our only implementation of the interface, but that doesn't mean that we should violate the layers.

brunnre8 · 2023-07-15T09:40:52Z

server/config.ts

+			maxDaysHistory: number | undefined;
+	  }
+	| undefined;
+


that type seems odd to me?

You shouldn't set that to an object or undefined, things that want it optional shall say so when they mean it.

Meaning, the user of the type should do

export type ConfigType = { // ... sqliteConfig?: SqliteConfig; // ... }

Or whatever, but not that funny type that is intrinsically this way.
That's a hassle to use

brunnre8 · 2023-07-15T09:42:03Z

defaults/config.js

+	//
+	// `maxDaysHistory`:
+	// 		Defines the maximum number of days of history to keep in the database.
+	// 		Undefined/-1/0 is treated an unlimited.


exactly undefined
We should have one way and only one way of configuring something.

brunnre8 · 2023-07-15T09:44:15Z

test/plugins/sqlite.ts

@@ -37,7 +37,7 @@ describe("SQLite Message Storage (stateful tests)", function () {
 		// Delete database file from previous test run
 		await cleanup();

-		store = new MessageStorage("testUser");
+		store = new MessageStorage("testUser", undefined as any);


such casts are very brittle... if you can avoid so please do and in this case you trivially could by providing a correct default value

azeemba · 2023-07-16T15:36:56Z

I appreciate the feedback but this has slipped in priority for me. Happy for someone to take over the PR or to close the PR.

brunnre8 · 2023-07-16T20:03:50Z

thanks for your efforts

azeemba force-pushed the pruning branch from 66aca8a to 4124304 Compare May 17, 2023 18:38

brunnre8 requested changes May 18, 2023

View reviewed changes

Support user-based overriding for days-of-history setting

a7dc4d7

brunnre8 requested changes Jul 15, 2023

View reviewed changes

brunnre8 added the Status: needs-new-owner PR abandoned by its original owner and up for grabs label Jul 16, 2023

brunnre8 closed this Nov 4, 2023

azeemba deleted the pruning branch November 4, 2023 18:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune old events from sqlite database #4737

Prune old events from sqlite database #4737

azeemba commented May 17, 2023

brunnre8 left a comment

brunnre8 May 18, 2023

brunnre8 May 18, 2023

azeemba May 19, 2023

fnutt commented May 19, 2023 •

edited

brunnre8 commented May 19, 2023 •

edited

brunnre8 left a comment

brunnre8 Jul 15, 2023

brunnre8 Jul 15, 2023

brunnre8 Jul 15, 2023

brunnre8 Jul 15, 2023

azeemba commented Jul 16, 2023

brunnre8 commented Jul 16, 2023

Prune old events from sqlite database #4737

Prune old events from sqlite database #4737

Conversation

azeemba commented May 17, 2023

brunnre8 left a comment

Choose a reason for hiding this comment

brunnre8 May 18, 2023

Choose a reason for hiding this comment

brunnre8 May 18, 2023

Choose a reason for hiding this comment

azeemba May 19, 2023

Choose a reason for hiding this comment

fnutt commented May 19, 2023 • edited

brunnre8 commented May 19, 2023 • edited

brunnre8 left a comment

Choose a reason for hiding this comment

brunnre8 Jul 15, 2023

Choose a reason for hiding this comment

brunnre8 Jul 15, 2023

Choose a reason for hiding this comment

brunnre8 Jul 15, 2023

Choose a reason for hiding this comment

brunnre8 Jul 15, 2023

Choose a reason for hiding this comment

azeemba commented Jul 16, 2023

brunnre8 commented Jul 16, 2023

fnutt commented May 19, 2023 •

edited

brunnre8 commented May 19, 2023 •

edited