Skip to content

Commit writes made through owned SQL connections - #68

Open
umd0730 wants to merge 1 commit into
wireservice:masterfrom
umd0730:fix/owned-connection-transactions
Open

umd0730 wants to merge 1 commit into
wireservice:masterfrom
umd0730:fix/owned-connection-transactions

Conversation

@umd0730

@umd0730 umd0730 commented Sep 21, 2026

Copy link
Copy Markdown

Passing a connection string to Table.to_sql currently loses inserted rows with SQLAlchemy 2: the function opens a connection, inserts successfully, then closes it without committing. A two-row table written to a file-backed SQLite database is empty when read through a new connection.

Manage a transaction only when to_sql owns the engine/connection. Successful writes commit; exceptions roll back and release the owned connection and engine. Existing connections retain caller-controlled transactions, including when insertion fails. This preserves csvsql's outer transaction and does not reintroduce unconditional transaction nesting from the changes discussed in #40/#42 and the 0.7.2 changelog.

Add seven regression cases for persisted ordinary/chunked writes, failed ordinary/chunked insert cleanup, and caller-owned commit/rollback/error handling. Document the ownership distinction and database-specific DDL limitations.

Validation on Windows / Python 3.12.14:

  • Before the production fix, the initial five-case regression suite had three failures and two passes: both persisted-write cases lost their rows, and the failure path left its connection open.
  • Full agate-sql suite: 25 passed with SQLAlchemy 2.0.54; 25 passed with SQLAlchemy 1.4.54 (one legacy API deprecation warning).
  • csvkit's tests/test_utilities/test_csvsql.py, importing this agatesql source: 23 passed (two existing empty-input dialect-sniff warnings).
  • flake8 agatesql tests, isort agatesql tests --check-only, and git diff --check: passed.

Transaction behavior was exercised using real file-backed SQLite databases and separate connections. No live PostgreSQL/MySQL service or documentation build was tested locally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant