fix: roll back the connection when a chunked read_sql_query fails - #3437
Open
hsusul wants to merge 1 commit into
Open
fix: roll back the connection when a chunked read_sql_query fails#3437hsusul wants to merge 1 commit into
hsusul wants to merge 1 commit into
Conversation
read_sql_query() wraps both of its return paths in a try/except that calls con.rollback() before re-raising. For chunksize=None that works, but the chunked path returns a generator, so nothing inside _iterate_results() runs until the caller iterates -- long after the except clause has been unwound. A failing statement therefore left the connection in an aborted-transaction state, and every later query on that connection failed with a confusing secondary error (e.g. PostgreSQL's "current transaction is aborted"). Move the rollback into _iterate_results() so both paths behave identically. GeneratorExit is not an Exception, so abandoning the iterator early still does not trigger a rollback.
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.
Feature or Bugfix
Detail
read_sql_query(..., chunksize=N)never rolled back the connection when the query failed, because_iterate_resultsis a generator and its body runs after the enclosingtry/excepthas already been unwound. The non-chunked path (chunksize=None) has always rolled back. This fixes the chunked path so both behave identically.Relates
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Affected API
awswrangler._databases.read_sql_query, and therefore every public wrapper that routes through it withchunksize:wr.postgresql.read_sql_query/read_sql_tablewr.mysql.read_sql_query/read_sql_tablewr.redshift.read_sql_query/read_sql_tablewr.sqlserver.read_sql_query/read_sql_tablewr.oracle.read_sql_query/read_sql_tableCurrent behavior
read_sql_querywraps both of its return paths in atry/exceptthat callscon.rollback()before re-raising:_iterate_resultsis a generator function, so calling it only builds a generator object —cursor.execute(...)does not run until the caller iterates, by which point theexceptclause has already been unwound. The rollback is therefore dead code for every chunked read: a failing statement leaves the connection in an aborted-transaction state, and the next query on that same connection fails with a confusing secondary error (on PostgreSQL/Redshift:current transaction is aborted, commands ignored until end of transaction block).The same applies to a failure that happens part-way through iteration (e.g. a dropped connection during
fetchmany).Reproduction
No database server and no AWS credentials required — a DB-API-shaped stub is enough:
Output on
main(609db3c):Corrected behavior
Both paths roll back the connection before re-raising, so a failed chunked read leaves the connection in the same usable state as a failed non-chunked read.
Root cause
Lazy generator evaluation: the
tryblock inread_sql_queryonly covers the construction of the generator, not its execution.Implementation
Move the rollback into
_iterate_results, wrapping its body — this covers both the initialcursor.executeand any failure duringfetchmany/frame construction.read_sql_queryis otherwise unchanged, and the non-chunked path is untouched.GeneratorExitis aBaseException, not anException, so abandoning the iterator early (break,.close(), garbage collection) still does not trigger a rollback — normal early exit is not an error.Regression tests
New
tests/unit/test_databases.py(pure unit tests, stub connection, no server and no AWS credentials):test_read_sql_query_rolls_back_on_execute_error[None]/[1]— parametrized overchunksize, asserts identical rollback behavior on both paths and that the cursor context manager is exited. The[1]case fails onmain.test_read_sql_query_chunked_rolls_back_on_fetch_error— failure part-way through iteration (first chunk yielded successfully, secondfetchmanyraises); fails onmain.test_read_sql_query_chunked_does_not_roll_back_on_success— no rollback on the happy path, and the yielded frame carries the expected values.test_read_sql_query_chunked_does_not_roll_back_on_early_exit—.close()on the iterator does not roll back.Before the fix: 2 failed, 3 passed. After: 5 passed.
Validation
Run locally on macOS / Python 3.13.5 (pandas 3.0.1, pyarrow 25.0.0, moto 5.2.2):
pytest tests/unit/test_databases.pypytest tests/unit/{test_databases,test_moto,test_utils,test_data_types,test_sql_params_formatter,test_s3_vectors_mocked,test_metadata}.pyruff format --check .ruff check .mypy awswranglermain; none in_databases.pydoc8 --ignore-path docs/source/stubs --max-line-length 120 docs/sourceuv lock --checkuv buildgit diff --checkThe one failure,
test_s3_vectors_mocked.py::test_delete_vector_index_arn_with_name_raises, reproduces identically on unmodifiedmainand is a local-environment artifact (botocore.exceptions.NoRegionError: You must specify a region) — unrelated to this change.Not run: every suite that needs live AWS infrastructure or a real database server —
tests/unit/test_postgresql.py,test_mysql.py,test_redshift.py,test_sqlserver.py,test_oracle.py(these are the integration tests that exercise the touched code path against real engines), plus the Athena/Glue/S3/DynamoDB/OpenSearch/Timestream/QuickSight/EMR suites andtests/load.tests/unit/test_neptune_parsing.pywas skipped becausegremlin_pythonis not installed locally. The new tests are the credential-free substitute for the affected path.Compatibility and ownership
rollback()is called, mirroring what the non-chunked path has always done.try/rollbacksee a harmless second rollback on an already-clean transaction.