cockroachdb / cockroachdb/cockroach
kvcoord: decide whether to re-enable or remove the write buffer's locking-read durability transformations
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
The transaction write buffer's locking-read transformations,
- `kv.transaction.write_buffering.transformations.get.enabled`
- `kv.transaction.write_buffering.transformations.scans.enabled`
rewrite replicated locking reads to unreplicated durability, deferring the replicated acquisition to the commit-time buffer flush. This is a sound optimization for ordinary SQL locking reads (e.g. `SELECT FOR UPDATE`), whose locks protect subsequent writes: if the best-effort unreplicated lock is lost, the commit-time flush detects any conflicting write via exclusion-timestamp validation and retries the transaction.
The downgrade is unsound, however, for callers for whom the lock itself is the correctness artifact. SQL advisory locks (`pg_advisory_xact_lock`) promise mutual exclusion for as long as the transaction runs; a transformed lock can be silently dropped on a range split (guaranteed when `kv.lock_table.unreplicated_lock_reliability.split.enabled` is false), a lease transfer, or under lock-table memory pressure, and the loss is undetectable because there is no write to the key whose exclusion could be validated. Two sessions could then both "hold" the same exclusive advisory lock (#172225).
The transformations are disabled by default for now, and it is an open decision what to do with them:
- **Re-enable them** after giving callers that rely on the lock itself a way to opt out of the transformation — e.g. a per-request durability-timing attribute on locking reads expressing that the durability guarantee must hold from evaluation onward, letting the buffer keep transforming ordinary locking reads while passing advisory-lock acquisitions through untransformed.
- **Remove them** as a failed experiment (possibly pursuing some other optimization for locking reads under buffered writes instead).
Whichever way this goes, also remove the test pins added alongside the default flip (`pkg/sql/advisorylock`, `TestPGLocksAdvisoryNotGrantedWaiting`, and the `advisory_lock` logic test) if the transformations are removed or advisory locks become exempt.
Contributor guide
Research direction
Start by tracing the write-buffer transformations controlled by `kv.transaction.write_buffering.transformations.get.enabled` and `.scans.enabled`, then review the advisory-lock behavior described in `pkg/sql/advisorylock`. Compare the re-enable and removal options, including `TestPGLocksAdvisoryNotGrantedWaiting` and the `advisory_lock` logic test; done means the chosen behavior is implemented and the noted test pins are removed or updated.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, sql
- Domain
- databases
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 30/100