Use passive_delete to handle LabelItem cascade

[mrhiggi-close]

When triggering a resync for gmail, we attempt to delete any unmatched imap UID entries during the yield_chunk iterations. If there is more than one chunk, then this query is still active in the session, and we get a Commands out of sync; you can't run this command now error.

Using passive_deletes=True, we can avoid lazy loading and deleting the associations in the ORM. We keep the cascade="all, delete-orphan" so that the ORM handles the deletion if we remove the label item from the association list.

This is the other approach I was considering in: #1063

[neob91-close]

The way I understand this, passive_deletes=True would stop deleting the related children inside the ORM, and it is expected that the database has proper ON CASCADE set up to do the deletions. Do we need to set these up?

Also, I wanted to understand this problem better (I don't think I fully grasped it before now), so I did some research, and I wonder - couldn't we just preemptively load the related children here to avoid them being loaded lazily while yield_per is streaming the results?

An alternative would be to collect the IDs of object to delete, and then just delete them in batches:

for imap_uid_ids_batch in batched(ids_to_delete, 1000): # pseudo-code
    for imap_uid in db_session.query(ImapUid).filter(ImapUid.id.in_(imap_uid_ids_batch)):
        imap_uid.delete()

[mrhiggi-close]

@neob91-close Yes, the passive deletes depends on the database to cascade for deletes. delete-orphans will still depend on the ORM when objects loaded into the session are removed from a relationship. Nothing is deleted at that point as far as the database is concerned, so the ORM has to manage the association table.

For constraints in the database, the imapuid_id and label_id declarations suggest a foreign key constraint with on delete cascade should be defined:

imapuid_id = Column(
        ForeignKey(ImapUid.id, ondelete="CASCADE"), nullable=False
)

Inspecting the table confirms the constraint exists on labelitem:

CONSTRAINT `labelitem_ibfk_1` FOREIGN KEY (`imapuid_id`) REFERENCES `imapuid` (`id`) ON DELETE CASCADE,
CONSTRAINT `labelitem_ibfk_2` FOREIGN KEY (`label_id`) REFERENCES `label` (`id`) ON DELETE CASCADE

As far as pre-emptively loading children: Yes, I think that would resolve the error. However, we're going to be loading uids for every entry in the folder when resyncing. The query explicitly loads only the ID and google message ID. Using passive deletes saves us from a bunch of loading and work.

Collecting the IDs and deleting in batches afterward: Yes, that could work too. Would that essentially be https://github.com/closeio/sync-engine/pull/1063/files, just pulling the call to common_delete above the calls to set uidvalidity? Essentially:

# Adapted to your pseudocode rather than common delete
for chunk in batched(ids_to_delete):
    for imap_uid in db_session.query(ImapUid).filter(ImapUid.id.in_(chunk)):
        imap_uid.delete()
imap_folder_info_entry.uidvalidity = uidvalidity
imap_folder_info_entry.highestmodseq = None
db_session.commit()

The simplest solution overall is probably eager loading the label items. I'll give that a test.

I'm still a little sceptical that we just call delete on the imapuid entries here, but use remove_deleted_uids in the generic implementation.

[mrhiggi-close]

Eager loading collections isn't supported via joinedload or subqueryload in yield_per:

sqlalchemy.exc.InvalidRequestError: Can't use yield_per with eager loaders that require uniquing or row buffering, e.g. joinedload() against collections or subqueryload().  Consider the selectinload() strategy for better flexibility in loading objects.

selectinload brings us back to the Commands out of sync error. Looking it up, SQLAlchemy has some guidance: https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html#joined-eager-loading-and-result-set-batching

Essentially, joined loading for collections never works with yield_per, and selectinload doesn't work in mysql.

I think passive deletes is potentially the smoothest solution. Looking into remove_deleted_uids, that cleans up associated message entries. But we deduplicate messages for gmail imapuids, so we probably can't just use that.

[neob91-close]

I appreciate the detailed investigation! The passive_deletes solution seemed quite hacky to me, but I guess it's fine if there's no better way that works. I think I'd still prefer the batched solution (seems less brittle to me, and easier to understand). Are there any drawbacks you can think of?

That said, I don't want to block this too much longer, I'm sure you have better things to do than discuss different options on this one issue, so if you're adamant about the passive_deletes, I guess I'm okay with this.

[neob91-close]

Nit: I think we should add a sanity check assertion that checks the number of ImapUid currently stored in the database is what we expect it to be. Currently it looks to me like we trust initial_sync to add stuff to the database, but I'd rather tests weren't so trusting.

[neob91-close]

I think you may have run black with a different config than is run in CI.

I run linting this way (I know, hacky and inefficient):

ip-192-168-100-38:close-ui neob91$ cat ../sync-engine/lint
#!/bin/bash
docker compose run --rm app bash -c 'pip3 install -r requirements/requirements-lint.txt; ruff check --fix inbox tests; black inbox tests'

[neob91-close]

I hope you don't mind me pushing to your branch, but I fixed the formatting and added a very simple test assertion. My only motivation here was to make your life easier (you should have a green mergeable PR when you come in). Let me know if I overstepped!

[mrhiggi-close]

I think you may have run black with a different config than is run in CI.

I run linting this way (I know, hacky and inefficient):

ip-192-168-100-38:close-ui neob91$ cat ../sync-engine/lint

#!/bin/bash

docker compose run --rm app bash -c 'pip3 install -r requirements/requirements-lint.txt; ruff check --fix inbox tests; black inbox tests'

I've been running it in the container, installed from the requirements. But I ran black then ruff.