[Django] #35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
19 views
Skip to first unread message
Django
Nov 19, 2024, 12:05:04 AM11/19/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael | Owner: Raphael Gaschignard
Gaschignard |
Type: | Status: assigned
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
(This refactor is motivated by an ongoing experiment to integrate async
cursor work into the ORM, by simplifying some cursor management)
In ``django.cdb.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-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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35918>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
-------------------------------------+-------------------------------------
Reporter: Raphael | Owner: Raphael Gaschignard
Gaschignard |
Type: | Status: assigned
Cleanup/optimization |
Component: Database | Version: dev
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
(This refactor is motivated by an ongoing experiment to integrate async
cursor work into the ORM, by simplifying some cursor management)
In ``django.cdb.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-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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35918>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Nov 19, 2024, 12:06:09 AM11/19/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
-------------------------------------+-------------------------------------
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Description changed by Raphael Gaschignard:
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:1>
Django
Nov 19, 2024, 12:11:07 AM11/19/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Raphael Gaschignard):
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Opened A PR on Github with a proposed refactor
(https://github.com/django/django/pull/18827/)
--
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:2>
Django
Nov 20, 2024, 4:26:41 AM11/20/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
To add the PR conclusion here, agreed to add the new result type
`ROW_COUNT` but not to rename `CURSOR`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:3>
Django
Nov 20, 2024, 6:40:59 PM11/20/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Raphael Gaschignard):
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
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
`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.
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
- 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.
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:4>
Django
Dec 19, 2024, 8:57:21 AM12/19/24
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):
* needs_better_patch: 0 => 1
--
* needs_better_patch: 0 => 1
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:5>
Django
Jan 3, 2025, 5:05:10 AM1/3/25
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):
* needs_better_patch: 1 => 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:6>
Django
Jan 3, 2025, 9:07:11 AM1/3/25
to django-...@googlegroups.com
#35918: SQLCompiler.execute_sql result_type CURSOR usage can be minimized
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Cleanup/optimization | Status: closed
-------------------------------------+-------------------------------------
Reporter: Raphael Gaschignard | Owner: Raphael
Type: | Gaschignard
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Sarah Boyce <42296566+sarahboyce@…>):
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"ddefc3fed1cf1f0d3fab455babbbc009b76e4196" ddefc3fe]:
{{{#!CommitTicketReference repository=""
revision="ddefc3fed1cf1f0d3fab455babbbc009b76e4196"
Fixed #35918 -- Added support for execute_sql to directly return row
counts.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35918#comment:7>
Reply all
Reply to author
Forward
0 new messages