-
Notifications
You must be signed in to change notification settings - Fork 44
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
base: master
Are you sure you want to change the base?
Cleanup database #518
Conversation
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this 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); |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
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.