[PATCH] fixes / improvements for (guix store database)

  • Done
  • quality assurance status badge
Details
3 participants
  • Caleb Ristvedt
  • Danny Milosavljevic
  • Ludovic Courtès
Owner
unassigned
Submitted by
Caleb Ristvedt
Severity
important

Debbugs page

C
C
Caleb Ristvedt wrote on 1 Jun 2020 23:31
(address . guix-patches@gnu.org)
87ftbernat.fsf@cune.org
After some pondering about why the database might be locked so
frequently, this is what I've managed to come up with. The first patch
is the most likely to actually help with that, and the others mostly
involve improving robustness.

Ideally we'd come up with a test to quantify how much these kinds of
changes affect contention over the database. For now, though, all that I
can think of is seeing how this affects the systems that have had issues
with that.

- reepca
From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
Date: Mon, 1 Jun 2020 18:50:07 -0500
Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing
statement reset
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
keep re-preparing statements that are frequently used. Unfortunately it
doesn't quite emulate the semantics of sqlite_finalize properly, because it
doesn't cause a commit if the statement being finalized is the last "active"
statement. We work around this by wrapping sqlite-finalize with our own
version that ensures sqlite-reset is called, which does The Right Thing™.

* guix/store/database.scm (sqlite-finalize): new procedure that shadows the
sqlite-finalize from (sqlite3).
---
guix/store/database.scm | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)

