From 1d1fd6e6865aa512c0e028e4688ef4908f5dd97a Mon Sep 17 00:00:00 2001
From: Ben Johnson <benbjohnson@yahoo.com>
Date: Thu, 22 Apr 2021 16:35:04 -0600
Subject: [PATCH] Remove SQLite write lock during WAL sync (again)

This commit reattempts a change to remove the write lock that was
previously tried in 998e831. This change will reduce the number of
locks on the database which should help reduce error messages that
applications see when they do not have busy_timeout set.

In addition to the lock removal, a passive checkpoint is issued
immediately before the read lock is obtained to prevent additional
checkpoints by the application itself. SQLite does not support
checkpoints from an active transaction so it cannot be done afterward.
---
 db.go      | 26 --------------------------
 replica.go |  5 +++++
 s3/s3.go   |  5 +++++
 3 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/db.go b/db.go
index 1fea632..f62ee0d 100644
--- a/db.go
+++ b/db.go
@@ -751,27 +751,6 @@ func (db *DB) Sync(ctx context.Context) (err error) {
 		return fmt.Errorf("ensure wal exists: %w", err)
 	}
 
-	// Start a transaction. This will be promoted immediately after.
-	tx, err := db.db.Begin()
-	if err != nil {
-		return fmt.Errorf("begin: %w", err)
-	}
-
-	// Ensure write transaction rolls back before returning.
-	defer func() {
-		if e := rollback(tx); e != nil && err == nil {
-			err = e
-		}
-	}()
-
-	// Insert into the lock table to promote to a write tx. The lock table
-	// insert will never actually occur because our tx will be rolled back,
-	// however, it will ensure our tx grabs the write lock. Unfortunately,
-	// we can't call "BEGIN IMMEDIATE" as we are already in a transaction.
-	if _, err := tx.ExecContext(ctx, `INSERT INTO _litestream_lock (id) VALUES (1);`); err != nil {
-		return fmt.Errorf("_litestream_lock: %w", err)
-	}
-
 	// Verify our last sync matches the current state of the WAL.
 	// This ensures that we have an existing generation & that the last sync
 	// position of the real WAL hasn't been overwritten by another process.
@@ -818,11 +797,6 @@ func (db *DB) Sync(ctx context.Context) (err error) {
 		checkpoint = true
 	}
 
-	// Release write lock before checkpointing & exiting.
-	if err := tx.Rollback(); err != nil {
-		return fmt.Errorf("rollback write tx: %w", err)
-	}
-
 	// Issue the checkpoint.
 	if checkpoint {
 		changed = true
diff --git a/replica.go b/replica.go
index 9d2cc47..0435315 100644
--- a/replica.go
+++ b/replica.go
@@ -607,6 +607,11 @@ func (r *FileReplica) snapshot(ctx context.Context, generation string, index int
 	r.muf.Lock()
 	defer r.muf.Unlock()
 
+	// Issue a passive checkpoint to flush any pages to disk before snapshotting.
+	if _, err := r.db.db.ExecContext(ctx, `PRAGMA wal_checkpoint(PASSIVE);`); err != nil {
+		return fmt.Errorf("pre-snapshot checkpoint: %w", err)
+	}
+
 	// Acquire a read lock on the database during snapshot to prevent checkpoints.
 	tx, err := r.db.db.Begin()
 	if err != nil {
diff --git a/s3/s3.go b/s3/s3.go
index 2878b86..cdcab6f 100644
--- a/s3/s3.go
+++ b/s3/s3.go
@@ -649,6 +649,11 @@ func (r *Replica) snapshot(ctx context.Context, generation string, index int) er
 	r.muf.Lock()
 	defer r.muf.Unlock()
 
+	// Issue a passive checkpoint to flush any pages to disk before snapshotting.
+	if _, err := r.db.SQLDB().ExecContext(ctx, `PRAGMA wal_checkpoint(PASSIVE);`); err != nil {
+		return fmt.Errorf("pre-snapshot checkpoint: %w", err)
+	}
+
 	// Acquire a read lock on the database during snapshot to prevent checkpoints.
 	tx, err := r.db.SQLDB().Begin()
 	if err != nil {