Code

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#9307 closed (fixed)

Pickling support for Oracle Query classes

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

Description

We need to add specific code to handle pickling of the Query class used by the Oracle backend. The attached patch is most of the work need to do that. I can't test it on Oracle, but it should mostly work (similar code works in test situations).

If somebody with access to Oracle could test this and report any errors (or success), I can fine-tune it appropriately.

Attachments (2)

oracle-pickle.diff (1.6 KB) - added by mtredinnick 6 years ago.
Pickling support for Oracle
oracle-pickle-v2.diff (2.3 KB) - added by jbronn 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by mtredinnick

Pickling support for Oracle

comment:1 Changed 6 years ago by jbronn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I applied the patch and ran the queries regression tests. It failed with the following error:

File "C:\django\trunk\tests\regressiontests\queries\models.py", line ?, in regressiontests.queries.models.__test__.API_TESTS
Failed example:
    out = pickle.dumps(Item.objects.all())
Exception raised:
    Traceback (most recent call last):
      File "C:\django\trunk\django\test\_doctest.py", line 1267, in __run
        compileflags, 1) in test.globs
      File "<doctest regressiontests.queries.models.__test__.API_TESTS[212]>", line 1, in <module>
        out = pickle.dumps(Item.objects.all())
      File "C:\Python25\lib\pickle.py", line 1366, in dumps
        Pickler(file, protocol).dump(obj)
      File "C:\Python25\lib\pickle.py", line 224, in dump
        self.save(obj)
      File "C:\Python25\lib\pickle.py", line 331, in save
        self.save_reduce(obj=obj, *rv)
      File "C:\Python25\lib\pickle.py", line 419, in save_reduce
        save(state)
      File "C:\Python25\lib\pickle.py", line 286, in save
        f(self, obj) # Call unbound method with explicit self
      File "C:\Python25\lib\pickle.py", line 649, in save_dict
        self._batch_setitems(obj.iteritems())
      File "C:\Python25\lib\pickle.py", line 663, in _batch_setitems
        save(v)
      File "C:\Python25\lib\pickle.py", line 331, in save
        self.save_reduce(obj=obj, *rv)
      File "C:\Python25\lib\pickle.py", line 401, in save_reduce
        save(args)
      File "C:\Python25\lib\pickle.py", line 286, in save
        f(self, obj) # Call unbound method with explicit self
      File "C:\Python25\lib\pickle.py", line 562, in save_tuple
        save(element)
      File "C:\Python25\lib\pickle.py", line 286, in save
        f(self, obj) # Call unbound method with explicit self
      File "C:\Python25\lib\pickle.py", line 753, in save_global
        (obj, module, name))
    PicklingError: Can't pickle <class 'django.db.models.sql.query.Query'>: it's not the same object as django.db.models.sql.query.Query

Changed 6 years ago by jbronn

comment:2 Changed 6 years ago by jbronn

With the updated patch the queries regression tests pass on Oracle.

The problem was that in __reduce__ the (QueryClass,) arguments refer to the original Query class, which is overridden in the query module. Thus, the "original" Query class (I called it BaseQuery) needs to be exposed for custom backend querysets to be pickled. I think we used to do this in the old queryset-refactor branch; I think it was named _Query.

comment:3 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

Urgh. I really, really want to avoid that type of code and I removed it for that reason. It's ugly. The normal class is Query. It shouldn't have to enter the witness-protection program and undergo a name change just because we need to override it somewhere else.

There must be some other solution that is local to the Oracle (and other custom) backend code. I'll try to work out a better way to test this locally, perhaps by making a fake PostgreSQL class or something to see what I can come up with. I hadn't thought of the name clash issue and didn't check that out in my parallel development attempt.

Thanks for testing so quickly and giving feedback, though. That helps.

comment:4 follow-up: Changed 6 years ago by mtredinnick

I forgot to mention, the more technical reason for wanting to avoid that type of code is that the current implementation is sort of poor form. It ties the query class to the connection at import time and I want to avoid those sort of module-level dependencies on connections so that we can talk to multiple storage locations at once, long-term. So removing those lines of code, rather than adding to them, is the longer-term plan. I just haven't worked out a non-sucky way to do that yet. Might be time to do that now.

comment:5 in reply to: ↑ 4 Changed 6 years ago by anonymous

Replying to mtredinnick:

I forgot to mention, the more technical reason for wanting to avoid that type of code is that the current implementation is sort of poor form. It ties the query class to the connection at import time and I want to avoid those sort of module-level dependencies on connections so that we can talk to multiple storage locations at once, long-term. So removing those lines of code, rather than adding to them, is the longer-term plan. I just haven't worked out a non-sucky way to do that yet. Might be time to do that now.

Yes... It's time to do this! O;-)

comment:6 Changed 5 years ago by mtredinnick

Attempts to try and do this using a __new__() method, which felt cleaner, were measurably slower. Given the number of times we create new Query classes, I've decided that pragmatism beats theoretical perfection here. So I'll go with Justin's latest patch.

comment:7 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [9272]) Fixed #9307 -- Added the ability to pickle the Query class used by the Oracle
backend.

This allows Querysets to be cached for Oracle and should provide a model for
adding pickling support to other (external) database backends that need a
custom Query class.

Thanks to Justin Bronn for some assistance with this patch.

comment:8 Changed 5 years ago by mtredinnick

(In [9273]) [1.0.X] Fixed #9307 -- Added the ability to pickle the Query class used by the
Oracle backend.

This allows Querysets to be cached for Oracle and should provide a model for
adding pickling support to other (external) database backends that need a
custom Query class.

Thanks to Justin Bronn for some assistance with this patch.

Backport of r9272 from trunk.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.