[Django] #25517: Concat() database function is not idempotent in SQLite
Django
----------------------------------------------+----------------------------
Reporter: wsmith323 | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords: concat
| coalesce sqlite
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+----------------------------
When running on sqlite, with a {{{Concat()}}} expression used in the
query, the {{{ConcatPair.coalesce()}}} method is called every time the SQL
is generated. This is normal, expected, and AFAIK, correct behavior.
The problem is that {{{ConcatPair.coalesce()}}} is NOT idempotent. So,
EVERY time the SQL is generated, each expression within the
{{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
function. If generated enough times, the SQL can reach a point where it
will crash the sqlite query parser.
This problem may not manifest itself in a typical request/response
context, as the SQL with the additional `COALESCE()` calls will work
identically to the original and the SQL may not be re-generated a
sufficient number of times to crash the parser.
However, in a long-running process (where this bug was found), it can be
easily triggered. For example, say I have a "base" queryset with a
{{{Concat()}}} within an {{{.annotate()}}}. I never actually evaluate this
queryset, but I use it to construct other querysets which I do evaluate.
Because all of these querysets share the same instance of
Query._annotations, evaluating ANY of these querysets will add an
additional level of COALESCE() to the SQL generated by the others.
I have a fix coded. I will submit a PR shortly.
--
Ticket URL: <https://code.djangoproject.com/ticket/25517>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: wsmith323 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wsmith323):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> When running on sqlite, with a {{{Concat()}}} expression used in the
> query, the {{{ConcatPair.coalesce()}}} method is called every time the
> SQL is generated. This is normal, expected, and AFAIK, correct behavior.
>
> The problem is that {{{ConcatPair.coalesce()}}} is NOT idempotent. So,
> EVERY time the SQL is generated, each expression within the
> {{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
> function. If generated enough times, the SQL can reach a point where it
> will crash the sqlite query parser.
>
> This problem may not manifest itself in a typical request/response
> context, as the SQL with the additional `COALESCE()` calls will work
> identically to the original and the SQL may not be re-generated a
> sufficient number of times to crash the parser.
>
> However, in a long-running process (where this bug was found), it can be
> easily triggered. For example, say I have a "base" queryset with a
> {{{Concat()}}} within an {{{.annotate()}}}. I never actually evaluate
> this queryset, but I use it to construct other querysets which I do
> evaluate. Because all of these querysets share the same instance of
> Query._annotations, evaluating ANY of these querysets will add an
> additional level of COALESCE() to the SQL generated by the others.
>
> I have a fix coded. I will submit a PR shortly.
New description:
When running on sqlite, with a `Concat()` expression used in the query,
the `ConcatPair.coalesce()` method is called every time the SQL is
generated. This is normal, expected, and AFAIK, correct behavior.
The problem is that `ConcatPair.coalesce()` is not idempotent. So,
**//every//** time the SQL is generated, each expression within the
{{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
function. If generated enough times, the SQL can reach a point where it
will crash the sqlite query parser.
This problem may not manifest itself in a typical request/response
context, as the SQL with the additional `COALESCE()` calls will work
identically to the original and the SQL may not be re-generated a
sufficient number of times to crash the parser.
However, in a long-running process (where this bug was found), it can be
easily triggered. For example, say I have a "base" queryset with a
`Concat()` within an `.annotate()`. I never actually evaluate this
queryset, but I use it to construct other querysets which I do evaluate.
Because all of these querysets share the same instance of
Query._annotations, evaluating ANY of these querysets will add an
additional level of `COALESCE()` to the SQL generated by the others.
I have a fix coded. I will submit a PR shortly.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:1>
Django
Reporter: wsmith323 | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Description changed by wsmith323:
Old description:
> When running on sqlite, with a `Concat()` expression used in the query,
> the `ConcatPair.coalesce()` method is called every time the SQL is
> generated. This is normal, expected, and AFAIK, correct behavior.
>
> The problem is that `ConcatPair.coalesce()` is not idempotent. So,
> **//every//** time the SQL is generated, each expression within the
> {{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
> function. If generated enough times, the SQL can reach a point where it
> will crash the sqlite query parser.
>
> This problem may not manifest itself in a typical request/response
> context, as the SQL with the additional `COALESCE()` calls will work
> identically to the original and the SQL may not be re-generated a
> sufficient number of times to crash the parser.
>
> However, in a long-running process (where this bug was found), it can be
> easily triggered. For example, say I have a "base" queryset with a
> `Concat()` within an `.annotate()`. I never actually evaluate this
> queryset, but I use it to construct other querysets which I do evaluate.
> Because all of these querysets share the same instance of
> Query._annotations, evaluating ANY of these querysets will add an
> additional level of `COALESCE()` to the SQL generated by the others.
>
> I have a fix coded. I will submit a PR shortly.
New description:
When running on sqlite, with a `Concat()` expression used in the query,
the `ConcatPair.coalesce()` method is called every time the SQL is
generated. This is normal, expected, and AFAIK, correct behavior.
The problem is that `ConcatPair.coalesce()` is not idempotent. So,
**//every//** time the SQL is generated, each expression within the
{{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
function. If generated enough times, the SQL can reach a point where it
will crash the sqlite query parser.
This problem may not manifest itself in a typical request/response
context, as the SQL with the additional `COALESCE()` calls will work
identically to the original and the SQL may not be re-generated a
sufficient number of times to crash the parser.
However, in a long-running process (where this bug was found), it can be
easily triggered. For example, say I have a "base" queryset with a
`Concat()` within an `.annotate()`. I never actually evaluate this
queryset, but I use it to construct other querysets which I do evaluate.
Because all of these querysets share the same instance of
`Query._annotations`, evaluating ANY of these querysets will add an
additional level of `COALESCE()` to the SQL generated by the others.
I have a fix coded. I will submit a PR shortly.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:2>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
sqlite | Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wsmith323):
* owner: nobody => wsmith323
* status: new => assigned
Old description:
> When running on sqlite, with a `Concat()` expression used in the query,
> the `ConcatPair.coalesce()` method is called every time the SQL is
> generated. This is normal, expected, and AFAIK, correct behavior.
>
> The problem is that `ConcatPair.coalesce()` is not idempotent. So,
> **//every//** time the SQL is generated, each expression within the
> {{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
> function. If generated enough times, the SQL can reach a point where it
> will crash the sqlite query parser.
>
> This problem may not manifest itself in a typical request/response
> context, as the SQL with the additional `COALESCE()` calls will work
> identically to the original and the SQL may not be re-generated a
> sufficient number of times to crash the parser.
>
> However, in a long-running process (where this bug was found), it can be
> easily triggered. For example, say I have a "base" queryset with a
> `Concat()` within an `.annotate()`. I never actually evaluate this
> queryset, but I use it to construct other querysets which I do evaluate.
> Because all of these querysets share the same instance of
> `Query._annotations`, evaluating ANY of these querysets will add an
> additional level of `COALESCE()` to the SQL generated by the others.
>
> I have a fix coded. I will submit a PR shortly.
New description:
When running on sqlite, with a `Concat()` expression used in the query,
the `ConcatPair.coalesce()` method is called every time the SQL is
generated. This is normal, expected, and AFAIK, correct behavior.
The problem is that `ConcatPair.coalesce()` is not idempotent. So,
**//every//** time the SQL is generated, each expression within the
{{{ConcatPair}}} instance is wrapped with an additional `COALESCE()` SQL
function. If generated enough times, the SQL can reach a point where it
will crash the sqlite query parser.
This problem may not manifest itself in a typical request/response
context, as the SQL with the additional `COALESCE()` calls will work
identically to the original and the SQL may not be re-generated a
sufficient number of times to crash the parser.
However, in a long-running process (where this bug was found), it can be
easily triggered. For example, say I have a "base" queryset with a
`Concat()` within an `.annotate()`. I never actually evaluate this
queryset, but I use it to construct other querysets which I do evaluate.
Because all of these querysets share the same instance of
`Query._annotations`, evaluating ANY of these querysets will add an
additional level of `COALESCE()` to the SQL generated by the others.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:3>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
sqlite | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wsmith323):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:4>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
sqlite | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by wsmith323):
* stage: Unreviewed => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:5>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Ready for checkin => Unreviewed
Comment:
Thanks for the report and patch, however, it's not appropriate to triage
your own tickets, see
https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#triage-workflow. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:6>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage:
sqlite | Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by wsmith323):
Oops. Sorry about that. Its been a while since I contributed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:7>
Django
Reporter: wsmith323 | Owner: wsmith323
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
sqlite |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Could you give a queryset that reproduces the issue and use that in the
test rather than reproducing with the low level APIs like `SQLCompiler`? I
think that will make the issue more understandable, at least to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:8>
Django
Reporter: wsmith323 | Owner:
Type: Bug | Status: new
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: concat coalesce | Triage Stage: Accepted
sqlite |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by wsmith323):
* owner: wsmith323 =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:9>
Django
Reporter: wsmith323 | Owner: jarshwah
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
sqlite | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jarshwah):
* status: new => assigned
* needs_better_patch: 1 => 0
* owner: => jarshwah
* stage: Accepted => Ready for checkin
Comment:
I think this should probably be backported to 1.8. It's a crashing bug if
Concat is reused on sqlite.
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:10>
Django
Reporter: wsmith323 | Owner: jarshwah
Component: Database layer | Version: master
(models, ORM) |
Keywords: concat coalesce | Triage Stage: Ready for
sqlite | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6c95b134e9b2d5641c123551c080305e90e6a89d" 6c95b13]:
{{{
#!CommitTicketReference repository=""
revision="6c95b134e9b2d5641c123551c080305e90e6a89d"
Fixed #25517 -- Made Concat function idempotent on SQLite.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:11>
Django
Reporter: wsmith323 | Owner: jarshwah
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: concat coalesce | Triage Stage: Ready for
sqlite | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"42e029f6c4d9b03a238c3f8b479258ca2cb034ed" 42e029f]:
{{{
#!CommitTicketReference repository=""
revision="42e029f6c4d9b03a238c3f8b479258ca2cb034ed"
[1.8.x] Fixed #25517 -- Made Concat function idempotent on SQLite.
Backport of 6c95b134e9b2d5641c123551c080305e90e6a89d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:12>
Django
Reporter: wsmith323 | Owner: jarshwah
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: concat coalesce | Triage Stage: Ready for
sqlite | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"7a3b486ccd4aba0a9cf69475895a9b89ae2438b3" 7a3b486]:
{{{
#!CommitTicketReference repository=""
revision="7a3b486ccd4aba0a9cf69475895a9b89ae2438b3"
[1.9.x] Fixed #25517 -- Made Concat function idempotent on SQLite.
Backport of 6c95b134e9b2d5641c123551c080305e90e6a89d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:13>
Django
Reporter: wsmith323 | Owner: jarshwah
Type: Bug | Status: closed
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: concat coalesce | Triage Stage: Accepted
sqlite |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* has_patch: 1 => 0
* version: master => 1.8
* severity: Normal => Release blocker
* stage: Ready for checkin => Accepted
Comment:
Oops, I didn't check if the tests passed when backporting to 1.8 and there
are a few test failures there. We can solve the SQLite failures by
backporting `BaseExpression.flatten()` from
134ca4d438bd7cbe8f0f287a00d545f96fa04a01 but there are also some MySQL two
failures in `tests/db_functions`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:14>
Django
Reporter: wsmith323 | Owner: jarshwah
Type: Bug | Status: closed
Component: Database layer | Version: 1.8
(models, ORM) |
Severity: Release blocker | Resolution: fixed
Keywords: concat coalesce | Triage Stage: Accepted
sqlite |
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Josh Smeaton <josh.smeaton@…>):
In [changeset:"61ea3718224e6264f2d21858da70e3cbff3bd72e" 61ea371]:
{{{
#!CommitTicketReference repository=""
revision="61ea3718224e6264f2d21858da70e3cbff3bd72e"
Refs #25517 -- Fixed backport inconsistencies.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25517#comment:15>