Repository navigation
fix(google-cloud-spanner): do not replay DDL statements on transaction retry - #18625
Merged
Merged
Conversation
…n retry DDL statements queued on a DBAPI connection are executed outside the Spanner read-write transaction via `run_prior_DDL_statements()` before transactional statements run, and any unexecuted DDLs remain in `connection._ddl_statements` across transaction retries. Exclude successful `StatementType.DDL` executions from `TransactionRetryHelper` so aborted transactions do not re-queue already-applied DDL statements, and commit table creation in `literal_round_trip_spanner`.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request ensures that successful DDL statements are not added to the transaction helper for retry in the Spanner DB-API cursor, accompanied by new unit tests. Additionally, it adds a commit call after table creation in the SQLAlchemy Spanner tests. The review feedback recommends using connection.dbapi_connection.commit() instead of the deprecated connection.connection.commit() to maintain compatibility with SQLAlchemy 1.4 and 2.0.
olavloite
requested changes
Oct 12, 2026
olavloite
approved these changes
Oct 12, 2026
|
|
||
| self.assertEqual(connection._ddl_statements, [sql]) | ||
| transaction_helper_mock.add_execute_statement_for_retry.assert_not_called() | ||
|
|
Contributor
There was a problem hiding this comment.
Can we also add this test to verify that successful DML statements are retried:
@mock.patch("google.cloud.spanner_dbapi.cursor.PeekIterator")
def test_execute_ddl_then_dml_retry_replays_prior_dml_without_replaying_ddl(
self, mock_peek_iterator
):
mock_database = mock.MagicMock()
connection = self._make_connection(self.INSTANCE, mock_database)
cursor = self._make_one(connection)
ddl_sql = "CREATE TABLE table_name (row_id INT64) PRIMARY KEY (row_id)"
insert_1_sql = "INSERT INTO table_name (row_id) VALUES (1)"
insert_2_sql = "INSERT INTO table_name (row_id) VALUES (2)"
# 1. Execute DDL
cursor.execute(sql=ddl_sql)
self.assertEqual(connection._ddl_statements, [ddl_sql])
metadata_mock = mock.Mock()
metadata_mock.trailing_metadata.return_value = {}
# 2. insert_1 succeeds; insert_2 aborts on attempt 1, triggering retry of insert_1 and retry of insert_2
connection.run_statement = mock.Mock(
side_effect=[
mock.MagicMock(), # insert_1 (attempt 1 -> succeeds)
Aborted("Aborted", errors=[metadata_mock]), # insert_2 (attempt 1 -> aborts)
mock.MagicMock(), # insert_1 (replayed by retry_transaction)
mock.MagicMock(), # insert_2 (attempt 2 -> succeeds)
]
)
cursor.execute(sql=insert_1_sql)
mock_database.update_ddl.assert_called_once_with([ddl_sql])
self.assertEqual(connection._ddl_statements, [])
self.assertEqual(len(cursor.transaction_helper._statement_result_details_list), 1)
# 3. insert_2 aborts and triggers retry_transaction()
cursor.execute(sql=insert_2_sql)
# Assert insert_1 was replayed, insert_2 was retried, and update_ddl was NOT called a second time
self.assertEqual(connection.run_statement.call_count, 4)
mock_database.update_ddl.assert_called_once_with([ddl_sql])
self.assertEqual(connection._ddl_statements, [])|
|
||
| finally: | ||
| if not self._in_retry_mode and not call_from_execute_many: | ||
| if ( |
Contributor
There was a problem hiding this comment.
For a follow-up PR, the following is also a bit weird:
cursor.execute("CREATE TABLE canceled_table (id INT64) PRIMARY KEY (id)")
connection.rollback()
# Later on the same connection:
cursor.execute("INSERT INTO users (id) VALUES (1)")
connection.commit()The DDL statement from the first line will be executed when the insert statement from the second transaction is executed. Do we think that is intentional?
Also, the DDL statement from the first line will:
- NOT be executed if the connection is closed after the rollback
- NOT be executed if the connection is changed to AutoCommit=True after the rollback and the connection then executes a query
- WILL be executed if the connection executes a DML statement or the application calls
executemany. - WILL be executed if the connection executes a DDL statement
1 task done
parthea
pushed a commit
that referenced
this pull request
Nov 12, 2026
🤖 I have created a release *beep* *boop* --- ## [3.72.0](google-cloud-spanner-v3.71.0...google-cloud-spanner-v3.72.0) (2026-11-12) ### Features * **dbapi:** wire timeout parameter through Connection to execute_sql ([#18484](#18484)) ([98fc304](98fc304)) * declare Python3.15 support ([8775794](8775794)) * **spanner:** auth login support for Spanner Omni endpoints ([#18273](#18273)) ([9e0548d](9e0548d)) * update API sources and regenerate ([#18797](#18797)) ([be4d954](be4d954)) ### Bug Fixes * **google-cloud-spanner:** clear buffered DDL statements on rollback and flush on autocommit ([#18636](#18636)) ([15d6a2c](15d6a2c)) * **google-cloud-spanner:** do not replay DDL statements on transaction retry ([#18625](#18625)) ([78b9da2](78b9da2)) * **spanner:** correctly wrap async streaming calls in metrics interceptor ([#18581](#18581)) ([2d19b0a](2d19b0a)) * **spanner:** eliminate duplicate metrics capture and measure full streaming latency ([#18605](#18605)) ([a7678fd](a7678fd)) * **spanner:** record streaming operation completion when stream finishes ([#18704](#18704)) ([ddeae47](ddeae47)) * **spanner:** release transaction lock if inline begin fails ([#18409](#18409)) ([dd24029](dd24029)) * **spanner:** stop deleting multiplexed sessions on rotation and close ([#18332](#18332)) ([c38e960](c38e960)) ### Performance Improvements * **spanner:** add fast-path for multiplexed session acquisition ([#18317](#18317)) ([9569b39](9569b39)) * **spanner:** avoid allocating empty RequestOptions on read and query paths ([#18422](#18422)) ([3743ff0](3743ff0)) * **spanner:** build ExecuteSqlRequest on the raw protobuf message ([#18379](#18379)) ([6d91fec](6d91fec)) * **spanner:** eliminate redundant tracing span in run_in_transaction ([#18357](#18357)) ([181e26a](181e26a)) * **spanner:** optimize built-in metrics hot path and harden concurrency ([#18329](#18329)) ([f2a791f](f2a791f)) * **spanner:** optimize query option merging and prevent in-place mutation ([#18358](#18358)) ([8cb3c6a](8cb3c6a)) * **spanner:** optimize query parameter encoding with direct type dispatch ([#18359](#18359)) ([f06497c](f06497c)) * **spanner:** optimize query result decoding and type resolution ([#18330](#18330)) ([ce72986](ce72986)) * **spanner:** optimize request ID header generation and retry closures ([#18408](#18408)) ([ad03b95](ad03b95)) * **spanner:** optimize row popping in StreamedResultSet ([#18316](#18316)) ([e66103d](e66103d)) * **spanner:** optimize single-chunk results and single-row lookups ([#18602](#18602)) ([05b3775](05b3775)) * **spanner:** prune lock and begin event allocations on single-use snapshots ([#18420](#18420)) ([ab56473](ab56473)) * **spanner:** short-circuit trace_call when tracing is inactive ([#18356](#18356)) ([58031be](58031be)) * **spanner:** support PartialResultSet.last with background stream draining ([#18320](#18320)) ([80efd63](80efd63)) ### Documentation * fix typos ([#18979](#18979)) ([5c8baeb](5c8baeb)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DDL statements queued on a DBAPI connection are executed outside the Spanner read-write transaction via
run_prior_DDL_statements()before transactional statements run, and any unexecuted DDLs remain inconnection._ddl_statementsacross transaction retries. ExcludeStatementType.DDLexecutions fromTransactionRetryHelperso aborted transactions do not replay DDL statements or re-queue already-applied DDL statements.Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: