Opened 23 months ago

Closed 7 months ago

Last modified 7 months ago

#20968 closed Cleanup/optimization (fixed)

Add a migrate hook for automatically running database setup SQL

Reported by: burton449geo@… Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: geodjango, spatialite, indexes
Cc: keniallee@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The database is Spatialite 4.1.0

On Syncdb these errors messages are shown:

Superuser created successfully.
Installing custom SQL ...
Installing indexes ...
AddGeometryColumn() error: "no such table: geometry_columns"
CreateSpatialIndex() error: "no such table: geometry_columns"

Change History (21)

comment:1 Changed 23 months ago by leplatrem

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Initializing the database with spatialite binary is the answer :

spatialite database.db "SELECT InitSpatialMetaData();"

comment:2 Changed 23 months ago by claudep

This should be done automatically:
https://github.com/django/django/blob/master/django/contrib/gis/db/backends/spatialite/creation.py#L102-L107

Maybe something changed in Spatialite 4.1? I cannot test right now.

comment:3 Changed 19 months ago by aaugustin

Since no one managed to reproduce the error, should I close this ticket as needsinfo?

comment:4 Changed 19 months ago by ramiro

  • Resolution set to needsinfo
  • Status changed from new to closed

Tried to reproduce this to not avail (tested with the GeoDjango tutorial project).

Environment:

