Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#25517 closed Bug (fixed)

Concat() database function is not idempotent in SQLite

Reported by: Warren Smith Owned by: Josh Smeaton
Component: Database layer (models, ORM) Version: 1.8
Severity: Release blocker Keywords: concat coalesce sqlite
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Warren Smith)

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.

Change History (15)

comment:1 Changed 4 years ago by Warren Smith

Description: modified (diff)

comment:2 Changed 4 years ago by Warren Smith

Description: modified (diff)

comment:3 Changed 4 years ago by Warren Smith

Description: modified (diff)
Owner: changed from nobody to Warren Smith
Status: newassigned

comment:4 Changed 4 years ago by Warren Smith

Has patch: set

comment:5 Changed 4 years ago by Warren Smith

Triage Stage: UnreviewedReady for checkin

comment:6 Changed 4 years ago by Tim Graham

Triage Stage: Ready for checkinUnreviewed

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!

comment:7 Changed 4 years ago by Warren Smith

Oops. Sorry about that. Its been a while since I contributed.

comment:8 Changed 4 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

comment:9 Changed 4 years ago by Warren Smith

Owner: Warren Smith deleted
Status: assignednew

comment:10 Changed 4 years ago by Josh Smeaton

Owner: set to Josh Smeaton
Patch needs improvement: unset
Status: newassigned
Triage Stage: AcceptedReady for checkin

I think this should probably be backported to 1.8. It's a crashing bug if Concat is reused on sqlite.

comment:11 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 6c95b13:

Fixed #25517 -- Made Concat function idempotent on SQLite.

comment:12 Changed 4 years ago by Tim Graham <timograham@…>

In 42e029f:

[1.8.x] Fixed #25517 -- Made Concat function idempotent on SQLite.

Backport of 6c95b134e9b2d5641c123551c080305e90e6a89d from master

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

In 7a3b486:

[1.9.x] Fixed #25517 -- Made Concat function idempotent on SQLite.

Backport of 6c95b134e9b2d5641c123551c080305e90e6a89d from master

comment:14 Changed 4 years ago by Tim Graham

Has patch: unset
Severity: NormalRelease blocker
Triage Stage: Ready for checkinAccepted
Version: master1.8

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.

comment:15 Changed 4 years ago by Josh Smeaton <josh.smeaton@…>

In 61ea371:

Refs #25517 -- Fixed backport inconsistencies.

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