Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#24335 closed Cleanup/optimization (fixed)

DateRange and friends require psycopg2 >= 2.5

Reported by: Claude Paroz Owned by: Tim Graham
Component: contrib.postgres Version: 1.8alpha1
Severity: Normal Keywords: 1.8-beta
Cc: Marc Tamlyn, ovangle@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, if your psycopg2 version is lower than 2.5, you cannot import any of contrib.postgres fields or forms, as ranges.py is unconditionally imported in their __init__.

On one side, this should be documented, and on the other side we should still allow other fields/forms (array/hstore) to be imported (maybe raise a RuntimeWarning for the ranges import failure?).

Change History (14)

comment:1 by Tim Graham, 10 years ago

Cc: Marc Tamlyn added
Keywords: 1.8-beta added
Triage Stage: UnreviewedAccepted

comment:2 by Thomas Stephenson, 10 years ago

Cc: ovangle@… added
Owner: set to Thomas Stephenson
Status: newassigned

comment:3 by Marc Tamlyn, 10 years ago

Personally I would be happy to simply require psycopg2 >= 2.5 - that version was released nearly 2 years ago now and I don't see any particular reason why we need to support older versions.

At the moment there is one mention of the required version, in docs/ref/databases.txt. This was updated to 2.0.9 earlier in this release cycle to support UUIDField.

comment:4 by Thomas Stephenson, 10 years ago

I've submitted a patch https://github.com/django/django/pull/4131 which conditionally defines the classes if an ImportError is raised when importing psycopg2.extras.Range.

I'm happy changing it to require psycopg2>=2.5, but obviously since this is my first contribution a suggestion like that was not my call to make and the bug as described had been accepted, so I implemented it directly as stated. Personally, my opinion on these matters is usually "continue support for the last two major versions of a package" (ie. >= 2.4).

Last edited 10 years ago by Thomas Stephenson (previous) (diff)

comment:6 by Thomas Stephenson, 10 years ago

I was aware of that, but I meant the two previous major versions (so, not including the current version). It's arbitrary anyway and since this is entirely new optional module, it's a little early to be littering the code with conditional imports.

>= 2.5 is OK with me.

Last edited 10 years ago by Thomas Stephenson (previous) (diff)

comment:7 by Thomas Stephenson, 10 years ago

I was aware of that, but I meant the two previous major versions (so, not including the current version). It's arbitrary anyway and since this is entirely new functionality in an optional module, it's a little early to be littering the code with conditional imports.

>= 2.5 is OK with me.

comment:8 by Claude Paroz, 10 years ago

Requiring psycopg2>=2.5 would literally force many users to use a pip-installed psycopg2 instead of the system one, as many stable distributions still have an older version. But if we agree that's an acceptable requirement for a contrib package, then go for it (and document it, of course).

comment:9 by Thomas Stephenson, 10 years ago

While testing this, it seems that https://github.com/django/django/commit/39d95fb6ada99c59d47fa0eae6d3128abafe2d58 dropped support for psycopg2 <= 2.4.3 globally (new_array_type was added in 2.4.3). I'm just going to add a check for the version >= 2.5 into django.db.backends.postgres_pyscopg2 and update docs/ref/databases.txt.

edit: I posted this before reading your comment @claudep (it has been sitting unposted in my browser window for a while now). Perhaps this needs more discussion.

Last edited 10 years ago by Thomas Stephenson (previous) (diff)

comment:10 by Marc Tamlyn, 10 years ago

The relevant array code added in that commit is actually only needed for contrib.postgres in theory, but it might be quite complicated to conditionally include it as it also changes the way GenericIPAddressField works, I think. For reference, 2.4.3 was released December 2011.

It is however worth noting that postgres provides an rpm package for rhel6 for psycopg2 2.4.5 if people care about that. Personally, I don't care all that much about OS level packages. I don't now if we have any kind of official policy on them.

comment:11 by Thomas Stephenson, 10 years ago

I'll defer completing this until I get some consensus about minimum supported versions.

It seems that 2.5 is an OK minimum for django.contrib.postgres, but 2.4.5 added support for Inet arrays to the Inet object, so that's the current minimum supported version for db.backends without a change to the current code (at least a more informative error than the current one about new_array_type being unavailable).

People using OS packages from lts releases of distros aren't going to be keeping django up to date anyway (they'll be using whatever OS package is provided of that on their OS, probably 1.3 or earlier). I'm a fan of backwards compatibility, but I think the listed requirements are more than reasonable, considering the age of the respective packages. Obviously it's not my call though, so...


Last edited 10 years ago by Thomas Stephenson (previous) (diff)

comment:12 by Tim Graham, 10 years ago

Has patch: set
Owner: changed from Thomas Stephenson to Tim Graham

New PR that implements this proposal from the mailing list thread:

  • Document 2.4.3 as the minimum if you don't want to use contrib.postgres
  • Document 2.5 as the minimum to use contrib.postgres
  • Require 2.5+ for Django's test suite to avoid having to add conditional imports/test skipping.

comment:13 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 3adc5f1ee6618a615db07d4a868b366a15c39f82:

Fixed #24335 -- Bumped required psycopg2 version to 2.4.5 (2.5 for contrib.postgres).

comment:14 by Tim Graham <timograham@…>, 10 years ago

In 730fb593ad19731e7018dd83236318bfe71de86a:

[1.8.x] Fixed #24335 -- Bumped required psycopg2 version to 2.4.5 (2.5 for contrib.postgres).

Backport of 3adc5f1ee6618a615db07d4a868b366a15c39f82 from master

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