Toggle diff (43 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index ef52036ede..d4251e580e 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -130,6 +130,36 @@ transaction after it finishes."
If FILE doesn't exist, create it and initialize it as a new database."
(call-with-database file (lambda (db) exp ...)))
+(define (sqlite-finalize stmt)
+ ;; Cached statements aren't reset when sqlite-finalize is invoked on
+ ;; them. This can cause problems with automatically-started transactions:
+ ;;
+ ;; "An implicit transaction (a transaction that is started automatically,
+ ;; not a transaction started by BEGIN) is committed automatically when the
+ ;; last active statement finishes. A statement finishes when its last cursor
+ ;; closes, which is guaranteed to happen when the prepared statement is
+ ;; reset or finalized. Some statements might "finish" for the purpose of
+ ;; transaction control prior to being reset or finalized, but there is no
+ ;; guarantee of this."
+ ;;
+ ;; Thus, it's possible for an implicitly-started transaction to hang around
+ ;; until sqlite-reset is called when the cached statement is next
+ ;; used. Because the transaction is committed automatically only when the
+ ;; *last active statement* finishes, the implicitly-started transaction may
+ ;; later be upgraded to a write transaction (!) and this non-reset statement
+ ;; will still be keeping the transaction from committing until it is next
+ ;; used or the database connection is closed. This has the potential to make
+ ;; (exclusive) write access to the database necessary for much longer than
+ ;; it should be.
+ ;;
+ ;; (see https://www.sqlite.org/lang_transaction.html)
+ ;; To work around this, we wrap sqlite-finalize so that sqlite-reset is
+ ;; always called. This will continue working even when the behavior is fixed
+ ;; in guile-sqlite3, since resetting twice doesn't cause any problems. We
+ ;; can remove this once the fixed guile-sqlite3 is widespread.
+ (sqlite-reset stmt)
+ ((@ (sqlite3) sqlite-finalize) stmt))
+
(define (last-insert-row-id db)
;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'.
;; Work around that.
--
2.26.2
Attachment: 0002-database-rewrite-query-procedures-in-terms-of-with-s.patch
Attachment: 0003-database-ensure-update-or-insert-is-run-within-a-tra.patch
Attachment: 0004-database-separate-transaction-handling-and-retry-han.patch
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl7V8koACgkQwWaqSV9/
GJwHXgf/R/mpXxFexv9B/V22BwF8+bNmDr6L0KVdFmaKz15nmPbMnnLtqmJTkrik
vbEtwQ9MT4we6yNRD76VaVEe+iE1Yk9WvCyBNUcoPL07dlzY/hN+0frSjQI4MqSc
GIktVbdt6Bf3bloCl5SHg2KrGEYY+ptWliEuY3AfQnBL/7YoSKfd4me5TLa1PB/4
RhyVqVFJHq/jeSQvirDYQoMCiRaDCxu8g+xV5/7ITE3Ue+gkM2aYKSrWz8cFhiJ4
/S3zFZYuI31d2J8jdkbcz5tuVG/ALpTTCIJa8cwIpRB2Yon6yZN4zoDX8ue3P8we
qEnsPdkxSPu7lckVk7OqQNQTvxpknQ==
=2ARa
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 4 Jun 2020 09:40
(name . Caleb Ristvedt)(address . caleb.ristvedt@cune.org)(address . 41658@debbugs.gnu.org)
87a71i943g.fsf@gnu.org
Hi,

Thanks for the thorough investigation and for the patches!

Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:

Toggle quote (19 lines)
> From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 18:50:07 -0500
> Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing
> statement reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
> keep re-preparing statements that are frequently used. Unfortunately it
> doesn't quite emulate the semantics of sqlite_finalize properly, because it
> doesn't cause a commit if the statement being finalized is the last "active"
> statement. We work around this by wrapping sqlite-finalize with our own
> version that ensures sqlite-reset is called, which does The Right Thing™.
>
> * guix/store/database.scm (sqlite-finalize): new procedure that shadows the
> sqlite-finalize from (sqlite3).

Nice. It would be great if you could report it upstream (Danny and/or
myself can then patch it directly in guile-sqlite3 and push out a
release) and refer to the issue from here.

We can have this patch locally in the meantime, unless it would break
once the new guile-sqlite3 is out. WDYT?

Toggle quote (23 lines)
> From ee24ab21122b1c75a7d67d7062550e15e54ab62f Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 19:21:43 -0500
> Subject: [PATCH 2/4] database: rewrite query procedures in terms of
> with-statement.
>
> Most of our queries would fail to finalize their statements properly if sqlite
> returned an error during their execution. This resolves that, and also makes
> them somewhat more concise as a side-effect.
>
> This also makes some small changes to improve certain queries where behavior
> was strange or overly verbose.
>
> * guix/store/database.scm (call-with-statement): new procedure.
> (with-statement): new macro.
> (last-insert-row-id, path-id, update-or-insert, add-references): rewrite to
> use with-statement.
> (update-or-insert): factor last-insert-row-id out of the end of both
> branches.
> (add-references): remove pointless last-insert-row-id call.
>
> * .dir-locals.el (with-statement): add indenting information.

LGTM!

Toggle quote (15 lines)
> From 7d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 21:43:14 -0500
> Subject: [PATCH 3/4] database: ensure update-or-insert is run within a
> transaction
>
> update-or-insert can break if an insert occurs between when it decides whether
> to update or insert and when it actually performs that operation. Putting the
> check and the update/insert operation in the same transaction ensures that the
> update/insert will only succeed if no other write has occurred in the middle.
>
> * guix/store/database.scm (call-with-savepoint): new procedure.
> (update-or-insert): use call-with-savepoint to ensure the read and the
> insert/update occur within the same transaction.

That’s a bit beyond my understanding, but I think you can also push this
one. :-)

Make sure “make check TESTS=tests/store-database.scm” is still happy.

Thanks a lot!

Ludo’.
D
D
Danny Milosavljevic wrote on 4 Jun 2020 10:00
(address . 41658@debbugs.gnu.org)
20200604190000.3c454a83@scratchpost.org
Hi Ludo,
Hi Caleb,

On Thu, 04 Jun 2020 18:40:35 +0200
Ludovic Courtès <ludo@gnu.org> wrote:

Toggle quote (4 lines)
> Nice. It would be great if you could report it upstream (Danny and/or
> myself can then patch it directly in guile-sqlite3 and push out a
> release) and refer to the issue from here.

I agree. It's easy to change sqlite-finalize in guile-sqlite3 to
call sqlite-reset, basically just adapt

