Copilot commented on code in PR #4424:
URL: https://github.com/apache/arrow-adbc/pull/4424#discussion_r3450235030
##########
c/driver/postgresql/connection.cc:
##########
@@ -489,6 +489,26 @@ AdbcStatusCode PostgresConnection::Commit(struct
AdbcError* error) {
return ADBC_STATUS_OK;
}
+AdbcStatusCode PostgresConnection::EnsureTransaction(struct AdbcError* error) {
+ if (autocommit_) {
+ return ADBC_STATUS_OK;
+ }
+ auto txstatus = PQtransactionStatus(conn_);
+ if (txstatus == PQTRANS_ACTIVE || txstatus == PQTRANS_INTRANS) {
+ return ADBC_STATUS_OK;
+ }
+
+ PGresult* result = PQexec(conn_, "BEGIN TRANSACTION");
+ if (PQresultStatus(result) != PGRES_COMMAND_OK) {
+ InternalAdbcSetError(error, "%s%s",
+ "[libpq] Failed to begin transaction: ",
PQerrorMessage(conn_));
+ PQclear(result);
+ return ADBC_STATUS_IO;
+ }
+ PQclear(result);
+ return ADBC_STATUS_OK;
+}
Review Comment:
EnsureTransaction() attempts to start a new transaction for any
non-(ACTIVE|INTRANS) state. If the connection is in PQTRANS_INERROR (aborted
transaction), sending "BEGIN TRANSACTION" will always fail with "current
transaction is aborted" and masks the real issue; callers should instead be
told to rollback before executing more statements (and avoid changing the error
classification to IO). Handling PQTRANS_UNKNOWN explicitly would also avoid
starting a transaction when libpq can't determine state.
##########
python/adbc_driver_postgresql/tests/test_dbapi.py:
##########
@@ -723,3 +723,50 @@ def test_bind_null_unknown_inference(postgres:
dbapi.Connection) -> None:
result = cur.fetchone()
assert result is not None
assert result[0] is None
+
+
+def test_transaction(postgres_uri: str) -> None:
+ with dbapi.connect(postgres_uri) as conn1, dbapi.connect(postgres_uri) as
conn2:
+ with conn1.cursor() as cur1:
+ cur1.execute("DROP TABLE IF EXISTS test_transaction")
+ conn1.commit()
+
+ with conn1.cursor() as cur1:
+ cur1.execute("CREATE TABLE test_transaction (a INTEGER)")
+
+ with conn2.cursor() as cur2:
+ with pytest.raises(dbapi.ProgrammingError) as excinfo:
+ cur2.execute("INSERT INTO test_transaction VALUES (1)")
+ assert excinfo.value.sqlstate == "42P01"
+ conn2.rollback()
+
+ with conn1.cursor() as cur1:
+ cur1.execute("INSERT INTO test_transaction VALUES (1)")
+
+ conn1.rollback()
+
+ with conn1.cursor() as cur1:
+ with pytest.raises(dbapi.ProgrammingError) as excinfo:
+ cur1.execute("INSERT INTO test_transaction VALUES (1)")
+
Review Comment:
The second `pytest.raises(... ) as excinfo` in `test_transaction` never
asserts anything about the captured exception, which makes the test less
deterministic (it would pass for any ProgrammingError). Since the first similar
block asserts sqlstate, this one should too.
--
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]
