django-simple-history icon indicating copy to clipboard operation
django-simple-history copied to clipboard

Published 20 hours ago • verified publisher icon jazzband

Updating an m2m set on a model with history causes a lot of duplicate queries

Open alimony opened this issue 1 year ago • 7 comments
trafficstars

If I have the models e.g. User and Group where Group has an m2m field to User and also has a field like history = HistoricalRecords(m2m_fields=[users]), calling any of the m2m set update methods such as group.users.set(users) or group.users.add(user1, user2) causes duplicate queries.

I have an example of this where I add 25 users to a group, and it generates 25 extra user selects and 25 extra group selects.

Is there any way to work around this? Since I do this right after creating a new Group, I tried bypassing history saving on the Group model as suggested by the documentation, but related sets seem unaffected by this.

alimony avatar Mar 14 '24 14:03 alimony

I'm not sure I'm able to reproduce your issue 🤔 Calling e.g. add() with multiple arguments should only create one historical record (see e.g. this test case)... Could you provide some (complete) test case code that reproduces your issue? 🙂

ddabble avatar Mar 15 '24 20:03 ddabble

This is not about duplicate records, but duplicate database queries, as identified by e.g. Django Debug Toolbar. It’s a performance issue. Can you also see duplicate queries by inspecting with debug toolbar? If not, I’ll try and make a test case.

alimony avatar Mar 15 '24 23:03 alimony

Ah, sorry, I misread your description 😅 I'm able to reproduce it, and will open a PR for it :)

ddabble avatar Mar 15 '24 23:03 ddabble

Thank you, lovely to hear! Let me know if I can help in any way.

alimony avatar Mar 16 '24 10:03 alimony

Any update on this?

alimony avatar Mar 20 '24 14:03 alimony

I've made a fix for it locally, but considering that @tim-schilling and I are the only currently active maintainers, I want to focus on finishing #1128 first, which seems close to being merged 🙂

ddabble avatar May 03 '24 12:05 ddabble

@alimony if this is an urgent thing for you, please write the test case and attempt a fix. Anything that gets things started will help tremendously.

tim-schilling avatar May 03 '24 13:05 tim-schilling

Thanks a lot @ddabble !

alimony avatar May 15 '24 15:05 alimony

@ddabble Any idea when we might see this in a release?

alimony avatar May 15 '24 17:05 alimony

I'm planning on creating a new release after #1344 has been merged 🙂

ddabble avatar May 15 '24 20:05 ddabble

@alimony I've created a GitHub release for version 3.6.0, but it's yet to be published to PyPI; see progress in #1345

ddabble avatar May 26 '24 23:05 ddabble