Merge pull request #1058 from ahrtr/20250818_writeto_1.4

[release-1.4] Update (*Tx)WriteTo to reuse the already opened file if WriteFlag not set

Author: ahrtr

db_test.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"runtime"
 	"strings"
 	"sync"
 	"testing"
@@ -760,6 +761,114 @@ func TestDB_Concurrent_WriteTo_and_ConsistentRead(t *testing.T) {
 	}
 }
 
+// TestDB_WriteTo_and_Overwrite verifies that `(tx *Tx) WriteTo` can still
+// work even the underlying file is overwritten between the time a read-only
+// transaction is created and the time the file is actually opened
+func TestDB_WriteTo_and_Overwrite(t *testing.T) {
+	testCases := []struct {
+		name      string
+		writeFlag int
+	}{
+		{
+			name:      "writeFlag not set",
+			writeFlag: 0,
+		},
+		/* syscall.O_DIRECT not supported on some platforms, i.e. Windows and MacOS
+		{
+			name:      "writeFlag set",
+			writeFlag: syscall.O_DIRECT,
+		},*/
+	}
+
+	fRead := func(db *bolt.DB, bucketName []byte) map[string]string {
+		data := make(map[string]string)
+		_ = db.View(func(tx *bolt.Tx) error {
+			b := tx.Bucket(bucketName)
+			berr := b.ForEach(func(k, v []byte) error {
+				data[string(k)] = string(v)
+				return nil
+			})
+			require.NoError(t, berr)
+			return nil
+		})
+		return data
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			db := btesting.MustCreateDBWithOption(t, &bolt.Options{
+				PageSize: 4096,
+			})
+			filePathOfDb := db.Path()
+
+			var (
+				bucketName   = []byte("data")
+				dataExpected map[string]string
+				dataActual   map[string]string
+			)
+
+			t.Log("Populate some data")
+			err := db.Update(func(tx *bolt.Tx) error {
+				b, berr := tx.CreateBucket(bucketName)
+				if berr != nil {
+					return berr
+				}
+				for k := 0; k < 10; k++ {
+					key, value := fmt.Sprintf("key_%d", rand.Intn(10)), fmt.Sprintf("value_%d", rand.Intn(100))
+					if perr := b.Put([]byte(key), []byte(value)); perr != nil {
+						return perr
+					}
+				}
+				return nil
+			})
+			require.NoError(t, err)
+
+			t.Log("Read all the data before calling WriteTo")
+			dataExpected = fRead(db.DB, bucketName)
+
+			t.Log("Create a readonly transaction for WriteTo")
+			rtx, rerr := db.Begin(false)
+			require.NoError(t, rerr)
+
+			// Some platforms (i.e. Windows) don't support renaming a file
+			// when the target file already exist and is opened.
+			if runtime.GOOS == "linux" {
+				t.Log("Create another empty db file")
+				db2 := btesting.MustCreateDBWithOption(t, &bolt.Options{
+					PageSize: 4096,
+				})
+				db2.MustClose()
+				filePathOfDb2 := db2.Path()
+
+				t.Logf("Renaming the new empty db file (%s) to the original db path (%s)", filePathOfDb2, filePathOfDb)
+				err = os.Rename(filePathOfDb2, filePathOfDb)
+				require.NoError(t, err)
+			} else {
+				t.Log("Ignore renaming step on non-Linux platform")
+			}
+
+			t.Logf("Call WriteTo to copy the data of the original db file")
+			f := filepath.Join(t.TempDir(), "-backup-db")
+			err = rtx.CopyFile(f, 0600)
+			require.NoError(t, err)
+			require.NoError(t, rtx.Rollback())
+
+			t.Logf("Read all the data from the backup db after calling WriteTo")
+			newDB, err := bolt.Open(f, 0600, &bolt.Options{
+				ReadOnly: true,
+			})
+			require.NoError(t, err)
+			dataActual = fRead(newDB, bucketName)
+			err = newDB.Close()
+			require.NoError(t, err)
+
+			t.Log("Compare the dataExpected and dataActual")
+			same := reflect.DeepEqual(dataExpected, dataActual)
+			require.True(t, same, fmt.Sprintf("found inconsistent data, dataExpected: %v, ddataActual : %v", dataExpected, dataActual))
+		})
+	}
+}
+
 // Ensure that opening a transaction while the DB is closed returns an error.
 func TestDB_BeginRW_Closed(t *testing.T) {
 	var db bolt.DB

tx.go

@@ -387,16 +387,43 @@ func (tx *Tx) Copy(w io.Writer) error {
 // WriteTo writes the entire database to a writer.
 // If err == nil then exactly tx.Size() bytes will be written into the writer.
 func (tx *Tx) WriteTo(w io.Writer) (n int64, err error) {
-	// Attempt to open reader with WriteFlag
-	f, err := tx.db.openFile(tx.db.path, os.O_RDONLY|tx.WriteFlag, 0)
-	if err != nil {
-		return 0, err
-	}
-	defer func() {
-		if cerr := f.Close(); err == nil {
-			err = cerr
+	var f *os.File
+	// There is a risk that between the time a read-only transaction
+	// is created and the time the file is actually opened, the
+	// underlying db file at tx.db.path may have been replaced
+	// (e.g. via rename). In that case, opening the file again would
+	// unexpectedly point to a different file, rather than the one
+	// the transaction was based on.
+	//
+	// To overcome this, we reuse the already opened file handle when
+	// WritFlag not set. When the WriteFlag is set, we reopen the file
+	// but verify that it still refers to the same underlying file
+	// (by device and inode). If it does not, we fall back to
+	// reusing the existing already opened file handle.
+	if tx.WriteFlag != 0 {
+		// Attempt to open reader with WriteFlag
+		f, err = tx.db.openFile(tx.db.path, os.O_RDONLY|tx.WriteFlag, 0)
+		if err != nil {
+			return 0, err
 		}
-	}()
+
+		if ok, err := sameFile(tx.db.file, f); !ok {
+			lg := tx.db.Logger()
+			if cerr := f.Close(); cerr != nil {
+				lg.Errorf("failed to close the file (%s): %v", tx.db.path, cerr)
+			}
+			lg.Warningf("The underlying file has changed, so reuse the already opened file (%s): %v", tx.db.path, err)
+			f = tx.db.file
+		} else {
+			defer func() {
+				if cerr := f.Close(); err == nil {
+					err = cerr
+				}
+			}()
+		}
+	} else {
+		f = tx.db.file
+	}
 
 	// Generate a meta page. We use the same page data for both meta pages.
 	buf := make([]byte, tx.db.pageSize)
@@ -423,13 +450,13 @@ func (tx *Tx) WriteTo(w io.Writer) (n int64, err error) {
 		return n, fmt.Errorf("meta 1 copy: %s", err)
 	}
 
-	// Move past the meta pages in the file.
-	if _, err := f.Seek(int64(tx.db.pageSize*2), io.SeekStart); err != nil {
-		return n, fmt.Errorf("seek: %s", err)
-	}
+	// Copy data pages using a SectionReader to avoid affecting f's offset.
+	dataOffset := int64(tx.db.pageSize * 2)
+	dataSize := tx.Size() - dataOffset
+	sr := io.NewSectionReader(f, dataOffset, dataSize)
 
 	// Copy data pages.
-	wn, err := io.CopyN(w, f, tx.Size()-int64(tx.db.pageSize*2))
+	wn, err := io.CopyN(w, sr, dataSize)
 	n += wn
 	if err != nil {
 		return n, err
@@ -438,6 +465,19 @@ func (tx *Tx) WriteTo(w io.Writer) (n int64, err error) {
 	return n, nil
 }
 
+func sameFile(f1, f2 *os.File) (bool, error) {
+	fi1, err := f1.Stat()
+	if err != nil {
+		return false, fmt.Errorf("failed to get fileInfo of the first file (%s): %w", f1.Name(), err)
+	}
+	fi2, err := f2.Stat()
+	if err != nil {
+		return false, fmt.Errorf("failed to get fileInfo of the second file (%s): %w", f2.Name(), err)
+	}
+
+	return os.SameFile(fi1, fi2), nil
+}
+
 // CopyFile copies the entire database to file at the given path.
 // A reader transaction is maintained during the copy so it is safe to continue
 // using the database while a copy is in progress.