DanielLeens commented on PR #11556:
URL: https://github.com/apache/seatunnel/pull/11556#issuecomment-6411602805
Thanks @nzw921rx for the careful read, and sorry for missing this in my last
review. I re-traced the bounded backfill on the current head `dbc53649a3`
(source read only, nothing was executed locally), and I agree with the P1
finding. My earlier "Ready to merge" conclusion no longer holds, so please
treat it as withdrawn until this is resolved.
This is how I confirmed it:
```text
PostgresSnapshotFetchTask.execute
-> changed = HIGH.isAfter(LOW) (the only gate before
the WAL backfill)
HIGH and LOW come from PostgresUtils.currentLsn -> currentXLogLocation,
the cluster-wide WAL position
-> PostgresWalSplitReadTask.execute
(PostgresWalFetchTask.java:203)
-> offsetContext.setStreamingStoppingLsn(HIGH)
-> PostgresStreamingChangeEventSource.processMessages (loop condition
around line 269)
loops while lastCompletelyProcessedLsn < stoppingLsn
lastCompletelyProcessedLsn is only assigned inside the readPending
callback, when a decoded message with isLastEventForLsn arrives
the no-message branch (around line 379) only dispatches a heartbeat
and pauses; it never advances that LSN
-> END watermark is dispatched only after super.execute() returns
```
So if the cluster WAL position moves from LOW to HIGH but the source
database's slot never decodes a message at or after HIGH (for example, commits
only in another database on the same instance), `super.execute()` keeps polling
and END is never emitted. The buffered snapshot rows are then never merged, and
the split cannot complete. The slot also keeps retaining WAL while it waits.
The comment this PR already adds in `PostgresUtils.currentLsn` (about
`txid_current()` creating an artificial gap that makes the backfill wait
forever) describes the same failure mechanism. That change closes one source of
undecodable gaps, but the boundary is still cluster-wide, so other sources of
undecodable WAL can still produce `HIGH > LOW`. Inference, not verified here:
periodic WAL records and WAL written for tables the slot filters out could
produce the same gap even without a second database. I have not confirmed that
on a live instance, so please treat it as a question to check while fixing this.
Also, the new ITs in `PostgresCDCIT` generate concurrent
INSERT/UPDATE/DELETE on the captured table, so a decodable message always lands
at or after HIGH. I could not find a case where the captured database is idle
while other WAL is produced, which is why CI was green.
I agree with the requested direction and with the caution that a plain
timeout followed by END could bring back the data loss this PR is fixing. A few
options to evaluate (suggestions, not verified against this code):
1. Make the HIGH boundary decodable from the source database: after reading
HIGH, emit a marker from the captured database that the slot is guaranteed to
decode (for example `pg_logical_emit_message` with the transactional flag, or a
write to a small watermark table), and use the LSN of that marker as the
stopping position. The marker must come after every snapshot-visible change and
still be read under the existing transaction-completeness rules.
2. Alternatively, derive the stop condition from the slot's confirmed or
received position (keepalive-advanced `lastReceivedLsn`), but only if it can be
shown that this cannot skip an in-flight transaction that commits below HIGH.
Whichever protocol is chosen, a regression test is needed: an exactly-once
initial-snapshot job on `PostgresCDCIT`/`OpengaussCDCIT` where only a second
database generates WAL during the snapshot and the captured database stays
idle, asserting that END is emitted and the job finishes without leaving the
backfill slot behind. This should fail on the current head.
Everything else from my previous review is unchanged: the two Medium
follow-ups (per-split backfill slot create/drop cost, and the deterministic
backfill slot name after a reader crash) stay non-blocking. My updated view is
"Ready to merge after fixes", with this P1 as the only blocker. I will
re-review once a new commit is pushed.
One process note: my earlier approval is still shown on the PR but is
superseded by this comment. I do not have write-level review controls, so a
committer may want to keep the PR out of the merge queue until the P1 is
resolved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
