db: run read-only Postgres transactions at REPEATABLE READ - #1057
Conversation
In this commit, we relax the isolation level for read-only Postgres transactions from SERIALIZABLE down to REPEATABLE READ. Read-write transactions are untouched and stay SERIALIZABLE. Every transaction we open today runs 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 itself and act as the pivot that causes a concurrent writer to be aborted. The daemon is extremely read heavy, so that adds up to a lot of needless abort pressure. On signet we measured 2,035 transaction commits per second with 99.6% of them changing nothing at all, against a Postgres instance sitting at 794m of a one core limit largely on predicate-lock bookkeeping. A read-only REPEATABLE READ transaction observes a single consistent snapshot taken when its first statement runs, which is precisely what our read paths already consume. What changes is that such a transaction takes no predicate locks, can never fail with a serialization error, and no longer appears in anyone else's conflict graph. Note that the READ ONLY access mode is not new here. We were already passing it through, so Postgres was already refusing writes in these transactions. That matters because Postgres only skips predicate lock acquisition for a transaction that is genuinely declared read only, so the access mode we already had is what makes the relaxed isolation level worth anything at all. This step stands on its own. Writers keep talking to each other through the full SSI conflict graph exactly as before, because Postgres tracks dependencies only among serializable transactions and a read-only REPEATABLE READ transaction was never able to protect a write in a different transaction anyway. The isolation choice funnels through a small txIsolationLevel helper that carries the reasoning. BaseDB also backs SQLite, where isolation levels behave differently and there is nothing to gain because the engine only ever admits a single writer, so the helper gates on the BackendType that BaseDB already carries and leaves every other combination fully serializable.
In this commit, we include the ConstraintName and Detail fields of a Postgres error in the message our mapped error types render, and add an IsUniqueConstraintViolation predicate to sit alongside the existing serialization and deadlock classifiers. The Error method of pgconn.PgError prints only the severity, the message and the SQLSTATE code, so both fields were being dropped on the floor before they ever reached a log line. That is a problem in two directions. The detail is the only thing that tells two very different 40001 aborts apart. A true serializable snapshot isolation abort carries a reason code naming the transaction's role in the conflict graph, while an ordinary write-write conflict on the same row carries no detail at all. Now that read-only transactions no longer take predicate locks, this is the signal that says whether a given write path still depends on SSI or would be equally happy at REPEATABLE READ, which is what we need in order to measure the effect of that change. The constraint name matters because the schema carries six partial unique indexes. Without it a 23505 raised by any of them is indistinguishable from a 23505 raised by the table's primary key, and telling those apart is the whole diagnostic value of the error. The new tests pin down the three conflict shapes that this work turns on, each against a real Postgres. A serializable read-write dependency cycle aborts with a reason code. A REPEATABLE READ write-write conflict on the same row still aborts as a retryable 40001 but carries no detail, which is the property that lets a shared row stand in for an SSI dependency. A REPEATABLE READ lost creation race does not abort at all and instead raises a 23505 that our retry predicate correctly refuses to retry.
In this commit, we write down the isolation level policy for the Postgres backend, the reasoning that produced it, and the audit of the write paths that decided where the boundary sits. The policy itself is two lines of code, but the reasoning behind it is not, and most of that reasoning is about paths the code does not touch. Read-only transactions now run at REPEATABLE READ. Writers stay at SERIALIZABLE, and this explains why they should stay there for now. The short version of the argument is that the upside of relaxing writers is small and the risk is not. Writers are a rounding error in this workload, so relaxing the 0.4% of transactions that change anything cannot recover much of the predicate-lock cost that the read-only change already removes. Against that, an audit of all 64 write closures found roughly a dozen that genuinely lean on the serialization graph, a far denser hit rate than the comparable audit in lnd, and several of them sit in fund-critical paths. None of them is a live bug today, because SSI is currently masking every one. Two structural problems compound this, and both are recorded here. Under REPEATABLE READ the same-row 40001 becomes the only remaining conflict signal, yet the actor-delivery store has no retry loop and the generic executor skips its own retry loop whenever it joins an ambient actor transaction, which is the normal case for the stores that write most often. Separately, a lost creation race stops being a retryable 40001 and becomes a 23505 that the retry loop correctly refuses to retry. We also deliberately do not add the configuration knob that lnd shipped for this. A knob whose non-default setting silently enables about a dozen known anomalies is a loaded gun with a safety catch rather than a feature, and it should wait until the paths listed here are hardened. The audit table and the inventory of the six partial unique indexes are included so the work does not have to be redone. The index inventory matters on its own, because a conflict target that does not match a partial index predicate never fires and the upsert quietly degrades back into a plain insert.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0075c21165
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ctx, "INSERT INTO chain_info (id, chain_name, "+ | ||
| "genesis_hash) VALUES (2, 'nope', '\\x01')", |
There was a problem hiding this comment.
This test adds a literal INSERT through ExecContext, and db/sqlerrors_postgres_test.go similarly adds literal SELECT, INSERT, and UPDATE statements. The package rules prohibit raw SQL in Go and require queries to be added to the query definitions and regenerated, so these isolation probes should use generated query methods or another sanctioned fixture instead.
AGENTS.md reference: db/AGENTS.md:L114-L115
Useful? React with 👍 / 👎.
| func PgErrorDetail(err error) string { | ||
| var pgErrV4 *pgconnv4.PgError | ||
| if errors.As(err, &pgErrV4) { |
There was a problem hiding this comment.
When a caller passes the error returned by TransactionExecutor.ExecTx, a PostgreSQL 23505 has already been wrapped in ErrSQLUniqueConstraintViolation by MapSQLError. That wrapper has no Unwrap method, so errors.As here cannot reach the *pgconn.PgError; consequently both new metadata extractors return empty values for the mapped store errors they are intended to diagnose. The tests miss this because they call the extractors with the raw driver error. Add an Unwrap method to ErrSQLUniqueConstraintViolation, as the serialization and deadlock wrappers already do.
Useful? React with 👍 / 👎.
In this commit, we add two subtests to TestPostgresConflictShapes that split the lost creation race by whether the losing transaction read the contested key before inserting it. The existing subtests covered the REPEATABLE READ side of that race but assumed, rather than checked, what SERIALIZABLE does with the same shape. The assumption was half wrong, and the half that was wrong matters. SSI promotes a creation race to a retryable 40001 only when the loser read the key first, because that read is what leaves the SIRead predicate lock the conflict graph is built from. A transaction that inserts blind, with no preceding read, gives the graph no dependency to find and already loses with a plain 23505 at SERIALIZABLE, exactly as it would at REPEATABLE READ. So a blind-write upsert whose ON CONFLICT target misses the index that can actually fire is exposed today. It is not masked by SSI and would not be unmasked by relaxing the level, which is the opposite of what we had recorded for those sites. Both halves are now pinned against a real Postgres: the blind race asserts a non-retryable 23505 under SERIALIZABLE, and the read-check race asserts a retryable 40001 carrying a pivot reason code.
In this commit, we fix four errors in the write-path audit that an adversarial review of the table turned up, and record the distinction that the new conflict-shape tests establish. The largest error was a blanket claim that every site in the table is currently masked by SSI and so is not a live bug today. That holds for the read-check sites, but not for the ones that insert blind. Those were never protected by the serialization graph at all, so they already lose with a 23505 at SERIALIZABLE. Shape B is now split into read-check and blind, each row says which it is, and the count is restated as fourteen sites of concern of which eleven genuinely depend on SSI. The table was also missing a row. UpsertSession targets session_id while oor_session_registry carries a partial unique index on the idempotency key, so two sessions racing on one key have different session IDs, the DO UPDATE never fires, and the loser gets a 23505. The dedup probe runs in a separate transaction, so the write is blind. This is the same shape as the UpsertOperation and CreatePendingBoardingSweep rows. UpsertBinding was classified A only, but oor_vtxo_bindings declares a second unique constraint that the conflict target does not cover, which makes it B as well. That constraint is declared inline in the CREATE TABLE rather than as a partial index, so the partial index inventory does not catch it, and the inventory now says so. Finally, the UpsertPendingIntent row pointed at the wrong read. The status guard is same-row with the write it races, so REPEATABLE READ still catches it; the real exposure is the anchor anti-join. The note about the UTXO audit insert is corrected too, since that insert names its conflict target rather than being targetless and is safe for a different reason than the one given.
|
Ran an adversarial pass over this before asking for a bot review. Summary of what moved and what didn't. What changed as a result. The audit table had four errors, all now fixed in two follow-up commits. The big one: the doc claimed every site in the table is currently masked by SSI and so isn't a live bug today. That's only true for the sites that read the contested key before inserting it. SSI promotes a creation race to a retryable The table was also missing a row:
Finally the What I pushed back on. The suggestion to fold the duplicated v4/v5 What was checked and held. That One latent thing I did not touch, since it's pre-existing and out of scope here: the ambient-join in @claude review this |
|
Claude finished @Roasbeef's task in 2m 12s —— View job Review: read-only Postgres at REPEATABLE READ
The core change is tight and well-scoped. 1. Missing
|
In this commit, we add the Unwrap method that ErrSQLUniqueConstraintViolation was missing. It was the only mapped Postgres error type without one: ErrSerializationError, ErrDeadlockError, ErrSchemaError and ErrDatabaseConnectionError all have it. The omission quietly defeated the extraction added alongside it. Once MapSQLError wraps a 23505, the mapped value is a dead end for errors.As, so PgErrorConstraint and PgErrorDetail can no longer reach the pgconn error underneath and both return empty. That is the normal case rather than a corner, because ExecTx hands callers the mapped error and never the raw driver one, and identifying which of the partial unique indexes actually fired is the entire reason the constraint name is surfaced. Rendering happened to keep working, which is why this was easy to miss. The Error method passes the raw DBError into the detail renderer directly, so a log line always looked right while programmatic extraction returned nothing. The existing test only ever called the extractors on the raw driver error, so it could not see any of this. It now asserts against the mapped error as well, which is the path callers actually take.
|
Thanks, the You're right that it's exactly the thing that undercut commit 2. I verified it the way you'd want: the mapped error really was a dead end for The test gap you identified was the real problem, so I fixed that too rather than just the method. On #2, agreed, and thanks for pulling the precedents. The rule is about production query paths going through sqlc for type safety and dialect portability, and these probes are the opposite case: they exist to construct specific conflict shapes at hand-picked isolation levels, which is precisely the control that generated queries would take away. Leaving them. On the ambient-join note, agreed that the blast radius is marginally larger now that readers are genuinely On reader retry, that's the right reading and worth having on the record: a read-only |
1 participant
In this PR, we relax the transaction isolation level for read-only Postgres
transactions from
SERIALIZABLEdown to a read-onlyREPEATABLE READ, andleave writers exactly where they are.
Today
BaseDB.BeginTxhardcodessql.LevelSerializablefor every transactionwe open, reads included. Under Postgres' serializable snapshot isolation that
isn't free for a reader: a read-only
SERIALIZABLEtransaction still takesSIReadpredicate locks and still joins the serialization conflict graph, so itcan both eat a
40001itself and act as the pivot that aborts a concurrentwriter. On a daemon as read heavy as ours, that's pure cost. A signet
measurement put us at 2,035 transaction commits per second with 99.6% of them
changing no rows at all, against a Postgres box sitting at 794m of a one core
limit largely on predicate-lock bookkeeping.
For a concrete version of that cost, the lumos client-connection ingress
long-poll is a read path paying the SSI toll for nothing at all. Each
pullIterationindb/mailbox_store.goopens aReadTxOption()transactionaround
PullMailboxEnvelopesand only ever runs a SELECT. The loop iswake-driven rather than a fixed-rate poll, since it selects on a notify channel
that
Appendfires, so the poll timer is a fallback and not the mechanism. Thetrouble was that the fallback interval was one second against a five second
wait window, and the knob for it was entirely unwired:
WithPullPollIntervalhad no production callers, there was no config field behind it, and the store
options builder never emitted it. So one second is flatly what ran on signet,
with no deployment able to have tuned it.
Worth stating the ordering, since the constant is moving. Before that fix, each
five second long-poll cost five read transactions, on the order of 290 per
second across the parked ingress loops in NobleSun's signet profile. Raising
the fallback above the wait window takes it to one per RPC, roughly 58 per
second. That change reduces how many of these transactions exist and this PR
reduces what each one costs, so the two compose rather than overlap, and
neither owns the combined win.
The first commit is the whole functional change, and it's deliberately
self-contained and independently landable. It follows
lightningnetwork/lnd#10997. The isolation choice funnels through a small
txIsolationLevelhelper that gates on theBackendTypethatBaseDBalreadycarries, so SQLite is untouched. There's nothing to win there anyway, since
SQLite only ever admits a single writer and is effectively serializable already.
Worth calling out that we open these transactions as
READ ONLYas well as atthe relaxed level, and the flag is doing at least as much work as the level is.
Postgres only skips predicate-lock acquisition for a transaction that is
genuinely declared read only, so the level on its own would buy us nothing. We
were already passing that flag through, which is why this lands as two lines.
The second commit surfaces the
ConstraintNameandDetailfields of apgconn.PgErrorin the message our mapped error types render.PgError.Errorprints only the severity, the message and the SQLSTATE, so both fields were
going on the floor before they reached a log line. The detail is the only thing
that tells two very different
40001aborts apart: a true SSI abort carries areason code naming the transaction's role in the conflict graph, while an
ordinary write-write conflict on the same row carries no detail at all. Now that
readers no longer take predicate locks, that's the signal that says whether a
given write path still leans on SSI or would be equally happy at
REPEATABLE READ. The constraint name matters for a different reason, covered below.The third commit is
docs/postgres_isolation.md, which writes down the policy,the operator caveats, and the audit that decided where the boundary sits.
See each commit message for a detailed description w.r.t the incremental
changes.
Why writers stay at SERIALIZABLE
This one is the actual deliverable rather than a shortfall. The task was scoped
to go all the way and move writers too, and the audit came back saying we
shouldn't. Both halves of that judgement matter.
The upside is small. Writers are a rounding error in this workload. If 99.6% of
transactions change nothing, then relaxing the remaining 0.4% can't recover much
beyond what the read-only change already takes off the table. The read side was
the entire problem.
The risk is not small. An audit of all 64 write closures found fourteen sites of
concern. Eleven of those genuinely depend on the serialization graph and would
break if we relaxed the level, and several of them sit in fund-critical paths.
For calibration, the comparable audit in lnd found four such shapes across about
340 closures, so even counting the eleven alone our hit rate is far denser. The
full table lives in
docs/postgres_isolation.mdrather than here, classifiedinto three shapes: read-check-then-write, a lost creation race that surfaces as
a
23505, and write skew.Two structural blockers compound it. Under
REPEATABLE READthe same-row40001becomes the only conflict signal we have left, butTxAwareActorDeliveryStore.ExecTxhas no retry loop at all, andTransactionExecutor.ExecTxskips its own whenever it joins an ambient actortransaction, which is the normal case for the ledger, audit, credit and activity
stores. So the one signal that would have to absorb the change isn't retried on
the dominant code path. Separately, a lost creation race that SSI reports as a
retryable
40001can become a23505unique violation underREPEATABLE READ,and a retry loop correctly refuses to retry that.
The doc also inventories the six partial unique indexes in the schema, which is
useful well beyond this change. An
ON CONFLICTtarget that doesn't match apartial index predicate never arbitrates against that index, so the upsert
quietly degrades into a plain insert and you find out about it later.
Unlike lnd#10999 we don't add a configuration knob for the write level. A knob
whose non-default setting silently enables about a dozen known anomalies is a
loaded gun with a safety catch rather than a feature. It should become
configurable once those paths are hardened, and not before.
To be explicit about this, since the audit table reads like a bug list: this PR
doesn't break any of those sites. Writers keep talking to each other through the
full conflict graph exactly as before, because Postgres tracks dependencies only
among serializable transactions, and a read-only
REPEATABLE READtransactionwas never able to protect a write in a different transaction anyway. The table
is a record of what would have to be fixed before writers could move, not of
what this change breaks.
The original draft went further and said that none of the audited sites is a
live bug today, on the grounds that SSI is masking all of them. An adversarial
pass over the table caught that, and it turns out to be half wrong in a way
worth knowing about. SSI promotes a lost creation race to a retryable
40001only when the losing transaction read the contested key before inserting it,
since that read is what leaves the predicate lock the conflict graph is built
from. Three of the fourteen sites insert blind, so there's no dependency for
SSI to find and they already lose with a bare
23505atSERIALIZABLEtoday.Those three are reachable now, independently of this work, and relaxing the
level would take nothing away from them. That's why the audit table now splits
shape B into read-check and blind, and why the count above is eleven rather than
fourteen. Both halves of the mechanism are pinned against a real Postgres in
TestPostgresConflictShapesrather than left as an argument. The remainingeleven are genuinely masked today.
What we're giving up
Snapshot isolation is not serializability, and this is a real if modest
weakening for readers. A read-only
REPEATABLE READtransaction is no longerguaranteed to observe a state corresponding to some serial ordering of the
writers running alongside it, so the read-only transaction anomaly described by
Fekete and O'Neil is once again permitted. The argument in the code comment is
that our read paths only ever consume a point-in-time view and never depended on
being ordered against writers in other transactions, and that a read feeding a
later write in a separate transaction was never protected across that boundary
at any isolation level. That claim is worth a reviewer checking rather than
taking at face value, since it's what the whole change rests on.
There's an operator consequence too, written up in the doc. A
REPEATABLE READtransaction pins its snapshot for its entire lifetime, and Postgres can't vacuum
row versions still visible to an open snapshot. Some of our read transactions
are long lived: the ancestry resolver walks a tree, and several round and VTXO
listings iterate large result sets. A slow one now delays cleanup and can bloat
tables, and it sits
idle in transactionwhile the daemon computes betweenqueries, so
idle_in_transaction_session_timeoutandstatement_timeoutneedto be generous enough to cover a full pass. None of these hold a transaction
open across a network call or an actor send, so their duration is bounded by
database and CPU work rather than by a remote peer.
What was and wasn't verified locally
The backend gate and the level selection are covered by a plain unit test that
runs everywhere. Everything that asserts on Postgres' own behaviour sits behind
the
test_postgresbuild tag and wants a Docker Postgres, and those have beenrun against a real fixture rather than left as an aspiration. They cover that
the requested options survive the pgx stdlib driver and reach the server as a
read-only
REPEATABLE READtransaction, that the server rejects a write insideone, that an SSI pivot abort carries a reason code, that a
REPEATABLE READsame-row conflict still raises a retryable
40001with no detail, that aREPEATABLE READlost creation race raises a non-retryable23505instead,and that under
SERIALIZABLEthe same race raises a23505when the loserinserts blind but a
40001with a pivot reason code when it reads first. Theisolation assertion reads back
SHOW transaction_isolationandSHOW transaction_read_only, so it's the server's own view of the transaction ratherthan a restatement of what we asked for.
What isn't verified here: the signet numbers are a reported measurement rather
than something reproducible from this branch, and the write-path audit was
produced by reading code rather than by running anything against a live
database. The claim that no read path depends on cross-transaction
serializability is reasoned from the call sites in the same way, and isn't
something a test can pin down.
Cross references
The retry gap named above is partly closed by #1053, which adds a retry loop to
the actor commit transaction. lightninglabs/lumos#718 and
lightninglabs/swapdk-server#265 are the incident this is meant to take pressure
off.