Oracle SQL compiler adding outer pagination selects causing ORA-00907: missing right parenthesis when used with select_for_update.

[michael.miller@…]

The Oracle backend SQL compiler is adding outer pagination selects when calling queryset.get causing parsing failures with select_for_update.

Using Django 1.7 rc 1 on Python 3.4.0 Windows 7. Can also reproduce with Django 1.7 rc 1 on Python 3.4.1 Centos 6.5.

Easy to reproduce with a simple project and one model. The only change I made was to add a print(query) to backend.oracle.base.py execute method.

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.oracle',
        'NAME': 'TNSORACLEDB',
        'USER': 'user',
        'PASSWORD': 'pass',
        'HOST': '',
        'PORT': '',
    },
}


class MyModel(models.Model):
    field1 = models.CharField(max_length=100)

Example of simple get having additional outer selects for pagination. There is one row in the table with a pk of 1.

>>> import django
>>> django.setup()
>>> from bar.models import MyModel
>>> y = MyModel.objects.get(pk=1)
ALTER SESSION SET NLS_TERRITORY = 'AMERICA'
ALTER SESSION SET NLS_DATE_FORMAT = 'YYYY-MM-DD HH24:MI:SS' NLS_TIMESTAMP_FORMAT = 'YYYY-MM-DD HH24:MI:SS.FF' TIME_ZONE = 'UTC'
SELECT 1 FROM DUAL WHERE DUMMY LIKE TRANSLATE(:arg0 USING NCHAR_CS) ESCAPE TRANSLATE('\' USING NCHAR_CS)
SELECT * FROM (SELECT ROWNUM AS "_RN", "_SUB".* FROM (SELECT "BAR_MYMODEL"."ID", "BAR_MYMODEL"."FIELD1" FROM "BAR_MYMODEL" WHERE "BAR_MYMODEL"."ID" = :arg0) "_SUB" WHERE ROWNUM <= 21) WHERE "_RN" > 0

I don't think the two outer selects should be here. These outer selects create invalid SQL for Oracle when used with select_for_update.

>>> from django.db import transaction
>>> with transaction.atomic(using='default'):
...     y = MyModel.objects.select_for_update(nowait=True).get(pk=1)
...     
SELECT * FROM (SELECT ROWNUM AS "_RN", "_SUB".* FROM (SELECT "BAR_MYMODEL"."ID", "BAR_MYMODEL"."FIELD1" FROM "BAR_MYMODEL" WHERE "BAR_MYMODEL"."ID" = :arg0 FOR UPDATE NOWAIT) "_SUB" WHERE ROWNUM <= 21) WHERE "_RN" > 0
Traceback (most recent call last):
  File "<input>", line 2, in <module>
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\query.py", line 349, in get
    num = len(clone)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\query.py", line 122, in __len__
    self._fetch_all()
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\query.py", line 964, in _fetch_all
    self._result_cache = list(self.iterator())
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\query.py", line 265, in iterator
    for row in compiler.results_iter():
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\sql\compiler.py", line 699, in results_iter
    for rows in self.execute_sql(MULTI):
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\models\sql\compiler.py", line 785, in execute_sql
    cursor.execute(sql, params)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\backends\utils.py", line 81, in execute
    return super(CursorDebugWrapper, self).execute(sql, params)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\backends\utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\utils\six.py", line 549, in reraise
    raise value.with_traceback(tb)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\backends\utils.py", line 65, in execute
    return self.cursor.execute(sql, params)
  File "C:\Python-Environments\Test-Environment\lib\site-packages\django\db\backends\oracle\base.py", line 898, in execute
    return self.cursor.execute(query, self._param_generator(params))
django.db.utils.DatabaseError: ORA-00907: missing right parenthesis

[michael.miller@…]

Forgot to say using cx_Oracle 5.1.3 on both Windows and Centos.

[michael.miller@…]

Just tried this with the latest development snapshot from github. Still experiencing the same error.

[Shai Berger]

Accepting for now on the quality of submission. Will investigate later. As an initial point, indeed, the select_for_update tests do not seem to include a test for using get() with select_for_update.

[michael.miller@…]

I have tried this with Django 1.7rc2 and the problem still exists. I can confirm that with Django 1.6.5 the problem does not happen.

Thank you for accepting this, hopefully you can get to it before the release of Django 1.7. This bug is a blocker for us as we use select_for_update with a get as part of our concurrency control system.

[Ramiro Morales]

Replying to michael.miller@…:

I have tried this with Django 1.7rc2 and the problem still exists. I can confirm that with Django 1.6.5 the problem does not happen.

Thank you for accepting this, hopefully you can get to it before the release of Django 1.7. This bug is a blocker for us as we use select_for_update with a get as part of our concurrency control system.

Thanks for your report.

The hardware requirements, licensing woes, changing versions, and the general difficulty in setting it up make a working Oracle development environment a scarce resource.

Considering that from what you express this ticket has such a high impact for you I'd like to ask you to test the following and report back with the results:

1. Please verify if 1.6 is affected by the issue.
2. If it isn't then please apply bisection method using git between the good version (the 1.6 git tag) and the closest bad one (e.g. the 1.7rc1 git tag). This should give you the ID of the commit where the regression got introduced.

This information could greatly enhance the possibility of a solution being found before 1.7 gets released.

Thanks again.

[Shai Berger]

Hi all,

The issue is very hard to solve. The change that introduced this is [da79ccca], a change to the get() function that made it return only up to 20 records (rather than all of them) when the criteria are not specific enough to select a single record. This was done, essentially, by adding [:20] to the queryset being used for get().

The issue is that, as far as I can tell, select-for-update and limit/offset cannot be done together on Oracle without some deep changes in the ORM; perhaps, generally, not even then.

Currently, On Oracle, the limit/offset is done by wrapping the original query in an external query that selects all the original query fields and adds a "row-number" fields, which can then be filtered on. FOR UPDATE cannot be added in this setup -- current code adds it to the inner query, which results in the reported error; I tried to put it on the external one, and that results in "ORA-02014: cannot select FOR UPDATE from view with DISTINCT, GROUP BY, etc."

Apparently, the ways to achieve limit/offset with select-for-update involve either putting the row-number much deeper in the query (using a windowing function, which requires a whole new treatment of the order-by clause) or limiting the fetch rather than the select (which is generally inappropriate, as the lock would still apply to the whole set, but is good enough for the case of get()). See ​http://stackoverflow.com/questions/6337126/oracle-for-update-select-first-10-rows. Each of the answers for that question also has problems in combination with other ORM features, like select_related().

We could solve the current problem by just disabling the [:20] feature on Oracle, at least when selecting for update (after all, this feature is mostly useful in debugging, and so is secondary to making correct code work). This will not solve the more general limit/offset issue, but may be good enough for many users. I propose that we take this route for now, if it is good enough for Michael.

PR forthcoming,

Shai.

[Shai Berger]

Tests are still running locally, but ​PR 3002 is up for review (it's a small one, the main issues with it are whether the partial solution is acceptable and whether querying connection features in get() is kosher).

[Shai Berger]

Full test-suite passed on both master and 1.7 (trivial cherry-pick) on Oracle.

[Aymeric Augustin]

That patch looks good to me.

[Florian Apolloner]

Can't we just revert the fix for #6785? It's just a Cleanup/optimization after all and hardly useful. Hell, even limiting .get() to unique columns would be better imo (although I realize that is probably backwards incompatible).

[michael.miller@…]

I'm not excited by all my gets now being wrapped in two additional selects. Our DBAs probably won't be either. Is it possible to create a db back end flag for slicing get results and have it turned off on Oracle. Even if it's not turned off in Oracle, having the flag there would allow us to turn it off in our own backend. (We fork the Django oracle backend and integrate Oracle Database Resident Connection Pooling).

If not the patch should work and thanks for looking at the problem. Sorry I'm at PyCon Australia and haven't had time to test.

[Shai Berger]

Replying to apollo13:

Can't we just revert the fix for #6785?

I'd like to have better reasons for reverting it than "Oracle is retarded and the Django backend hasn't managed to work around that."

Replying to michael.miller@…:

I'm not excited by all my gets now being wrapped in two additional selects. Our DBAs probably won't be either. Is it possible to create a db back end flag for slicing get results and have it turned off on Oracle.

I think it is better to do this, not via flags, but rather by providing a hook for the backend. That way, the Oracle backend (and also others) could implement #6785 by special fetches instead of slicing the query.

The kind of hook I have in mind is: Replace the line in QuerySet.get() which slices the query with code which gets a compiler and calls a new method on it, compiler.prepare_for_get(query). The default implementation of prepare_for_get() will slice the query, but the Oracle one will just mark it as a get. Then, the backend, when executing the query and fetching its results, will know to fetch only up to 21 rows (how exactly this part works is still not clear to me, because at that point the backend only gets sql, but we'll figure it out).

My capacity for working on any of this until the end of next week is quite limited, as I am on a family vacation.

If not the patch should work

All of the proposed solutions so far still leave the original deep problem unsolved: Sliced for-update queries do not work on Oracle. However, this has probably been the case for a very long time, and I don't see it reported, so apparently that should not be a release blocker. Thanks for validating that the smaller issue of get() is indeed the only problem for you.

[Shai Berger]

#23147 opened to track the larger issue of sliced for-update queries.

[michael.miller@…]

To confirm, yes, only a sliced get with select for update is a blocker. The issue of always having sliced gets with Oracle is secondary and something we could live with, as long as the select for update and get combination works, although my preference is for shai's hook to be implemented.

[Andrew Godwin]

The patch looks good to me - Shai, can we merge it and close the ticket?

[Shai Berger <shai@…>]

In f3c0cb0120aa5805daa880f18b24f40bacde651f:

[1.7.x] Fixed #23061: Avoided setting a limit on a query for get with select_for_update on Oracle

Thanks Michael Miller for reporting the issue.

Backport of 746f2a4bed from master

[Shai Berger]

Hoped I could find more time for this during the vacation, but it seems not to be happening. So, to stop the better from interfering with the good-enough, I pushed the patch and am reopening #6785 for a fetch-based implementation (at least as an option).