rustyconover opened a new issue, #4695:
URL: https://github.com/apache/arrow-adbc/issues/4695
## What happened
On a connection that has served an ordinary `SELECT`, the next transaction
is never actually opened. The driver skips the `BEGIN`, the statements run in
implicit-transaction (autocommit) mode and commit themselves, and
`AdbcConnectionRollback` then does nothing.
Every call involved returns `ADBC_STATUS_OK`, so a caller has no way to
detect it.
This costs more than rollback. **A multi-statement transaction is no longer
atomic** — if a later statement fails, the earlier statements stay permanently
committed:
```
no read before transaction -> rows [1] ATOMIC (ok)
read before transaction -> rows [1, 100] NOT ATOMIC -- partial work
committed
```
Regression: **1.11.0 (release 23) is correct, 1.12.0 (release 24) is not.**
It looks like an unintended side effect of #4424 (`feat!(c/driver/postgresql):
defer transaction start`, closing #4321) meeting a pre-existing issue in the
COPY reader.
## Root cause
Two things combine.
**1. The COPY reader leaves libpq's client-side result state unconsumed.**
In `TupleReader::GetCopyData()` (`c/driver/postgresql/statement.cc:97`), once
`PQgetCopyData()` returns `-1` the reader calls `PQgetResult()` exactly once:
```c
PQclear(result_);
result_ = PQgetResult(conn_);
const ExecStatusType pq_status = PQresultStatus(result_);
```
To be clear about what this does *not* mean: the COPY itself completes
perfectly. The server finishes it, the result is `PGRES_COMMAND_OK`, and all
the data arrives intact. Nothing is aborted or left dangling. What is left
undone is purely libpq's client-side bookkeeping — `PQtransactionStatus()`
reports `PQTRANS_ACTIVE` while the connection's async state is not idle, and
that only clears once `PQgetResult()` returns `NULL`. `TupleReader::Release()`
(`statement.cc:205`) doesn't drain either; it `PQclear`s the last result and
frees buffers.
Stepping through it in plain libpq, with a second connection watching the
server's view of the same backend:
```
after PQexecParams(COPY) : client=ACTIVE (PGRES_COPY_OUT)
after PQgetCopyData -> -1 : client=ACTIVE (1001 chunks)
server sees: state=idle xid=-
after 1st PQgetResult : client=ACTIVE (PGRES_COMMAND_OK) <--
driver stops here
server sees: state=idle xid=-
after 2nd PQgetResult : client=IDLE (NULL)
```
The server is already idle two steps before the client agrees.
**Why this stayed invisible for so long:** the data path is entirely
correct, so the COPY tests pass and always did. And the `PQexec` family
discards pending results before sending (`PQexecStart`), so the state
self-heals the moment anything else runs on the connection:
```
driver's state (no 2nd getresult): client=ACTIVE
next PQexec : PGRES_TUPLES_OK rows=1000
status after that PQexec : client=IDLE (self-healed)
```
Nothing consulted the status before #4424, so a stale value that repaired
itself on next use was harmless.
**2. `EnsureTransaction()` now reads that status and misinterprets it.** At
`c/driver/postgresql/connection.cc:505`:
```c
auto txstatus = PQtransactionStatus(conn_);
if (txstatus == PQTRANS_ACTIVE || txstatus == PQTRANS_INTRANS) {
return ADBC_STATUS_OK;
}
```
`PQTRANS_INTRANS` does mean "idle inside a transaction block", so skipping
`BEGIN` there is right. `PQTRANS_ACTIVE` means only that *a command has been
sent and has not yet completed* — the connection is **busy**, which is not
evidence of a transaction.
The resulting sequence:
1. `SELECT` runs as `COPY (...) TO STDOUT`; it completes successfully, but
the connection is left reporting `PQTRANS_ACTIVE`.
2. `set_autocommit(false)` sets `autocommit_ = false` and (by design since
#4424) emits nothing.
3. The `INSERT` calls `EnsureTransaction()`, which sees `ACTIVE`, assumes a
transaction is open, and skips `BEGIN`. The insert runs in its own implicit
transaction and commits.
4. `rollback()` finds the status now `PQTRANS_IDLE` — the insert's `PQexec`
drained the stale result — and returns early without sending `ROLLBACK`, per
the `#2673` guard at `connection.cc:1170`.
Both steps report success, which is what makes it silent.
## Confirming it directly
The driver already exposes the state through
`adbc.postgresql.transaction_status`, so this needs no debugger:
```
COPY read (default) use_copy=false
fresh connection : idle -
after execute_update : idle -
after read fully consumed : active idle
after set_autocommit(False) : active idle
after INSERT in "txn" : idle intrans
```
With `adbc.postgresql.use_copy=false` the connection is left `idle`, `BEGIN`
is issued, and the transaction behaves correctly. That isolates it to the COPY
reader.
## Reproduction
```python
"""adbc_driver_postgresql 1.12.0: a COPY-based read leaves the connection in
PQTRANS_ACTIVE, so EnsureTransaction() skips BEGIN and the following
transaction silently runs in autocommit -- rollback() becomes a no-op.
pip install adbc_driver_postgresql pyarrow
python repro.py
"""
import adbc_driver_manager as adbc
import adbc_driver_postgresql
import pyarrow as pa
URI = "postgresql://postgres:postgres@localhost:5432/postgres"
db = adbc_driver_postgresql.connect(URI)
def execute_update(conn, sql):
with adbc.AdbcStatement(conn) as stmt:
stmt.set_sql_query(sql)
stmt.execute_update()
def read(conn, sql):
"""A plain SELECT. The driver serves this with COPY (...) TO STDOUT."""
with adbc.AdbcStatement(conn) as stmt:
stmt.set_sql_query(sql)
stream, _ = stmt.execute_query()
return pa.RecordBatchReader._import_from_c(stream.address).read_all()
def case(read_before_transaction):
admin = adbc.AdbcConnection(db)
execute_update(admin, "DROP TABLE IF EXISTS repro_t")
execute_update(admin, "CREATE TABLE repro_t (id INTEGER)")
execute_update(admin, "INSERT INTO repro_t SELECT generate_series FROM
generate_series(1, 1000)")
conn = adbc.AdbcConnection(db)
if read_before_transaction:
read(conn, "SELECT * FROM repro_t")
conn.set_autocommit(False)
execute_update(conn, "INSERT INTO repro_t VALUES (999999)")
conn.rollback()
conn.set_autocommit(True)
# Verify from a third connection so the check itself cannot interfere.
checker = adbc.AdbcConnection(db)
count = read(checker, "SELECT COUNT(*) FROM
repro_t").column(0)[0].as_py()
label = "read before transaction" if read_before_transaction else "no
read before transaction"
print(f"{label:<28} -> {count:>5} rows after rollback "
f"({'OK' if count == 1000 else 'ROLLBACK WAS A NO-OP'})")
case(read_before_transaction=False)
case(read_before_transaction=True)
```
**1.12.0:**
```
no read before transaction -> 1000 rows after rollback (OK)
read before transaction -> 1001 rows after rollback (ROLLBACK WAS A
NO-OP)
```
**1.11.0:**
```
no read before transaction -> 1000 rows after rollback (OK)
read before transaction -> 1000 rows after rollback (OK)
```
The only difference between the two cases is one `SELECT` on the connection
beforehand.
### Server-side evidence
With `log_statement='all'` on PostgreSQL 16. Backend `[103]` is the healthy
case, `[106]` the broken one:
```
[103] statement: BEGIN TRANSACTION
[103] statement: INSERT INTO repro_t VALUES (999999)
[103] statement: ROLLBACK
[106] execute <unnamed>: COPY (SELECT * FROM repro_t) TO STDOUT (FORMAT
binary)
[106] statement: INSERT INTO repro_t VALUES (999999)
```
No `BEGIN`, and no `ROLLBACK` is sent at all.
## Candidate fixes
I built release 24 locally and measured four variants against PostgreSQL 16.
Both changes appear to be needed — neither is sufficient alone.
**A. Drain the result in `TupleReader::GetCopyData()`** (`statement.cc:97`):
```c
result_ = PQgetResult(conn_);
+ // The COPY is done, but libpq keeps reporting PQTRANS_ACTIVE until
+ // PQgetResult() returns NULL. Drain so PQtransactionStatus() reflects
+ // reality for anyone who consults it before the next PQexec.
+ while (PGresult* extra = PQgetResult(conn_)) {
+ PQclear(extra);
+ }
const ExecStatusType pq_status = PQresultStatus(result_);
```
**B. Don't treat `PQTRANS_ACTIVE` as an open transaction**
(`connection.cc:505`):
```c
- if (txstatus == PQTRANS_ACTIVE || txstatus == PQTRANS_INTRANS) {
+ if (txstatus == PQTRANS_INTRANS) {
```
| variant | status after full read | status after partial read | rollback
after full read | rollback after partial read |
|---|---|---|---|---|
| release 24 as shipped | active | active | no-op | no-op |
| A only | **idle** | active | **ok** | no-op |
| B only | active | active | **ok** | **ok** |
| A + B | **idle** | active | **ok** | **ok** |
A alone doesn't cover a stream released before the COPY is exhausted,
because the drain only runs when the reader reaches the end — `Release()` would
need to abandon the COPY explicitly too. B alone restores transaction semantics
but leaves `adbc.postgresql.transaction_status` reporting `active` for an idle
connection.
Even with A + B, a partially-consumed read still leaves the connection
`active`; B is what keeps that case correct. Draining in `Release()` as well
would close that last gap, but I didn't want to guess at the right cancellation
strategy for an abandoned COPY.
`c/driver/postgresql` tests: **244 passed, 6 skipped, 0 failed** — identical
for as-shipped, A only, and A + B (`ADBC_POSTGRESQL_TEST_URI` against
PostgreSQL 16). `PostgresStatementTest.TransactionStatus`, which asserts
`"active"` mid-read, still passes with A.
Happy to open a PR if this looks like the right direction, though you may
well prefer a different shape for the `Release()` path.
## Impact
Any pooled or long-lived connection that mixes reads and writes hits this —
read, then write in a transaction, on the same connection. That includes the
connection-pooling case #4321 set out to improve. I found it in a DuckDB
extension that scans and writes over pooled ADBC connections, where a
`ROLLBACK` between two bulk ingests silently kept its rows.
## Environment
- `adbc_driver_postgresql` 1.12.0 (broken) / 1.11.0 (correct), from PyPI
- Also built from source at tag `apache-arrow-adbc-24` for the fix testing
above
- PostgreSQL 16 (official `postgres:16` container)
- macOS 15 arm64 (Apple silicon), Python 3.14, libpq 18 from Homebrew
Also reproduced through the C driver manager (release 24 vs 23), so it is
not specific to the Python bindings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
