[Architecture] Concurrency and Visibility Flaws in Trigger-Based CDC Mode (Data Loss & Sequence Bottleneck)

[Teletele-Lin]

Description

Hi pg-trickle team! Thank you for this amazing project. We've been deeply inspired by the Differential Dataflow architecture.

However, when evaluating the trigger-based CDC logic (src/cdc.rs) under extreme high-concurrency OLTP workloads, we identified two critical architectural flaws caused by the inherent limitations of PostgreSQL's extension/trigger mechanisms. We'd like to share these findings and our kernel-level (Executor Hook) approach for discussion.

Issue 1: The "Long Transaction LSN Black Hole" (Silent Data Loss)

In trigger mode, the buffer table relies on pg_current_wal_insert_lsn() and filters incremental data purely by LSN boundaries (lsn > prev_lsn AND lsn <= tick_watermark) during scheduling, bypassing XID snapshot visibility checks for performance. The Anomaly:

1. A transaction updates a row at $T_1$, acquiring an early LSN=100, but remains uncommitted (Long Transaction).
2. The scheduler wakes up at $T_2$, setting the upper boundary to LSN=500. It skips the LSN=100 row because it's invisible under MVCC. The scheduler then advances the lower safety bound (prev_lsn) to 500.
3. The long transaction finally commits at $T_3$.
4. In the next refresh, the scheduler scans WHERE lsn > 500. The previously committed row (LSN=100) is permanently skipped, resulting in silent delta data loss.

Issue 2: Sequence Cache Inversion vs. Global Lock Bottleneck

Because statement-level triggers might fetch identical LSNs for bulk row updates, pg-trickle strictly relies on change_id BIGSERIAL to sort micro-sequences (e.g., retrieving rn_asc=1 and rn_desc=1 during compaction). The Anomaly: To prevent the notorious Sequence Cache Inversion (where Tx2 commits before Tx1, but Tx1 previously cached a smaller sequence number, causing chronological state overwriting), BIGSERIAL explicitly relies on PostgreSQL's default CACHE 1 behavior. Consequently, any high-concurrency DML on base tables is severely throttled by synchronous contention over the global sequence_rel LWLock during MLOG insertions. The TP performance bottleneck is inevitable.

Our Proposed Solution (Kernel Executor Hook Approach)

In our internal implementation build (where we have the privilege to modify the PG kernel), we completely abandoned the trigger + BIGSERIAL patterns to bypass these paradoxes:

1. Abandoning Statement Triggers for Executor Hooks: Instead of SQL-level triggers, we intercept data at the Executor node (ExecInsert/ExecUpdate). We capture the precise, unique physical LSN of the generated WAL record for the specific HeapTuple.
2. Abandoning Sequences for Physical LSN Ordering: Since the physical tuple WAL LSN guarantees absolute, monotonically increasing uniqueness even within the same bulk statement, we use it natively to order row-level mutations, safely abandoning BIGSERIAL and its CACHE 1 lock contention.
3. Reintroducing XID for Visibility: To patch the LSN black hole, we log txid$ and rely strictly on MVCC snapshots (with plans to migrate to a global CSN mapping in the future) to filter uncommitted data, ensuring no delta sets leak through the scheduler's threshold.

Finally, we completely agree that pushing WAL Logical Decoding to production as the primary CDC backend is the ultimate endgame to resolve these synchronization paradoxes.

Would love to hear your thoughts on these edge cases and whether there are any interim safeguards planned for the trigger mode!

[BaardBouvet]

Hi @Teletele-Lin — thank you for the detailed and well-researched report. We
took a deep look at the code paths you flagged. Short version: Issue 1 is a
real bug we need to fix, although the underlying mechanism is slightly
different from what you described. Issue 2 is real but more nuanced. Your
proposed solution is excellent prior art and informed our planned fix, even
though we can't follow it directly as a contrib-style extension.

Issue 1 — Long-transaction frontier skip ✅ confirmed bug

