kvdb+sqldb: run read-only Postgres transactions at REPEATABLE READ

docs/postgres.md

@@ -52,6 +52,28 @@ resource exhaustion in case LND experiencing high concurrent load:
 * `db.postgres.channeldb-with-global-lock=false` to run the channeldb_kv table
   with a single writer (default is false).
 
+## Transaction isolation
+
+`lnd` opens read-write transactions at the `SERIALIZABLE` isolation level.
+Read-only transactions are opened at `REPEATABLE READ`, which in Postgres is
+snapshot isolation: the transaction reads from a single consistent snapshot,
+taken when its first statement runs. Reads therefore acquire no `SIRead`
+predicate locks and take no part in Postgres' serializable snapshot isolation
+conflict graph, which substantially cuts the number of `40001` serialization
+failures that `lnd` and its concurrent writers have to retry through.
+
+Operators should be aware that some of `lnd`'s read transactions are long
+lived. Loading the graph cache at startup and each `GraphSession` used for
+pathfinding hold a read transaction open for their full duration, and each
+holds its snapshot for that whole time. Two consequences follow. First,
+Postgres cannot vacuum row versions that are still visible to an open snapshot,
+so a very slow or stuck read transaction delays cleanup and can bloat tables.
+Second, such a session sits in the `idle in transaction` state whenever `lnd`
+is computing between queries, so if `idle_in_transaction_session_timeout` is
+configured it must be generous enough to cover a full pathfinding pass or
+Postgres will terminate the transaction mid-flight. The same caution applies to
+`statement_timeout` for the individual queries these transactions run.
+
 ## Important note about replication
 
 In case a replication architecture is planned, streaming replication should be avoided, as the master does not verify the replica is indeed identical, but it will only forward the edits queue, and let the slave catch up autonomously; synchronous mode, albeit slower, is paramount for `lnd` data integrity across the copies, as it will finalize writes only after the slave confirmed successful replication.

docs/release-notes/release-notes-0.22.0.md

@@ -96,6 +96,23 @@
 
 ## Performance Improvements
 
