Opened 7 years ago

Closed 2 years ago

#28975 closed Cleanup/optimization (fixed)

Skip automatic creation of postgis extension if it already exists

Reported by: sphrak Owned by: amureki
Component: GIS Version: 2.0
Severity: Normal Keywords: create extension, postgis, psycopg2, postgresql
Cc: amureki Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hello,
I have found an issue with the documentation and/or the behaivor of the postgis extension create method.

Background
I am trying to migrate my app that uses the postgis extension inside a docker container. I do create the extension beforehand as superuser since I dont intend to run the database connection as a postgres superuser.

Problem
As per the documentation found here: https://docs.djangoproject.com/en/2.0/ref/contrib/gis/install/postgis/

I find it confusing that the documentation states "The command is run during the migrate process." and "An alternative is to use a migration operation in your project:". Because what seems to be the problem is that even if you remove the create extension statement from the migration file, it still runs regardless - rendering the option to put the create extension statement inside the migration file somewhat redundant and probably not very useful.

Possible solution
Ideally the behaivor would be only create extension if its in a migration file and if its not - its the users concern to create it however it see fit.

The relevant part is this:

testing.api |   File "/usr/lib/python3.6/site-packages/django/contrib/gis/db/backends/postgis/base.py", line 26, in prepare_database
testing.api |     cursor.execute("CREATE EXTENSION IF NOT EXISTS postgis")

Full trace:

testing.api | POSTGRES: I'm still down. Standby.
testing.api |                               List of databases
testing.api |    Name    |  Owner   | Encoding | Collate |  Ctype  |   Access privileges   
testing.api | -----------+----------+----------+---------+---------+-----------------------
testing.api |  postgres  | postgres | UTF8     | C       | C.UTF-8 | 
testing.api |  template0 | postgres | UTF8     | C       | C.UTF-8 | =c/postgres          +
testing.api |            |          |          |         |         | postgres=CTc/postgres
testing.api |  template1 | postgres | UTF8     | C       | C.UTF-8 | =c/postgres          +
testing.api |            |          |          |         |         | postgres=CTc/postgres
testing.api |  testing   | postgres | UTF8     | C       | C.UTF-8 | =Tc/postgres         +
testing.api |            |          |          |         |         | postgres=CTc/postgres+
testing.api |            |          |          |         |         | testing=CTc/postgres
testing.api | (4 rows)
testing.api | 
testing.api | POSTGRES: I'm up, I'm up, I'm f***** up - but I'm up
testing.api | Collecting selenium==3.3.1 (from -r /app/src/core/requirements/testing.txt (line 1))
testing.api |   Downloading selenium-3.3.1-py2.py3-none-any.whl (930kB)
testing.api | Collecting django-debug-toolbar==1.8 (from -r /app/src/core/requirements/testing.txt (line 2))
testing.api |   Downloading django_debug_toolbar-1.8-py2.py3-none-any.whl (205kB)
testing.api | Requirement already satisfied: Django>=1.8 in /usr/lib/python3.6/site-packages (from django-debug-toolbar==1.8->-r /app/src/core/requirements/testing.txt (line 2))
testing.api | Collecting sqlparse>=0.2.0 (from django-debug-toolbar==1.8->-r /app/src/core/requirements/testing.txt (line 2))
testing.api |   Downloading sqlparse-0.2.4-py2.py3-none-any.whl
testing.api | Requirement already satisfied: pytz in /usr/lib/python3.6/site-packages (from Django>=1.8->django-debug-toolbar==1.8->-r /app/src/core/requirements/testing.txt (line 2))
testing.api | Installing collected packages: selenium, sqlparse, django-debug-toolbar
testing.api | Successfully installed django-debug-toolbar-1.8 selenium-3.3.1 sqlparse-0.2.4
testing.api | Traceback (most recent call last):
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 83, in _execute
testing.api |     return self.cursor.execute(sql)
testing.api | psycopg2.ProgrammingError: permission denied to create extension "postgis"
testing.api | HINT:  Must be superuser to create this extension.
testing.api | 
testing.api | 
testing.api | The above exception was the direct cause of the following exception:
testing.api | 
testing.api | Traceback (most recent call last):
testing.api |   File "manage.py", line 22, in <module>
testing.api |     execute_from_command_line(sys.argv)
testing.api |   File "/usr/lib/python3.6/site-packages/django/core/management/__init__.py", line 371, in execute_from_command_line
testing.api |     utility.execute()
testing.api |   File "/usr/lib/python3.6/site-packages/django/core/management/__init__.py", line 365, in execute
testing.api |     self.fetch_command(subcommand).run_from_argv(self.argv)
testing.api |   File "/usr/lib/python3.6/site-packages/django/core/management/base.py", line 288, in run_from_argv
testing.api |     self.execute(*args, **cmd_options)
testing.api |   File "/usr/lib/python3.6/site-packages/django/core/management/base.py", line 335, in execute
testing.api |     output = self.handle(*args, **options)
testing.api |   File "/usr/lib/python3.6/site-packages/django/core/management/commands/migrate.py", line 77, in handle
testing.api |     connection.prepare_database()
testing.api |   File "/usr/lib/python3.6/site-packages/django/contrib/gis/db/backends/postgis/base.py", line 26, in prepare_database
testing.api |     cursor.execute("CREATE EXTENSION IF NOT EXISTS postgis")
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 100, in execute
testing.api |     return super().execute(sql, params)
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 68, in execute
testing.api |     return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 77, in _execute_with_wrappers
testing.api |     return executor(sql, params, many, context)
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 85, in _execute
testing.api |     return self.cursor.execute(sql, params)
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/utils.py", line 89, in __exit__
testing.api |     raise dj_exc_value.with_traceback(traceback) from exc_value
testing.api |   File "/usr/lib/python3.6/site-packages/django/db/backends/utils.py", line 83, in _execute
testing.api |     return self.cursor.execute(sql)
testing.api | django.db.utils.ProgrammingError: permission denied to create extension "postgis"
testing.api | HINT:  Must be superuser to create this extension.