A small but important correction first: the trigger path does not bypass
MVCC for performance. Trigger inserts go through the normal heap, and the
refresh delta query (WHERE lsn > prev_lsn AND lsn <= new_lsn) runs under
default READ COMMITTED, so uncommitted change-buffer rows are correctly
invisible (see src/cdc.rs#L109-117
and src/refresh/mod.rs#L3865).

The bug is that MVCC visibility and LSN ordering are orthogonal
dimensions, and we only gate on the latter:

1. Backend A modifies a tracked table at LSN 0/100, doesn't commit.
2. Scheduler tick captures tick_watermark = pg_current_wal_lsn() = 0/500
   (src/scheduler.rs#L2630-2636),
   reads the buffer (A's row invisible → not consumed), advances and
   persists frontier to 0/500.
3. A commits. Row at 0/100 becomes visible but is now below the frontier.
4. Next tick scans WHERE lsn > 0/500 → row is permanently skipped.

We confirmed there are currently no safeguards: no backend_xmin probe,
no snapshot check, no xid column on the buffer, no holdback margin. The
existing pg_trickle.tick_watermark_enabled GUC only enforces
cross-source consistency within a single tick — it does not address this
race.

In practice the window requires a transaction that performs DML on a
tracked table and then stays open longer than the scheduler tick interval.
That's unlikely for sub-second OLTP but realistic for batch jobs, idle
psql sessions with open transactions, or 2PC. Either way, "unlikely" is
not acceptable — the project's stated rule is that data loss is never
worth a performance gain, so this is a P0 fix for us.

Planned fix

Tracking plan: plans/safety/PLAN_FRONTIER_VISIBILITY_HOLDBACK.md (PR
incoming).

The fix stays inside the extension boundary:

• Per-tick xmin probe of pg_stat_activity (+ pg_prepared_xacts)
  to find the oldest in-progress transaction; cap the new frontier at
  the watermark observed when that xmin first appeared.
• New GUC pg_trickle.frontier_holdback_mode = xmin | none | lsn:<bytes>,
  defaulting to xmin (safe). none preserves today's fast path for
  benchmarks and for operators with strong workload guarantees.
• Metrics for holdback distance and oldest-xmin age, plus a WARNING
  when holdback persists beyond a configurable threshold.
• E2E regression test that intentionally straddles a tick with an open
  transaction (and an inverted test asserting that mode = none
  reproduces the loss — to keep the unsafe mode honestly documented).

The WAL / logical-decoding CDC backend (src/wal_decoder.rs) is immune
to this race because logical decoding only emits committed changes ordered
by commit LSN. Promoting it from "alternative" to "default" is on the
roadmap and remains the strategic endgame, exactly as you suggested. The
trigger path still has to be correct as the fallback in clusters that
can't run wal_level = logical.

Issue 2 — BIGSERIAL + CACHE 1 contention ⚠️ valid, scoped separately

You're right that change_id BIGSERIAL
(src/cdc.rs#L415)
inherits the default CACHE 1, and that compaction does rely on
change_id ordering for ROW_NUMBER() OVER (PARTITION BY pk_hash ORDER BY change_id) to disambiguate rows that share an LSN inside a statement
trigger. So we can't simply drop the sequence.

What we can do without changing semantics:

• Default the sequence to a larger CACHE (e.g. 32) — this only widens
  the per-backend reservation window. Gaps in change_id are harmless
  because the compaction logic only requires deterministic intra-pk
  ordering, which the sequence still guarantees within a single backend.
• Document the trade-off so users with extreme write concurrency can tune
  CACHE higher.
• For deployments that genuinely need lock-free ordering, the WAL backend
  uses commit LSN + per-record sequence numbers from pg_logical_slot_get_changes,
  sidestepping BIGSERIAL entirely.

We'll open a separate tracking issue for the cache tuning so it can ship
independently of the visibility fix.

On the executor-hook approach

Your kernel design is genuinely elegant — capturing the per-tuple WAL LSN
at ExecInsert/ExecUpdate time eliminates the LSN-vs-visibility skew at
the source. We can't ship that as a contrib extension (no executor hooks
exist for tuple-level WAL LSN exposure on stock PostgreSQL 18, and any
solution requiring a kernel patch would block adoption on managed
PostgreSQL services), but the same correctness property is what the xmin
holdback gives us at the cost of slightly delayed visibility under long
transactions.

For users who can run a patched kernel: yes, your approach is strictly
better, and we'd be very interested in collaborating if you're open to
publishing the patch — even as an optional companion module that
pg_trickle could detect and prefer when present.

Thanks again for the careful analysis. We'll link the PR here when the
holdback lands; please do flag anything else you find.

[Teletele-Lin]

Hi @BaardBouvet — thank you for the prompt analysis and for confirming the fix for Issue 1!

Regarding Issue 2 (BIGSERIAL + CACHE 1 contention), while it’s true that sequences guarantee monotonicity within a single backend, adjusting CACHE > 1 is still fundamentally unsafe.

The blind spot here is that sequence caching is decoupled from the actual chronological execution order enforced by row-level locks. We don’t even need session-level cache reuse to break the timeline; a simple interleaved execution of two transactions will cause immediate Sequence Cache Inversion:

The Row-Lock vs. Cache De-synchronization Scenario:

1. Transaction A (opened earlier) requests a sequence and caches [16, 31]. It remains open.
2. Transaction B (opened later) requests a sequence and caches [33, 64].
3. Transaction B executes an UPDATE on a specific row WHERE id=1. It consumes change_id=33, commits, and releases the row lock.
4. Transaction A now executes an UPDATE or DELETE on that exact same row id=1 (effectively modifying the data that Transaction B just committed). It consumes change_id=16 from its pre-existing cache and commits.

The Paradox:
Chronologically and physically in the primary database, Transaction A is the final state of that row.
However, in the change buffer, the sequence values are 33 (older commit) and 16 (latest commit). Because the DVM compaction logic strictly trusts the sequence value (33 > 16), it will mistakenly assume Transaction B is the latest event.

Result: The pipeline will overwrite the true state of the row with the obsolete data from Transaction B.

Because of this physical disconnect between out-of-band sequence assignments and in-band row locks, CACHE 1 remains a strict mathematical necessity for any tracking mechanism resolving state conflicts via sequences. Thus, increasing the cache cannot be used as a safe tuning lever for extreme write-throughput, which strongly underscores why Logical Decoding natively bypasses this bottleneck.

[grove]

@Teletele-Lin — you are correct, and thank you for the rigorous counter-analysis.

We reviewed the actual compaction and delta pipeline code against your scenario and confirmed the flaw in our earlier response. With CACHE > 1, a backend that pre-allocated a low sequence block can commit after a backend holding a higher block — row-lock serialization order and change_id order are then decoupled. Because compaction (ROW_NUMBER() OVER (PARTITION BY pk_hash ORDER BY change_id)) and the scan delta pipeline (DISTINCT ON … ORDER BY change_id DESC) both trust change_id to identify the final state of a row, this causes the pipeline to silently retain stale data. The corruption scenario you described is real.

CACHE 1 is a hard correctness invariant for the trigger-based CDC path, not a tuning lever. The earlier suggestion to evaluate CACHE 32+ was wrong and has been retracted. PR #596 documents this invariant directly in the code (near both BIGSERIAL definitions in src/cdc.rs and in the dvm/operators/scan.rs module doc) so it cannot be silently changed in the future.

The throughput ceiling that CACHE 1 imposes is a fundamental constraint of the trigger model: the only structural fix is the WAL/logical-decoding CDC backend, which uses commit-LSN ordering and sidesteps BIGSERIAL entirely. That promotion is already on our roadmap and remains the strategic endgame, as you noted.

Thanks again for pushing back precisely — this is exactly the kind of review that makes the project safer.