Skip to content

Cleanup database - #518

Open
Degit22 wants to merge 4 commits into
brief-rss:masterfrom
Degit22:cleanup
Open

Cleanup database#518
Degit22 wants to merge 4 commits into
brief-rss:masterfrom
Degit22:cleanup

Conversation

@Degit22

@Degit22 Degit22 commented Apr 21, 2022

Copy link
Copy Markdown

My database is already almost 2 GB :(
https://monosnap.com/file/fVMNr18k0eH6YFDRO8A2DcU9ypeo9N
Entries are not deleted, but only marked as deleted when the recycle bin is emptied. The disk is simply littered with unnecessary data.
This patch implements the deletion of entries when emptying the trash.
This will save space on the system disk, and reduce the backup of the Firefox profile.

@tanriol

tanriol commented Apr 23, 2022

Copy link
Copy Markdown
Member

Hello and thank you for your contributions. Unfortunately, I'm stretched really thin at the moment, so may be unable to meaningfully react for quite some time. Will try to review in a few days.

Comment thread modules/database.js
for(const id of ids.values()) {
let tx = this.db().transaction(['entries', 'revisions'], 'readwrite');
let request = tx.objectStore('entries').delete(id);
DbUtil.requestPromise(request);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this just for error reporting?

@tanriol tanriol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks reasonable in general, although I've marked a couple important points and a couple minor questions.

Comment thread modules/database.js
let request = tx.objectStore('entries').delete(id);
DbUtil.requestPromise(request);

let request2 = tx.objectStore('revisions').delete(id);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please don't assume that entry IDs and revision IDs match (and in general that there's just one revision per entry). Yes, that's the case at the moment, but I'd prefer being able to eventually change that.

Comment thread modules/database.js
});
let ids = await query.getIds();
for(const id of ids.values()) {
let tx = this.db().transaction(['entries', 'revisions'], 'readwrite');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you benchmarked this? I'm afraid that the initial cleanup will take a lot of time in this mode. I think this code should process entries to be deleted in batches of 1000 or so (possibly reusing Migrator.BATCH_SIZE)

Comment thread ui/feedlist.js

emptyTrash: function cmd_emptyTrash() {
ViewList.db.query(ViewList.getQueryForView('trash-folder')).markDeleted('deleted');
ViewList.db.cleanupEntries();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note that markDeleted is async and I'm not really sure whether you'll have any guarantees on the order of these operations. Also you may want to hook into entry expiration too for the sake of users who don't empty their trash manually.

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.

2 participants