Change History (19)

comment:1 by Curtis Maloney, 7 years ago

Either we should allow the migration, or have it in the db backend but gated somehow...

Having the migration (my preference) but ALWAYS performing the action _anyway_ is just pointless.

comment:2 by sphrak, 7 years ago

Owner: changed from nobody to sphrak
Status: newassigned

comment:3 by sphrak, 7 years ago

Component: GISDatabase layer (models, ORM)

comment:4 by Tim Graham, 7 years ago

Summary: Postgis create extension runs regardless if in migration file or notSkip automatic creation of postgis extension if it already exists
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:5 by Ramiro Morales, 7 years ago

I think the issue reported isn't that creation of the extension must be skipped if it already exist (as I think that's what the IF NOT EXISTS postgis part of CREATE EXTENSION IF NOT EXISTS postgis is about) But rather that the query is tried unconditionally.

I'd vote for removing the automatic creation of the postgis extension from postgis backend DatabaseWrapper.prepare_databse() we added in 1.8 and move to having users to perform it explicitly either:

  • Manually, externally -- For cases like the OP describes
  • Via a migration -- For cases in which the user Django connects to Postgres as has DB superuser rights there as it's assumed

comment:6 by sphrak, 7 years ago

Has patch: set
Needs tests: set
Last edited 7 years ago by Tim Graham (previous) (diff)

in reply to:  description comment:7 by sphrak, 7 years ago

This issue is currently stalled while waiting on response here: https://groups.google.com/forum/#!topic/django-developers/EjouxahkqUc

Last edited 7 years ago by sphrak (previous) (diff)

comment:8 by Tim Graham, 7 years ago

Component: Database layer (models, ORM)GIS
Resolution: worksforme
Status: assignedclosed

As Shai said on the mailing list, CREATE EXTENSION IF NOT EXISTS postgis; doesn't seem to fail if the extension already exists, even if the user isn't a superuser. I see NOTICE: extension "postgis" already exists, skipping in this case. Unless I'm misunderstanding the original report, I don't see a need to make any changes.

comment:9 by Mariusz Felisiak, 2 years ago

#33885 was a duplicate. Maybe we should skip creation 🤔, if not needed, as we did for CreationExtension() operation in d693a086def21d843c03ec3f5b5e697ed2463d45.

comment:10 by Johannes Maron, 2 years ago

Has patch: unset
Needs tests: unset
Resolution: worksforme
Status: closednew

As mentioned in #33885 the current behavior of implicitly creating the PG extensions, where all other PG extensions need to be created manually or via a migration seems inconsistent.
Therefore, I'd recommend deprecating the behavior and schedule it for deletion in Django 5.

comment:11 by Mariusz Felisiak, 2 years ago

Other extensions are needed to unlock various features on some PostgreSQL versions. postgis is always required for django.contrib.gis.db.backends.postgis, so IMO it's not the same case. We would force all users to manually create a migration with PostGISExtension() when starting to work with GIS on PostgreSQL. What do you think about my proposition?

comment:12 by amureki, 2 years ago

Cc: amureki added

As I am also affected by this, I would be happy to introduce a fix for that, which would be following Mariusz's suggestion.

in reply to:  12 comment:13 by Mariusz Felisiak, 2 years ago

Owner: changed from sphrak to amureki
Status: newassigned

Replying to amureki:

As I am also affected by this, I would be happy to introduce a fix for that, which would be following Mariusz's suggestion.

Thanks, feel-free to submit a patch.

comment:14 by Mariusz Felisiak, 2 years ago

Rust, Do you have time to work on this?

in reply to:  14 comment:15 by amureki, 2 years ago

Replying to Mariusz Felisiak:

Rust, Do you have time to work on this?

I did prepare a patch. However, I am struggling with the tests a bit.
Do you mind if I submit a PR and ask for your assistance?

comment:16 by Mariusz Felisiak, 2 years ago

Do you mind if I submit a PR and ask for your assistance?

Of course not, please do.

in reply to:  16 comment:17 by amureki, 2 years ago

Replying to Mariusz Felisiak:

Do you mind if I submit a PR and ask for your assistance?

Of course not, please do.

<3

Not sure, how to modify this ticket to catch the new PR (as there already was one that is closed), but here is a link:
https://github.com/django/django/pull/15941

comment:18 by Mariusz Felisiak, 2 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:19 by GitHub <noreply@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In 8403da36:

Fixed #28975 -- Made PostGIS backend skip extension creation if installed.

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