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 , 7 years ago
comment:2 by , 7 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 7 years ago
Component: | GIS → Database layer (models, ORM) |
---|
comment:4 by , 7 years ago
Summary: | Postgis create extension runs regardless if in migration file or not → Skip automatic creation of postgis extension if it already exists |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
comment:5 by , 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 , 7 years ago
Has patch: | set |
---|---|
Needs tests: | set |
comment:7 by , 7 years ago
This issue is currently stalled while waiting on response here: https://groups.google.com/forum/#!topic/django-developers/EjouxahkqUc
Replying to sphrak:
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.
comment:8 by , 7 years ago
Component: | Database layer (models, ORM) → GIS |
---|---|
Resolution: | → worksforme |
Status: | assigned → closed |
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 , 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 , 2 years ago
Has patch: | unset |
---|---|
Needs tests: | unset |
Resolution: | worksforme |
Status: | closed → new |
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 , 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?
follow-up: 13 comment:12 by , 2 years ago
Cc: | 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.
comment:13 by , 2 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
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:15 by , 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?
follow-up: 17 comment:16 by , 2 years ago
Do you mind if I submit a PR and ask for your assistance?
Of course not, please do.
comment:17 by , 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 , 2 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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.