Repository navigation
feat(django-google-spanner): add batch_ddl database option - #19067
sakthivelmanii wants to merge 5 commits into
Conversation
Scope autocommit-disabled DDL batching in DatabaseSchemaEditor to DatabaseCreation.create_test_db() so test database schema setup sends queued CREATE TABLE, index, and foreign key statements in batched UpdateDatabaseDdl requests while keeping autocommit enabled during test execution.
There was a problem hiding this comment.
Code Review
This pull request introduces DDL batching during test database creation for the Django Google Spanner backend. It sets a _batch_ddl_in_schema_editor flag on the connection during create_test_db and overrides __enter__ and __exit__ in DatabaseSchemaEditor to temporarily disable autocommit, batching DDL statements together and restoring autocommit on exit. Comprehensive unit and mock server tests are added to verify this behavior. A critical issue was identified in DatabaseSchemaEditor.__exit__ where an exception raised during super().__exit__ (such as during deferred DDL execution) would not trigger a rollback because exc_type is initially None. A code suggestion is provided to track exceptions occurring during the parent exit call to ensure proper rollback.
| def __exit__(self, exc_type, exc_value, traceback): | ||
| try: | ||
| super().__exit__(exc_type, exc_value, traceback) | ||
| finally: | ||
| if getattr(self, "_restore_autocommit", False): | ||
| try: | ||
| if exc_type is not None: | ||
| self.connection.rollback() | ||
| finally: | ||
| self.connection.set_autocommit(True) |
There was a problem hiding this comment.
If an exception occurs during super().__exit__ (for example, when executing deferred DDL statements), exc_type passed to __exit__ is initially None. As a result, exc_type is not None evaluates to False, and self.connection.rollback() is not called, leaving the connection in an inconsistent state. We should track whether an exception occurred either inside the with block or during super().__exit__ to ensure we roll back correctly.
def __exit__(self, exc_type, exc_value, traceback):
has_exception = exc_type is not None
try:
return super().__exit__(exc_type, exc_value, traceback)
except Exception:
has_exception = True
raise
finally:
if getattr(self, "_restore_autocommit", False):
try:
if has_exception:
self.connection.rollback()
finally:
self.connection.set_autocommit(True)There was a problem hiding this comment.
Updated __exit__ to track clean exit across super().__exit__() and separate explicit commit()/rollback() from set_autocommit(True) so autocommit is always restored even when deferred or server-side DDL execution fails.
| if exc_type is not None: | ||
| self.connection.rollback() | ||
| finally: | ||
| self.connection.set_autocommit(True) |
There was a problem hiding this comment.
set_autocommit(True) only triggers a flush of the buffered DDL statements in google-cloud-spanner >= 3.72.0. So we should probably also update the minimum version required for Django.
There was a problem hiding this comment.
Updated google-cloud-spanner minimum version to >= 3.72.0 in setup.py and noxfile.py.
| if os.environ.get("RUNNING_SPANNER_BACKEND_TESTS") == "1": | ||
| self.mark_skips() | ||
| super().create_test_db(*args, **kwargs) | ||
| self.connection._batch_ddl_in_schema_editor = True |
There was a problem hiding this comment.
This is a bit of a code smell. It introduces a private flag that is currently only used by tests, but that someone who inspects the code could find and set, and then it will also be used in production. Can we try to make a design that makes a clear choice between one of the following:
- A test-only fix that makes sure that our tests use DDL batching
- A feature that makes DDL batching a public and supported feature for Spanner Django
A test-only solution could look like this:
# In django_spanner/creation.py
from django_spanner.schema import DatabaseSchemaEditor
class BatchingDatabaseSchemaEditor(DatabaseSchemaEditor):
"""Schema editor that batches DDL statements by toggling autocommit."""
def __enter__(self):
super().__enter__()
self._restore_autocommit = False
if (
not self.collect_sql
and not self.connection.in_atomic_block
and self.connection.get_autocommit()
):
self.connection.set_autocommit(False)
self._restore_autocommit = True
return self
def __exit__(self, exc_type, exc_value, traceback):
has_exception = exc_type is not None
try:
super().__exit__(exc_type, exc_value, traceback)
except Exception:
has_exception = True
raise
finally:
if getattr(self, "_restore_autocommit", False):
try:
if has_exception:
self.connection.rollback()
finally:
self.connection.set_autocommit(True)
class DatabaseCreation(BaseDatabaseCreation):
def create_test_db(self, *args, **kwargs):
if os.environ.get("RUNNING_SPANNER_BACKEND_TESTS") == "1":
self.mark_skips()
# Temporarily use the batching schema editor during test database creation
original_schema_editor_class = self.connection.SchemaEditorClass
self.connection.SchemaEditorClass = BatchingDatabaseSchemaEditor
try:
return super().create_test_db(*args, **kwargs)
finally:
self.connection.SchemaEditorClass = original_schema_editor_classOr if we want to support this publicly for both tests and applications, then we could add an option to DATABASES:
# In settings.py
DATABASES = {
"default": {
"ENGINE": "django_spanner",
"OPTIONS": {
"batch_ddl": True, # Defaults to True or False depending on what we find the safest
},
}
}
# In DatabaseSchemaEditor.__enter__
batch_ddl = self.connection.settings_dict.get("OPTIONS", {}).get("batch_ddl", True)
if batch_ddl and not self.collect_sql and not self.connection.in_atomic_block and self.connection.get_autocommit():
...I would personally lean towards making it a public option, and maybe disable it by default in the first release, and consider making it the default in a follow-up release.
There was a problem hiding this comment.
Replaced the private flag with the public OPTIONS["batch_ddl"] option (defaulting to False), stripped batch_ddl in get_new_connection(), enabled it in the test suite scripts, and documented it in README.rst.
Summary
OPTIONS["batch_ddl"](defaultFalse) to batch DDL statements generated within aDatabaseSchemaEditorcontext (manage.py migrate, test database setup, andschema_editor()blocks) into batchedUpdateDatabaseDdlrequests by toggling connection autocommit.batch_ddlinDatabaseWrapper.get_new_connection()before callingspanner_dbapi.connect().google-cloud-spanner >= 3.72.0so buffered DDL statements are flushed onset_autocommit(True)and cleared onrollback().'OPTIONS': {'batch_ddl': True}in the Django 5.2, Django 6.0, and foreign-key test scripts and document the option inREADME.rstanddocs/.