Fixed #3214 -- Support batch custom sql loading (db-dependant) #1533

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@claudep
Member
claudep commented Aug 30, 2013

No description provided.

@mjtamlyn
Member

With the migrations support, are .sql files even needed any more?

@claudep
Member
claudep commented Aug 30, 2013

I've not yet played with the new migration support, so I can't judge. But does the migrations support the use case of, let's say, adding a specialized stored procedure?

@claudep
Member
claudep commented Aug 30, 2013

Oh, and let's discuss the general issue on the ticket instead.

@mjtamlyn
Member

@andrewgodwin what's the status of .SQL files in a post migrations world?

@andrewgodwin
Member

@mjtamlyn They should go away, there's a RunSQL operation you can use in migrations instead (that accepts either single statements or mutiple ones with its own internal splitter regex). I didn't get around to removing the support for them on syncdb-type apps, but they'll be ignored if you have migrations.

@akaariai akaariai commented on the diff Sep 25, 2013
django/core/management/commands/migrate.py
@@ -212,6 +212,9 @@ def model_installed(model):
with transaction.commit_on_success_unless_managed(using=connection.alias):
for sql in custom_sql:
cursor.execute(sql)
+ if connection.vendor == 'mysql':
+ while cursor.nextset():
+ pass
@akaariai
akaariai Sep 25, 2013 Member

Usually non-test code doing connection.vendor checks should instead use some database feature flag or backend method. At least this needs an explanation why this is done.

@akaariai akaariai commented on the diff Sep 25, 2013
django/db/backends/__init__.py
@@ -856,6 +856,13 @@ def fulltext_search_sql(self, field_name):
"""
raise NotImplementedError('Full-text search is not implemented for this database backend')
+ def prepare_bulk_sql(self, sql):
+ """
+ Hook to let backends split some possibly multiline sql content if
+ they don't support multiple sql statements in one execute() call.
+ """
+ return [sql]
@akaariai
akaariai Sep 25, 2013 Member

Should the default stay as split_statements()

@akaariai akaariai commented on the diff Sep 25, 2013
tests/initial_sql_regress/tests.py
@@ -28,10 +28,20 @@ def test_custom_sql(self):
"""
connection = connections[DEFAULT_DB_ALIAS]
custom_sql = custom_sql_for_model(Simple, no_style(), connection)
- self.assertEqual(len(custom_sql), 9)
+ if connection.vendor == 'sqlite':
+ # With SQLite, each statement is separated
+ self.assertEqual(len(custom_sql), 9)
+ elif connection.vendor == 'postgresql':
+ # One global and one Postgres-specific
+ self.assertEqual(len(custom_sql), 2)
+ else:
+ self.assertEqual(len(custom_sql), 1)
@akaariai
akaariai Sep 25, 2013 Member

This will be called upon by third party db maintainers. I think it is OK to check each core DB here, and just no do any check for non-core databases at all.

@akaariai akaariai commented on the diff Sep 25, 2013
tests/initial_sql_regress/tests.py
cursor = connection.cursor()
for sql in custom_sql:
cursor.execute(sql)
+ if connection.vendor == 'mysql':
+ while cursor.nextset():
+ pass
@akaariai
akaariai Sep 25, 2013 Member

Here the same question, why this?

@akaariai
Member

Hmmh, reading andrew's comment after reviewing code changes... Should we just close this one?

@claudep
Member
claudep commented Sep 26, 2013

Yes, we can close it. However, it should be used as a base (at least for testing) for an equivalent patch applied to the RunSQL method.

@claudep claudep closed this Sep 26, 2013
@claudep claudep deleted the claudep:3214 branch Apr 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment