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

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

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 4677 bytes --]

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:

- CHICKEN Scheme:
  https://api.call-cc.org/5/doc/sql-de-lite#def:with-transaction
- Go: https://pkg.go.dev/database/sql#Conn.BeginTx
- Racket: https://docs.racket-lang.org/db/query-api.html#%28def._%28%28lib._db%2Fbase..rkt%29._call-with-transaction%29%29
- Ruby: https://www.rubydoc.info/gems/sqlite3/1.4.2/SQLite3/Database#transaction-instance_method
- Rust:
  https://docs.rs/rusqlite/latest/rusqlite/struct.Connection.html#method.transaction

In languages not offering an equivalent API, it was instead recommended
to manually issue BEGIN/COMMIT/ROLLBACK statements. But given that
`with-sqlite-transaction' exists, it would be less confusing to make it
back out on exception (and re-raise the original error).


In GNU Emacs 29.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.38,
cairo version 1.17.8)
Windowing system distributor 'The X.Org Foundation', version 11.0.12101009
System Description: Arch Linux

Configured using:
 'configure --sysconfdir=/etc --prefix=/usr --libexecdir=/usr/lib
 --with-tree-sitter --localstatedir=/var --with-cairo
 --disable-build-details --with-harfbuzz --with-libsystemd
 --with-modules --with-x-toolkit=gtk3 'CFLAGS=-march=x86-64
 -mtune=generic -O2 -pipe -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2
 -Wformat -Werror=format-security -fstack-clash-protection
 -fcf-protection -g
 -ffile-prefix-map=/build/emacs/src=/usr/src/debug/emacs -flto=auto'
 'LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -flto=auto''

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY INOTIFY
PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS
TREE_SITTER WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LC_MESSAGES: 
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo
move-toolbar gtk x-toolkit xinput2 x multi-tty make-network-process
emacs)

Memory information:
((conses 16 36631 11549)
 (symbols 48 5153 0)
 (strings 32 13273 1917)
 (string-bytes 1 378652)
 (vectors 16 9346)
 (vector-slots 8 149327 16481)
 (floats 8 21 19)
 (intervals 56 235 0)
 (buffers 984 10))


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Eli Zaretskii]

> Date: Mon, 13 Nov 2023 01:47:01 +0100
> From: Vasilij Schneidermann <mail@vasilij.de>
> 
> 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)

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

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]

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

The `unwind-protect` part does not look correct since the rollback is
always performed, even after the body form completed successfully and a
commit was done. Either a commit or a rollback should be done, not both.
Perhaps `condition-case` would be the better option, but then the error
would need to be re-raised after doing the rollback...

A minor mistake is the result variable not using an uninterned symbol
(or alternatively, `prog1` could replace the use of the result variable).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Eli Zaretskii]

> Date: Thu, 16 Nov 2023 21:25:53 +0100
> From: Vasilij Schneidermann <mail@vasilij.de>
> Cc: 67142@debbugs.gnu.org
> 
> > Does the change below look correct?  (I'm not an expert on SQLite or
> > DB programming in general.)
> 
> The `unwind-protect` part does not look correct since the rollback is
> always performed, even after the body form completed successfully and a
> commit was done. Either a commit or a rollback should be done, not both.

Oops.  Is the below better?

> A minor mistake is the result variable not using an uninterned symbol
> (or alternatively, `prog1` could replace the use of the result variable).

I don't understand this part, sorry.  Why do we need a symbol to
return the result of the body?

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index aad0aa4..4488896 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -24,18 +24,25 @@
 ;;; 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)))
     `(let ((,db-var ,db)
            (,func-var (lambda () ,@body)))
        (if (sqlite-available-p)
-           (unwind-protect
-               (progn
-                 (sqlite-transaction ,db-var)
-                 (funcall ,func-var))
-             (sqlite-commit ,db-var))
+           (let (result commit)
+             (unwind-protect
+                 (progn
+                   (sqlite-transaction ,db-var)
+                   (setq result (funcall ,func-var))
+                   (setq commit (sqlite-commit ,db-var))
+                   result)
+             (or commit (sqlite-rollback ,db-var))))
          (funcall ,func-var)))))
 
 (provide 'sqlite)

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

[Visuwesh]

[வெள்ளி நவம்பர் 17, 2023] Eli Zaretskii wrote:

>> A minor mistake is the result variable not using an uninterned symbol
>> (or alternatively, `prog1` could replace the use of the result variable).
>
> I don't understand this part, sorry.  Why do we need a symbol to
> return the result of the body?

db-var and func-var are uninterned symbols in the macro expansion but
the variables 'result' and 'commit' marked below aren't.  If the intent
is not to expose these variables to the macro's BODY, the inner let
should also uninterned symbols right?

> diff --git a/lisp/sqlite.el b/lisp/sqlite.el
> index aad0aa4..4488896 100644
> --- a/lisp/sqlite.el
> +++ b/lisp/sqlite.el
> @@ -24,18 +24,25 @@
>  ;;; 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)))
>      `(let ((,db-var ,db)
>             (,func-var (lambda () ,@body)))
               --------


>         (if (sqlite-available-p)
> -           (unwind-protect
> -               (progn
> -                 (sqlite-transaction ,db-var)
> -                 (funcall ,func-var))
> -             (sqlite-commit ,db-var))
> +           (let (result commit)
                    ^^^^^^^^^^^^^
                    Use uninterned symbols here like above (marked with -).


> +             (unwind-protect
> +                 (progn
> +                   (sqlite-transaction ,db-var)
> +                   (setq result (funcall ,func-var))
> +                   (setq commit (sqlite-commit ,db-var))
> +                   result)
> +             (or commit (sqlite-rollback ,db-var))))
>           (funcall ,func-var)))))
>  
>  (provide 'sqlite)

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

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

> > I don't understand this part, sorry.  Why do we need a symbol to
> > return the result of the body?
> 
> db-var and func-var are uninterned symbols in the macro expansion but
> the variables 'result' and 'commit' marked below aren't.  If the intent
> is not to expose these variables to the macro's BODY, the inner let
> should also uninterned symbols right?

Correct, this is what I was hinting at. As annoying as it is, when
writing unhygienic macros one should use uninterned symbols to avoid
exposing additional variables to the BODY argument.

Alternatively, the pattern of `(let ((return (...))) ... return)` can be
replaced with `(prog1 (...) ...)`.

@Eli: The new patch looks better and closer to how the issue is solved
in the Ruby sqlite3 gem. I'm still not sure about this use of
`unwind-protect` being correct, but it does preserve the backtrace in
case of an error better than when I used `(condition-case e (...) (error
(apply #'signal e)))`.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Eli Zaretskii]

> Date: Fri, 17 Nov 2023 10:08:36 +0100
> From: Vasilij Schneidermann <mail@vasilij.de>
> Cc: Eli Zaretskii <eliz@gnu.org>, 67142@debbugs.gnu.org
> 
> > db-var and func-var are uninterned symbols in the macro expansion but
> > the variables 'result' and 'commit' marked below aren't.  If the intent
> > is not to expose these variables to the macro's BODY, the inner let
> > should also uninterned symbols right?
> 
> Correct, this is what I was hinting at. As annoying as it is, when
> writing unhygienic macros one should use uninterned symbols to avoid
> exposing additional variables to the BODY argument.
> 
> Alternatively, the pattern of `(let ((return (...))) ... return)` can be
> replaced with `(prog1 (...) ...)`.
> 
> @Eli: The new patch looks better and closer to how the issue is solved
> in the Ruby sqlite3 gem. I'm still not sure about this use of
> `unwind-protect` being correct, but it does preserve the backtrace in
> case of an error better than when I used `(condition-case e (...) (error
> (apply #'signal e)))`.

OK, how about the below?

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index aad0aa4..8a52573 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -24,19 +24,28 @@
 ;;; 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)))
+        (func-var (gensym))
+        (res-var (gensym))
+        (commit-var (gensym)))
     `(let ((,db-var ,db)
-           (,func-var (lambda () ,@body)))
+           (,func-var (lambda () ,@body))
+           ,res-var ,commit-var)
        (if (sqlite-available-p)
            (unwind-protect
                (progn
                  (sqlite-transaction ,db-var)
-                 (funcall ,func-var))
-             (sqlite-commit ,db-var))
-         (funcall ,func-var)))))
+                 (setq ,res-var (funcall ,func-var))
+                 (setq ,commit-var (sqlite-commit ,db-var))
+                 ,res-var)
+             (or ,commit-var (sqlite-rollback ,db-var))))
+         (funcall ,func-var))))
 
 (provide 'sqlite)
 

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

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 1605 bytes --]

> OK, how about the below?
> 
> diff --git a/lisp/sqlite.el b/lisp/sqlite.el
> index aad0aa4..8a52573 100644
> --- a/lisp/sqlite.el
> +++ b/lisp/sqlite.el
> @@ -24,19 +24,28 @@
>  ;;; 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)))
> +        (func-var (gensym))
> +        (res-var (gensym))
> +        (commit-var (gensym)))
>      `(let ((,db-var ,db)
> -           (,func-var (lambda () ,@body)))
> +           (,func-var (lambda () ,@body))
> +           ,res-var ,commit-var)
>         (if (sqlite-available-p)
>             (unwind-protect
>                 (progn
>                   (sqlite-transaction ,db-var)
> -                 (funcall ,func-var))
> -             (sqlite-commit ,db-var))
> -         (funcall ,func-var)))))
> +                 (setq ,res-var (funcall ,func-var))
> +                 (setq ,commit-var (sqlite-commit ,db-var))
> +                 ,res-var)
> +             (or ,commit-var (sqlite-rollback ,db-var))))
> +         (funcall ,func-var))))
>  
>  (provide 'sqlite)

Thank you, this looks very good. I've tested it on my own code using the
SQLite support and there don't appear to be any (obvious issues).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Eli Zaretskii]

> Date: Mon, 20 Nov 2023 22:14:00 +0100
> From: Vasilij Schneidermann <mail@vasilij.de>
> Cc: visuweshm@gmail.com, 67142@debbugs.gnu.org
> 
> Thank you, this looks very good. I've tested it on my own code using the
> SQLite support and there don't appear to be any (obvious issues).

Thanks for testing and for the valuable feedback, I've now installed
this on the emacs-29 branch, and I'm therefore closing this bug.

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

[J.P.]

Vasilij Schneidermann <mail@vasilij.de> writes:

>> OK, how about the below?
>> 
>> diff --git a/lisp/sqlite.el b/lisp/sqlite.el
>> index aad0aa4..8a52573 100644
>> --- a/lisp/sqlite.el
>> +++ b/lisp/sqlite.el
>> @@ -24,19 +24,28 @@
>>  ;;; 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)))
>> +        (func-var (gensym))
>> +        (res-var (gensym))
>> +        (commit-var (gensym)))
>>      `(let ((,db-var ,db)
>> -           (,func-var (lambda () ,@body)))
>> +           (,func-var (lambda () ,@body))
>> +           ,res-var ,commit-var)
>>         (if (sqlite-available-p)
>>             (unwind-protect
>>                 (progn
>>                   (sqlite-transaction ,db-var)
>> -                 (funcall ,func-var))
>> -             (sqlite-commit ,db-var))
>> -         (funcall ,func-var)))))
>> +                 (setq ,res-var (funcall ,func-var))
>> +                 (setq ,commit-var (sqlite-commit ,db-var))
>> +                 ,res-var)
>> +             (or ,commit-var (sqlite-rollback ,db-var))))
>> +         (funcall ,func-var))))
>>  
>>  (provide 'sqlite)
>
> Thank you, this looks very good. I've tested it on my own code using the
> SQLite support and there don't appear to be any (obvious issues).

Not obvious on account of the indentation, but it seems the second

  (funcall ,func-var)

snuck outside the `if' form with that fix, making body run twice if you
have SQLite.

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

