Skip to content

[18.0][FIX] mass_mailing_list_dynamic: optimize sync operations#16

Merged
OCA-git-bot merged 1 commit into
OCA:18.0from
moduon:18.0-mass_mailing_list_dynamic-sync-optimize
Mar 30, 2026
Merged

[18.0][FIX] mass_mailing_list_dynamic: optimize sync operations#16
OCA-git-bot merged 1 commit into
OCA:18.0from
moduon:18.0-mass_mailing_list_dynamic-sync-optimize

Conversation

@yajo
Copy link
Copy Markdown
Member

@yajo yajo commented Mar 23, 2026

Now there is only 1 ORM operation per list and 1 global unlink of garbage
contacts at the end.

@moduon MT-13677

Copy link
Copy Markdown
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Looking good 👍

Copy link
Copy Markdown
Contributor

@Shide Shide left a comment

Choose a reason for hiding this comment

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

Code review

@pedrobaeza pedrobaeza added this to the 18.0 milestone Mar 23, 2026
@pedrobaeza pedrobaeza changed the title [FIX] mass_mailing_list_dynamic: optimize sync operations [18.0][FIX] mass_mailing_list_dynamic: optimize sync operations Mar 23, 2026
Copy link
Copy Markdown
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

Thanks for improving the performance. The only problem I see with this is that the recordset operations creates a new recordset, so they are not very good at performance. Alternatives are to store a set of IDs instead.

Comment thread mass_mailing_list_dynamic/models/mailing_list.py Outdated
@yajo yajo force-pushed the 18.0-mass_mailing_list_dynamic-sync-optimize branch from 7f912e1 to 9ba49f6 Compare March 24, 2026 07:46
@yajo
Copy link
Copy Markdown
Member Author

yajo commented Mar 24, 2026

The only problem I see with this is that the recordset operations creates a new recordset, so they are not very good at performance

Thanks for the pointer. However, these are the most impactful performance problems:

  1. Field assignment triggers constraints:
    one.contact_ids -= contact_to_detach
    (this is related to [FIX] mass_mailing_partner: optimize duplicates check and error message #17 where the constraint triggered a flush because of a search within).
  2. Unlink triggers flushes:
    contact_to_detach.filtered(lambda r: not r.list_ids).unlink()
  3. Create too:

Removing those problems, the difference between in-memory recordsets or in-memory lists of IDs becomes irrelevant, while making the code easier to maintain.

Comment thread mass_mailing_list_dynamic/models/mailing_list.py Outdated
Now there is only 1 ORM operation per list and 1 global unlink of garbage
contacts at the end.

@moduon MT-13677
@yajo yajo force-pushed the 18.0-mass_mailing_list_dynamic-sync-optimize branch from 9ba49f6 to 06c2000 Compare March 30, 2026 09:43
Copy link
Copy Markdown
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge patch

@OCA-git-bot
Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 18.0-ocabot-merge-pr-16-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot
Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@OCA-git-bot OCA-git-bot merged commit 0c9f374 into OCA:18.0 Mar 30, 2026
7 checks passed
@OCA-git-bot
Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 71c5c12. Thanks a lot for contributing to OCA. ❤️

@yajo yajo deleted the 18.0-mass_mailing_list_dynamic-sync-optimize branch March 30, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants