Opened 5 weeks ago

Last modified 3 days ago

#35918 assigned Cleanup/optimization

SQLCompiler.execute_sql result_type CURSOR usage can be minimized

Reported by: Raphael Gaschignard Owned by: Raphael Gaschignard
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Raphael Gaschignard Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Raphael Gaschignard)

(This refactor is motivated by an ongoing experiment to integrate async cursor work into the ORM, by simplifying some cursor management)

In django.db.models.sql.compiler.Compiler.execute_sql, we can pass in the following result types: SINGLE, MULTI, NO_RESULTS, and CURSOR.

execute_sql's docstring to that effect does not reflect this.

  • SINGLE returns a single row. It closes the cursor it uses to query.
  • MULTI returns many rows (wrapped in a cursor iterator). This either closes the cursor it uses to query, or returns an iterator that takes ownership of the cursor to close the cursor once reading of all the results are done.
  • NO_RESULTS returns nothing. It closes the cursor it uses to query.
  • CURSOR returns the cursor, without closing the cursor, effectively making the caller in charge of closing the cursor

CURSOR returns an unclosed cursor that has to be manage by the caller. In practice, though, apart from a single test usage, Django's codebase currently only uses CURSOR to do one thing: get the number of rows, then close the cursor.

To simplify cursor resource management, I have a two one-pronged proposal:

  • a new result type, ROW_COUNT, returns the rows and closes the cursor for you. This covers all non-test usage of CURSOR in Django currently.

- CURSOR is renamed to LEAK_CURSOR, as a way to more strongly indicate that you are now in charge of the cursor

Main point here is to reduce the number of places an open cursor might come into play.

Change History (5)

comment:1 by Raphael Gaschignard, 5 weeks ago

Description: modified (diff)

comment:2 by Raphael Gaschignard, 5 weeks ago

Has patch: set

Opened A PR on Github with a proposed refactor (https://github.com/django/django/pull/18827/)

comment:3 by Sarah Boyce, 5 weeks ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

To add the PR conclusion here, agreed to add the new result type ROW_COUNT but not to rename CURSOR.

comment:4 by Raphael Gaschignard, 4 weeks ago

Description: modified (diff)
Patch needs improvement: unset

comment:5 by Sarah Boyce, 3 days ago

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top