+* [Read-only Postgres transactions now run at `REPEATABLE READ` instead of
+  `SERIALIZABLE`](https://github.com/lightningnetwork/lnd/pull/10997). In
+  Postgres that is snapshot isolation: a read-only transaction still reads from
+  a single consistent snapshot for its whole lifetime, taken when its first
+  statement runs. That snapshot is no longer guaranteed to correspond to a
+  serial ordering of the writers running alongside it, which is acceptable
+  because `lnd`'s read paths only consume a point-in-time view and never
+  depended on being ordered against writers in other transactions. In exchange,
+  such a transaction takes no part in Postgres' serializable snapshot isolation
+  conflict graph: it acquires no `SIRead` predicate locks, is not itself subject
+  to SSI serialization failures, and can no longer cause a concurrent writer to
+  be aborted as a pivot. Since `lnd` is very read heavy, this removes a large
+  amount of needless abort pressure. Read-write transactions are unaffected and
+  remain `SERIALIZABLE`, and the SQLite backend is untouched. See
+  [docs/postgres.md](../postgres.md) for the operator-facing note on long-lived
+  read transactions.
+
 ## Deprecations
 
 # Technical and Architectural Updates

kvdb/sqlbase/db.go

@@ -25,6 +25,11 @@ const (
 	// transaction if it fails with an error that permits transaction
 	// repetition.
 	DefaultNumTxRetries = 50
+
+	// postgresDriverName is the name of the registered SQL driver that is
+	// used whenever the shared SQL backend is backed by Postgres. It is
+	// used to select behavior that only applies to Postgres.
+	postgresDriverName = "pgx"
 )
 
 // Config holds a set of configuration options of a sql database connection.
@@ -95,6 +100,13 @@ type db struct {
 // Enforce db implements the walletdb.DB interface.
 var _ walletdb.DB = (*db)(nil)
 
+// isPostgres returns true if the backend is backed by a Postgres database. The
+// shared SQL backend is also used for SQLite, so any Postgres specific behavior
+// needs to be gated on this.
+func (d *db) isPostgres() bool {
+	return d.cfg.DriverName == postgresDriverName
+}
+
 var (
 	// dbConns is a global set of database connections.
 	dbConns   *dbConnSet

kvdb/sqlbase/readwrite_tx.go

@@ -25,6 +25,43 @@ type readWriteTx struct {
 	locker sync.Locker
 }
 
+// txIsolationLevel returns the isolation level that a transaction against the
+// given database should be opened with.
+//
+// Read-write transactions always run at SERIALIZABLE, since that is the level
+// the kvdb abstraction has always promised for writers. Read-only transactions
+// on Postgres are instead opened at REPEATABLE READ, which in Postgres is
+// snapshot isolation: the transaction reads from a single consistent snapshot
+// for its whole lifetime, taken when its first statement runs rather than at
+// BEGIN.
+//
+// That is a real, if modest, weakening. Snapshot isolation is not
+// serializability, so the reader is no longer guaranteed to observe a state
+// that corresponds to some serial ordering of the writers running alongside it.
+// The read-only transaction anomaly described by Fekete and O'Neil is once
+// again permitted. We accept that here because our read paths only ever consume
+// a point-in-time view of the database, much like bbolt's View transactions do,
+// and never depended on being ordered against writers in other transactions. A
+// read that feeds a later write in a separate transaction was never protected
+// across that boundary at any isolation level.
+//
+// In exchange, a read-only REPEATABLE READ transaction takes no part in
+// Postgres' serializable snapshot isolation conflict graph. It acquires no
+// SIRead predicate locks, is not itself subject to SSI serialization failures,
+// and can no longer cause a concurrent writer to be aborted as a pivot. Since
+// lnd is extremely read heavy, this removes a large amount of needless abort
+// pressure from the system.
+//
+// SQLite is always effectively serializable because it only ever admits a
+// single writer, so there is nothing to gain there and we leave it alone.
+func txIsolationLevel(db *db, readOnly bool) sql.IsolationLevel {
+	if readOnly && db.isPostgres() {
+		return sql.LevelRepeatableRead
+	}
+
+	return sql.LevelSerializable
+}
+
 // newReadWriteTx creates an rw transaction using a connection from the
 // specified pool.
 func newReadWriteTx(db *db, readOnly bool) (*readWriteTx, error) {
@@ -50,7 +87,7 @@ func newReadWriteTx(db *db, readOnly bool) (*readWriteTx, error) {
 		context.Background(),
 		&sql.TxOptions{
 			ReadOnly:  readOnly,
-			Isolation: sql.LevelSerializable,
+			Isolation: txIsolationLevel(db, readOnly),
 		},
 	)
 	if err != nil {

kvdb/sqlbase/readwrite_tx_postgres_test.go

@@ -0,0 +1,187 @@
+//go:build kvdb_postgres
+
+package sqlbase
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"github.com/btcsuite/btcwallet/walletdb"
+	embeddedpostgres "github.com/fergusstrange/embedded-postgres"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// testPgPort is the port that the embedded Postgres instance used by
+	// this package listens on. A dedicated port is used here so that this
+	// test binary can run alongside the one of the kvdb/postgres package,
+	// which brings up its own instance.
+	testPgPort = 9877
+
+	// testPgDsnTemplate is the connection string template for the embedded
+	// Postgres instance above.
+	testPgDsnTemplate = "postgres://postgres:postgres@localhost:%d/" +
+		"postgres?sslmode=disable"
+
+	// testPgMaxConnections is the maximum number of connections that the
+	// embedded Postgres instance accepts.
+	testPgMaxConnections = 20
+)
+
+// newPostgresTestBackend spins up an embedded Postgres instance and returns a
+// SQL backend that is connected to it.
+func newPostgresTestBackend(t *testing.T) *db {
+	t.Helper()
+
+	Init(testPgMaxConnections)
+
+	// Keep all of the state of the embedded instance contained in a
+	// temporary directory that is removed once the test completes.
+	runtimePath := t.TempDir()
+
+	pg := embeddedpostgres.NewDatabase(
+		embeddedpostgres.DefaultConfig().
+			Port(testPgPort).
+			RuntimePath(runtimePath).
+			DataPath(filepath.Join(runtimePath, "data")).
+			Logger(io.Discard).
+			StartParameters(map[string]string{
+				"max_connections": fmt.Sprintf(
+					"%d", testPgMaxConnections,
+				),
+			}),
+	)
+	require.NoError(t, pg.Start())
+	t.Cleanup(func() {
+		require.NoError(t, pg.Stop())
+	})
+
+	backend, err := NewSqlBackend(context.Background(), &Config{
+		DriverName:      postgresDriverName,
+		Dsn:             fmt.Sprintf(testPgDsnTemplate, testPgPort),
+		Timeout:         time.Minute,
+		Schema:          "public",
+		TableNamePrefix: "test",
+		SQLiteCmdReplacements: SQLiteCmdReplacements{
+			"BLOB":                "BYTEA",
+			"INTEGER PRIMARY KEY": "BIGSERIAL PRIMARY KEY",
+		},
+	})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		require.NoError(t, backend.Close())
+	})
+
+	return backend
+}
+
+// TestPostgresTxIsolationLevel asserts that the isolation level that Postgres
+// itself reports for a transaction matches what we expect: read-only
+// transactions run at repeatable read while read-write transactions remain
+// serializable. We also assert the read-only flag that Postgres reports, so
+// that dropping it from the tx options would be caught here as well.
+func TestPostgresTxIsolationLevel(t *testing.T) {
+	backend := newPostgresTestBackend(t)
+
+	tests := []struct {
+		name         string
+		readOnly     bool
+		expected     string
+		expectedFlag string
+	}{
+		{
+			name:         "read-only",
+			readOnly:     true,
+			expected:     "repeatable read",
+			expectedFlag: "on",
+		},
+		{
+			name:         "read-write",
+			readOnly:     false,
+			expected:     "serializable",
+			expectedFlag: "off",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			tx, err := newReadWriteTx(backend, test.readOnly)
+			require.NoError(t, err)
+			defer func() {
+				require.NoError(t, tx.Rollback())
+			}()
+
+			var level string
+			row := tx.tx.QueryRow("SHOW transaction_isolation")
+			require.NoError(t, row.Scan(&level))
+			require.Equal(t, test.expected, level)
+
+			var readOnly string
+			row = tx.tx.QueryRow("SHOW transaction_read_only")
+			require.NoError(t, row.Scan(&readOnly))
+			require.Equal(t, test.expectedFlag, readOnly)
+		})
+	}
+}
+
+// TestPostgresReadTxSnapshotStability asserts the property that the relaxed
+// isolation level is chosen for, rather than just the knob itself: a read-only
+// transaction keeps reading from the snapshot it started with, even after a
+// concurrent writer on a different connection has committed over the same key.
+func TestPostgresReadTxSnapshotStability(t *testing.T) {
+	backend := newPostgresTestBackend(t)
+
+	var (
+		bucketKey = []byte("snapshot")
+		key       = []byte("key")
+		before    = []byte("before")
+		after     = []byte("after")
+	)
+
+	// Seed the key with its original value.
+	err := backend.Update(func(tx walletdb.ReadWriteTx) error {
+		bucket, err := tx.CreateTopLevelBucket(bucketKey)
+		if err != nil {
+			return err
+		}
+
+		return bucket.Put(key, before)
+	}, func() {})
+	require.NoError(t, err)
+
+	readTx, err := newReadWriteTx(backend, true)
+	require.NoError(t, err)
+	defer func() {
+		require.NoError(t, readTx.Rollback())
+	}()
+
+	// Take the first read. Note that this is what pins the snapshot, since
+	// Postgres only acquires it once the first statement of a repeatable
+	// read transaction runs, not at BEGIN.
+	readBucket := readTx.ReadBucket(bucketKey)
+	require.NotNil(t, readBucket)
+	require.Equal(t, before, readBucket.Get(key))
+
+	// Now overwrite the key and commit, using a separate connection from
+	// the pool.
+	err = backend.Update(func(tx walletdb.ReadWriteTx) error {
+		return tx.ReadWriteBucket(bucketKey).Put(key, after)
+	}, func() {})
+	require.NoError(t, err)
+
+	// The write is visible to anyone starting fresh.
+	err = backend.View(func(tx walletdb.ReadTx) error {
+		require.Equal(t, after, tx.ReadBucket(bucketKey).Get(key))
+
+		return nil
+	}, func() {})
+	require.NoError(t, err)
+
+	// The long lived read transaction, however, must still observe the
+	// value that its snapshot was taken at.
+	require.Equal(t, before, readTx.ReadBucket(bucketKey).Get(key))
+}