fix(postgres): serialize concurrent row merges before reading clocks

[marcobambini]

Problem

Closes #70.

Two PostgreSQL transactions can read the same old row clocks, independently decide that their incoming value wins, and then serialize only at the base-row write. If the transaction carrying col_version = 2 waits behind the one carrying col_version = 3, it can overwrite the committed value and its clock with lower/2. Applying the same payloads serially converges on higher/3 in either order.

Fix

Acquire a transaction-level advisory lock in merge_insert before reading the row's causal length or column clocks. The lock covers the qualified metadata table and encoded primary key, and remains held through deferred writes until the caller commits or rolls back. This protects payload merges and direct inserts into cloudsync_changes, including row deletion/resurrection and block merge paths.

Use a fixed pool of 256 lock keys per database to bound shared lock-table usage during large imports. Hash collisions serialize unrelated rows. Transactions touching multiple rows in different orders can deadlock; callers must retry the whole transaction on 40P01.

• READ COMMITTED: clock reads after waiting see the committed winner.
• SERIALIZABLE: retain PostgreSQL conflict detection and whole-transaction retry on 40001.
• REPEATABLE READ: reject merges with 0A000, since waiting cannot refresh the transaction snapshot. This extends the existing fragment restriction to all row merges.
• SQLite uses a no-op hook because its writers are already serialized.

The isolation behavior and retry requirements are documented in API.md.

Reproduce the original bug

The regression builds two monolithic payloads for the same row/column, with distinct origin sites and explicit versions 3 (higher) and 2 (lower). Both serial orders must yield higher/3. With dblink, session x applies higher inside an open transaction; session y starts applying lower. The test observes wait_event_type = 'Lock' before committing x, then checks both the value and the stored column version. This avoids timing-dependent overlap.

From a checkout of this PR, run the new test against the original source in a separate worktree:

repro_dir="$(mktemp -d /tmp/sqlite-sync-70.XXXXXX)"
git worktree add --detach "$repro_dir" 9c56e9af19d7978b55b175edaef256754bfdef02
git -C "$repro_dir" submodule update --init --recursive
cp scripts/test-postgres-docker.sh "$repro_dir/scripts/"
cp test/postgresql/67_concurrent_merge.sql "$repro_dir/test/postgresql/"
(cd "$repro_dir" && ./scripts/test-postgres-docker.sh 67_concurrent_merge.sql)

Expected failure before the fix (observed):

ERROR: seeded=False first=higher rollback=False: expected higher/3, got lower/2

Test the fix

From this PR's repository root, with Docker running:

# Focused regression
./scripts/test-postgres-docker.sh 67_concurrent_merge.sql

# Complete PostgreSQL suite
./scripts/test-postgres-docker.sh
POSTGRES_TAG=15-bookworm ./scripts/test-postgres-docker.sh
POSTGRES_TAG=18-bookworm ./scripts/test-postgres-docker.sh

# SQLite unit and regression suites
make unittest

The Docker script builds the extension from source, runs psql with ON_ERROR_STOP, and removes its container and volumes on exit. It publishes no ports and uses no existing database. Build images remain cached.

The regression covers fresh and existing rows, both arrival orders, commit and rollback, SERIALIZABLE failure/retry, the REPEATABLE READ SQLSTATE, and 10,000 direct row merges with at most 256 advisory locks. It also checks that rollback releases the locks and is included in full_test.sql.

Validation completed

• Original source: reproduced lower/2 replacing higher/3.
• Focused regression after the fix: passed.
• Complete PostgreSQL suites in Docker on 15, 17 and 18: zero failures.
• make unittest on macOS: passed, including audit regressions and bootstrap policy tests.
• bash -n scripts/test-postgres-docker.sh and git diff --check: passed.

[andinux]

Updated review of the locking strategy at 3985837, including the bounded-lock requirement demonstrated by 9d0abb3.

Findings

1. False contention and deadlocks from the 256-slot pool (medium–high). database_merge_lock() maps every (table, pk) into a database-wide pool of 256 transaction-level advisory locks. Disjoint rows, including rows in different tables, can acquire colliding slots in opposite order and deadlock (40P01). Assuming uniform hashing, two 20-row batches share about 1.45 slots on average; two 30-row batches share about 3.14. These are collision estimates, not deadlock probabilities, which also depend on timing and acquisition order. A 10k-row transaction will effectively occupy the whole pool and block other merges using it. The bulk regression checks bounded lock usage, but not this concurrency cost. API.md documents the effects; it does not call them “rare.”

2. Repeated preparation (low). Each merge_insert() creates, prepares, executes, and finalizes the lock statement, including repeated changes to the same row. A 10k-row payload with five column changes per row adds roughly 50k such cycles. Cache the statement if retaining this path. Skipping repeated acquisitions is a separate optimization that must respect transaction and savepoint rollback.

Correction: a full 64-bit hash does not meet the bounded-lock requirement

A larger hash removes practically all accidental collisions, but still allocates roughly one advisory lock per distinct row until transaction end. It therefore reintroduces the same class of exhaustion addressed by 9d0abb3: that fix selects at most 64 stale-fragment candidates before acquiring transaction-level locks.

Increasing max_locks_per_transaction supplies more shared capacity; it does not bound demand. Multiple apply calls in one transaction still accumulate locks, and releasing a savepoint does not release successful work's locks. Unbounded per-row advisory locks should therefore not be the preferred replacement here.