(define sqlite-finalize
(let ((f (pointer->procedure
int
(dynamic-func "sqlite3_finalize" libsqlite3)
(list '*))))
(lambda (stmt)
;; Note: When STMT is cached, this is a no-op. This ensures caching
;; actually works while still separating concerns: users can turn
;; caching on and off without having to change the rest of their code.
(when (and (stmt-live? stmt)
(not (stmt-cached? stmt)))
(let ((p (stmt-pointer stmt)))
(sqlite-remove-statement! (stmt->db stmt) stmt)
(set-stmt-live?! stmt #f)
(f p))))))

so that it calls sqlite-reset in the "when"'s new "else" branch there.

(we could also always call sqlite3_reset on sqlite-finalize anyway, it wouldn't
hurt but it wouldn't help either)

I agree that sqlite-finalize should model sqlite's finalization behavior as
much as possible.

Also, the comment about this being a no-op is not true then anymore.

We should definitely also pick up Caleb's comment upstream:

+ ;; Cached statements aren't reset when sqlite-finalize is invoked on
+ ;; them. This can cause problems with automatically-started transactions:
+ ;;
+ ;; "An implicit transaction (a transaction that is started automatically,
+ ;; not a transaction started by BEGIN) is committed automatically when the
+ ;; last active statement finishes. A statement finishes when its last cursor
+ ;; closes, which is guaranteed to happen when the prepared statement is
+ ;; reset or finalized. Some statements might "finish" for the purpose of
+ ;; transaction control prior to being reset or finalized, but there is no
+ ;; guarantee of this."
+ ;;
+ ;; Thus, it's possible for an implicitly-started transaction to hang around
+ ;; until sqlite-reset is called when the cached statement is next
+ ;; used. Because the transaction is committed automatically only when the
+ ;; *last active statement* finishes, the implicitly-started transaction may
+ ;; later be upgraded to a write transaction (!) and this non-reset statement
+ ;; will still be keeping the transaction from committing until it is next
+ ;; used or the database connection is closed. This has the potential to make
+ ;; (exclusive) write access to the database necessary for much longer than
+ ;; it should be.
+ ;;

@Caleb:

and pull request so this is auditable?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl7ZKLgACgkQ5xo1VCww
uqW8HAf+LapIRcQdeHgWmjm9LAW3w8Ww1MFSZhulb/a/GKz4tC5EMCOlnIs7Otaj
kdpeQopGmt8lPHNQNYc85jszOZe+ROzDC7+iraeYIXtoVdQGm5uROts8DH25YGDc
GiEg+Q77vJtS73u+f2iG/18UCWOsM0Czkgnv1kV7fFb+yedhl+OoPmueyHbZVl/n
dHhut5EwuyXRimz0yErqY4atoFrqU9fPWCe7JZHhz+krHhHavE2AUL6TFBroayDf
O2m5aWECT7yUNR0wuyUN9NCLQPNuJ9D4WtHn8B+3KYQ6IT9Mj0jtIhwkRk2k/dy4
T0WyJaaKSB5WgM5WYouCeR7OMKfSSg==
=hCaa
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 5 Jun 2020 09:19
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
87bllx32oz.fsf@gnu.org
Hi Danny!

Danny Milosavljevic <dannym@scratchpost.org> skribis:

Toggle quote (3 lines)
> I agree. It's easy to change sqlite-finalize in guile-sqlite3 to
> call sqlite-reset, basically just adapt

[...]

Toggle quote (5 lines)
> @Caleb:
>
> Could you file an issue at https://notabug.org/guile-sqlite3/guile-sqlite3/issues
> and pull request so this is auditable?

Agreed.

Danny, once this is merged upstream, could you tag a new release? There
are a few other useful improvements in there.

Thanks,
Ludo’.
L
L
Ludovic Courtès wrote on 6 Jun 2020 14:38
control message for bug #41658
(address . control@debbugs.gnu.org)
87lfkzub78.fsf@gnu.org
severity 41658 important
quit
C
C
Caleb Ristvedt wrote on 7 Jun 2020 22:52
Re: [bug#41658] [PATCH] fixes / improvements for (guix store database)
(name . Ludovic Courtès)(address . ludo@gnu.org)
87o8pu5cjx.fsf@cune.org
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (29 lines)
> Hi,
>
> Thanks for the thorough investigation and for the patches!
>
> Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:
>
>> From cce653c590be1506e15044e445aa9805370ac759 Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
>> Date: Mon, 1 Jun 2020 18:50:07 -0500
>> Subject: [PATCH 1/4] database: work around guile-sqlite3 bug preventing
>> statement reset
>> MIME-Version: 1.0
>> Content-Type: text/plain; charset=UTF-8
>> Content-Transfer-Encoding: 8bit
>>
>> guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
>> keep re-preparing statements that are frequently used. Unfortunately it
>> doesn't quite emulate the semantics of sqlite_finalize properly, because it
>> doesn't cause a commit if the statement being finalized is the last "active"
>> statement. We work around this by wrapping sqlite-finalize with our own
>> version that ensures sqlite-reset is called, which does The Right Thing™.
>>
>> * guix/store/database.scm (sqlite-finalize): new procedure that shadows the
>> sqlite-finalize from (sqlite3).
>
> Nice. It would be great if you could report it upstream (Danny and/or
> myself can then patch it directly in guile-sqlite3 and push out a
> release) and refer to the issue from here.

with corresponding pull request

Toggle quote (3 lines)
> We can have this patch locally in the meantime, unless it would break
> once the new guile-sqlite3 is out. WDYT?

I've attached an updated patch series that both includes the
guile-sqlite3 fix as a patch to the guile-sqlite3 package and adopts the
workaround for situations where older guile-sqlite3's must be used (for
example, when building guix from scratch on foreign distros that haven't
incorporated the fix yet). The only changes are the addition of the
now-second patch and fixing up some spacing in the comment in the first
patch.

Toggle quote (18 lines)
>> From 7d34c27c33aed3e8a49b9796a62a8c19d352e653 Mon Sep 17 00:00:00 2001
>> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
>> Date: Mon, 1 Jun 2020 21:43:14 -0500
>> Subject: [PATCH 3/4] database: ensure update-or-insert is run within a
>> transaction
>>
>> update-or-insert can break if an insert occurs between when it decides whether
>> to update or insert and when it actually performs that operation. Putting the
>> check and the update/insert operation in the same transaction ensures that the
>> update/insert will only succeed if no other write has occurred in the middle.
>>
>> * guix/store/database.scm (call-with-savepoint): new procedure.
>> (update-or-insert): use call-with-savepoint to ensure the read and the
>> insert/update occur within the same transaction.
>
> That’s a bit beyond my understanding, but I think you can also push this
> one. :-)

Basically, it's like combining the body of two separate compare-and-swap
loops into a single compare-and-swap loop. This ensures that the view is
consistent (since if it isn't, the "compare" will fail and we'll
retry). It addresses a problem that doesn't exist in practice yet, since
update-or-insert is only called from within a call-with-transaction
currently. But if someone ever wanted to call it from outside of a
call-with-transaction, this would ensure that it still worked correctly.

Toggle quote (2 lines)
> Make sure “make check TESTS=tests/store-database.scm” is still happy.

Works on my system.

Related question: does berlin export /var/guix over NFS as per
that could interact poorly with our use of WAL mode:

"All processes using a database must be on the same host computer; WAL
does not work over a network filesystem." -

- reepca
From 614213c80a7ea15f7aab9502e6c33206ac089d05 Mon Sep 17 00:00:00 2001
From: Caleb Ristvedt <caleb.ristvedt@cune.org>
Date: Mon, 1 Jun 2020 18:50:07 -0500
Subject: [PATCH 1/5] database: work around guile-sqlite3 bug preventing
statement reset
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
keep re-preparing statements that are frequently used. Unfortunately it
doesn't quite emulate the semantics of sqlite_finalize properly, because it
doesn't cause a commit if the statement being finalized is the last "active"
work around this by wrapping sqlite-finalize with our own version that ensures
sqlite-reset is called, which does The Right Thing™.

* guix/store/database.scm (sqlite-finalize): new procedure that shadows the
sqlite-finalize from (sqlite3).
---
guix/store/database.scm | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

Toggle diff (45 lines)
diff --git a/guix/store/database.scm b/guix/store/database.scm
index ef52036ede..15f5791a08 100644
--- a/guix/store/database.scm
+++ b/guix/store/database.scm
@@ -130,6 +130,38 @@ transaction after it finishes."
If FILE doesn't exist, create it and initialize it as a new database."
(call-with-database file (lambda (db) exp ...)))
+(define (sqlite-finalize stmt)
+ ;; As of guile-sqlite3 0.1.0, cached statements aren't reset when
+ ;; sqlite-finalize is invoked on them (see
+ ;; https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). This can
+ ;; cause problems with automatically-started transactions:
+ ;;
+ ;; "An implicit transaction (a transaction that is started automatically,
+ ;; not a transaction started by BEGIN) is committed automatically when the
+ ;; last active statement finishes. A statement finishes when its last cursor
+ ;; closes, which is guaranteed to happen when the prepared statement is
+ ;; reset or finalized. Some statements might "finish" for the purpose of
+ ;; transaction control prior to being reset or finalized, but there is no
+ ;; guarantee of this."
+ ;;
+ ;; Thus, it's possible for an implicitly-started transaction to hang around
+ ;; until sqlite-reset is called when the cached statement is next
+ ;; used. Because the transaction is committed automatically only when the
+ ;; *last active statement* finishes, the implicitly-started transaction may
+ ;; later be upgraded to a write transaction (!) and this non-reset statement
+ ;; will still be keeping the transaction from committing until it is next
+ ;; used or the database connection is closed. This has the potential to make
+ ;; (exclusive) write access to the database necessary for much longer than
+ ;; it should be.
+ ;;
+ ;; (see https://www.sqlite.org/lang_transaction.html)
+ ;; To work around this, we wrap sqlite-finalize so that sqlite-reset is
+ ;; always called. This will continue working even when the behavior is fixed
+ ;; in guile-sqlite3, since resetting twice doesn't cause any problems. We
+ ;; can remove this once the fixed guile-sqlite3 is widespread.
+ (sqlite-reset stmt)
+ ((@ (sqlite3) sqlite-finalize) stmt))
+
(define (last-insert-row-id db)
;; XXX: (sqlite3) currently lacks bindings for 'sqlite3_last_insert_rowid'.
;; Work around that.
--
2.26.2
Attachment: 0002-gnu-guile-sqlite3-add-patch-to-fix-sqlite-finalize-b.patch
Attachment: 0003-database-rewrite-query-procedures-in-terms-of-with-s.patch
Attachment: 0004-database-ensure-update-or-insert-is-run-within-a-tra.patch
Attachment: 0005-database-separate-transaction-handling-and-retry-han.patch
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEdNapMPRLm4SepVYGwWaqSV9/GJwFAl7d0jIACgkQwWaqSV9/
GJzIwQgAiQz4aUAmaoawzhYslWzEytu5mokJi5mv4+ZRFQlI4JD4oK1hw/PjWC3X
vj05QIAfJywvw4x1mOB9p58Du+8/KV+CYaNJGoh4jyu6gxudPzdhIy7Nwxjor7v2
Lpjm6j7AHlOfWgls8oGFTzcsU70F33yD8BKEKN8Uwq5GDhWP7o1xDKOuKHsLoUg1
EBzFD90Nw3RUBtQIbPC8ep+YNIgcRuW1NXPu1wH1p8SSSOzM1WiltO5ul6mvDkc4
TXRFYq9hrvHrMb1FVMaxfLxtcdbzDl0nG0eEbKjpZIbBjEqcLia4ZZtoBBaunCTf
0qez5U5yBqivwczlFK2zqPVmBYpsuA==
=QgPR
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 9 Jun 2020 01:42
(name . Caleb Ristvedt)(address . caleb.ristvedt@cune.org)
87h7vkmy07.fsf@gnu.org
Hi,

Caleb Ristvedt <caleb.ristvedt@cune.org> skribis:


[...]

Toggle quote (8 lines)
>> Nice. It would be great if you could report it upstream (Danny and/or
>> myself can then patch it directly in guile-sqlite3 and push out a
>> release) and refer to the issue from here.
>
> Reported as https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12,
> with corresponding pull request
> https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13.

Awesome, thank you.

Toggle quote (11 lines)
>> We can have this patch locally in the meantime, unless it would break
>> once the new guile-sqlite3 is out. WDYT?
>
> I've attached an updated patch series that both includes the
> guile-sqlite3 fix as a patch to the guile-sqlite3 package and adopts the
> workaround for situations where older guile-sqlite3's must be used (for
> example, when building guix from scratch on foreign distros that haven't
> incorporated the fix yet). The only changes are the addition of the
> now-second patch and fixing up some spacing in the comment in the first
> patch.

OK.

Toggle quote (15 lines)
>>> * guix/store/database.scm (call-with-savepoint): new procedure.
>>> (update-or-insert): use call-with-savepoint to ensure the read and the
>>> insert/update occur within the same transaction.
>>
>> That’s a bit beyond my understanding, but I think you can also push this
>> one. :-)
>
> Basically, it's like combining the body of two separate compare-and-swap
> loops into a single compare-and-swap loop. This ensures that the view is
> consistent (since if it isn't, the "compare" will fail and we'll
> retry). It addresses a problem that doesn't exist in practice yet, since
> update-or-insert is only called from within a call-with-transaction
> currently. But if someone ever wanted to call it from outside of a
> call-with-transaction, this would ensure that it still worked correctly.

Makes sense, thanks for explaining.

Toggle quote (4 lines)
> Related question: does berlin export /var/guix over NFS as per
> http://hpc.guix.info/blog/2017/11/installing-guix-on-a-cluster? If so,
> that could interact poorly with our use of WAL mode:

No, it doesn’t. (Also, in the setup described above, there’s only one
guix-daemon instance and it accesses the database via the local file
system.)

Toggle quote (20 lines)
> From 614213c80a7ea15f7aab9502e6c33206ac089d05 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Mon, 1 Jun 2020 18:50:07 -0500
> Subject: [PATCH 1/5] database: work around guile-sqlite3 bug preventing
> statement reset
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> guile-sqlite3 provides statement caching, making it unnecessary for sqlite to
> keep re-preparing statements that are frequently used. Unfortunately it
> doesn't quite emulate the semantics of sqlite_finalize properly, because it
> doesn't cause a commit if the statement being finalized is the last "active"
> statement (see https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). We
> work around this by wrapping sqlite-finalize with our own version that ensures
> sqlite-reset is called, which does The Right Thing™.
>
> * guix/store/database.scm (sqlite-finalize): new procedure that shadows the
> sqlite-finalize from (sqlite3).

[...]

Toggle quote (6 lines)
> +(define (sqlite-finalize stmt)
> + ;; As of guile-sqlite3 0.1.0, cached statements aren't reset when
> + ;; sqlite-finalize is invoked on them (see
> + ;; https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12). This can
> + ;; cause problems with automatically-started transactions:

I think it’s enough to link to the upstream issue, which has the problem
well documented.

Toggle quote (16 lines)
> From e3cf7be4491f465d3041933596d3caad1ea64e83 Mon Sep 17 00:00:00 2001
> From: Caleb Ristvedt <caleb.ristvedt@cune.org>
> Date: Sun, 7 Jun 2020 22:30:41 -0500
> Subject: [PATCH 2/5] gnu: guile-sqlite3: add patch to fix sqlite-finalize bug
>
> Adds patch that fixes
> https://notabug.org/guile-sqlite3/guile-sqlite3/issues/12. This can be
> discarded once the patch is integrated into the next guile-sqlite3 release.
> Note that the patch is identical to the pull request at
> https://notabug.org/guile-sqlite3/guile-sqlite3/pulls/13.
>
> * gnu/packages/patches/guile-sqlite3-reset-on-sqlite-finalize.patch: new
> patch.
> * gnu/packages/guile.scm (guile-sqlite3): use it.
> * gnu/local.mk (dist_patch_DATA): add it.

I’d skip it: we have a workaround and the release may be out soon.

Danny, thoughts on getting a new release out?

The rest is still fine with me, thank you!

Ludo’.
L
L
Ludovic Courtès wrote on 23 Jun 2020 14:51
control message for bug #41658
(address . control@debbugs.gnu.org)
87imfhxxh6.fsf@gnu.org
tags 41658 fixed
close 41658
quit
?
Your comment

This issue is archived.

To comment on this conversation send an email to 41658@patchwise.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 41658
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch