zhangwenchao-123 commented on PR #1884:
URL: https://github.com/apache/cloudberry/pull/1884#issuecomment-5188586586
Hi guy, thank you for your contribution, but the pr looks also have some
problems as following:
1: It breaks distributed snapshot atomicity.
The current ordering maintains a critical invariant: if a distributed
snapshot considers a gxid completed (gxid < xmax and not in
inProgressXidArray), the transaction has already committed locally on every QE.
This is what makes a distributed read atomic across segments.
With this patch, a snapshot taken after ProcArrayEndGxact() but during the
COMMIT PREPARED broadcast treats the gxid as committed while some QEs have not
yet processed COMMIT PREPARED. On a QE, the distributed verdict only applies
after the local xid is committed (the DistributedLog mapping is written at
local commit); on a lagging QE the tuple falls back to local visibility rules,
where the xid is still a prepared, in-progress transaction. Consequences:
- Cross-segment mixed reads: the same snapshot sees the new tuple version
on QEs that already committed and the old version on QEs that haven't. The
window is the entire phase-2 broadcast, including the retry loop
(ResetAllGangs() + retries bounded by dtx_phase2_retry_second), which can last
seconds or longer.
- Non-repeatable reads within a single snapshot: a REPEATABLE READ
transaction reading a lagging QE before the commit is applied sees the old
version; re-reading with the same snapshot after the QE applies COMMIT PREPARED
sees the new version, because the verdict switches from the local fallback to
the distributed "committed" verdict.
- The one-phase path is worse: doNotifyingOnePhaseCommit() runs from
prepareDtxTransaction() before the QD writes its own commit record and before
the QE has committed anything, so the gxid disappears from snapshots before the
transaction is durable anywhere.
2: It breaks checkpoint / crash recovery.
ProcArrayEndGxact() also clears tmGxact->gxid and tmGxact->includeInCkpt.
getDtxCheckPointInfo() relies on scanning the proc array for entries with
includeInCkpt set to include committed-but-not-yet-forgotten distributed
transactions in the checkpoint record, so that crash recovery (recoverTM()) can
re-dispatch COMMIT PREPARED to the QEs (see the protocol comment near
getDtxCheckPointInfo() in CreateCheckPoint()). Today this flag is cleared only
after the FORGET record(doInsertForgetCommitted()).
With this patch, if a checkpoint runs during the broadcast/retry window and
the QD then crashes before the FORGET record is written, the committed
transaction is missing from the checkpoint's committed-gxid list. Recovery will
not re-deliver COMMIT PREPARED, and the prepared transactions on the QEs are
treated as in-doubt and rolled back — a durably committed distributed
transaction gets partially rolled back on the segments. Committed data is lost.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
