Opened 7 years ago

Closed 3 years ago

#10399 closed Cleanup/optimization (fixed)

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

Reported by: mtredinnick Owned by: charettes
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 charettes 3 years ago.
Testcase
0001-Added-a-context-manager-to-capture-queries-while-tes.patch (12.0 KB) - added by charettes 3 years ago.
Added CaptureQueriesContext with tests
0002-Fixed-10399-Tested-that-o2o-field-updates-are-not-co.patch (2.1 KB) - added by charettes 3 years ago.
Testcase for this ticket using CaptureQueriesContext

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to 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 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:4 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:5 Changed 3 years ago by akaariai

  • 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 3 years ago by charettes

  • Owner changed from mtredinnick to charettes
  • Status changed from new to assigned

I'll write that testcase.

comment:7 Changed 3 years ago by charettes

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

comment:8 Changed 3 years ago by claudep

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 3 years ago by charettes

  • 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 3 years ago by charettes

Added CaptureQueriesContext with tests

Changed 3 years ago by charettes

Testcase for this ticket using CaptureQueriesContext

comment:10 Changed 3 years ago by charettes

  • Patch needs improvement unset

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

comment:11 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 3 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 3 years ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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