Only cancel the distributed transaction that was found deadlocked - #8790
Only cancel the distributed transaction that was found deadlocked#8790ibrahim halatci (ihalatci) wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8790 +/- ##
==========================================
+ Coverage 88.69% 88.70% +0.01%
==========================================
Files 289 289
Lines 64889 64891 +2
Branches 8180 8180
==========================================
+ Hits 57553 57564 +11
+ Misses 4973 4965 -8
+ Partials 2363 2362 -1 🚀 New features to boost your workflow:
|
Out-of-scope finding 1: the detector can re-cancel an already-unwinding transaction ("mechanism (b)")Not fixed by this PR. Recorded here so the reviewer can decide whether it warrants its own tracker. What it isInstrumenting the detector for this change surfaced three distinct mis-cancellation mechanisms, not one:
Mechanism
if (backendData->transactionId.transactionNumber != 0) /* :1231 */then sets A backend still unwinding from the first cancel still satisfies Amplifier. After a detection, if (foundDeadlock) { deadlockTimeout = deadlockTimeout / 20.0; }so the follow-up cycle arrives ~20x sooner. This is why (b) fires readily at stock settings with no injection. Reproduction: 14 of 20 iterations produced two cancels for the same pid and the same transaction number, ~1 ms apart. Why it was left out of this PRThe obvious fix — skip the cancel if
The flag is therefore not a durable "already cancelled" marker. A correct fix needs separate state or a different signal, which is a larger change than this PR's scope and a different risk profile. Unverified lead, worth checking if this is filed
That makes the #8776 artifact — a raw "canceling statement due to user request" — exactly this rewrite failing to fire. A mechanism-(b) second SIGINT arriving after the transaction number has been zeroed would hit the This is a hypothesis, not a finding. It hinges on whether the following statement re-assigns a distributed transaction number before the error is raised; if it does, the gate passes and you would see the opposite artifact. It is cheap to falsify, and it would change this PR's "not proven to be the cause" framing if it held. Repro recipe
Trap warning for anyone re-running the instrumentationThe |
Out-of-scope finding 2:
|
CancelTransactionDueToDeadlock() only checked that the victim backend was in *some* distributed transaction, not the one that was found in the deadlock cycle. Passing the deadlocked transaction id down and comparing it closes two distinct ways of signalling a backend that was never in the cycle. First, a time-of-check/time-of-use window. The caller looks the backend up while walking the cycle in CheckForDistributedDeadlocks() and only sends the SIGINT after it has finished picking the youngest victim, so a backend that finishes its deadlocked transaction in between can be signalled while running an unrelated statement. Second, and not previously recognised, a cross-node transaction number collision. Distributed transaction numbers are not globally unique: every node initialises its own counter to 1 (backend_data.c:564) and advances it independently (:862), which is why DistributedTransactionId carries an initiator at all. But AssociateDistributedTransactionWithBackendProc() matches a graph node to a local backend on transactionNumber alone (distributed_deadlock_detection.c:398-402); it never compares initiators. CheckForDistributedDeadlocks() only filters the *starting* node of the search to this node (:146), so deadlockPath routinely contains nodes initiated elsewhere, and each is matched against every local originator backend. A graph node (initiator = B, number = 42) therefore associates with an unrelated local backend holding (initiator = localGroupId, number = 42), and the old transactionNumber != 0 guard let that through. The Assert at :411 cannot catch this, because it tests the live backend's initiator, which is legitimately localGroupId, not the graph node's. Either way the result is user visible: once the victim has consumed its cancelledDueToDeadlock flag, multi_log_hook does not rewrite the message either, so the cancelled statement reports a bare "canceling statement due to user request" for a statement that was never deadlocked. This is the other half of the problem fixed in 392c5e2 (#3956), which stopped a stale flag from producing a wrong deadlock *message* but left the detector able to cancel a transaction it never examined. The new check is not a new restriction so much as the enforcement, at cancel time, of an invariant association should already guarantee: we can only SIGINT a local PGPROC, and a local backend that originated its transaction necessarily has initiatorNodeIdentifier == localGroupId (:405, :411), so requiring the graph node to agree only rejects associations that were already wrong. Nothing is orphaned when the check rejects. For the TOCTOU case the cycle was stale and the next detector round rebuilds a fresh graph, which is the behaviour we already had for a backend that had left its distributed transaction entirely. For the collision case the real cycle involving B's transaction is resolved by node B's own detector, since each node only starts its search from transactions it initiated (:146) -- whereas the old code cancelled a bystander and left the deadlock unresolved. A zero transaction number cannot reach the comparison. Wait graph edges do carry zero-numbered ids for non-distributed backends (lock_graph.c:860-865, :879-884) and BuildAdjacencyListsForWaitGraph() creates nodes for them unfiltered (:482-485), but association requires the live backend to be in a distributed transaction (:390, i.e. transactionNumber != 0) and requires the graph node's number to equal it (:398-402), so a zero-numbered node never gets an initiatorProc and never becomes a cancellation candidate. Comparing the transaction number and the initiator node is deliberate even though DistributedTransactionIdCompare() also considers the timestamp: that function sorts ids gathered from every node, whereas here we compare against a single live backend, whose transaction numbers come from a monotonic per-node counter. Not addressed here on purpose: CheckForDistributedDeadlocks() still calls LogCancellingBackend() unconditionally after this function, so that line can now claim a cancellation that was declined. It is pre-existing and debug-only, since LogCancellingBackend() returns early unless citus.log_distributed_deadlock_detection is on, which is off by default. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
0fc3261 to
16f05a8
Compare
Note: rebased + formatting-only change to the code hunkThis branch has been rebased onto current What changed. DistributedTransactionId *backendXactId = &backendData->transactionId;
...
if (backendXactId->transactionNumber == transactionId->transactionNumber &&
backendXactId->initiatorNodeIdentifier == transactionId->initiatorNodeIdentifier)Net Why this matters for the review. The earlier adversarial review leaned in part on the code hunk being byte-identical to Verification of the amendment:
|
2 participants
This is a product defect in the distributed deadlock detector. It was found while investigating a
nightly
statement_cancel_error_messagefailure, but see the honest framing at the bottom — it isnot proven to be the cause of that failure.
The defect
CheckForDistributedDeadlocksassociates graph nodes with live backends and then SIGINTs the victim.Association matches on the transaction number alone
(
distributed_deadlock_detection.c:398-402); the initiating node is never compared. TheAssertat:411checks the live backend's initiator field, which was bound from that same backend at:396-397, so it is structurally incapable of catching a graph-node/live-backend mismatch.The old cancel-side guard only checked
transactionNumber != 0. Two ways that is insufficient::183andthe
kill()at:211, so the SIGINT lands on a different transaction than the one founddeadlocked.
nextTransactionNumberis initialised to1per node (
backend_data.c:564) and bumped locally (:862). Every node's counter starts at 1,so a transaction initiated on node 1 can be matched to a same-numbered local transaction and
SIGINT an innocent backend.
Fix
The cancel path now verifies both the transaction number and the initiating node — i.e. it
enforces at cancel time the invariant that association should already guarantee but does not.
3 files, +28/−5. No test file and no expected output is touched by this PR.
Why this is safe to merge independently of causation
The change is monotonic: the new guard fires on a strict subset of the occasions the old one did,
so the rate of stray SIGINTs under this patch is less than or equal to the rate without it. It cannot
make any cancellation-related failure more likely. The proof is four short steps, all local:
backendData->transactionId.transactionNumber != 0. That expression ischaracter-for-character the body of
IsInDistributedTransaction()(
lock_graph.c:974).kill()ifAssociateDistributedTransactionWithBackendProc()returned true for it — non-associated nodescontinueatdistributed_deadlock_detection.c:190, andyoungestAliveTransactionisassigned only after that filter (
:193-205).IsInDistributedTransaction()at:390andtransaction-number equality at
:398-402. So the transaction id handed to the new guard alwayshas a non-zero number.
backendDatato carry that same non-zero number. Satisfying ittherefore satisfies the old one automatically. New implies old; the converse does not hold.
The residual difference between the two is exactly the bug: the old test passes whenever the victim
holds any distributed transaction, so it accepts a victim that finished the deadlocked transaction
and began an unrelated one. The new test does not.
The cost of a false suppression is bounded and self-healing: the detector re-runs on the next cycle
and re-detects the same cycle, so the worst case is a delayed resolution rather than an orphaned
deadlock. Nothing is orphaned by the extra rejection in any case — the DFS starts only from the
node's own transactions (
distributed_deadlock_detection.c:146), so a foreign node's deadlock isresolved by that node's own detector.
What is left open, deliberately
A third mechanism was found and is reproduced but not fixed here: the daemon can issue a second
SIGINT for a transaction that has not finished unwinding. Under an amplified detector, 14 of 20
iterations issued two SIGINTs to the same backend for the same transaction number about 1 ms apart.
It is left out of this PR because the obvious fix is wrong: skipping when the
cancelledDueToDeadlockflag is set does not work, because that flag is cleared on read by the error-message hook. It is
also pre-existing and, per the analysis below, benign in practice. It will be tracked separately.
Honest framing
This closes two real mis-cancellation holes. It is not proven to be the cause of the PG16 nightly
failure in #8776, and both routes we proposed from this fix to that failure have since been refuted —
one by the shape of the detector code, one by PostgreSQL source. Recording both so they are not
re-derived:
src/backend/tcop/postgres.c,:3365setsQueryCancelPending = falseunconditionally at the topof the block, and
:3421guards the generic error with!DoingCommandRead, commented "If we arereading a command from the client, just ignore the cancel request." This also explains why the
repeat SIGINT above fired 14 times and converted zero times.
deadlock_timeoutcannot expose the mis-cancellation. A sweep over100 ms / 200 ms / 500 ms / 1 s produced 60 cancels, all correctly matched, zero mismatched. That
null is structural, not a small sample: only an association-matched node reaches the
kill()at:211(non-matchescontinueat:190), so a correct match is true by construction. The onlyroute to a mismatch is the narrow window between
:183and:211, whichdeadlock_timeoutdoesnot widen — it controls the interval between detector cycles.
Refs #8776