search

deltachat / deltachat-core-rust

Delta Chat Rust Core library, used by Android/iOS/desktop apps, bindings and bots 📧
https://delta.chat/en/contribute
Other
658 stars 84 forks source link

Add expiration_timestamp column to imap table #3232

Open link2xt opened 2 years ago

link2xt commented 2 years ago

I propose to add a column expiration_timestamp to imap table which will say when the message should be deleted from IMAP server. This column should be calculated at the time of message reception according to the current delete_server_after or at the time of message being seen for disappearing messages.

This approach solves several problems:

  1. Speed up delete_expired_imap_messages. No need to query msgs table anymore to find messages that should be deleted in the IMAP loop. IMAP loop accesses only the imap table and is completely decoupled from the UI-related msgs table.
  2. Remove the need for dc_estimate_deletion_cnt for server-related setting and scary "are you sure?" dialog. Changing the setting does not affect previously received messages, similarly to how disappearing messages work. Less chance for user to accidentally delete all messages from the server by setting the wrong (too low) value.
  3. Remove the need for tombstones, i.e. messages in msgs table with cleared out text and other columns, serving only to store message timestamp and rfc724_mid for later deletion from IMAP. No need for prune_tombstones. delete_expired_messages can directly plan deletion from IMAP by modifying imap.expiration_timestamp column and delete msgs entry rather than turning msgs entry into a tombstone.

See related comment here: https://github.com/deltachat/deltachat-core-rust/pull/3201#issuecomment-1092749242

We can then probably revert #3225 to make IMAP deletion more timely (no need to wait for next IDLE loop as explained in https://github.com/deltachat/deltachat-core-rust/pull/3225#issuecomment-1100777445) if delete_expired_imap_messages becomes fast. Actually, delete_expired_imap_messages will be removed completely as a result of this change and become part of move_delete_messages which will treat expired messages as targeted for deletion.

cc @hpk42

User-visible change of this will be delete_server_after setting affecting only new messages and removal of the confirmation dialog.

link2xt commented 2 years ago

It may be confusing that messages are still disappearing when the setting was previously turned on but is now turned off. If we decide that we want to keep old behavior, it's possible to add reception timestamp column for delete_server_after and expiration timestamp column for disappearing messages.

I think only affecting new messages is better than current behavior, because currently you can turn "delete immediately" setting on and accidentally remove all your messages, plus the confirmation dialog is not needed with new approach. And adding only a single column is easier to maintain and removing columns is technically impossible, so i'd rather add only one column if we can.

link2xt commented 2 years ago

There is also a question of setting ephemeral_timestamp after UID validity change, i.e. in resync_folder_uids. We can't determine if messages are entirely new or old. Should we unset the timer and never delete these messages or set the timer as if all messages are new? Keeping the messages seems to be safer.

Hocuri commented 2 years ago

Remove the need for dc_estimate_deletion_cnt for server-related setting and scary "are you sure?" dialog. Changing the setting does not affect previously received messages, similarly to how disappearing messages work. Less chance for user to accidentally delete all messages from the server by setting the wrong (too low) value.

This means that after turning on the setting, some old messages from before turning it on will stay on the server forever. Which sounds like a disadvantage, but I'm not using this feature anyway, so ping @adbenitez.

3. Remove the need for tombstones, i.e. messages in msgs table with cleared out text and other columns, serving only to store message timestamp and rfc724_mid for later deletion from IMAP.

Note that we also keep the tombstones to prevent downloading the messages again if they somehow got a new UID on the server (were moved or something). Nothing that can't be solved of course (by checking the imap table in prefetch_should_download).

adbenitez commented 2 years ago

yes, people have issues with full inbox already because DC leaves some emails in the inbox and don't delete them properly (maybe due to network errors at the time it tried to delete or perhaps bugs), with this proposed change, then people will be even more dependent in 3rd party tools to clean the inbox, would be nice if delta proposed to clean the inbox when reaching full inbox and have an option to clear the inbox

hpk42 commented 1 year ago

On Mon, Apr 18, 2022 at 14:39 -0700, link2xt wrote:

I propose to add a column expiration_timestamp to imap table which will say when the message should be deleted from IMAP server. This column should be calculated at the time of message reception according to the current delete_server_after or at the time of message being seen for disappearing messages.

I am generally to move expiration business out of msgs table. Was so far thinking of an expiration table with "imap_expires_at" and "device_expiry_at" timestamp columns.

Is this idea neccessarily tied to changing user-facing behaviour of what server auto-deletion does? IMO it's better and easier to understand that "server auto-deletion" works on historic messages. IIRC DC warns how many messages will be deleted in the confirm dialog, no?