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

Cleanup database #518

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

Cleanup database #518

wants to merge 4 commits into from

Conversation

Degit22
Copy link

@Degit22 Degit22 commented Apr 21, 2022

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
Copy link
Member

tanriol commented Apr 23, 2022

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.

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
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?

Copy link
Member

@tanriol tanriol left a comment

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.

let request = tx.objectStore('entries').delete(id);
DbUtil.requestPromise(request);

let request2 = tx.objectStore('revisions').delete(id);
Copy link
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.

});
let ids = await query.getIds();
for(const id of ids.values()) {
let tx = this.db().transaction(['entries', 'revisions'], 'readwrite');
Copy link
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)

@@ -1163,6 +1163,7 @@ export let Commands = {

emptyTrash: function cmd_emptyTrash() {
ViewList.db.query(ViewList.getQueryForView('trash-folder')).markDeleted('deleted');
ViewList.db.cleanupEntries();
Copy link
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