Upgrade dbm, implement sqlite3 autocommit by youknowone · Pull Request #6616 · RustPython/RustPython

305-338: LGTM!

The AutocommitMode enum and its TryFromBorrowedObject implementation are well-designed. The identity checks (obj.is(&vm.ctx.true_value)) correctly distinguish between True/False and integer values, matching CPython's behavior where autocommit=1 is treated differently from autocommit=True.

---

360-361: LGTM!

The autocommit field correctly uses the derived Default implementation, which initializes to AutocommitMode::Legacy.

---

883-883: LGTM!

The autocommit field uses PyMutex<AutocommitMode> for thread-safe access, consistent with other mutable state in the Connection struct.

---

963-970: LGTM!

The autocommit mode is correctly extracted from ConnectArgs and assigned to the connection during reinitialization.

---

1778-1791: Consistent with execute() implementation.

The autocommit check mirrors the execute() method. The same verification regarding AutocommitMode::Disabled behavior applies here.

---

2018-2030: Good safety improvement for cleanup on closed connections.

This early check ensures that when iterating a cursor after the connection is closed, the statement is properly reset to release file locks. The error message is clear and matches CPython's behavior.

Note: There's a theoretical TOCTOU gap between is_closed() and the lock acquisition, but this is acceptable since:

1. Connection closure is typically single-threaded from Python's perspective
2. The later db_lock(vm) call would catch any race condition
3. This is primarily a best-effort cleanup improvement

---

1569-1579: AI summary inconsistency: Cursor does not have autocommit property.

The AI-generated summary mentions "Added public methods: autocommit (getter) on class Cursor" and "autocommit: PyMutex" in Cursor, but the actual code correctly implements autocommit only at the Connection level. This is the expected behavior since autocommit is a connection-level setting in Python's sqlite3 module.

---

364-368: LGTM!

The Traverse implementation for ConnectArgs correctly excludes autocommit since AutocommitMode is a simple Copy enum with no Python object references that need GC traversal.

---

1683-1696: No changes required—the transaction logic correctly implements CPython behavior.

The code correctly handles AutocommitMode::Disabled (corresponding to autocommit=False). Per PEP-249 and CPython sqlite3 specifications, when autocommit=False, implicit transactions via BEGIN are required before DML statements, and the user must explicitly call commit() or rollback(). The term "manual commit mode" refers to requiring explicit commits after the implicit transaction begins, not the absence of implicit transactions.

The condition *zelf.connection.autocommit.lock() != AutocommitMode::Enabled correctly:

• Calls begin_transaction() for Disabled mode (autocommit=False) → implicit transactions as required ✓
• Calls begin_transaction() for Legacy mode → respects isolation_level ✓
• Skips begin_transaction() for Enabled mode (autocommit=True) → true SQLite autocommit ✓