cockroachdb / cockroachdb/cockroach

sql: transaction advisory locks are not released by ROLLBACK TO SAVEPOINT

Open
#173,479 1 comment 0 reactions 0 assignees View on GitHub
A-sql-executor branch-master branch-release-26.3 C-bug O-agent P-1 T-sql-foundations
Dominant language
Go
Stars
32.5k
Forks
4.1k
PR merge metrics
PR metrics pending

Description

**Describe the problem**

A transaction-level advisory lock acquired after a `SAVEPOINT` is not released
when that savepoint is rolled back. It continues to block other sessions until
the transaction ends.

Worse, `pg_locks` reports the opposite: the advisory row disappears on
`ROLLBACK TO SAVEPOINT`, so the lock is invisible while it is still enforcing.
A session blocked on it has no way to find the holder through
`pg_locks` or `crdb_internal.cluster_held_advisory_locks`.

PostgreSQL releases the lock in this situation, and its `pg_locks` agrees with
the actual lock state.

**To Reproduce**

Start a single-node cluster (`cockroach demo --insecure`) and open two SQL
shells against it.

Session A — acquire under a savepoint, then roll the savepoint back:

```sql
BEGIN;
SAVEPOINT s;
SELECT pg_advisory_xact_lock(5550001);
ROLLBACK TO SAVEPOINT s;

SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND objid = 5550001;
-- count
-- -------
-- 0 <- CockroachDB reports the lock is gone
```

Session B, while A is still open — try to take the same key:

```sql
SET lock_timeout = '3s'; -- only to bound the wait; without it, B hangs
BEGIN;
SELECT pg_advisory_xact_lock(5550001);
-- ERROR: canceling statement due to lock timeout
```

Now end A's transaction (`ROLLBACK;` or `COMMIT;`) and retry in B — it
succeeds immediately, confirming the lock was held by A the whole time.

Control: a key that was never locked is acquired by B instantly, so the probe
is not simply always timing out.

**Expected behavior**

`ROLLBACK TO SAVEPOINT` should release advisory locks acquired after the
savepoint, matching PostgreSQL.

Running the identical script against PostgreSQL 17:

| | `pg_locks` after rollback | session B acquiring the key |
|---|---|---|
| PostgreSQL 17 | 0 rows | succeeds in 0.2 ms |
| CockroachDB | 0 rows | times out after 3 s |
| PG control (held, no rollback) | — | times out after 3 s |

At minimum the two should be self-consistent. Today `pg_locks` claims a release
that did not happen, which is the most confusing possible combination: an
invisible lock that still blocks.

**Additional data / screenshots**

Root cause: advisory locks are acquired as **replicated** KV locks, by design.

[`pkg/sql/advisorylock/manager.go:221-234`](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/advisorylock/manager.go#L221-L234):

```go
// Lock the key in the appropriate mode, we are going to be locking
// a non-existing key. Additionally, for simplicity this lock will
// be replicated, since otherwise we need some ability to detect if
// the lock was lost.
b.AddRawRequest(&kvpb.GetRequest{
RequestHeader: kvpb.RequestHeader{Key: encodedKey},
KeyLockingStrength: lockMode,
LockNonExisting: true, // Key will not exist.
KeyLockingDurability: lock.Replicated,
})
```

Savepoint rollback is sequence-number based. It drops *unreplicated* locks and
marks write intents as ignored, but replicated locks are durable lock records
and are only cleaned up at transaction end by lock resolution —
[`lock_table.go:4148`](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/kv/kvserver/concurrency/lock_table.go#L4148)
explicitly declines to track sequence numbers for them ("savepoint rollbacks
are rare enough").

Meanwhile the SQL layer truncates its own in-memory tracking on rollback —
[`conn_executor_savepoints.go:230`](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/conn_executor_savepoints.go#L230)
calls
[`Manager.OnSQLRollbackToSavepoint`](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/advisorylock/manager.go#L327),
which pops the acquisition stack. That stack is what feeds `pg_locks` (its only
consumer is
[`GetHeldLocks`](https://github.com/cockroachdb/cockroach/blob/8812064a015d2faf99d3fc7e15880f94042954b0/pkg/sql/advisorylock/manager.go#L293)),
so the reporting rolls back while KV does not.

**Environment:**
- CockroachDB version: master, `v26.4.0-alpha` (verified at public master `8812064`)
- Server OS: Linux
- Client app: `psql` / `pgx`
- Compared against: PostgreSQL 17 (official `postgres:17` Docker image)

**Additional context**

Impact: an application using advisory locks for mutual exclusion inside a
transaction that also uses savepoints — including any ORM or framework that
wraps nested blocks in savepoints, and PL/pgSQL exception handlers, which open
a savepoint per block — will hold locks it believes it released. Those locks
serialize unrelated work for the rest of the transaction and are undiagnosable
from `pg_locks`.

Two possible directions:

1. Make advisory locks savepoint-aware (matching PostgreSQL). This likely means
giving up replicated locks, or teaching savepoint rollback to release the
specific replicated locks acquired since the savepoint.
2. If holding to transaction end is accepted as intended, then `pg_locks`
should keep reporting the lock, and the divergence from PostgreSQL should be
documented.

Option 2 is much cheaper but leaves a real PostgreSQL incompatibility.

Jira issue: CRDB-66795

Epic CRDB-65516

Contributor guide

Open the contributing guide

Research direction

Start with pkg/sql/advisorylock/manager.go, conn_executor_savepoints.go, and kv/kvserver/concurrency/lock_table.go, then reproduce the two-session SQL sequence in the issue. Trace how savepoint rollback updates advisory-lock tracking versus replicated KV locks. Done means the rollback behavior and pg_locks reporting are self-consistent and match the chosen compatibility direction, with coverage for the reproduced case.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, sql
Domain
databases
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.