multi: add opt-in repeatable read isolation for Postgres write transactions - #10999
Open
Roasbeef wants to merge 16 commits into
Open
multi: add opt-in repeatable read isolation for Postgres write transactions#10999Roasbeef wants to merge 16 commits into
Roasbeef wants to merge 16 commits into
Conversation
In this commit, we relax the isolation level used for read-only transactions on the shared SQL kvdb backend when it's backed by Postgres. Read-write transactions are untouched and remain SERIALIZABLE. lnd currently opens every transaction at SERIALIZABLE. Under Postgres' serializable snapshot isolation, even a read-only transaction takes SIRead predicate locks and fully participates in the serialization conflict graph, so it can both suffer a 40001 abort and cause one for a concurrent writer by acting as a pivot. Since lnd is extremely read heavy, that adds up to a lot of needless abort pressure. REPEATABLE READ in Postgres is snapshot isolation. A read-only transaction still reads from a single consistent snapshot for its whole lifetime, taken when its first statement runs rather than at BEGIN. This is a genuine, if modest, weakening: that snapshot is no longer guaranteed to correspond to a serial ordering of the writers running alongside it, so the read-only transaction anomaly of Fekete and O'Neil becomes possible again. We're fine with that, because these read paths only ever consume a point-in-time view of the database, much like bbolt's View transactions do, and a read feeding a later write in a separate transaction was never protected across that boundary at any isolation level. What we get in return is that the transaction takes no part in the SSI conflict graph at all: no SIRead predicate locks, no exposure to SSI serialization failures, and no chance of aborting a concurrent writer as a pivot. The sqlbase package is shared with the SQLite backend, so we gate this on the driver name. SQLite is always effectively serializable and its driver ignores the requested isolation level entirely, so there's nothing to gain there.
In this commit, we apply the same isolation level change to the native SQL backend that the previous commit applied to the kvdb SQL backend: read-only transactions on Postgres now run at REPEATABLE READ, while read-write transactions stay SERIALIZABLE. A read-only transaction still reads from a single consistent snapshot for its whole lifetime, taken at its first statement. It gives up the guarantee that the snapshot corresponds to a serial ordering of the concurrent writers, which these read paths never relied on, and in return stops participating in Postgres' serializable snapshot isolation conflict graph: no SIRead predicate locks, no exposure to SSI serialization failures, and no aborting a concurrent writer as a pivot. BaseDB is also used for SQLite, so it needs to know which backend it's talking to. We add a BackendType to BaseDB mirroring the one that sqldb/v2 already carries, and set it at the two construction sites.
In this commit, we mirror the isolation level change into sqldb/v2. BaseDB already carries a BackendType here, so we can gate directly on it: read-only transactions on Postgres are opened at REPEATABLE READ, everything else stays SERIALIZABLE.
In this commit, we add the release notes entry for the isolation level change, along with an operator facing note in docs/postgres.md. The latter spells out that reads now run under snapshot isolation, and warns that lnd holds some read transactions open for a long time. The graph cache load and each pathfinding GraphSession keep a snapshot pinned for their whole duration, which is worth knowing before setting idle_in_transaction_session_timeout or statement_timeout.
In this commit, we make syncNewChannel write the peer's link node row unconditionally, rather than returning early when a link node already exists for that peer. The link node prune paths (MarkChanFullyClosed and pruneLinkNode) read the peer's set of open channels and, when that set is empty, delete the peer's link node. A concurrent channel open reads the link node row, sees that it's already there, and skips the write. Each transaction reads what the other writes, but their write sets are disjoint, which is the classic write skew shape: under snapshot isolation both transactions commit, and we're left with an open channel whose peer has no link node. Serializable isolation catches this today, but we're preparing to run write transactions at repeatable read, so instead we make the two transactions collide on the same row. The write is a verbatim re-write of the existing record, so any state that has accumulated for the link node (extra addresses, last seen time) is preserved exactly, and the only observable difference is that one of the two racing transactions now aborts with a retryable serialization error. We also update the isolation assumptions spelled out in the comments of the two prune paths.
In this commit, we make PruneGraphNodes acquire the store's cache mutex before it opens its write transaction. It was the only mutator of the graph that didn't, in either of the two store implementations, while all of its siblings already do. That mutex does more than guard the reject and channel caches: it's also the in-process serialization point against the batched channel edge insertion path. addChannelEdge reads the row of each node an edge attaches to, but doesn't write it. PruneGraphNodes range scans the node bucket and then deletes the nodes it finds to be unreferenced. With no mutual exclusion, the two can interleave such that the prune deletes a node that the edge being added still references, which leaves a dangling edge in the graph. Under serializable isolation the database would abort one of the two, but under snapshot isolation their write sets are disjoint and both commit. The lock ordering here is cacheMu -> DB, which is what the rest of both stores already does. The in-memory graph cache is updated by the ChannelGraph wrapper once the store call returns, exactly as it is for PruneGraph, so nothing further is needed here.
In this commit, we rework the bucket creation path of the SQL kvdb backends so that two transactions racing to create the same bucket no longer leave the loser with a unique constraint violation. Both CreateBucket and CreateBucketIfNotExists used to select the row for the key, and then, if nothing came back, run a bare insert. Under serializable isolation that's fine: the select takes a predicate lock on the index page, so the concurrent insert of the same key shows up as a read/write conflict and the loser is aborted with a retryable serialization failure. Under repeatable read there is no predicate lock, so the loser instead runs head first into the unique index and gets back a 23505 unique violation. MapSQLError turns that into ErrSQLUniqueConstraintViolation, which ExecuteSQLTransactionWithRetry does not retry, so it would be handed straight back to the caller. Since CreateTopLevelBucket delegates here, that's a hard error on paths as central as opening a channel. We instead phrase the insert as an upsert whose update is a no-op. The conflict is then reported as a serialization failure at both isolation levels, which the retry loop knows how to handle: on the retry, the select at the top finds the winner's row. The conflict target of each statement has to line up with the partial unique index that covers the row being inserted, of which there are two, one for top level rows and one for nested rows, so we keep the two flavours of the statement apart just like Put already does. The walletdb semantics are unchanged: CreateBucketIfNotExists returns the existing bucket, CreateBucket returns ErrBucketExists when the bucket is already there (including when it lost the race, since the retry then finds it), and both return ErrIncompatibleValue when the key holds a value rather than a bucket. Two tests are added against the embedded postgres fixture. The first races several transactions creating the same bucket through the real code path and asserts that they all succeed with a single row created. The second races the raw statements at both serializable and repeatable read, and pins down the failure modes: the bare insert yields 23505 under repeatable read, while the upsert yields 40001 everywhere.
In this commit, we close a hole in the bookkeeping that decides when a
watchtower client session may be deleted. A session is only ever deleted
once it has been found closable, and closability is only ever evaluated
in MarkChannelClosed, which walks the set of sessions that have acked an
update for the channel being closed. A session joins that set the first
time it acks an update for the channel, from its own sessionQueue
goroutine, which shares no lock with the channel close path.
So a session that acks its first update for a channel after that channel
was closed is never evaluated for it. If that channel was the last open
channel of the session, then nothing will ever mark the session closable
and it holds on to the tower's storage forever. This is reachable today
purely on ordering, without any concurrency involved.
We fix it by evaluating the session in AckUpdate itself whenever the
channel it acked an update for is already closed. Like the rogue update
path right above it, this only records the session in the closable
sessions bucket, so it is picked up by the closable session handler on
the next startup rather than being handed to the caller. The evaluation
is best effort: an AckUpdate that fails takes down the session queue
that issued it, and the ack itself must be persisted no matter what, so
a failure to work out whether the session has become closable is logged
rather than returned.
The rogue update branch has the same hole, and it isn't enough there to
only consider a session when the rogue count saturates it. A rogue ack
that exhausts a session whose other channels have all been closed makes
that session closable just as well, so we let that branch fall through
to the same evaluation, which subsumes the marking it did inline.
That leaves the concurrent version of the same problem. The channel's
set of sessions is only read by MarkChannelClosed and only added to by
AckUpdate, and the closed height is the other way around, so the two
transactions read what the other writes while writing disjoint rows.
That's write skew: serializable isolation aborts one of them, but under
snapshot isolation both commit, each having assumed the other would
evaluate the session. The same shape exists one level down, between
MarkChannelClosed's evaluation of a session and the concurrent
CommitUpdate or AckUpdate that changes the very state that evaluation
reads. There the consequence is worse than a leak: a session could be
marked closable off a view that has already been invalidated, and then
deleted while it still has updates to make good on.
Both are turned into same-row conflicts by writing a row back with the
value it already holds, which puts a row that a transaction would
otherwise only read into its write set:
- AckUpdate and MarkChannelClosed both write the channel's db-ID row,
so a close can't race the addition of a session to the channel.
- MarkChannelClosed writes the body row of every session it evaluates,
which is the row that CommitUpdate and AckUpdate both write, so an
evaluation can't race a change to the session it is evaluating.
The loser of either race is aborted with a retryable serialization
error, and on the retry it observes the state the winner left behind:
AckUpdate then sees the channel as closed and evaluates its own session,
while MarkChannelClosed sees the new session in the channel's set.
Making retries of AckUpdate more likely brings a pre-existing problem
with it, which we fix here as well. RangeIndex.Add applies its changes
to the in-memory range index as soon as the key/value changes have been
staged, but that index is cached on the ClientDB and outlives the
transaction. A retry on top of an index that a rolled back attempt has
already mutated finds the height covered, stages nothing, and commits an
ack that made it into every part of the database except the range index
it belongs in, after which IsAcked reports an update as backed up that
isn't. AckUpdate now drops the range index it dirtied before each retry
and after giving up for good, so the next reader loads it back from the
database.
The invariant the two of them now maintain together is spelled out on
MarkChannelClosed.
In this commit, we fold the Detail field of a Postgres error into the error we hand back from MapSQLError, but only for the errors that report a transaction conflict. The error string that the driver builds leaves that field out, which is unfortunate because for a conflict it is the only thing that says which kind of conflict was actually hit. A plain write-write conflict reports a concurrent update, whereas an abort that only serializable isolation raises spells out the reason code of the pivot that was cancelled. Being able to tell the two apart in the logs tells an operator how much of their abort pressure would go away by running write transactions at repeatable read, and how much of it is real contention that would remain. The detail of any other error is deliberately left alone. For a constraint violation in particular, Postgres spells out the offending column values, and in our schemas that means payment session keys, invoice hashes and raw key/value bucket keys. None of that belongs in a log line, let alone in an error that may travel back over an RPC. The error parsing exists twice in each module, once for the builds that have SQLite and once for the ones that don't, so the helper is put in a file of its own that carries no build tags rather than being written out twice. While here, the copy that predates SQLite is brought back in line with its twin, which it had drifted from: it did not classify a failed SQL transaction or a detected deadlock as a serialization error at all, so neither would have been retried on the platforms that use it.
In this commit, we add a release notes entry for the four write paths that were hardened so that read-write Postgres transactions can move to repeatable read, which is the other half of the change that already put read-only transactions there in lightningnetwork#10997.
In this commit, we make KVStore.DeleteNode acquire the store's cache mutex before it opens its write transaction, closing the last mutator of the graph that didn't. This is the same shape as the PruneGraphNodes fix in the previous commits: the mutex is not just there to guard the reject and channel caches, it's also the in-process serialization point against the batched channel edge insertion path. addChannelEdge reads the row of each node an edge attaches to, but never writes it. DeleteNode removes a node row outright. Without mutual exclusion the two can interleave such that the delete removes a node that the edge being added still references, which leaves a dangling edge in the graph. Under serializable isolation the database aborts one of the two, but under snapshot isolation their write sets are disjoint and both commit. DeleteNode is currently only reachable from tests, since the sole caller of VersionedGraph.DeleteNode is test code, so this isn't a live bug. We're closing it anyway so that every mutator on the store follows the same rule, and so that a future production caller doesn't reintroduce the hazard. The lock ordering here is cacheMu -> DB, which is what the rest of the store already does. The in-memory graph cache is updated by the VersionedGraph wrapper once the store call returns, so nothing further is needed here.
In this commit, we add a db.postgres.tx-isolation option that selects the isolation level read-write Postgres transactions are opened with. It takes 'serializable', which is the default and what lnd has always used, or 'repeatable-read'. Read-only transactions already run at REPEATABLE READ and are unaffected by it. Under SERIALIZABLE, Postgres aborts any pair of transactions whose interleaving isn't equivalent to running them one after the other, and on a busy node that costs a lot of retries. REPEATABLE READ on Postgres is snapshot isolation, which still rules out dirty reads, non-repeatable reads, phantom reads and lost updates: a transaction that writes a row another in-flight transaction has already written is aborted with a serialization failure. The one anomaly class that remains is write skew, where two transactions each read what the other writes but write disjoint sets of rows, so neither conflicts and both commit. The write paths known to be exposed to that were hardened in the preceding commits, which is what makes offering this at all defensible. The option is validated in PostgresConfig.Validate, which rejects anything other than the two levels we support, and by a choice tag so that the flag parser catches a typo before we ever get that far. An empty value is accepted and means the default, so that a config built programmatically doesn't have to spell it out. The knob is threaded to the single txIsolationLevel decision point via a new field on BaseDB rather than being consulted at the call sites. It stays off by default until it has accumulated soak time on real nodes.
In this commit, we mirror the tx-isolation option into sqldb/v2, which carries its own copy of the Postgres config and of the BaseDB that opens transactions. The shape is identical to the previous commit: the config accepts 'serializable' or 'repeatable-read', validates the value, and hands a bool to the txIsolationLevel helper through BaseDB. Nothing in lnd routes through sqldb/v2 yet, but keeping the two in step means the option doesn't silently stop working as subsystems are migrated over.
In this commit, we teach the shared SQL kvdb backend about the new tx-isolation option. The sqlbase config grows a WriteTxRepeatableRead flag that the Postgres backend fills in from its own config, and the txIsolationLevel helper consults it when deciding what level to open a read-write transaction at. The helper is also restructured slightly so that the "not Postgres" case is answered first. SQLite is always effectively serializable because it only ever admits a single writer and its driver ignores the requested level entirely, so the option must never reach it. Note that the bulk migration path in migration_bulk_postgres.go picks its own isolation levels and doesn't go through this helper, so it is unaffected.
In this commit, we wire the new option into lnd's own config. The default DB config pins it to 'serializable' so that the level lnd runs at doesn't change for anyone who doesn't ask for it, and GetPostgresConfigKVDB carries the value across into the kvdb flavored Postgres config so that the kvdb SQL backends see it too. The native SQL store already receives the sqldb config verbatim, so nothing is needed there.
In this commit, we document the new db.postgres.tx-isolation option in docs/postgres.md and in the sample config, and add release notes for it along with the DeleteNode cache mutex fix. The docs spell out what REPEATABLE READ does and does not rule out, so that anyone reaching for the option knows that write skew is the class of anomaly they're taking on, and that it's only safe because the write paths exposed to it were hardened first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we add the final piece of the Postgres isolation series: an opt-in
db.postgres.tx-isolationoption that runs write transactions atREPEATABLE READinstead ofSERIALIZABLE. This is stacked on #10998 (which is stacked on #10997), so only the last six commits are new here until those land.The background: under
SERIALIZABLE, Postgres runs SSI and aborts transactions on rw-antidependency cycles, including false positives from predicate-lock range scans, which is the abort pressure behind reports like #10995. #10997 already moves read-only transactions toREPEATABLE READ, and #10998 hardens the handful of write paths that a full audit of the codebase flagged as exposed to write skew, the one anomaly class that snapshot isolation admits. With those in place, the remaining step is letting operators actually turn the dial for writes, which this PR does.The option is a named enum (
serializable, the default, orrepeatable-read) rather than a boolean, since it names the thing it controls the same way Postgres does and leaves room for finer levels later. It's validated twice: a go-flagschoicetag rejects typos at parse time, andPostgresConfig.Validate()covers programmatically built configs. The value plumbs through to the threetxIsolationLevelhelpers the earlier PRs introduced, each restructured to answer the non-Postgres case first, so the knob structurally cannot reach SQLite. The option stays experimental and opt-in until it has accumulated soak time on real nodes;docs/postgres.mdgains a section explaining exactly what is and isn't given up, and what to do if an operator observes trouble.We also close the last known gap from the audit:
KVStore.DeleteNodewas the one remaining graph mutator that didn't takecacheMubefore its transaction (currently unreachable in production, but the same dangling-edge shape that #10998 fixed forPruneGraphNodes), and it now takes the mutex like its siblings.The isolation tests from the earlier PRs are parametrized over the knob:
SHOW transaction_isolationandSHOW transaction_read_onlyare asserted against a real Postgres for all four (read-only x knob) combinations at all three layers, alongside negative cases proving SQLite and unrecognized backends stay at the safe default regardless of the knob. A config test drives the real go-flags parser end to end.See each commit message for a detailed description w.r.t the incremental changes.
Related to #10995.