Opened 8 years ago

Closed 4 years ago

#10399 closed Cleanup/optimization (fixed)

Optimise cross-table update for one-to-one fields

Reported by: Malcolm Tredinnick Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When we do an update involving a OneToOneField (model inheritance cases being an obvious example), the update queryset does something akin to filter(pk__in=inner_query) where inner_query describes the child model. However, the field in this particular case is known to be one-to-one, not many-to-one. So the reverse relation will only match one item. Thus, we might as well do filter(pk=some_value), which is much faster.

Detecting this case can probably be handled in UpdateQuery.add_update_fields() in some fashion, I suspect.

(This is another one of those long-term SQL optimisation projects that I'll get to at some point. Making a tracking bug for it, since it's been floating around for a while.)

Attachments (3)

0001-Fixed-10399-Tested-that-o2o-field-updates-are-not-co.patch (4.4 KB) - added by Simon Charette 4 years ago.
Testcase
0001-Added-a-context-manager-to-capture-queries-while-tes.patch (12.0 KB) - added by Simon Charette 4 years ago.
Added CaptureQueriesContext with tests
0002-Fixed-10399-Tested-that-o2o-field-updates-are-not-co.patch (2.1 KB) - added by Simon Charette 4 years ago.
Testcase for this ticket using CaptureQueriesContext

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by Malcolm Tredinnick

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedAccepted

comment:2 Changed 6 years ago by Chris Beaven

Severity: Normal
Type: Cleanup/optimization

This is a pretty old ticket... is it still just as relevant today with the current state of the ORM?

comment:3 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:4 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:5 Changed 4 years ago by Anssi Kääriäinen

Easy pickings: set
Needs tests: set

I believe this one is now fixed for normal querying. The changes done to join path generation likely fix this one for update queries, too.

Anybody willing to write a test case for this ticket?

comment:6 Changed 4 years ago by Simon Charette

Owner: changed from Malcolm Tredinnick to Simon Charette
Status: newassigned

I'll write that testcase.

Changed 4 years ago by Simon Charette

Testcase

comment:7 Changed 4 years ago by Simon Charette

Added a patch with a testcase that passes on (py273, py323) X (SQLite3, Postgresql).

comment:8 Changed 4 years ago by Claude Paroz

CaptureQueriesContext is a nice addition. From the bikeshedding department:

  • separate commit for adding CaptureQueriesContext
  • specific tests for CaptureQueriesContext in test_utils
  • when __getitem__ is applied to a list, I'd rather use an argument named i or index

I'd suggest you go ahead with the commits when above points are addressed (unless someone else has other concerns, of course).

comment:9 Changed 4 years ago by Simon Charette

Easy pickings: unset
Has patch: set
Needs tests: unset
Patch needs improvement: set

Thanks for your review, was wondering if I should commit CaptureQueriesContext separately.

Changed 4 years ago by Simon Charette

Added CaptureQueriesContext with tests

Changed 4 years ago by Simon Charette

Testcase for this ticket using CaptureQueriesContext

comment:10 Changed 4 years ago by Simon Charette

Patch needs improvement: unset

Added tests for CaptureQueriesContext, included claudep's suggestions and split the patch in two.

comment:11 Changed 4 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:12 Changed 4 years ago by Simon Charette <charette.s@…>

In 952ba5237ea62e7647cdd5214b1df79c0e7cea38:

Added a context manager to capture queries while testing.

Also made some import cleanups while I was there.

Refs #10399.

comment:13 Changed 4 years ago by Simon Charette <charette.s@…>

Resolution: fixed
Status: assignedclosed

In fb3d85bd14f1f7a8969ef8c54f2f0603e161f428:

Fixed #10399 -- Tested that o2o field updates are not constrained by an inner query.

Note: See TracTickets for help on using tickets.
Back to Top