Subject: bug#67142: 29.1; with-sqlite-transaction commits on exception rather than rolling back

From: Eli Zaretskii <[email-masked]>
Date: Thu, 16 Nov 2023 16:12:00 +0200

> Date: Mon, 13 Nov 2023 01:47:01 +0100
> From: Vasilij Schneidermann <[email-masked]>
> 
> During experimentation with built-in SQLite support, I've noticed that
> if I use `with-sqlite-transaction' and there is a mistake in the body,
> `sqlite-commit' is executed rather than `sqlite-rollback', thereby
> committing the changes done up to the error instead of backing out.
> 
> This appears to be unusual behavior. From my quick research, the
> following SQLite bindings in other languages have an equivalent to
> `with-sqlite-transaction' and back out changes on error:

Thanks.

Does the change below look correct?  (I'm not an expert on SQLite or
DB programming in general.)

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index aad0aa4..df06647 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -24,7 +24,11 @@
 ;;; Code:
 
 (defmacro with-sqlite-transaction (db &rest body)
-  "Execute BODY while holding a transaction for DB."
+  "Execute BODY while holding a transaction for DB.
+If BODY completes normally, commit the changes and return
+the value of BODY.
+If BODY signals an error, or transaction commit fails, roll
+back the transaction changes."
   (declare (indent 1) (debug (form body)))
   (let ((db-var (gensym))
         (func-var (gensym)))
@@ -32,10 +36,13 @@ with-sqlite-transaction
            (,func-var (lambda () ,@body)))
        (if (sqlite-available-p)
            (unwind-protect
-               (progn
+               (let (result)
                  (sqlite-transaction ,db-var)
-                 (funcall ,func-var))
-             (sqlite-commit ,db-var))
+                 (setq result (funcall ,func-var))
+                 (or (sqlite-commit ,db-var)
+                     (signal 'sqlite-error (list "SQLite commit failed")))
+                 result)
+             (sqlite-rollback ,db-var))
          (funcall ,func-var)))))
 
 (provide 'sqlite)