Creating spatial metadata tables (per https://docs.djangoproject.com/en/dev/ref/contrib/gis/install/spatialite/#creating-a-spatial-database-for-spatialite) -- Not strictly needed but useful to get the versions of the C-level components printed to the console:

(spatialite41)ramiro@mang:~/venv/spatialite41/src/geodjango$ spatialite db.sqlite3 "SELECT InitSpatialMetaData();"
SpatiaLite version ..: 4.1.0	Supported Extensions:
	- 'VirtualShape'	[direct Shapefile access]
	- 'VirtualDbf'		[direct DBF access]
	- 'VirtualText'		[direct CSV/TXT access]
	- 'VirtualNetwork'	[Dijkstra shortest path]
	- 'RTree'		[Spatial Index - R*Tree]
	- 'MbrCache'		[Spatial Index - MBR cache]
	- 'VirtualSpatialIndex'	[R*Tree metahandler]
	- 'VirtualFDO'		[FDO-OGR interoperability]
	- 'SpatiaLite'		[Spatial SQL - OGC]
PROJ.4 version ......: Rel. 4.7.1, 23 September 2009
GEOS version ........: 3.2.2-CAPI-1.6.2
the SPATIAL_REF_SYS table already contains some row(s)
 InitSpatiaMetaData() error:"table spatial_ref_sys already exists"
0

DB schema manage.py commands:

(spatialite41)ramiro@mang:~/venv/spatialite41/src/geodjango$ PYTHONPATH=~/django/upstream python manage.py sqlall world
BEGIN;
CREATE TABLE "world_worldborder" (
    "id" integer NOT NULL PRIMARY KEY AUTOINCREMENT,
    "name" varchar(50) NOT NULL,
    "area" integer NOT NULL,
    "pop2005" integer NOT NULL,
    "fips" varchar(2) NOT NULL,
    "iso2" varchar(2) NOT NULL,
    "iso3" varchar(3) NOT NULL,
    "un" integer NOT NULL,
    "region" integer NOT NULL,
    "subregion" integer NOT NULL,
    "lon" real NOT NULL,
    "lat" real NOT NULL
)
;
SELECT AddGeometryColumn('world_worldborder', 'mpoly', 4326, 'MULTIPOLYGON', 2, 1);
SELECT CreateSpatialIndex('world_worldborder', 'mpoly');

COMMIT;
(spatialite41)ramiro@mang:~/venv/spatialite41/src/geodjango$ PYTHONPATH=~/django/upstream python manage.py migrate
Operations to perform:
  Synchronize unmigrated apps: gis, sessions, admin, messages, auth, staticfiles, contenttypes, world
  Apply all migrations: (none)
Synchronizing apps without migrations:
  Creating tables...
    Creating table django_admin_log
    Creating table auth_permission
    Creating table auth_group_permissions
    Creating table auth_group
    Creating table auth_user_groups
    Creating table auth_user_user_permissions
    Creating table auth_user
    Creating table django_content_type
    Creating table django_session
    Creating table world_worldborder
  Installing custom SQL...
  Installing indexes...
Installed 0 object(s) from 0 fixture(s)
Running migrations:
  No migrations needed.

You just installed Django's auth system, which means you don't have any superusers defined.
Would you like to create one now? (yes/no): no

Also ran the GeoDjango tests without problems:

(spatialite41)ramiro@mang:~/django/upstream/tests$ PYTHONPATH=.. python runtests.py --settings=test_gis django.contrib.gis
Testing against Django installed in '/home/ramiro/django/upstream/django'
Creating test database for alias 'default'...
Creating test database for alias 'other'...
.s...s....s..s............s.......sssssssssss..........ssssss...s............s....s.......sss...s.s.s.s.......ssssss..........................................................................................................................................................................................
----------------------------------------------------------------------
Ran 302 tests in 5.177s

OK (skipped=38)
Destroying test database for alias 'default'...
Destroying test database for alias 'other'...

If you have a system-wide spatialite installation of a different version (e.g. in the case of Ubuntu 12.04 it could be the 3.0.0~beta20110817-3 .deb package) then make sure you direct GeoDjango to use the 4.1.x one using the SPATIALITE_LIBRARY_PATH setting in setting.py:

SPATIALITE_LIBRARY_PATH='/path/to/your/spatialite/4.1.x/libspatialite.so'

Closing 'needsinfo' for now. Please reopen with a similar description of your environment if you can reproduce and wish to push this further.

comment:5 Changed 19 months ago by ramiro

  • Resolution changed from needsinfo to invalid

Actually I can reproduce the issue if I don't create the spatial tables with spatialite db.sqlite3 "SELECT InitSpatialMetaData();" then the index creation fails.

So comment:1 is right. I'm wrong in the last comment because I assumed these tables were created automatically if not present. That's only true then creating the test DB to run the GeoDjango tests.

So what one needs to do is actually follow the documentation (https://docs.djangoproject.com/en/dev/ref/contrib/gis/install/spatialite/#creating-a-spatial-database-for-spatialite) because at no point it describes that step as optional.

comment:6 Changed 8 months ago by txomon

It does specify that step as optional in 1.7 docs:

https://docs.djangoproject.com/en/1.7/ref/contrib/gis/install/spatialite/#creating-a-spatial-database-for-spatialite

Maybe an update would be recommended?

EDIT: Sorry, misread, maybe refactor a little the text to be more hard to misread? =)

Last edited 8 months ago by txomon (previous) (diff)

comment:7 Changed 7 months ago by kenial

Using spatialite db.sqlite3 "SELECT InitSpatialMetaData();" is the answer, though, however, can we prevent Django users from meeting this error by adding workaround codes, which would be in syncdb command or database file creation code of django.contrib.gis.db.backends.spatialite?

p.s: txomon, I'm one of them recently updated it and feel sorry. As you know, using spatialite is something... feels like rolling under the hood :(

comment:8 follow-up: Changed 7 months ago by claudep

  • Has patch set
  • Resolution invalid deleted
  • Status changed from closed to new
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/optimization
  • Version changed from 1.5 to master

comment:9 in reply to: ↑ 8 Changed 7 months ago by kenial

Replying to claudep:

What about this patch?
https://github.com/django/django/pull/3697

if InitSpatialMetaData query is executed in init_connection_state() method, then it will check it every time when opening a connection. As you know, there is no connection pooling on sqlite3 (and SpatiaLite) backend by default. It means that every query execution will lead to execute PRAGMA table_info(geometry_columns);, I think it's a sort of redundancy. My patch updating django/contrib/gis/apps.py intended to execute InitSpatialMetaData query when doing migrate: https://github.com/django/django/pull/3695

What do you think about?

comment:10 Changed 7 months ago by kenial

  • Cc keniallee@… added

comment:11 Changed 7 months ago by claudep

I recognize my patch has the drawback of adding a query for each database connection creation.
What I don't like in your solution, it's the backend-specific "pollution" in the main gis code. If we miss a backend-specific hook during app initialization, then let's create that hook. But importing anything from django.contrib.gis.db.backends.<backend> inside apps.py is not an option.

comment:12 follow-ups: Changed 7 months ago by claudep

Another option would be to use the pre-migrate signal.

comment:13 in reply to: ↑ 12 Changed 7 months ago by kenial

If we're going with the hook, it still does "migrate" stuff in context of another application (in this case, gis). So hook isn't looking good. (is there a way to completely split into migrate and gis context in app initialization?)

pre-migrate signal seems to be nice, but I find out ... it's model based. We're doing this on database-level. Maybe it's safe to be executed repeatedly, because when migrating only.

My original approach was to add a method for this purpose (name was before_migration) to BaseDatabaseWrapper with empty implementation, and call it when running migrate command. For sure, django.contrib.gis.db.backends.spatialite.base.DatabaseWrapper implements this before_migration method to init metadata table.

comment:14 in reply to: ↑ 12 Changed 7 months ago by kenial

Here's my original approach: https://github.com/django/django/pull/3698

How about this?

comment:15 Changed 7 months ago by claudep

I've rewritten the patch with the migrate hook idea. I also added a patch which moves a PostGIS operation to add some weight to this solution.

comment:16 Changed 7 months ago by timgraham

  • Component changed from GIS to Database layer (models, ORM)
  • Summary changed from Error creating Indexes on Syncdb to Add a migrate hook for automatically running database setup SQL
  • Triage Stage changed from Accepted to Ready for checkin

comment:17 follow-up: Changed 7 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 8f97413faed5431713c034897cda486507bf0cc3:

Fixed #20968 -- Checked Spatialite metadata before migrations

Thanks Kenial S. Lee for the initial patch and Tim Graham for
the review.

comment:18 in reply to: ↑ 17 Changed 7 months ago by kenial

Great work! Thanks :)

comment:19 Changed 7 months ago by timgraham

  • Has patch unset
  • Triage Stage changed from Ready for checkin to Accepted

Claude, PostGIS on the django-master-trusty build is failing since this change. Could you take a look?

comment:21 Changed 7 months ago by Claude Paroz <claude@…>

In df30ae07fc7d7500bbbe51ed3361982f645169f2:

Fixed postgis test database initialization

Refs #20968. Allow querying template_postgis presence without
existing test database.
Thanks Tim Graham for the review.

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