Alternatives without a new guard table

Option	Pros	Cons
Conditional winner-clock writes with rollback/retry	Avoids per-row advisory-lock allocation and hash-induced contention; uses existing data/metadata.	Requires correct validation of every decision dependency and replay of the complete affected merge attempt, especially lifecycle and block operations.
64-bit row locks with a transaction budget and a bulk gate	Fine-grained concurrency for small transactions; bounded advisory-lock contribution; no new table.	Bulk mode serializes merges; unknown-size transactions need an explicit rollback/retry contract when the budget is exceeded.
Larger fixed pool with ordered acquisition	Bounds possible keys and reduces collisions.	Only mitigation: bulk transactions still occupy much of the pool. Sorting one payload does not establish ordering across an arbitrary caller transaction.
One advisory lock per table	Simple and low lock usage.	Serializes uploads to the same table; multi-table transactions still need ordering/retry.
Require SERIALIZABLE	Uses PostgreSQL conflict detection instead of explicit merge advisory locks.	Changes the caller contract and requires whole-transaction retries on serialization failures.
Permanent lock target in existing metadata	Exact row locking without a new table or per-row advisory allocation.	A larger redesign: sentinel creation, causal semantics, bootstrap, export, and cleanup must remain correct.

Conditional writes and retry: proposed direction, not yet a proven complete fix

Capture a consistent view of the state used to decide the winner. Within a savepoint, perform the merge and condition the winner-clock write on that observed state still matching. For an initially absent clock, insertion must detect a concurrent creation. If validation fails, roll back the complete attempt, reread committed state, and recompute the decision.

For the original stale-winner case: both transactions read clock 1; the higher update commits clock 3; the lower update's write expecting 1 fails, its tentative value write is rolled back, and the retry sees 3 and loses.

An added WHERE col_version = expected alone is insufficient:

• Equal-version decisions also depend on values and sometimes site IDs. Use a reliable revision token or equivalent complete validation, captured consistently with the decision inputs.
• merge_insert_col() and merge_flush_pending() write application values before winner clocks. A failed comparison must undo those writes, not merely skip the clock update. Deferred batches must replay the affected PK group and rebuild in-memory pending state; checkpoints and applied counts must reflect only accepted work.
• Delete/resurrection depends on row causal state as well as the individual column clock. A separate unlocked sentinel check is not atomic validation and leaves a race.
• Different block updates may pass their individual clock checks yet materialize an incomplete assembled value. That path needs coordination or validation covering materialization as well.
• Cover payload merges and direct cloudsync_changes inserts. Bound local retries and surface a retryable failure on exhaustion. Local reread/retry assumes READ COMMITTED; do not silently relax the existing isolation policy.

Conditional writes can still deadlock on ordinary database resources: transaction A updates row X, transaction B updates row Y, then each tries to update the other's row. Successful conditional updates retain write locks, so both can wait before either comparison completes. PostgreSQL resolves this with 40P01.

This is not a reason to reject conditional writes: it removes the pool's artificial conflicts between unrelated rows. With one shared row and consistent resource ordering, the usual outcome is waiting followed by a failed comparison, not a deadlock. Keep base-table/metadata lock acquisition consistent and retain whole-transaction deadlock retry. A stale-comparison retry and a deadlock retry have different rollback requirements.

A bounded advisory-lock fallback is possible

Use a common advisory merge gate. Normal transactions acquire it shared, then take 64-bit row locks up to a transaction-wide budget. Bulk transactions acquire it exclusively before any merge work and skip per-row advisory locks. All merge entry points must honor the gate.

For example, a budget of 128 distinct row keys means at most 129 distinct advisory keys for a normal transaction; a bulk transaction needs only the gate. This bounds this feature's per-transaction contribution, not all lock usage across the server. Capacity must still account for concurrent transactions and other workloads.

Choose bulk mode upfront when size is known. Otherwise, exceeding the budget must stop the transaction and require rollback/retry in bulk mode; the extension cannot transparently replay arbitrary earlier caller SQL. Do not upgrade a shared gate while holding row locks: concurrent upgrades can deadlock. A database-wide gate is simplest but bulk work blocks all participating merges; per-table gates reduce interference at the cost of more ordering and budget complexity. Committing smaller batches is another option only when the caller accepts the changed atomicity.

Recommendation

Prototype conditional winner-clock writes with complete rollback/retry first. This directly addresses the advisory-lock growth and false-contention problems without a new table. The main correctness work is validating lifecycle and block state, not eliminating ordinary PostgreSQL deadlocks.

Before replacing the pool, validate the original stale-winner regression, equal-version ties, missing clocks, delete/resurrection, deferred multi-column batches, concurrent block materialization, direct inserts, and rollback/retry behavior. Add disjoint concurrent uploads and a bulk upload alongside a small disjoint upload, checking convergence and advisory-lock usage. These are proposed checks, not completed validation of the new design.

If that broader change is unsuitable for this PR, use the bounded advisory-lock/bulk-gate design only with an explicit transaction budget, mode-selection/retry contract, and acceptance of bulk serialization. A full 64-bit hash alone is not a bounded solution.

PostgreSQL references: READ COMMITTED conditional-update behavior, locks and deadlocks, lock-pool configuration.