GIS distance queries use deprecated ST_distance_sphere

[Christian von Roques]

When using django.contrib.gis with PostGIS 2.3.0, distance queries will create SQL using the ST_distance_sphere function, which is deprecated since PostGIS 2.2.0 (see ​http://trac.osgeo.org/postgis/ticket/2748 ). The old function still works, but now issues a deprecation-warning. The problem is that ST_distance_sphere has become more than 40× slower than its replacement ST_DistanceSphere due to the added deprecation warning.

My city_cities table has 143260 rows. The following query executes in about 7.5 seconds:

SELECT min(ST_distance_sphere(ST_Point(0,0), location)) FROM cities_city;

Whereas the same query with ST_DistanceSphere instead of ST_distance_sphere completes in less than 170ms.

I've prepared a patch to stop Django from using the deprecated functions. ​https://github.com/roques/django/tree/postgis22_deprecated The problem with this patch is that the non-deprecated functions only became available with PostGIS 2.2.0 and thus the patch would raise the minimum required PostGIS version from 2.1.0.

As I can see the situation, we have a choice between the following three scenarios:

1. Raise the required version of PostGIS from 2.1.0 to 2.2.0 and use the non-deprecated functions.
2. Keep using the deprecated functions and live with all distance queries being often unusably slow.
3. Complicate the code to detect the PostGIS version and use ST_DistanceSphere when available or ST_distance_sphere otherwise.

I would prefer option 1, but people stuck with PostGIS <2.2.0 would then be unable to make distance queries with newer releases of Django.

[Simon Charette]

I think we should favor the third option. It's already a pattern we use for the MySQL GIS function names and it doesn't really make the code more complex.

[Claude Paroz]

Definitely option 3. We won't raise the minimum PostGIS version in Django 1.11.

[Mjumbe Poe]

I've started adding some tests at ​https://github.com/mjumbewu/django/tree/postgis22_deprecated. I tried handling the pre/post PostGIS 2.2 condition in there as well. It's not the most elegant solution, and it's not easy to test, since the PostGIS version is cached after it's first calculated (so I can't just override_settings). I had to test it by just running the tests twice against different postgis versions (which is why there are test in there commented out). Can anyone else think of better tests for this?

[Claude Paroz]

In PostGISDistanceOperator, you should be able to check connection.ops.spatial_version. Then for testing, you should be able to mock spatial_version with something like:

    with mock.patch('django.contrib.gis.db.backends.postgis.operations.PostGISOperations.spatial_version', new_callable=mock.PropertyMock) as mock_spatial_version:
        mock_spatial_version.return_value = '2.1.0'
        ... your test

I wouldn't care about fixing the deprecated GeoQuerySet API (where mocking would be more difficult).

[Christian von Roques]

Now that I spent some more time with the code, option 3 was not that hard to implement.
More tests are still needed. However, mocking spatial_version won't do, as my changes only call it once in PostGISOperations.__init__. However, changing settings.POSTGIS_VERSION before instantiating PostGISOperations should work.

[Christian von Roques]

I've added a test to the test-suite to check the PostGIS version dependent behaviour.

However, my first concise implementation evaluates spatial_version in PostGISOperations.__init__, which in turn evaluates an SQL-query, which confuses quite a few of Django's tests (e.g. backends.tests.PostgreSQLTests.test_nodb_connection). To appease the test-suite I modified my implementation to defer evaluation of spatial_version using @cached_property, which works, but is quite a bit larger. — My branch ​https://github.com/roques/django/tree/postgis22_deprecated contains both, the nice and the deferred/lazy implementation.

What is the policy for cases like this?

[Claude Paroz]

Could you please first fix the test which is still using the deprecated syntax. That is, move the test in DistanceFunctionsTests and replace:
AustraliaCity.objects.exclude(id=hillsdale.id).distance(hillsdale.point, spheroid=True)
by
AustraliaCity.objects.exclude(id=hillsdale.id).annotate(distance=Distance('point', hillsdale.point, spheroid=True))

The fix might mostly (exclusively?) concern django/contrib/gis/db/models/functions.py.

[Christian von Roques]

I've moved and adapted @mjumbewu's testcase as indicated.
Will it be OK to squash his changeset with mine in the end, losing attribution for his work?

[Tim Graham]

Yes, the usual procedure is to write "Thanks <name> for the initial patch." in the commit message.

[Claude Paroz]

And then please make your branch a pull request so as we can comment on it.

[Christian von Roques]

​PR

[Claude Paroz]

Thanks! As stated on the PR and in comment:5, I don't think we should update the deprecated GeoQuerySet-related names. Let's concentrate on database function names (as in ​https://docs.djangoproject.com/en/1.10/ref/contrib/gis/functions/).

[Christian von Roques]

I've reverted all fixes related to GeoQuerySet.
Please take a look at the code, as I'm unsure about some of the stylistic choices I made.

[Tim Graham <timograham@…>]

In cbae4d31:

Fixed #27448 -- Switched use of functions deprecated in PostGIS 2.2.

Thanks Claude Paroz and Tim Graham for reviews, and
Mjumbe Wawatu Poe for the initial regression test.