[#903] Grant the first replay of a conflict, and bound the rest by its class - #904
Conversation
…bound the rest by its class
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
The diagnosis here is right, and extracting replayable() into a pure, table-tested function is a real improvement. The lock wait an engine spends before reporting a conflict genuinely is charged to the attempt that hit it, so the window was already spent by the time it was first consulted — that was a real bug.
My concern is that the fix over-corrects: three of the four engines pay for the one that needs it. Two changes would resolve that, plus a guard for a pre-existing bug this PR makes materially more likely to fire.
What a conflicted write now costs the caller
| Path | master | this PR |
|---|---|---|
MySQL lock wait timeout (innodb_lock_wait_timeout = 50 s, the default this PR cites) |
50 s | 100 s |
| SQL Server deadlock victim, picked ~12 s in (the CI case cited) | 12 s | ~72 s |
Long write() — e.g. subtree delete — picked as victim at the end |
6 min | 12 min |
Each row is a write() that fails either way; the replays don't rescue it. What changes is how long the caller and its worker thread are held before the error comes back.
Blocking
1. The free replay goes to the one conflict class that doesn't need it — JDBCStorage.java:1021
The stated rationale for skipping the clock on attempt 1 is that the preceding wait "is unbounded on three of the four engines here". That's true — and MySQL is the fourth. Its lock wait is bounded, by innodb_lock_wait_timeout, and LOCK_WAIT_RETRY_WINDOW_NANOS exists specifically to stop a second 50-second wait from being taken.
Because attempt==1 returns before the window is ever consulted, that window can never prevent the single most expensive replay it was introduced to prevent. A deployment that raised the timeout to 120 s or 300 s is held for twice that.
// the engine asked for the transaction to be rerun: no clock denies that first
// rerun — but only where the wait preceding it is not bounded by the engine itself
-if (attempt==1) {
+if (attempt==1 && conflict==Conflict.PROMPT) {
return true;
}
return elapsedNanos<windowOf(conflict);One line, and the 10 s window is live again. It also resolves the companion problem at JDBCStorage.java:911: an attempt that alone outlasts every window — a multi-minute EntryContainer.deleteEntry under a subtree-delete control, or EntryContainer.clear() — is currently guaranteed a second full run before the window is consulted on attempt 2. The database does the work twice and the caller waits twice as long as it did on master.
2. Bound the attempt rather than compensating with a wider window — JDBCStorage.java:77
Widening PROMPT_CONFLICT_RETRY_WINDOW_NANOS from 10 s to 60 s multiplies by six how long a conflicted write pins an OpenDJ worker thread. The failure mode it's tuned for — a deadlock storm between concurrent writers — hits every worker at once, so with the pool sized to the CPU count the server can appear hung for a minute where it previously returned errors in twelve seconds. Reads queue behind the same pool.
The root cause isn't the window; it's that the wait is charged to the attempt and nothing caps the attempt. That's fixable with one statement on the transaction connection: SET LOCK_TIMEOUT on SQL Server, SET LOCAL lock_timeout on PostgreSQL, and MySQL already has innodb_lock_wait_timeout. Only Oracle lacks a session-level equivalent for plain DML — so the unconditional replay is really needed for one engine out of four, and all four currently pay for it. Once an attempt is bounded, the elapsed-time window works exactly as originally designed.
The PR defers that work, which is what leaves the trailing attempt uncapped and forces the free replay in the first place. Suggestion: either bring the session timeout into this PR, or keep the window at 10 s until it lands.
3. A replayed config change dies on a null EntryContainer — BackendImpl.java:856
Storage.write()'s contract requires an idempotent WriteOperation. The one in BackendImpl.applyConfigurationChange isn't: removeDeletedBaseDNs performs global, non-transactional side effects that a rollback can't undo, and it does them before the transactional work.
Traced through: deregisterBaseDN(baseDN), then rootContainer.unregisterEntryContainer(baseDN) — which is entryContainers.remove(baseDN) at RootContainer.java:247 — then ec.close(), and only then ec.delete(txn). If the commit deadlocks, the rollback restores the tables but the deregistration and the close are permanent. cfg is reassigned at the very end of run(), so the replay still sees the removed DN in the old config, calls remove on a map that no longer holds it, gets null, and NPEs in ec.close(). The operator sees a NullPointerException instead of a deadlock, with the backend already half-deregistered.
This is reachable on master too — the PR doesn't introduce it. What changes is the odds: this PR strictly increases how often a conflict is replayed, and the first replay is now unconditional. The durable fix is to move the deregistration and close() outside write(), after a successful commit; the minimum is a null guard plus deriving the set to remove from rootContainer rather than from cfg.
Worth fixing in this PR
Eight low-cost items — two of them are traps a future change would fall into silently
-
conflictOfshould prefer the most specific classification in the chain —JDBCStorage.java:981. It returns the class of the first matchingSQLException, so a wrapper carrying a class 40 state witherrorCode 0downgrades a real1205fromAFTER_LOCK_WAITtoPROMPTand hands it the wide window. Walk the whole chain and keep the most specific answer found. -
Make
windowOfrefuseConflict.NONE—JDBCStorage.java:1028. Today it silently returns the widest window, and the "never gets here" guarantee lives entirely in one early return inreplayable. Throw, or return 0, so a future reordering fails loudly instead of granting a non-conflict the longest retry budget in the class. -
One constant for 1205, not two names for it —
JDBCStorage.java:89,:101.MYSQL_LOCK_WAIT_TIMEOUTandMSSQL_DEADLOCK_VICTIMare the same literal, so substituting one for the other inclassOforisConflictleaves all 66 test cases green while inverting the intent. -
Resolve the driver once —
JDBCStorage.java:1002.classOfis the third independentdriverName.contains(...)cascade in the file, afterdialectOfandisConflict, and no test cross-checks them for agreement. MariaDB (org.mariadb.jdbc.*) already gets different answers from two of them:classOfclassifies its lock wait timeout asPROMPTwhiledialectOfreturns null for the same connection. Extracting adialectOfDriverName(String)and keying all three off it makes them consistent by construction. -
Log the bound that actually applies —
JDBCStorage.java:916. The warning still readsattempt %d of %dagainstMAX_RETRIES, but the effective cap is now the class window — a MySQL lock wait timeout stops at attempt 2 of a promised 10, with nothing in the log saying why. Naming the conflict class and the window makes the two bounds distinguishable from outside. -
isRetryableConflictno longer has a caller insrc/main—JDBCStorage.java:959. Sincewrite()switched toreplayable()it survives as a thin wrapper kept alive by the test and one javadoc link — while carrying the longest doc block in the file, which now documents behaviour reached only throughconflictOf. Either fold the doc ontoconflictOfand delete the wrapper, or havewrite()keep calling it. -
The javadoc's own example no longer distinguishes the two windows —
JDBCStorage.java:56. At the default it names (50 s), both windows now yield exactly two attempts:AFTER_LOCK_WAITgives attempt 1 @50 s granted, attempt 2 @100 s →100 >= 10→ stop;PROMPTgives the same with100 >= 60. So theConflictenum and theMYSQL_LOCK_WAIT_TIMEOUTclassifier are inert in exactly the configuration they document. Fixing item 1 above makes the comment true again. -
TimeUnit.SECONDS.toNanos(10)—JDBCStorage.java:64,:77. The hand-expanded10L * 1000L * 1000L * 1000Lneeds a trailing//10 scomment to be readable, and that comment is the only thing standing between a dropped factor and a 60 ms window that still compiles. It also retires the test's privateseconds()helper.
Test coverage
The line that held the bug is the one line not under test — JDBCStorage.java:880. replayable() is covered by 66 cases, and that's the right thing to have extracted. But the original defect was in write()'s wiring of the clock — that startedAt is taken once, before the first attempt, and never reset — and no test touches it. A refactor that moves final long startedAt=System.nanoTime(); inside the for loop would keep all 66 cases green while restoring an unbounded retry loop: every attempt would see an elapsed time near zero and replay to MAX_RETRIES regardless of the window. Driving write() with an injectable clock and a scripted failing WriteOperation would anchor it; at minimum, assert that the elapsed argument grows monotonically across attempts.
The two tests stopped checking each other — JDBCStorageRetryTest.java:134. testIsRetryableConflict now derives its expectation from the same column testConflictClass asserts (expected != NONE), so the claim that every pre-existing row keeps its old answer isn't pinned by anything. Flip any failures() row from NONE to PROMPT, deliberately or by fat-finger, and both tests move together and the suite stays green — silently widening the set of failures that get replayed. Keeping the boolean as its own independent column, or asserting the retryability rows against a literal list, restores the cross-check.
Reviewed against pull/904/head vs master. The BackendImpl finding and the dead-code finding were traced through the source and confirmed; the rest are read from the diff and its surrounding call sites. Durations are derived from the constants in the diff and the engine defaults the PR's own javadoc cites — no benchmark was run.
…cts alone, and keep one window The first replay was granted to every conflict, which handed it to the one class that does not need it. A MySQL lock wait timeout is reported only once innodb_lock_wait_timeout has elapsed - the engine has already bounded that wait - so a free replay buys a second wait of the same length, 100 s at the 50 s default where master released the worker after 50 s. Worse, the window was never consulted for it: the grant returned first, so the 10 s bound could not fire against the single most expensive replay it was introduced to stop. The grant now goes to Conflict.PROMPT only, whose preceding wait SQL Server, Oracle and PostgreSQL all leave unbounded - no window survives it, and measuring one against it is what left issue OpenIdentityPlatform#903 with zero replays. AFTER_LOCK_WAIT is measured against the window from the first attempt, which is what the class was introduced to do. With the grant narrowed, the 60 s widening of the prompt window was compensating for a defect rather than for anything real, and it multiplied by six how long a conflicted write pins a worker thread - a deadlock storm hits every worker at once. Reverted to 10 s. That leaves the two window constants bound to the same literal, a name apiece for one value, so they are collapsed into RETRY_WINDOW_NANOS and windowOf() is deleted. The Conflict enum survives and is no longer inert at the documented default: it decides the grant. Also: - conflictOf() walks the whole cause chain and keeps the most specific class found, so a wrapper carrying a bare class 40 state no longer downgrades the AFTER_LOCK_WAIT of the SQLException it wraps. - isRetryableConflict() is deleted; write() has called replayable() since the previous commit and nothing in src/main called the wrapper. Its doc moves onto conflictOf(). - The replay warning names the conflict class and both bounds, in ms - whole seconds read "0 s" for most of a burst - since the effective cap is usually the window and a log naming only MAX_RETRIES said nothing about why an operation gave up at attempt 2 of a promised 10. The line that held the original bug - startedAt read once outside the retry loop - had no test: replayable() is handed an elapsed time rather than measuring one, so moving that read inside the loop kept every case green while restoring an unbounded retry. testWriteDrivesTheRetryLoop drives write() through a scripted clock over a new nanoTime() seam and pins both lines. The step size is load-bearing in each direction: at 4 s the run must stop on the third attempt, which the relocated read turns into ten, and at 12 s only the grant can produce a second attempt, so removing it stops the run on the first. Claude-Session: https://claude.ai/code/session_0123YLkjSmyp15GynenKE9vP
|
Addressed in 52adad3. Diagnosis accepted on the core of it: the free replay was granted to the one conflict class that does not need it, 1. The free replay is now
|
| Path | master | previous PR | now |
|---|---|---|---|
| MySQL lock wait timeout (50 s default) | 50 s | 100 s | 50 s |
| SQL Server victim picked ~12 s in | 12 s | ~72 s | ~24 s |
Long write() victim at the end (6 min attempts) |
6 min | 12 min | 12 min |
The last row is the one item 1 does not fix and item 2 defers; it needs the bounded attempt.
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
Thanks for 52adad385c — it takes the previous round where it needed to go. The grant is now attempt==1 && conflict==Conflict.PROMPT, the two windows are back to one 10 s RETRY_WINDOW_NANOS, conflictOf walks the chain to its end and keeps the most specific class, windowOf and the caller-less isRetryableConflict are gone, TimeUnit.SECONDS.toNanos replaces the hand-expanded literal, the warning names both bounds, and write()'s clock is finally reachable from a test. That closes items 1 and 2 and most of the low-cost list.
Two things still block, and two design questions are worth settling before this merges rather than after.
Blocking
1. The BackendImpl finding from the last round is untouched — BackendImpl.java:856
The PR still touches only JDBCStorage.java and JDBCStorageRetryTest.java, so this stands exactly as written before: in applyConfigurationChange's WriteOperation, removeDeletedBaseDNs deregisters the base DN and closes the EntryContainer before the transactional ec.delete(txn), and neither side effect is undone by a rollback. A replayed config change re-enters with the DN already gone from RootContainer.entryContainers, so unregisterEntryContainer returns null and ec.close() throws — the operator sees a NullPointerException where a deadlock happened, with the backend half-deregistered.
It is reachable on master too; what this PR changes is the probability, since the first replay of a prompt conflict is now unconditional. A null guard plus deriving the removal set from rootContainer rather than from cfg is the minimum; moving the deregistration and close() after a successful commit is the fix.
2. classOf keys the lock-wait number off contains("mysql"), so a MySQL-wire-compatible driver gets the grant it must not have — JDBCStorage.java:1009
return String.valueOf(driver).contains("mysql") && e.getErrorCode()==MYSQL_LOCK_WAIT_TIMEOUT
? Conflict.AFTER_LOCK_WAIT : Conflict.PROMPT;config.getDBDirectory() is an arbitrary JDBC URL handed to DriverManager, so the connection may come from org.mariadb.jdbc.*, or an Aurora/Percona-branded driver. Its ER_LOCK_WAIT_TIMEOUT is the same 1205 under the same class 40 state: isConflict matches it, the contains("mysql") test does not, and it is classified PROMPT. replayable(1, 50 s, PROMPT) then returns true and the caller pays a second full innodb_lock_wait_timeout — precisely the doubled bounded wait RETRY_WINDOW_NANOS exists to refuse, and the inverse of the fix that just landed.
This is the sharp edge of "resolve the driver once" from the last round: dialectOf, isConflict and classOf are three independent contains(...) cascades over the same string, and MariaDB already gets inconsistent answers from two of them (dialectOf returns null for it while classOf happily classifies it). Extracting one dialectOfDriverName(String) and keying all three off it makes them agree by construction and costs less than the third cascade does.
Worth settling before merge
3. The grant is keyed to the attempt number, not to the conflict — JDBCStorage.java:1033
if (attempt==1 && conflict==Conflict.PROMPT) {
return true;
}
return elapsedNanos<RETRY_WINDOW_NANOS;The javadoc's argument for the grant is that the wait preceding a prompt conflict is charged to the attempt that hit it and is unbounded, "so there is no window that some wait does not outlast". That argument does not mention the attempt number, and it holds identically on attempt 2. So the #903 shape survives, one attempt later:
- SQL Server, single engine: attempt 1 is a prompt conflict at 2 s (granted); attempt 2 waits 12 s on a row lock before the monitor picks it as the victim.
replayable(2, 14 s, PROMPT)skips the grant and evaluates14 s < 10 s→ the engine's "rerun the transaction" is refused by a clock that a lock wait, not a replay, spent. - MySQL tuned to
innodb_lock_wait_timeout=3: attempt 1 failsAFTER_LOCK_WAITat 3 s (3 < 10, replayed); attempt 2 becomes a deadlock victim at 15 s cumulative → refused for the same reason.
I am not asking for the grant to be widened to every prompt attempt — that makes the loop unbounded in wall-clock terms and leaves MAX_RETRIES as the only real cap, which is the opposite trade. The point is that attempt==1 is a proxy standing in for the real invariant, and the real one is "no clock can bound a wait that nothing else bounds". While that holds, the taxonomy cannot be made to work; the moment the attempt itself is bounded — SET LOCK_TIMEOUT on SQL Server, SET LOCAL lock_timeout on PostgreSQL, innodb_lock_wait_timeout already on MySQL — the grant, the Conflict enum, the per-hop most-specific walk and the driver-keyed vendor read all become unnecessary, and the plain window governs both classes. Dialect.lockTimeoutSql already exists in this file for all four engines; it is simply never issued on the transaction connection, only on the stamp session.
If it is bounded here, the diff gets smaller rather than bigger. If it is deferred again, please say so in the issue with a follow-up, because the current shape is ~100 lines and 40 test rows of scaffolding whose only job is to stand in for one SET.
4. A MySQL lock wait timeout now provably never replays — JDBCStorage.java:1036, JDBCStorageRetryTest.java:186
At the stock innodb_lock_wait_timeout=50 the first attempt returns at ~50 s elapsed; the grant does not extend to AFTER_LOCK_WAIT, and 50 s < 10 s is false — so the replay count is zero in every deployment that has not tuned the timeout below 10 s. Master behaved the same way, so this is not a regression; but master had no vocabulary for it, and this PR pins it as intended in { "mysql lock wait timeout at the default 50 s", 1, seconds(50), ..., false }.
That is a defensible trade (one bounded wait beats two), yet Storage.write()'s contract asks for a rolled-back operation to be retried, and a lock wait timeout is the most reliably transient conflict of the set — MySQL deployments get no write retry for the conflict class they see most. Please make the choice explicit in the RETRY_WINDOW_NANOS javadoc rather than leaving it to be inferred from a test row, and note that item 3 dissolves it: once the attempt carries a lock bound, a tuned-down timeout becomes the normal case rather than the exception that one green row treats it as.
Relatedly, MAX_RETRIES = 10 is now unreachable for any conflict preceded by a wait longer than the window — at most two attempts ever run — while the warning still promises "attempt %d of %d".
Worth fixing in this PR
Six items, all local to the diff
-
The "(the first replay, granted past it)" suffix is inferred, not observed —
JDBCStorage.java:916. It is derived fromelapsedNanos>=RETRY_WINDOW_NANOS, which coincides with the grant only becauseattempt==1 && PROMPTis currently the sole path to an over-window replay. Changereplayablein any direction and the line keeps claiming "the first replay" while being the third, with no test on the text. Havereplayablereport which bound it applied, and log that. -
The class and the SQLState in the log come from different hops —
JDBCStorage.java:913.conflictOfnow walks to the end and returns the most specific class, whileconflictSummarystill returns the firstSQLExceptionit meets. For the chain the new row"lock wait timeout under a bare class 40 wrapper"covers, the line readsafter a AFTER_LOCK_WAIT conflict ... SQLState 40001, error 0— the 1205 that decided the class never appears, so the log cannot explain its own decision. -
conflictOfis walked twice per failed attempt, eagerly —JDBCStorage.java:914.replayablehas just computed it; the warning recomputes it as an argument, andLocalizedLogger.warnshort-circuits onisWarnEnabled(), so on a server with no error-log publisher enabled bothconflictOf(...)andconflictSummary(...)are built and discarded. Compute the class once inwrite()and pass it to both. -
A raw enum constant reaches the operator —
JDBCStorage.java:914.Conflict.toString()rendersafter a PROMPT conflict/after a AFTER_LOCK_WAIT conflict: leaked internals, and the article does not agree. A short label on the enum ("reported promptly" / "reported after a lock wait") fixes both. -
Still two names for 1205 —
JDBCStorage.java:80,:92.MSSQL_DEADLOCK_VICTIMandMYSQL_LOCK_WAIT_TIMEOUTare the same literal twelve lines apart, and the collision across engines is the single fact both javadoc blocks exist to state. Stating it twice in two places that can drift independently is what the last round asked to avoid. -
The test asserts the exact number of clock reads —
JDBCStorageRetryTest.java:340. The comment explains why — the scripted clock advances per read, so an added read rescales the timeline silently — which is fair, but it makes the fixture the reason for the assertion: adding a duration metric, or a clock read inside the trace branch, fails the suite without any behaviour changing. A clock that advances per attempt, or aLongSupplierpassed into the helper, removes the coupling and the assertion together.
PR description
The body still describes the design 52adad385c replaced: per-class windows of 60 s and 10 s, a windowOf(conflict) lookup, and a replayable that grants attempt 1 for any conflict. The cost table follows from those constants, so "deadlock (any engine): caller held for 60 s + one attempt" and "MySQL lock wait timeout: two waits of 50 s" now both state the opposite of what the code does, and the quoted snippet no longer matches replayable. Squash-merge lands that text in the history — please refresh it.
Reviewed against pull/904/head (52adad385c) vs master. The classOf, replayable, logging, constant and test findings were read from the files at head; the BackendImpl finding is carried over unchanged from the previous round and was traced through the source then. Durations are derived from the constants in the diff and the engine defaults the javadoc itself cites — no benchmark was run.
…dlock-retry-window The replay decision is asked as two questions rather than one: replayReason() of OpenIdentityPlatform#879 says what the failure is - it alone reads committing, partlyCommitted and dropped - and replayableWithin(), the bounds half of replayable(), says whether the attempt count and the window of OpenIdentityPlatform#903 allow another. Neither subsumes the other: a dropped connection is worth replaying and carries no conflict class, which the class-first form of replayable() would have refused. conflictOf() keeps the most specific class of OpenIdentityPlatform#903 and takes the traversal of OpenIdentityPlatform#879, walking every chain of the failure without the suppressed links of the release; isRetryableConflict() is that walk asked as a yes or no.
…and read the conflict class once Round 3 of review on OpenIdentityPlatform#904. Report rather than infer. The line that says "the first replay, granted past it" was derived from elapsed >= window, which coincides with the grant only while that grant is the sole way past the window; grantedPastTheWindow() is now a question of its own, and both the loop and the log ask it. Read the class once. write() calls conflictOf() and hands the result to replayReason() and to the bounds, which no longer take a driver name at all: three walks of the chains per failed attempt become one, two callers can no longer drift into disagreeing about the same failure, and the warn is guarded by isWarnEnabled() so a server with no error-log publisher builds none of it. Name the link the class was decided on. conflictOf() keeps the most specific class in the chains while conflictSummary() named the first conflict it met, so a lock wait timeout under a bare class 40 wrapper logged "error 0" - the 1205 that chose the bound never appeared. Delete replayable() and isRetryableConflict(). Both had become wrappers no src/main caller reaches after the split; their content is one comparison at each site. State the MySQL trade in the javadoc of the window rather than in a test row: at the stock innodb_lock_wait_timeout a lock wait timeout is reported past the window on the first check and gets no grant, so it is never replayed. Master behaved the same way and said nothing. Advance the scripted clock of testWriteDrivesTheRetryLoop per attempt instead of per read, which retires the assertion on the number of reads and with it the coupling to the fixture. Both mutants still die: deleting the grant gives "attempts made expected [2] but found [1]", and a startedAt moved inside the loop gives "expected [3] but found [10]". Filed OpenIdentityPlatform#915 for bounding the attempt itself - the change that retires the grant, the Conflict enum and the driver-keyed vendor number together - with the 1222/55P03 classification it has to carry.
|
Addressed in 2a9c912, on top of a4c85de — the master merge landed after your review, and it dissolves two Of the two blockers, one is taken as a documentation change and one I am holding. Item 3 is filed as #915, 1.
|
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
Thanks for 2a9c912bfe. Two of the four items from the last round are closed the way I asked:
- Item 3 (the grant keyed to the attempt number) — answered rather than papered over: #915 is filed, and
grantedPastTheWindow's javadoc namesattempt==1as a proxy for the invariant and says #915 retires the method rather than widening it. I am not re-raising it. - Item 4 (a MySQL lock wait timeout never replays) — the
RETRY_WINDOW_NANOSjavadoc now states the trade outright instead of leaving it to be inferred from a green test row. That is the right place for it.
grantedPastTheWindow being a named question the log line asks, rather than elapsed >= window re-derived after the fact, is a real improvement too — the claim can no longer outlive the decision that justifies it. And the class is read once and handed to both questions.
Two things still block. One is new, in the code that commit added; the other is the item from the last round whose answer is a javadoc argument that does not hold.
Blocking
1. conflictOf scans for the strongest verdict with the truncating walk — JDBCStorage.java:1288
static Conflict conflictOf(Throwable t, String driver) {
// asked for the most specific class first, ...
if (firstLinkMatching(t, WITHOUT_THE_RELEASE, e -> classOf(e, driver)==Conflict.AFTER_LOCK_WAIT)!=null) {That overload (line 1172) passes MAX_CHAIN_LINKS. The identical construction 600 lines above does not, and the comment there says why — JDBCStorage.java:648:
// ... Walked to its end rather than to MAX_CHAIN_LINKS: the seen set already terminates it, and the verdict
// weakens under truncation rather than simply going unnoticed - a SESSION past the budget would come back
// as TREE. The strongest verdict wins, so it is asked for in that order.
static FailureScope failureScope(Throwable failure, Dialect dialect) {
if (firstLinkMatching(failure, WITH_THE_RELEASE, EVERY_LINK, ...conflictOf is the same shape — strongest verdict first, fallback second — and it truncates in the direction that comment calls out. An AFTER_LOCK_WAIT link past the 64th visited Throwable is not found; the second walk then finds a bare class 40 link within budget and the verdict comes back PROMPT. grantedPastTheWindow(1, 50 s, PROMPT) is true, so the write buys a second full innodb_lock_wait_timeout — the one thing the window and the whole Conflict taxonomy exist to refuse. Truncation here does not lose a replay, it grants one.
64 links is generous and mssql-jdbc is a good reason for the constant not being smaller, but that constant is a guard against a chain long enough to matter, and this is the walk where being wrong is not merely "unnoticed". Please pass EVERY_LINK as failureScope does; the seen set already terminates it.
2. The MySQL-wire-compatible driver still takes the grant, and the argument added for why it cannot does not hold — JDBCStorage.java:1296-1313
The code is unchanged from the last round; what is new is the reasoning above it:
A MySQL-wire-compatible driver would be classified that way too, and would take the grant that a lock wait timeout must not have; it cannot reach this code, because such a deployment fails long before a transaction of it can conflict -
openTree(createOnDemand)issuescreate table ... k bytea, a type no MySQL-wire engine has.
Both halves are wrong for the deployment that matters — a backend created under com.mysql.cj.jdbc and later opened through MariaDB Connector/J, an Aurora- or Percona-branded driver:
-
The DDL is guarded —
JDBCStorage.java:1607:if (!isExistsTable(treeName)) { commitStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")", true);
Every table is already there, so no
create tableis issued at all. Nothing fails. -
The write path of an unrecognised driver is not something a MySQL-wire engine chokes on either —
JDBCStorage.java:1733:}else { //ANSI SQL: try update before insert with not exists return update(treeName,key,value) || insert(treeName,key,value);
Plain
update, theninsert ... where not exists; both run happily on every MySQL-wire engine. (Were it the ANSIMERGEthe javadoc says it is, the argument would still only coverput— adeleteconflicts on its own.)
So the deployment runs, dialectOf returns null for it, no lock timeout is ever issued on it, and its 1205 under class 40 comes back PROMPT and takes the grant — the inverse of the fix this PR is making. This is the sharp edge of "resolve the driver once" from two rounds ago: dialectOf (line 360), isConflict (1362), classOf (1309) and upsert (1703) are four independent contains(...) cascades over the same string, and they already disagree about MariaDB. One dialectOf(String driverName) that the Connection-taking overload delegates to, keyed on by all four, makes them agree by construction and costs less than the fourth cascade does.
Worth settling before merge
3. "Most specific wins" is applied to unrelated links, not only to the wrapper it was introduced for — JDBCStorage.java:1288, 1390
The javadoc justifies the ordering with the wrapper case: a wrapper carrying a bare class 40 state of its own must not downgrade the AFTER_LOCK_WAIT of the SQLException it wraps. That is right. But the walk covers all the chains, so the rule fires just as well when the AFTER_LOCK_WAIT link is not an ancestor of the operative failure at all: a deadlock (1213, 40001) whose transaction collected a 1205 link earlier comes back AFTER_LOCK_WAIT, loses the grant #903 exists to give it, and conflictSummary — which repeats the ordering by hand at line 1390 so the two agree — logs SQLState 40001, error 1205, naming a link that did not fail the attempt. Master named the first conflict of the chain, which was the deadlock.
Either restrict the refinement to the wrapper shape (take the class of the first conflicting link and refine it only along that link's own cause/next chain), or say in the javadoc why a sibling 1205 should outrank the deadlock that actually ended the transaction.
4. The description understates what the grant costs
Its cost table says "10 s + one attempt" for a deadlock on any engine. The write() javadoc is the accurate one — "a conflicted operation holds its caller for the window plus one attempt, and a prompt conflict for two attempts when that is longer" — and while #915 is open, "two attempts" has no upper bound on SQL Server (LOCK_TIMEOUT -1 by default) or Oracle: where master released a worker thread after one unbounded wait, this holds it for two. A defensible trade, and I am not asking to change it — but the description is where a reviewer forms the expectation, so please make it say what the javadoc says.
Worth fixing in this PR
Two on the code, one on the walks, two on the tests
5. The class is computed for the one path that discards it — JDBCStorage.java:1025. conflictOf runs unconditionally, and replayReason throws the answer away on its first line (if (partlyCommitted) return null;, line 1104). RootContainer.open() commits DDL and raises partlyCommitted for the rest of the write, which is also the path most likely to carry deeply wrapped chains. Test partlyCommitted first, or classify lazily.
6. "Three walks of the chains per failed attempt become one" does not hold — the commit message claims it; one failed attempt under PostgreSQL walks them up to eight times: isConnectionFailure (1015), conflictOf's two (1288, 1291), replayReason's isConnectionFailure when the class is NONE (1110), and conflictSummary's up to four (1390-1402). The AFTER_LOCK_WAIT walk cannot match under a driver whose name has no mysql in it, yet still visits every link. One walk collecting the strongest class and the link it was decided on would serve conflictOf and conflictSummary together — which is what "so that the line names the link the decision was taken on" wants anyway — and the dialect of item 2 would skip it entirely on the other three engines.
7. conflictOf and failureScope are the same idiom twice, and have already drifted — item 1 is the drift. A shared "strongest verdict over an ordered list of predicates, with one documented budget" helper would have made it impossible to introduce, and would keep the third verdict #915 adds from copying whichever of the two its author happens to read first.
8. Nothing drives an AFTER_LOCK_WAIT conflict through write() — JDBCStorageRetryTest.java:1085. Both writeRuns rows use sql(0, "40001") against a mock whose class name matches no driver, i.e. PROMPT in both. The predicate rows pin the decision, but nothing pins that write() hands the predicate the right class: a regression that dropped the class on the way to replayableWithin, or passed a null driver, leaves this provider green while shipping the doubled 50 s wait. One row with a mysql-named driver and error 1205, expecting a single attempt, closes it.
9. The retry-loop test bypasses the file's own fixture — JDBCStorageRetryTest.java:1109. new JDBCStorage(mock(JDBCBackendCfg.class), null) leaves getDBDirectory() returning null, unlike storageOver() (line 972). It is safe only because the scripted failure is not a connection failure: make a row a class 08 state and distrustPool() reaches CachedConnection.distrustPool(null) → poolDistrustedAt.merge(null, ...), an NPE from a ConcurrentHashMap null key that has nothing to do with the assertion under test. Stub getDBDirectory() with a unique StubDriver url the way storageOver() does.
Items 1 and 2 are what I would like to see before this merges; 3 and 4 are a decision and a description; the rest are cheap. The BackendImpl.applyConfigurationChange finding from the earlier rounds is still open and still untouched here — if you would rather not carry it in this PR, split it into its own issue and I will stop attaching it to this one.
…dlock-retry-window OpenIdentityPlatform#877 (OpenIdentityPlatform#882) has landed and touches the same file. Two things had to be decided: * The import block: TimeUnit of this branch beside the Executor and AtomicBoolean of OpenIdentityPlatform#882, all three kept. * nanoTime(). Both branches added the same overridable clock to JDBCStorage, with the same signature and the same body - OpenIdentityPlatform#877 to classify a statement that reached its bound, this branch to measure the retry window of write() - at opposite ends of the file, so git marked nothing and the merge did not compile: "method nanoTime() is already defined in class JDBCStorage". One method now, with a comment naming both of the things measured on it. Compiles, and JDBCStorageRetryTest (89), JDBCStatementBoundTestCase (37), CachedConnectionTestCase (64), StampConnectionTestCase (5), BulkCursorTest (12) and PersistentCompressedSchemaTest (8) pass - 215 together.
|
@maximthomas #877 (#882) landed and left this branch conflicting. I have merged master into it and pushed the result to your branch — The marked conflict: the import block
The one git did not mark, and it does not compileBoth branches added the same overridable clock to long nanoTime() {
return System.nanoTime();
}Same signature, same body, for the same reason — a clock a test can drive rather than a real second the suite has to wait out. #877 measures what a statement that reached its bound actually took; this branch measures the retry window of One method now, at the #882 site, with a comment naming both of the things measured on it. Your StateCompiles. Worth knowing for the next round: #882 also left #884 and #893 conflicting, both in this same file, and both had a silent conflict of their own — a removed |
…nd refuse the grant to an engine this backend does not know Round 4 of review on OpenIdentityPlatform#904. The class of a conflict was read by two ordered walks - the strongest verdict asked for first - and only one of them was given the budget that walk needs. `conflictOf` went through the two-argument `firstLinkMatching` and got MAX_CHAIN_LINKS; `failureScope`, which is the same shape, passes EVERY_LINK and says why: the verdict weakens under truncation rather than going unnoticed. An AFTER_LOCK_WAIT link past the 64th visited Throwable, with a bare class 40 link inside the budget, came back PROMPT and was granted the replay past the window - a second full innodb_lock_wait_timeout, the one thing the window exists to refuse. Truncation there granted a replay rather than losing one. The ordering is gone rather than re-budgeted. `conflictVerdict()` is one walk that keeps the strongest class it meets, and `Conflict` is declared in order of how much each class restricts the replay, so "strongest wins" is a property of the list rather than a rule the reader of one call site has to reconstruct. `failureScope` is now the only ordered predicate list left, so there is no second copy of the idiom to drift from. The walk also hands back the link the class was read from, so `conflictSummary` names it by construction instead of repeating the order by hand - which is what had drifted. `classOf` claimed a MySQL-wire-compatible driver could not reach it, because such a deployment would die on `create table ... k bytea`. It does reach it: every create table and create index of openTree(createOnDemand) is guarded by a catalog read, so a backend created under com.mysql.cj.jdbc and later opened through MariaDB Connector/J issues no DDL at all, and its writes go down the ANSI branch of upsert - an update and an insert, not a statement a MySQL-wire engine refuses. Such a driver reports innodb_lock_wait_timeout as 1205 under class 40 exactly as Connector/J does, and reading that number only under a name carrying "mysql" handed it the grant. A conflict under an engine dialectOf() does not recognise is now Conflict.UNKNOWN_ENGINE: replayed as before, since class 40 says replayable whatever the engine, but refused the replay past the window, because the grant rests on knowing the engine bounded nothing. Which engine a driver is, is one answer now. dialectOf(String driverName) is the single cascade, the Connection-taking overload delegates to it, and isConflict, classOf, upsert, getTableDialect, the index DDL of openTree, hashParam, commitsBeforeDdl and the cursor's paging clause all key on it. Nine contains() cascades over the same string is how a deployment ends up given one engine's SQL and another engine's conflict class. write() no longer classifies a partly committed attempt at all: replayReason() refuses that attempt its replay before anything about the failure matters, and it is the path most likely to carry deeply wrapped chains, since RootContainer.open() commits DDL and raises the flag for the rest of the write. The rule stays in replayReason(), which is still the only place that reads the three flags. One failed, replayed, logged attempt now walks the chains three times where it walked them six, and five where it walked them eight. Tests: the retry-loop rows carry an engine and a failure of their own, so that write() is pinned to hand the predicate the class of its own failure rather than a class no row can tell apart - a mysql-named driver with error 1205 and an unrecognised one both make a single attempt where a prompt conflict makes two. The fixture takes a getDBDirectory() of its own, the way storageOver() gives one to every other fixture in the file: a row scripting a connection failure would otherwise reach distrustPool(null). New: the class read from a chain longer than MAX_CHAIN_LINKS, and the bounds of an unrecognised engine.
|
Fixed in c00ec28: items 1, 2, 5, 6, 7, 8 and 9. Item 3 is answered in the javadoc rather than by restricting the rule — say the word and I will restrict it instead. Item 4 is in the description. 1.
|
| attempt failed on | walks now | before |
|---|---|---|
| a conflict | 3 — isConnectionFailure(e, con), isConnectionFailure(failure) for the pool, conflictVerdict |
6 |
| a connection the release reported dropped | 5 | 8, the count you gave |
| a connection dropped under the operation | 3 | 6 |
conflictSummary walks nothing in the first case: it is handed the link. And the AFTER_LOCK_WAIT walk that visited every link to match nothing under a driver with no mysql in its name is gone with the ordering.
7. conflictOf and failureScope were the same idiom twice. Closed by the single walk above: failureScope is now the only method left with an ordered predicate list, so there is no second copy of the idiom to drift from.
8. Nothing drove an AFTER_LOCK_WAIT conflict through write(). The writeRuns rows now carry an engine and a failure of their own. a lock wait timeout is granted no replay — mysqlConnection, error 1205, 12 s step, one attempt — is the row you asked for; an unrecognised engine is granted no replay is the same for item 2. The two existing rows moved to postgresConnection and 40P01, so no row is classified by a mock's name any more.
9. The retry-loop test bypassed the file's own fixture. It now takes a getDBDirectory() of its own, StubDriver.PREFIX + pools.incrementAndGet(), the way storageOver() gives one to every other fixture in the file.
BackendImpl.applyConfigurationChange: already split out
#907, filed last round with the trace, and listed under "Out of scope" in the description. Worth adding that the symptom there is not an NPE — Registry.deregisterBaseDN (BackendConfigManager.java:1552) throws UNWILLING_TO_PERFORM on backendsByName.get(baseDN)==null one line before unregisterEntryContainer could return null, so a null guard at the point the finding names is dead code. That is in the issue.
JDBCStorageRetryTest — 95 cases, 0 failures; the description's mutant list has the two new rows and their measured messages. Still no Docker here, so the four engine suites green-skip and confirm nothing.
vharseko
left a comment
vharseko
left a comment
There was a problem hiding this comment.
Thanks for c00ec28ef0. Both blocking items of the last round are closed, and closed properly rather than papered over:
- Item 1 (the truncating conflict walk) —
conflictVerdictnow passesEVERY_LINK, andMAX_CHAIN_LINKSgrew a javadoc saying which walks get which budget and why.testTheConflictClassIsReadFromEveryLinkOfTheChainpins it with a 64-link tail, and the mutant list records it failing when the budget is put back. - Item 2 (the MySQL-wire-compatible driver taking the grant) —
dialectOf(String)is now the one cascade,classOfrefuses the grant to anulldialect throughUNKNOWN_ENGINE, and the MARIADB rows pin it in both providers. This is the "resolve the driver once" ask from two rounds ago, landed where it actually pays.
Also closed: item 5 (partlyCommitted ? NOT_CLASSIFIED : conflictVerdict(...) — the discarding path no longer walks anything), item 8 (a mysql-named row now drives write()), item 9 (getDBDirectory() stubbed with a StubDriver url), item 6's single walk (one ConflictVerdict carrying the class and the link, serving both the decision and the log line), and item 7 — conflictVerdict and failureScope now share the budget vocabulary instead of drifting. Item 3 is answered in the conflictOf javadoc, deliberately and with the asymmetry stated; I am not re-raising it. BackendImpl is split into #907 as I offered — thank you, it stops attaching itself to this PR.
What is left is thinner than the last three rounds. Two items are the unfinished halves of asks I already made, which is why I am blocking on them rather than filing them; the rest are cheap.
Blocking
1. The dialect half of item 6 did not land, and EVERY_LINK made it matter more — JDBCStorage.java:1975-1987
Item 6 last round asked for two things: one walk collecting the strongest class and its link, and "the dialect of item 2 would skip it entirely on the other three engines". The first landed. The second did not, and the walk it was meant to bound is now unbounded.
walkLinks(failure, WITHOUT_THE_RELEASE, EVERY_LINK, e -> {
final Conflict conflict=classOf(e, driver);
...
return strongest[0]==STRONGEST_CONFLICT;
});STRONGEST_CONFLICT is AFTER_LOCK_WAIT, and classOf returns it only under dialect==Dialect.MYSQL && e.getErrorCode()==MYSQL_LOCK_WAIT_TIMEOUT (line 2010-2022). So on PostgreSQL, Oracle and SQL Server the ceiling is PROMPT, and under an unrecognised driver it is UNKNOWN_ENGINE — neither equals STRONGEST_CONFLICT, and the early exit never fires. On three of the four engines the walk always runs to the end of the chain, however early the answer became final.
Each link then costs the dialect twice: classOf resolves it at line 2014, and isConflict resolves it again at 2072 for the same link and the same string. And write() classifies before it knows the failure is replayable at all (line 1640), so this runs on every failed write — a plain 23000 from adding an existing entry included — on the engine whose driver chains every error of a message through setNextException, which is the reason MAX_CHAIN_LINKS was introduced in the first place.
Both come out of the same two lines:
final Dialect dialect=dialectOf(driver); // once, outside the lambda
final Conflict ceiling=ceilingOf(dialect); // AFTER_LOCK_WAIT for MYSQL, UNKNOWN_ENGINE for null,
// PROMPT for the other three
...
return strongest[0]==ceiling;That is what makes the unbounded walk affordable, which is what item 1 of the last round asked for. As a side effect it also removes the reliance on Conflict.values()[length-1]: the stop condition stops being "whatever constant is last" and becomes a property of the dialect, so the class #915 adds cannot silently move it.
2. The row added for item 8 cannot fail the way item 8 asked it to — JDBCStorageRetryTest.java:1154
I asked for "one row with a mysql-named driver and error 1205, expecting a single attempt". You added it — and the row next to it makes it unable to discriminate:
{ "a lock wait timeout is granted no replay", mysqlConnection.class, sql(1205, "40001"), 12L, 1 },
{ "an unrecognised engine is granted no replay", mariadbConnection.class, sql(1205, "40001"), 12L, 1 },AFTER_LOCK_WAIT and UNKNOWN_ENGINE are refused the grant identically and governed by the window identically, so both rows say "1 attempt" for either class. The regression item 8 named — the class dropped on the way to replayableWithin, or read off a null driver — leaves the mysql row green, because misreading it as UNKNOWN_ENGINE produces exactly the expected number.
The other half is the assertion that would have caught it. storageOverAnEngine carries it deliberately (line 977):
assertTrue(JDBCStorage.driverNameOf(con).contains(engineName),
"a mock of " + engine.getSimpleName() + " reaches no " + engineName + " branch: " + ...);testWriteDrivesTheRetryLoop omits it while its comment says the mock's class name is read "the way storageOverAnEngine() does it" (line 1166). A Mockito naming change, or a renamed fixture interface, moves the mysql row into the unrecognised bucket with no test saying so.
Either add that assertion here too, or make the row discriminate on its own — a step of 3 s replays under AFTER_LOCK_WAIT (the "mysql lock wait timeout tuned under the window" shape) and would separate the two classes through write() rather than beside it.
Worth fixing in this PR
Three items: one test row, one dead pair, one javadoc claim
3. The grant's own boundary is the one point not pinned — JDBCStorageRetryTest.java:252. replays() has attempt 2 at seconds(10) and attempt 1 at seconds(12), seconds(600), seconds(3) — but nothing at attempt 1 / seconds(10), which is where elapsedNanos>=RETRY_WINDOW_NANOS (line 2064) decides. Change that >= to > and replayableWithin falls through to elapsed<window = false: the zero-replay behaviour of #903 at exactly the window, with every row of both providers green and testTheGrantIsTheOnlyReplayPastTheWindow unmoved, since its sweep uses seconds(11). One row closes it.
4. conflictOf(Throwable, String) and conflictSummary(Throwable, String) have no caller in src/main — JDBCStorage.java:1965, :2090. write() uses conflictVerdict (1640) and conflictSummary(ConflictVerdict, Throwable) (1663); the two-argument forms are reached only from JDBCStorageRetryTest. This is the shape of the isRetryableConflict wrapper from round 1, back one refactor later — and with the same consequence: the whole classification javadoc, the collision table, the EVERY_LINK argument and the strongest-wins rule now hang off a method production never calls, while conflictVerdict's own javadoc points back at it. Each call also starts a second full unbounded walk, which testConflictSummaryNamesTheLinkTheClassWasDecidedOn pays three times over one chain. Point the tests at conflictVerdict and delete the pair, or keep them and say in the javadoc that they are test seams.
5. The MAX_CHAIN_LINKS javadoc claims more than holds — JDBCStorage.java:86-95. It now reads "a question truncation can only leave unanswered, which is isConnectionFailure and the fallbacks of conflictSummary". For conflictSummary that is right. For isConnectionFailure it is not, on the pool side: a class 08 link past the 64th visited Throwable leaves dropped false at write() lines 1577 and 1626, distrustPool() is never called, and the pool keeps handing out — unvalidated — every connection it had established before the same restart or failover. That is a verdict weakening under truncation, not a question going unanswered, which is the exact test this round applied to conflictVerdict. The behaviour is master's and I am not asking to change it in this PR; I am asking the new javadoc not to assert the opposite, or isConnectionFailure to take EVERY_LINK alongside the other two.
PR description
Two corrections, since squash-merge lands this text in the history — and the last two rounds already spent an item on the description each time.
The failures() churn is off by one. The description says "Two of them — the two with no driver at all — change class, from PROMPT to UNKNOWN_ENGINE". Checked against master: of the 23 pre-existing rows the two without a driver are { "class 40 is driver independent", sql(0, "40001"), null, true }, which does go PROMPT → UNKNOWN_ENGINE, and { "unknown driver", sql(1205, "HY000"), null, false }, which was already non-replayable and is NONE now — unchanged. The second UNKNOWN_ENGINE null-driver row, { "class 40 with 1205, no driver" }, is added by this PR rather than inherited. So it is one row, not two.
"What it does not change" contradicts the paragraph after it. "a failure is replayable for exactly the same inputs as before" is true of the matching predicate and false of the walk, and the next paragraph is the one that says so: on master isRetryableConflict went through the truncating overload, so a chain whose only conflict-bearing link sits past the 64th visited Throwable was not replayable and now is. That is the change I asked for last round and I am not re-arguing it — but the sentence should be scoped to isConflict rather than to replayability, or the widening named where the claim is made. (No row covers that shape either: testTheConflictClassIsReadFromEveryLinkOfTheChain puts a bare 40001 at the head of the chain, which the truncating walk would already have matched.)
Items 1 and 2 are the halves of last round's items 6 and 8 that did not land; 3 to 5 are cheap and 3 is one line. Nothing here touches the design — the taxonomy, the grant and the window are where I asked for them to be, and #915 carries the part that dissolves them.
Reviewed against pull/904/head (c00ec28ef0) vs master; line numbers are that head's. The dead-pair, the early-exit and the failures() churn findings were traced through both files and through master's versions of them. No database and no benchmark — the walk cost is read off the call sites, not measured.
2 participants
Fixes #903.
Problem
JDBCStorage.write()bounds its replay loop twice — byMAX_RETRIES(10) and by a 10 s wall-clock windowadded in #867. The window is started before the first attempt and checked only after an attempt has failed:
So an attempt that alone outlasts the window leaves the loop with zero replays, whatever the conflict.
The window's javadoc said why that was believed safe — "every engine reports one in well under a second" —
and that is the part that does not hold. Detection is prompt; time to victim is not. The wait that precedes
a conflict is charged to the attempt that hit it, and it is unbounded on three of the four engines here:
Dialect.lockTimeoutSqlis issued on the stamp session only, never on a transaction connection.Run 33010633197
lost
MsSqlTestCase>TestCase.testConcurrentWritersInsertingDistinctKeysto exactly this: a SQL Server victimpicked ~12 s into the first attempt, one failure out of 31989 tests. The run's own error log holds two replay
lines for the whole class, both
40001/1205, both succeeding on the first replay — so classification wasright,
MAX_RETRIES(which logs nine lines) had not fired, and the failing transaction logged nothing at all.The window was the only guard left, and it had been spent inside one attempt.
The fix
One window, 10 s, unchanged from master — and one grant on top of it. The first replay of a conflict the
engine reports promptly is never denied by the clock. The wait ahead of such a conflict belongs to the
attempt and is unbounded, so there is no window that some wait does not outlast: measuring one against it does
not bound the wait, it only leaves the operation with no replay at all. That is #903.
The grant stops there. It does not extend to a conflict the engine reported only after a lock wait timeout of
its own, because that wait is already bounded and replaying it costs the same wait again — which is the one
thing the window exists to refuse. Telling the two apart is the whole of the new classification:
1205/40001(42000underxopenStates)PROMPT40001,40P01PROMPT60/61000PROMPT1213/40001PROMPT1205/40001AFTER_LOCK_WAIT40001,40P01, ...UNKNOWN_ENGINEThe last row is the MySQL-wire-compatible case: MariaDB Connector/J, an Aurora- or Percona-branded driver.
It reports
innodb_lock_wait_timeoutas1205under class 40 exactly as Connector/J does, and a backendcreated under
com.mysql.cj.jdbcopens through it unharmed — everycreate tableandcreate indexofopenTree(createOnDemand)is guarded by a catalog read, so an existing backend issues no DDL at all, and itswrites go down the ANSI branch of
upsert. Reading1205only under a name carryingmysqlwould hand thatdeployment the grant, and with it a second full 50 s wait. The grant rests on knowing the engine bounded
nothing, so an engine this backend does not recognise is refused it; the cost is one replay of the window's
length for an engine whose conflicts are in fact prompt, which is the direction worth being wrong in.
Which engine a driver is, is now one answer rather than five:
dialectOf(String driverName)— theConnection-taking overload delegates to it — keys the column types, the upsert, the paging clause, whether aDDL statement commits, and the class of a conflict. Five
contains(...)cascades over the same string is howa deployment ends up given one engine's SQL and another engine's conflict class.
The decision is extracted whole into two pure functions, which is what lets the regression be tested without a
database:
grantedPastTheWindowis a question of its own rather than a branch, so that the line reporting a replay canname the bound that was applied instead of re-deriving it from the clock.
The class and the link it was read from are one answer,
ConflictVerdict, produced by a single walk that keepsthe strongest class it meets —
Conflictis declared in order of how much each class restricts the replay, so"strongest wins" is a property of the list rather than a rule the reader of one call site has to reconstruct.
write()asks for it once and hands it to both decisions and to the line reporting them, and does not ask atall where the answer is discarded: a partly committed attempt is refused its replay by that flag alone, and it
is the path most likely to carry deeply wrapped chains, since
RootContainer.open()commits DDL and raises theflag for the rest of the write.
What it costs
What the
write()javadoc says, which is the accurate statement: a conflicted operation holds its caller forthe window plus one attempt, and a prompt conflict for two attempts when that is longer.
innodb_lock_wait_timeout, 50 s by defaultThe second row is the trade, stated plainly. The wait ahead of a prompt conflict is charged to the attempt
that hit it and nothing bounds it: SQL Server's
LOCK_TIMEOUTis-1by default and Oracle's enqueue wait isunlimited, and
Dialect.lockTimeoutSqlis issued on the stamp session only, never on a transactionconnection. So where master released the worker thread after one such wait, this holds it for two — a victim
picked ~12 s in costs ~24 s, and a long
write()picked as victim at its end costs twice its own length. Thatis deliberate: the operation fails either way on master, and the replay is what resolves it. #915 removes the
trade by bounding the attempt itself, and retires the grant with it.
No replay budget is widened. What changes is that a prompt conflict whose attempt outlasted the window now gets
one replay instead of none.
The other side of that trade is stated in the
RETRY_WINDOW_NANOSjavadoc rather than left to a test row: atthe stock
innodb_lock_wait_timeout=50a MySQL lock wait timeout is reported past the window on the firstcheck and gets no grant, so it is never replayed — the conflict class a MySQL deployment sees most. Master
behaved identically; this PR is the first place that says so. A deployment that tunes the timeout below 10 s
gets its replays back.
What it does not change
The matching predicate.
isConflictmatches the same failures it did on master, andclassOfreturnsNONEiffisConflictdoes not match, so a failure is replayable for exactly the same inputs as before —nulls, non-
SQLExceptions and a cyclic cause chain included. That matters because the vendor numbers collideacross engines:
1205is a SQL Server deadlock victim, a MySQL lock wait timeout, and a fatal Oracle"not a data file". The number added here refines a match already made by the state; it never makes one.
All 23 pre-existing rows of
JDBCStorageRetryTest.failures()keep their old replayability. Two of them —the two with no driver at all — change class, from
PROMPTtoUNKNOWN_ENGINE: replayed as before, no longergranted the replay past the window.
The one thing the walk of the chains does change: the conflict class is now read from every link rather than
from
MAX_CHAIN_LINKSof them, the wayfailureScope()already read its own. That budget guards a questiontruncation can only leave unanswered; here it weakens the verdict instead — an
AFTER_LOCK_WAITlink past the64th visited
Throwable, with a bare class 40 link inside the budget, would come backPROMPTand be grantedthe replay the class exists to refuse. The
seenset terminates the walk either way.Tests
JDBCStorageRetryTest— 95 cases, no database, syntheticSQLExceptions carrying the real numbers and states.Written before the fix, and each stage was watched failing first. The mutants, re-checked at this head:
grantedPastTheWindowreturnsfalse) → 5 failures, among themdeadlock reported after a long lock wait expected [true] but found [false]— the CI regression itself — andthe first replay is granted past the window: attempts made expected [2] but found [1], which pins it throughwrite()rather than only through the pure function;startedAtinside the retry loop →the window bounds the run, not the attempt: attempts made expected [3] but found [10];conflictSummarythe link the class was read from (named = null, so it falls back) → 2failures,
testConflictSummaryNamesTheLinkTheClassWasDecidedOnonSQLState 40001, error 0;MAX_CHAIN_LINKS(as it was, and asfailureScope()deliberately is not) →a lock wait timeout past MAX_CHAIN_LINKS came back as a conflict the window does not bound expected [AFTER_LOCK_WAIT] but found [PROMPT];classOfreturnsPROMPTfor a null dialect) → 5 failures: thetwo
failures()rows with no driver, theMARIADBrow of both providers, andan unrecognised engine is granted no replay: attempts made expected [1] but found [2]throughwrite().The scripted clock advances per attempt, not per read, so a clock read added anywhere in
write()leavesevery row answering as it does now.
MsSqlTestCase,MySqlTestCase,PgSqlTestCase,OracleTestCase— no Docker on this machine, so all fourgreen-skip (390 run / 232 skipped over the JDBC suites, 0 failures). Read honestly: they confirm nothing about
the fix, and even with a database
testConcurrentWritersInsertingDistinctKeysfinishes locally in ~0.2 swithout reaching a conflict at all. The unit rows are the evidence.
Out of scope
Bounding the lock wait where it is taken —
SET LOCK_TIMEOUTand its equivalents on the transactionconnection — is the only change that would bound an attempt itself, and it would retire the grant, the
Conflictenum and the driver-keyed vendor read along with it. It alters the failure every deployment sees(SQL Server turns the wait into error 1222,
HY000, whichisConflictdoes not match today) and needs aproperty and container coverage of its own. Filed as #915 with the cases it has to cover.
BackendImpl.applyConfigurationChangeperforms global, non-transactional side effects inside aWriteOperationthat every storage engine may replay. Pre-existing on master, and
PDBStorage.write()reaches it identically,so it is not a JDBC defect. Filed as #907 with the trace.