[J.P.]

[-- Attachment #1: Type: text/plain, Size: 332 bytes --]

"J.P." <jp@neverwas.me> writes:

> Not obvious on account of the indentation, but it seems the second
>
>   (funcall ,func-var)
>
> snuck outside the `if' form with that fix, making body run twice if you
> have SQLite.

I guess folks are too busy to get excited about fixing this. Here's a
patch in case that ever changes. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Run-successful-body-once-in-with-sqlite-transaction.patch --]
[-- Type: text/x-patch, Size: 3783 bytes --]

From e9319447a1403736cd8aac766d29ba2d98591eff Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 10 Mar 2024 20:13:42 -0700
Subject: [PATCH] Run successful body once in with-sqlite-transaction

* lisp/sqlite.el (with-sqlite-transaction): Tuck misplaced body of
else form back into feature-test control structure whence it escaped.
* test/lisp/sqlite-tests.el: New file to accompany
test/src/sqlite-tests.el.
(Bug#67142)
---
 lisp/sqlite.el            |  7 +++---
 test/lisp/sqlite-tests.el | 51 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/sqlite-tests.el

diff --git a/lisp/sqlite.el b/lisp/sqlite.el
index 46e35ac18d8..efc5997fb5c 100644
--- a/lisp/sqlite.el
+++ b/lisp/sqlite.el
@@ -32,7 +32,8 @@ with-sqlite-transaction
 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."
+back the transaction changes before allowing the signal to
+propagate."
   (declare (indent 1) (debug (form body)))
   (let ((db-var (gensym))
         (func-var (gensym))
@@ -48,8 +49,8 @@ with-sqlite-transaction
                  (setq ,res-var (funcall ,func-var))
                  (setq ,commit-var (sqlite-commit ,db-var))
                  ,res-var)
-             (or ,commit-var (sqlite-rollback ,db-var))))
-         (funcall ,func-var))))
+             (or ,commit-var (sqlite-rollback ,db-var)))
+         (funcall ,func-var)))))
 
 (provide 'sqlite)
 
diff --git a/test/lisp/sqlite-tests.el b/test/lisp/sqlite-tests.el
new file mode 100644
index 00000000000..d4892a27efc
--- /dev/null
+++ b/test/lisp/sqlite-tests.el
@@ -0,0 +1,51 @@
+;;; sqlite-tests.el --- Tests for sqlite.el  -*- lexical-binding: t; -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+(require 'sqlite)
+
+(ert-deftest with-sqlite-transaction ()
+  (skip-unless (sqlite-available-p))
+  (let ((db (sqlite-open)))
+    (sqlite-execute db "create table test (a)")
+    (should
+     (eql 42 (with-sqlite-transaction db
+               (sqlite-execute db "insert into test values (1)")
+               (should (equal '((1)) (sqlite-select db "select * from test")))
+               42)))
+    ;; Body runs exactly once.
+    (should (equal '((1)) (sqlite-select db "select * from test")))))
+
+(ert-deftest with-sqlite-transaction/rollback ()
+  (skip-unless (sqlite-available-p))
+  (let ((db (sqlite-open)))
+    (sqlite-execute db "create table test (a)")
+    (should (equal '(sqlite-error
+                     ("SQL logic error" "no such function: fake" 1 1))
+                   (should-error
+                    (with-sqlite-transaction db
+                      (sqlite-execute db "insert into test values (1)")
+                      (sqlite-execute db "insert into test values (fake(2))")
+                      42))))
+    ;; First insertion (a=1) rolled back.
+    (should-not (sqlite-select db "select * from test"))))
+
+;;; sqlite-tests.el ends here
-- 
2.44.0

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

[Eli Zaretskii]

> From: "J.P." <jp@neverwas.me>
> Cc: eliz@gnu.org, visuweshm@gmail.com, mail@vasilij.de
> Date: Sun, 10 Mar 2024 20:43:04 -0700
> 
> "J.P." <jp@neverwas.me> writes:
> 
> > Not obvious on account of the indentation, but it seems the second
> >
> >   (funcall ,func-var)
> >
> > snuck outside the `if' form with that fix, making body run twice if you
> > have SQLite.
> 
> I guess folks are too busy to get excited about fixing this. Here's a
> patch in case that ever changes. Thanks.

Thanks, installed on the emacs-29 branch.