[PostGIS 2.x] Use postgis template database if available, but check for it just once #439

Closed
wants to merge 10 commits into from

2 participants

@fcurella

original ticket and discussion: https://code.djangoproject.com/ticket/16455

This commit partially reverts 91ef2a5

@claudep
Django member

What about running tests without a template, should we tweak _create_test_db to add the CREATE EXTENSION query?

@claudep claudep and 1 other commented on an outdated diff Oct 19, 2012
django/contrib/gis/db/backends/postgis/creation.py
geom_index_type = 'GIST'
geom_index_ops = 'GIST_GEOMETRY_OPS'
geom_index_ops_nd = 'GIST_GEOMETRY_OPS_ND'
+ def __init__(self, *args, **kwargs):
+ super(PostGISCreation, self).__init__(*args, **kwargs)
+ cursor = self.connection.cursor()
+ cursor.execute('SELECT datname FROM pg_database;')
+ db_names = [row[0] for row in cursor.fetchall()]
+ template_postgis = getattr(settings, 'POSTGIS_TEMPLATE', 'template_postgis')
+
+ if template_postgis in db_names:
+ self.template_postgis = template_postgis
+
@claudep
Django member
claudep added a note Oct 19, 2012

I don't think it's accurate to query the database at init time, as we are not even sure yet that we will need this info. I'd rather have a cached property, so it will only get fetched when we really need it.

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@claudep claudep and 1 other commented on an outdated diff Oct 19, 2012
django/contrib/gis/db/backends/postgis/creation.py
+ if self.template_postgis is None:
+ # Get a new connection to the database.
+ settings_dict = self.connection.settings_dict.copy()
+ settings_dict['NAME'] = test_database_name
+ # We have to change the engine, otherwise the postgis backend will
+ # complain about the database not being yet spatial.
+ settings_dict['ENGINE'] = 'django.db.backends.postgresql_psycopg2'
+ backend = load_backend(settings_dict['ENGINE'])
+ new_connection = backend.DatabaseWrapper(
+ settings_dict,
+ alias='__create_extensions_test_db__',
+ allow_thread_sharing=False)
+
+ cursor = new_connection.cursor()
+ cursor.execute("CREATE EXTENSION postgis;")
+ cursor.execute("CREATE EXTENSION postgis_topology;")
@claudep
Django member
claudep added a note Oct 19, 2012

I wouldn't create the postgis_topology extension, as AFAIK it is not used currently in Django.

for some reason I though postgis_topology was the extension equivalent of spatial_ref_sys.sql, but now I see they don't actually have anything to do with each other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@claudep claudep and 1 other commented on an outdated diff Oct 20, 2012
django/contrib/gis/db/backends/postgis/creation.py
class PostGISCreation(DatabaseCreation):
geom_index_type = 'GIST'
geom_index_ops = 'GIST_GEOMETRY_OPS'
geom_index_ops_nd = 'GIST_GEOMETRY_OPS_ND'
+ @property
@claudep
Django member
claudep added a note Oct 20, 2012

That's already better, but there is even a cached_property decorator in django.utils.functional that does the job of caching the value.

I didn't know about cached_property. Interesting implementation too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin commented on an outdated diff Nov 19, 2012
django/contrib/gis/db/backends/postgis/creation.py
class PostGISCreation(DatabaseCreation):
geom_index_type = 'GIST'
geom_index_ops = 'GIST_GEOMETRY_OPS'
geom_index_ops_nd = 'GIST_GEOMETRY_OPS_ND'
+ @cached_property
+ def template_postgis(self):
+ cursor = self.connection.cursor()
+ cursor.execute('SELECT datname FROM pg_database;')
@aaugustin
Django member

It would be more elegant, and possibly more efficient, to use:

template_postgis = getattr(settings, 'POSTGIS_TEMPLATE', 'template_postgis')
cursor.execute('SELECT 1 FROM pg_database WHERE datname = %s LIMIT 1;', template_postgis)
if cursor.fetchone():
    return template_postgis

This is how QuerySet.exists() is implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin and 1 other commented on an outdated diff Nov 19, 2012
django/contrib/gis/db/backends/postgis/creation.py
@@ -67,5 +81,29 @@ def sql_indexes_for_field(self, model, f, style):
return output
def sql_table_creation_suffix(self):
- postgis_template = getattr(settings, 'POSTGIS_TEMPLATE', 'template_postgis')
- return ' TEMPLATE %s' % self.connection.ops.quote_name(postgis_template)
+ if self.template_postgis is not None:
+ qn = self.connection.ops.quote_name
+ return ' TEMPLATE %s' % qn(self.template_postgis)
+ return ''
+
+ def _create_test_db(self, verbosity, autoclobber):
+ test_database_name = super(PostGISCreation, self)._create_test_db(verbosity, autoclobber)
+ if self.template_postgis is None:
+ # Get a new connection to the database.
+ settings_dict = self.connection.settings_dict.copy()
+ settings_dict['NAME'] = test_database_name
+ # We have to change the engine, otherwise the postgis backend will
@aaugustin
Django member

Accessing self.template_postgis just above already made a query to the database. If that worked, why is it necessary to create a new connection now?

self.connection is connected to original database (eg: myproject), while here we need to create the extension on the test db (eg: test_myproject), which is a different database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@claudep claudep and 1 other commented on an outdated diff Nov 20, 2012
django/contrib/gis/db/backends/postgis/creation.py
class PostGISCreation(DatabaseCreation):
geom_index_type = 'GIST'
geom_index_ops = 'GIST_GEOMETRY_OPS'
geom_index_ops_nd = 'GIST_GEOMETRY_OPS_ND'
+ @cached_property
+ def template_postgis(self):
+ template_postgis = getattr(settings, 'POSTGIS_TEMPLATE', ('template_postgis',))
@claudep
Django member
claudep added a note Nov 20, 2012

If POSTGIS_TEMPLATE exists, it will be a string, not a tuple. So you'd better make the tuple in the execute method below instead.

that was totally derp on my part.. I don't know what I was thinking. Thanks for the catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@aaugustin aaugustin commented on an outdated diff Nov 22, 2012
django/contrib/gis/db/backends/postgis/creation.py
@@ -67,5 +79,18 @@ def sql_indexes_for_field(self, model, f, style):
return output
def sql_table_creation_suffix(self):
- postgis_template = getattr(settings, 'POSTGIS_TEMPLATE', 'template_postgis')
- return ' TEMPLATE %s' % self.connection.ops.quote_name(postgis_template)
+ if self.template_postgis is not None:
+ qn = self.connection.ops.quote_name
+ return ' TEMPLATE %s' % qn(self.template_postgis)
+ return ''
+
+ def _create_test_db(self, verbosity, autoclobber):
+ test_database_name = super(PostGISCreation, self)._create_test_db(verbosity, autoclobber)
+ if self.template_postgis is None:
+ # Connect to the test database in order to create the postgis extension
+ self.connection.close()
+ self.connection.settings_dict["NAME"] = test_database_name
+ cursor = self.connection.cursor()
+ cursor.execute("CREATE EXTENSION postgis; COMMIT;")
@aaugustin
Django member
cursor.execute("CREATE EXTENSION postgis")
cursor.commit()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@claudep
Django member

Thanks, committed in fbd1df8 and d46b24e

@claudep claudep closed this Nov 23, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment