[#885] Bound the wait of a JDBC DDL for a lock another session holds

[maximthomas]

Part of #885 — its section 1. The read bound of section 2 is a PR of its own; nothing here depends on it.

Problem

The DDL of the JDBC backend is the one part of it that takes locks, and three engines out of four wait for one essentially forever: lock_wait_timeout is a year on MySQL, LOCK_TIMEOUT is -1 on SQL Server, lock_timeout is 0 on PostgreSQL. Only Oracle's ddl_lock_timeout gives up at once, and it defaults to 0 — give up immediately.

So a create table, a create index or a drop table that arrives while an unrelated transaction of another session holds the table can queue behind it and never come back. What that hangs is not a background job:

• the open of a backend — openTree() creates a table and three indexes per tree, and RootContainer.open() opens every tree of every base DN;
• dsconfig create-backend-index on a running server, which opens a tree on a live instance;
• and on PostgreSQL a queued CREATE INDEX parks every writer of that table behind its own lock request while it waits, so one blocked DDL becomes a stalled backend.

#882 bounded every statement of this backend by the class of the work it belongs to, and deliberately left the bulk class — which the DDL belongs to — unbounded. That decision stands and is the reason this PR exists: a query timeout cannot tell a statement that is working from one that is queued. A create index on a populated table legitimately takes as long as it takes; a create index waiting for a metadata lock has done nothing at all. Only the second is worth ending, and only a lock bound can name it.

Change

org.openidentityplatform.opendj.jdbc.ddl.lock.timeout, in seconds. The default is 5, which is COMMENT_LOCK_TIMEOUT_SECONDS — the bound the table stamp of #866 already carries while taking its lock on the very tables this DDL creates and drops. 0 or negative leaves the wait exactly as unbounded as it was before; a value that is not a number keeps the default, as Integer.getInteger() has it, so a typo cannot silently unbound it; a value past MAX_BOUND_SECONDS is clamped down through the existing clampSeconds(). Every bound of this backend reads its property through one boundSeconds(), so a later change to how these are read cannot leave this one behaving unlike the rest.

The bound is applied around the DDL, never as a session setting of the pooled connection. This is the placement the issue asks about, and the alternative has a hazard worth stating: PostgreSQL's lock_timeout and SQL Server's LOCK_TIMEOUT bound every lock wait, row locks included, and isConflict() classifies neither 55P03 nor error 1222 as a replayable conflict. A session-wide setting would therefore turn ordinary row contention into client-visible failures where today the write waits and succeeds. Scoping it to the DDL avoids that entirely.

Per engine, in the unit its own setting takes:

Engine	read back first	set around the DDL	put back afterwards
PostgreSQL	nothing	a savepoint, then set local lock_timeout = <ms>	nothing — a set local ends with the transaction the DDL commits
MySQL	select @@session.lock_wait_timeout	set session lock_wait_timeout = <s>	the value read back
SQL Server	select @@lock_timeout	set lock_timeout <ms>	the value read back
Oracle	nothing	nothing	—

Each of those constants answers those three settings itself — they are abstract methods of Dialect, not a switch with a default: throw — because two of them are asked before the first try of withDdlLockBound(): a constant added later without them would take out every DDL of the backend on the one path whose contract is that no statement of the bound is ever the failure of a DDL.

The bound never loosens a session that already gives up sooner. The displaced value is read before the bound is built, and where the session is at least as tight as this property nothing is set at all — no setting, no reset, and no rewrite of a failure this bound had no part in. That is the same argument that leaves Oracle alone, applied where the value costs nothing to respect. In each engine's own encoding: MySQL compares seconds (lock_wait_timeout has no value meaning "wait forever" — its range starts at 1); SQL Server compares milliseconds, where -1 is "wait forever" and is replaced, while 0 is "do not wait at all" and is kept, so a plain min() would have read the loosest value there is as the tightest. PostgreSQL is the one place this does not reach: the value is not in hand there, and reading it back is the round trip a set local exists to save — and what is loosened there is loosened for the length of the DDL's own transaction and no longer.

MySQL gets lock_wait_timeout and never innodb_lock_wait_timeout. The first is the metadata lock a DDL takes; the second is the row lock, already 50 s by default, and it is what write()'s conflict replay depends on. The issue names both; only the first belongs here.

Oracle is left alone deliberately. Its ddl_lock_timeout defaults to 0, which is already tighter than anything this would set — applying ours would loosen it — and a deployment that raised it globally did so on purpose. Putting it back would also mean reading v$parameter, a privilege the account of a backend often does not have. The javadoc of the property says so, since that is what an operator reads after an ORA-00054.

The set, the readback and the reset go through the existing executeSessionStatement(), which already documents why a session setting has to reach the server as a plain batch rather than through prepareStatement: the SQL Server driver runs a prepared statement through sp_executesql, and a setting made there is reverted when that call returns — before the statement it is meant to protect ever runs. Every round trip of the bound itself carries a bound of its own, though: the readback, the savepoint, the setting and the reset run under boundedSessionCall(), which holds the socket read timeout for the length of the call. None of them takes a lock or reads a table, so a wait of one of them is a database that has stopped answering rather than work in progress — and unbounded, the reset parked the thread opening a backend from the finally of a DDL that had already failed, which is the hang #877 and #882 exist to end. The DDL between them keeps its own class, which ships unbounded.

Two call sites, both through one helper (withDdlLockBound):

• commitStatement(sql, ddl) when ddl == true — the funnel every DDL of a transaction goes through: the create table and three create index of openTree(), and the drop table of deleteTree(). The importer reaches the same funnel. ddl == false — clearTree()'s delete from — is left alone, waiting for its row locks as it does now;
• the drop table loop of removeStorageFiles(), which bypasses that funnel and commits once at the end, so the bound is set once around the whole loop. On PostgreSQL one set local covers every drop of that single transaction.

No statement of the bound is ever the failure of a DDL. A backend that opened before this bound existed has to open still, so every way the bound can go wrong degrades to "the DDL runs anyway", reported once:

• a session that has no such variable, or answers the readback with something no SET of it would take back, is given no bound at all — nothing is ever set that could not be taken off again;
• a connection in auto-commit is given no set local: outside a transaction block PostgreSQL answers that with a warning no driver raises, so the DDL would run with no bound and the log would read exactly like a bounded one. getAutoCommit() is asked rather than assumed, and costs no round trip;
• a connection that refuses the setting outright runs the DDL anyway (this is not hypothetical: it is the shape JDBCStorageRetryTest already drives, and the first revision of this change failed that suite). On PostgreSQL the transaction is taken back to a savepoint taken in front of the setting — a statement that fails inside a transaction there aborts it, and the DDL would otherwise fail with 25P02 rather than running unbounded;
• a setting that reached the server and failed on the close() of the statement carrying it has left the DDL bounded after all, and no driver says which of the two happened. So the DDL of that path runs under the same rewrite as any other, the reset still gives the value back after it, and on PostgreSQL the rollback to that savepoint undoes the set local as well — which is why the warning says the wait may be unbounded rather than that it is;
• a reset that fails is reported and never thrown: it runs from a finally while the caller may be being unwound, where a throw takes the place of whatever brought it there (JLS 14.20.2). The connection it failed on is kept out of the pool (CachedConnection.keepOutOfThePool()), because the validation of the next borrow is isValid() — a liveness check a connection carrying a stale setting passes, after which SQL Server would cut every lock wait of that borrower at our bound, row locks included. The report names the statement that was issued rather than re-reading the property at log time, and is throttled rather than said once for the life of the storage: each occurrence now costs the pool a connection.

A DDL that does give up at the bound is reported as what it is. It arrives as a bare 55P03 / ERROR 1205 / error 1222, naming neither the wait it ended nor the property that ended it — the same gap timedOut() closes for the bound of a statement. The rewrite names the property and says raising it, or setting it to 0, is the remedy.

Three details of that rewrite matter more than the message:

• only a failure this bound could still be what ended is renamed. The time is measured on the monotonic clock, the way timedOut() measures its own, and a wait that ran more than LOCK_BOUND_SLACK_MILLIS past the bound is left exactly as the engine reported it. More than one wait of an engine reports the same number: MySQL reports the row lock of innodb_lock_wait_timeout — 50 s by default, and what a create index under ALGORITHM=COPY waits on — as the same ERROR 1205 as a metadata lock, and naming this property for one of those sends an operator to raise the one setting that cannot help. There is no guard under the bound to go with it: the states matched here are what an engine says when a lock wait ran out, and nothing this backend issues says them otherwise;
• it reuses the classification already in the file rather than adding a second one. isLockTimeout() is extracted out of scopeOf(), which is behaviour-preserving for failureScope(). It is asked directly rather than through failureScope(), which also reads any SQLTimeoutException as a moment — a statement its own class bound cancelled is one of those, and timedOut() has already named the property that ended it. The chain is walked WITHOUT_THE_RELEASE, as that constant documents: this asks what the engine did with the statement, and the release of the connection runs after that outcome was decided, so a 55P03 out of the rollback that gave the connection back cannot rename a DDL that failed for something else;
• the SQLState and the vendor number are carried over and the failure chained, so a caller that classifies this reads exactly what it read before. A MySQL lock wait stays the class 40 conflict write() knows; PostgreSQL 55P03 and SQL Server 1222 stay the non-conflicts they were. No failure becomes replayable, and none stops being.

Nor can a bounded DDL be replayed into a fresh wait each attempt: commitsBeforeDdl() is true on MySQL and Oracle, so partlyCommitted is raised before the DDL is issued and takes it out of write()'s replay; PostgreSQL and SQL Server report states that are no conflict.

What it costs is the round trips around each DDL — three on MySQL and SQL Server (read the value back, set the bound, give the value back), two on PostgreSQL (the savepoint and the setting), none on Oracle — and only on the cold path. An existing backend issues no DDL at all: every statement of openTree() is guarded by a catalog read. A session already tighter than the bound pays none of them past the readback.

Out of scope

• Making 55P03 / 1222 replayable conflicts. That is only needed by the session-wide placement this PR rejects, and it would change write() semantics for row contention nobody asked about.
• Oracle's ddl_lock_timeout, for the reason above.
• Reading PostgreSQL's own lock_timeout back so the bound cannot loosen it there, for the reason above — one round trip, one method, if it is wanted.
• The unbounded default of bulk.timeout, which [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 shipped deliberately.

Testing

JDBCDdlLockBoundTestCase, 36 cases, no database needed: the SQL each of the four dialects is told around a DDL and in what order, the default and every misconfiguration of the property, a session already tighter than the bound (and SQL Server's 0 against its -1), a connection in auto-commit, the savepoint taken and rolled back, the reset that happens even when the DDL throws, PostgreSQL issuing no reset, the connection kept out of the pool when the reset fails, the socket read timeout armed around the session statements, a lock wait far past the bound left as it is, clearTree() being left alone (driven through the real ImporterImpl), the drop of a tree and the drop loop of a removed backend, and each way the bound degrades instead of failing the DDL.

mvn -o -pl opendj-server-legacy -am -Pprecommit \
    -Dit.test='JDBCDdlLockBoundTestCase,CachedConnectionTestCase,JDBCStatementBoundTestCase,JDBCStorageRetryTest,StampConnectionTestCase' \
    -Dfailsafe.failIfNoSpecifiedTests=false -Dmaven.javadoc.skip=true verify

JDBCDdlLockBoundTestCase 36, CachedConnectionTestCase 86, JDBCStatementBoundTestCase 37, JDBCStorageRetryTest 66, StampConnectionTestCase 5 — 230 tests, 0 failures, 0 errors, 0 skips.

Passing is not on its own enough, so the guards were built with their defect put back:

variant	result
the connection left in the pool when the reset fails	testAConnectionWhoseBoundCouldNotBeTakenOffIsKeptOutOfThePool fails, expected [true] but found [false]
the elapsed check dropped from the rewrite	testALockWaitFarPastTheBoundIsLeftExactlyAsItIs fails: a 1205 that arrived after 50 s comes back named as the 5s of ...ddl.lock.timeout
the transaction block assumed rather than asked	testAConnectionInAutoCommitIsGivenNoSetLocal fails, lists don't have the same size expected [1] but found [2] — the set local issued
the rollback to the savepoint left out	testTheTransactionIsTakenBackToBeforeASettingThatFailed fails, Wanted but not invoked: connection.rollback(savepoint)
the round trips of the bound left unbounded	testTheRoundTripsOfTheBoundCarryOneOfTheirOwn fails, armed no socket read timeout: []
MySQL loosening a session that gives up sooner	testAMysqlSessionAlreadyTighterThanTheBoundKeepsWhatItHas fails, expected [2] but found [4]
SQL Server reading -1 as the tightest value there is	testWhatEachEngineIsToldAroundADdl fails for that engine, expected [4] but found [2], and testARewrittenSqlServerLockWaitIsMadeNoMoreReplayable with it
the DDL of the degraded path run outside the rewrite	testADdlBoundedByASettingWhoseCloseFailedStillNamesTheProperty fails: the failure keeps its bare 1205
the restore left out of the branch that handles a SET which failed	testASettingThatBrokeOnTheCloseOfItsStatementIsStillTakenOff fails — the value never given back
the catch of restoreDdlLockBound() allowed to throw	that case and both ...ValueThatCouldNotBeGivenBack... cases fail, the restore's own SQLException taking the place of the DDL's outcome
the state and the vendor number dropped from the rewrite	testALockTheDdlGaveUpOnNamesTheProperty (expected [55P03] but found [null]), testARewrittenMysqlLockWaitStaysTheConflictAWriteKnows and testARewrittenSqlServerLockWaitIsMadeNoMoreReplayable (expected [1205] but found [0], expected [1222] but found [0])

Under a real lock: testTheDdlGivesUpOnALockAnotherSessionHolds in jdbc/TestCase, inherited by PgSqlTestCase, MySqlTestCase, MsSqlTestCase and OracleTestCase. It sets the property to 2 s, opens a tree, holds an uncommitted insert on it from a second raw DriverManager session — the lock every engine conflicts a drop table with — and asserts deleteTree() fails rather than hanging. The failure is asserted by the engine's own verdict (lockNotAvailable(): ORA-00054 / 55P03 / 1205 / 1222) before the property name is looked for, so a drop that failed because the table was not there or because a privilege was missing no longer passes it — on Oracle above all, where nothing of ours is set and only the absence of our property name was being checked. @Test(timeOut = 120000), so a regression is a failure rather than a hung suite.

The other half of that contract on a real engine: testAConnectionGetsItsLockBoundBackAfterADdl borrows through CachedConnection.getConnection(), reads what the session carries, runs a DDL that reads it again from inside the bound, and reads it a third time afterwards — so the value a pooled connection is left with is asserted by the engine rather than by the string handed to a mock. PostgreSQL asserts the other shape of it, which a single "it is back" assertion would have got wrong there: the set local is still in force when withDdlLockBound() returns and is gone only after the commit that ends the transaction.

[vharseko]

Requesting changes on the first three; the rest are smaller and marked as such.

I read the description and #885 first, and where a code comment already states a trade-off deliberately — postgres, oracle, executeSessionStatement() staying outside the bound — I say so in the comment and only record what the stated reasoning does not cover. Nothing here asks for the session-wide placement the description rejects: that argument stands.

Worth changing

1. A restore that fails leaves the bound on a pooled connection (1350). The javadoc's escape hatch — the next borrow validates and drops it — is a liveness check, so it does not fire.
2. The two statements around the DDL carry no bound of their own in the one case where the DDL does carry one (bulk.timeout set), and the restore in the finally can hang the very open #877/#882 exist to unblock (1400, 1305).
3. On mysql the rewritten message can name a wait and a property that had nothing to do with the failure (1376).

Smaller

4. set local is a silent no-op without a transaction block (1036).
5. On postgres a failed SET ends as a failed backend open, not as "unbounded, as before" (1282) — recording the consequence; the trade-off itself is stated and I am not re-arguing it.
6. Three IllegalStateExceptions reachable before the first try (1052).
7. ddlLockBoundSeconds() duplicates StatementBound.seconds() (945).
8. The property javadoc does not say oracle is exempt (940) — documentation only.
9. An Importer left unclosed in the new test (JDBCDdlLockBoundTestCase:414).

Checked and not raising: the per-statement placement of the bound — the description quantifies the cost, and openTree() really is guarded by isExistsIndex() on all four dialects, so it stays the cold path; and sequential = true against the system property the class sets, since opendj-server-legacy/pom.xml runs surefire with <parallel>none</parallel>.

[vharseko]

Requesting changes. Checked against the head of this branch (11491f1), including the machinery this change leans on — CachedConnection's pooling and validation, timedOut()/Backstop, isConflict/isRetryableConflict/write(). Line numbers below are of the new file.

What stands — six comments

• JDBCStorage.java:1272 — on mysql and sql server the bound overwrites a deployment's own tighter lock_wait_timeout / LOCK_TIMEOUT instead of taking the minimum, which is the argument used a few lines up to leave oracle alone. The displaced value is already in hand.
• JDBCStorage.java:1293 — where the SET reached the server and only close() failed, the DDL runs with the bound in force but outside the try that rewrites its failure, while the log says the wait was left unbounded.
• JDBCStorage.java:1357 — the "left behind" warning re-reads ddlLockBoundSeconds() at log time instead of naming the value actually stranded on the connection.
• JDBCStorage.java:436 — ddlLockBoundLeftBehindWarned is once per storage, so only the first poisoned pooled connection is ever reported.
• TestCase.java:192 — nothing on a real engine asserts the restore, though sessionLockBound() is already there for it.
• TestCase.java:238 — the oracle branch of the new container test passes for any failure that is not slow.

Withdrawn

This review crossed with review 5132390217 from the same account, submitted 70 seconds before it; when I checked this PR for existing rounds there were none. Five of my comments repeated points that review already makes, in places better. I have deleted them from the diff so nobody reads the same thing twice, and record here what they were and why they are retracted:

• the bound stranded on a pooled connection when the restore fails — already at 1350, which also names the pool.destroy(this) path.
• the rewrite naming a property that bounded nothing on a 1205 — already at 1376, with the same ALGORITHM=COPY case.
• lockNotAvailable() walking WITH_THE_RELEASE — already at the end of that same 1376 comment, and framed better there: failureScope() uses the same walk deliberately, so it may be the intended convention, which mine did not allow for.
• the three parallel switches and their default: throw — already at 1052, which is also where I was wrong: those throws are reachable from withDdlLockBound() before any try, not the dead code I called them.
• scoping the bound per transaction rather than per statement — that review checked the per-statement placement and declined to raise it, having verified openTree() is guarded by isExistsIndex() on all four dialects. I had not verified that; it stays the cold path and the point does not stand.

Nothing in that review is disputed here.

[maximthomas]

All fifteen taken, in e325f6f. Two landed differently from the remedy proposed and one is scoped narrower than the finding; those three are argued in their threads and summarised below. The description is rewritten for the result rather than patched.

Requested changes

1. A failed reset strands the bound on a pooled connection (1350)

The escape hatch was isValid(), a liveness check such a connection passes — you are right that it does not fire. CachedConnection.poolable is no longer final, keepOutOfThePool() turns it off during the borrow the way relaxReadBound() does at establish time, and close() then takes the pool.destroy(this) path, permit included.

2. The round trips of the bound carry no bound of their own (1400, 1305)

Both cases hold; the reset in the finally is the worse one. All of them — readback, savepoint, setting, reset — now run under boundedSessionCall(), which holds the socket read timeout at SESSION_STATEMENT_BOUND_SECONDS. The backstop rather than a query timeout: the cancel layer is the one executeSessionStatement() has to stay out of, and it is also the layer that cannot help against a server that has stopped answering.

3. On MySQL the rewrite can name a wait and a property that had nothing to do with the failure (1376)

Landed differently. The guard is there, but as an upper bound, because timedOut()'s check does not catch your case: it only returns early when elapsed < bound - CLOCK_SLACK_MILLIS, and a 1205 after 50 s passes that at a bound of 5 s. A lock timer fires at its own value, so a wait that ran far longer was ended by something else — LOCK_BOUND_SLACK_MILLIS past the bound, the failure is left exactly as it arrived. Your smaller point about lockNotAvailable() decided itself against the code: WITH_THE_RELEASE is documented as being for questions about the connection, and this asks what the engine did with the statement, so it is WITHOUT_THE_RELEASE now.

Smaller

4. set local is a silent no-op without a transaction block (1036)

inATransactionBlock() asks getAutoCommit() — no round trip on any of the four drivers — and issues nothing where it answers true, reporting through the same warning.

5. On PostgreSQL a failed SET ends as a failed backend open (1282)

A savepoint is taken in front of the setting and rolled back to in the catch. It closes more than 25P02: the rollback also undoes a set local that reached the server and failed only on close(), which is the other half of finding 11.

6. Three IllegalStateExceptions reachable before the first try (1052)

The three settings are abstract methods of Dialect, answered per constant; the default: throw branches are gone. A constant added later cannot compile without supplying them.

7. ddlLockBoundSeconds() duplicates StatementBound.seconds() (945)

One boundSeconds(property, defaultSeconds), called by both.

8. The property javadoc does not say Oracle is exempt (940)

Said, as its own paragraph, with alter system set ddl_lock_timeout as the answer to an ORA-00054.

9. An Importer left unclosed in the new test (JDBCDdlLockBoundTestCase:414)

try-with-resources, with the assertion moved inside the block so the statements of close() stay out of the case.

From the second review

10. The bound overwrites a deployment's own tighter setting instead of taking the minimum (1272)

Scoped narrower. Taken on MySQL and SQL Server, and slightly stronger than min(): where the session is already at least as tight, nothing is issued — no setting, no reset, no rewrite. -1 on SQL Server is handled as "wait forever" rather than as the tightest value there is, which a plain min() would have got wrong. PostgreSQL is left out: the value is not in hand there and reading it back is the round trip a set local exists to save. One method if you want it there too.

11. A SET whose close() failed leaves the DDL bounded but outside the rewrite (1293)

The catch falls through into the same try that maps the failure, so such a DDL is reported as the bounded one it is. On PostgreSQL the savepoint rollback settles it the other way — the bound really is off, which is what the log line says. On MySQL and SQL Server the wording changed to "may bound nothing", since no driver tells the two apart; the reset moved from that catch into the finally, so the DDL now runs under a setting that may be in force instead of having it taken off in front of it.

12. The "left behind" warning re-reads the property at log time (1357)

It names the statement that was issued, verbatim, and says the connection may carry it — which is all that is known — while asserting what is certain: that connection is closed rather than pooled.

13. ddlLockBoundLeftBehindWarned reports only the first poisoned connection (436)

Throttled AtomicLong at 10 s, the shape CachedConnection uses for the read bound it could not lift. It spreads less than it did now that the connection is destroyed, but each occurrence costs the pool a connection, which is worth repeating rather than not.

14. Nothing on a real engine asserts the reset (TestCase.java:192)

testAConnectionGetsItsLockBoundBackAfterADdl: borrow, read, DDL that reads from inside the bound, read again. PostgreSQL asserts the other shape of the contract — still in force after withDdlLockBound() returns, gone after the commit — which a single "it is back" assertion would have failed. The existing case uses dialect() now instead of dialectOf() plus an assertNotNull.

15. The Oracle branch passes for any failure that is not slow (TestCase.java:238)

Both branches now assert lockNotAvailable(failure, dialect) first — ORA-00054 / 55P03 / 1205 / 1222 — so a drop that failed because the table was missing or the privilege was, or that never reached the engine, fails the case.

Verification

JDBCDdlLockBoundTestCase 36, CachedConnectionTestCase 86, JDBCStatementBoundTestCase 37, JDBCStorageRetryTest 66, StampConnectionTestCase 5 — 230 tests, 0 failures. Each new guard was also built with its defect put back; the eight variants and the case each one fails are in the description.

[vharseko]

All fifteen are closed, and I checked each against the head rather than against the answer — including the
three that landed differently, which I go through below. The code is where I would approve it.

Requesting changes on one thing that is not the code.

Blocking

build.log is committed at the root of the repository

e325f6f4dc — the commit that answers this review — adds build.log: 3125 lines, about 196 KB of raw Maven
output.

I read it through. No credentials: every password hit is a test class name (PasswordModifyExtendedRequest TestCase, testAPasswordHoldingASlashPairIsNotLeftInTheLog). It does carry local paths, though —
/private/tmp/opendj-885-ddl and /Users/maximthomas/.m2 — and squash-merge puts them in the history of
master permanently, where a later removal does not reach them.

Nothing caught it because .gitignore has no rule for it: *.class, target/, hs_err_pid* and the IDE
entries, and nothing for *.log. So the fix is two lines — drop the file, and add the rule so the next one
does not arrive the same way.

That is the whole of the request. Everything below is either verified, a dismissal, or an answer to the
question you put to me.

The three that landed differently

Item 3 (1376) — the upper guard rather than timedOut()'s check. You are right that the check I pointed
at does not catch the case: timedOut() returns the failure untouched only while elapsed < bound - CLOCK_SLACK_MILLIS, and a 1205 arriving at 50 s clears that comfortably at a bound of 5 s. The inversion is
right for the reason you give — a lock timer fires at its own value, unlike a cancel a server is free to act
on late — and gaveUpOnTheLock (:1630) leaves the failure exactly as it arrived past
seconds*1000 + LOCK_BOUND_SLACK_MILLIS. The absence of a lower guard is argued in that method's javadoc, so
I am not re-opening it.

Item 10 (1272) — scoped narrower. I checked the part that decides whether the narrowing is safe: the early
return if (bound==null) { return action.run(); } (:1395) sits outside the try that calls
gaveUpOnTheLock, so a session that keeps its own tighter setting also keeps its own failure, unrenamed.
"No setting, no restore, and no rewrite of a failure this bound had no part in" is accurate as written. Reading
SQL Server's -1 as "wait forever" rather than as the tightest value there is is the part a plain min()
would have got wrong, and it is right here.

Item 11 (1418) — the restore moved into the finally. The catch reports, rolls the postgres transaction
back to the savepoint, and falls through into the same try that maps a failure through gaveUpOnTheLock, so
a DDL left bounded by a setting whose close() failed is now reported as the bounded one it is. The trade you
name — mysql and sql server running the DDL under a setting that may be in force rather than having it taken
off in front of it — is the right way round, since that setting is the thing this PR exists to put there.

I also checked the one way item 2's fix could have been undone by the machinery around it: boundedSessionCall
holds a backstop, and applyBackstop arms nothing while a statement of an unbounded class is in flight, so an
outer unbounded hold would have left the round trips unbounded after all. It cannot happen —
execute(statement, StatementBound.BULK) lives inside the action lambda at both call sites (:2704,
:2714), so the readback, the savepoint and the setting all run before the first unbounded hold is taken, and
the restore after it is released. And keepOutOfThePool() is the shape you describe: poolable is volatile
rather than final, turned off during the borrow, and close() takes the destroy path.

The question you put to me: postgres and the displaced value

Leave it. Do not pay the round trip there. set local exists precisely to avoid the readback; what is loosened
is loosened for the length of the DDL's own transaction and is gone with the commit that ends it; and the
asymmetry is stated at the constant, which is where the next reader looks. One method's worth of code is not
the cost — the round trip on every DDL of a backend opening its trees is, and it buys a rule that has almost
nothing to bite on there.

CodeQL 1283 is a false positive, and worth dismissing as one

JDBCStorage.java:1541, raised on this round's code: "Missing catch of NumberFormatException" on
Long.valueOf(rows.getString(1).trim()).

It is caught. That call runs inside the lambda handed to boundedSessionCall, and the call site one line below
is catch (SQLException | RuntimeException e) with the comment saying so outright — "a value that is not a
number arrives unchecked". The rule looks for a catch naming NumberFormatException or
IllegalArgumentException and does not credit a broad RuntimeException. Worth closing the alert with that
reason rather than leaving it open on the repository after the merge.

---

Two things I checked and am not raising. The description is genuinely rewritten for this round rather than
patched — keepOutOfThePool, boundedSessionCall, the savepoint, boundSeconds, the already-tighter rule,
the abstract Dialect methods, the upper guard and the 36 cases are all in it — which is not something I could
say of the other three branches this week. And JDBCDdlLockBoundTestCase.java carries the standalone
Copyright 2026 3A Systems, LLC. a new file takes.

Reviewed against pull/936/head (e325f6f4dc). CI I cannot report on: at the time of writing almost every job
of that head is still pending — the build-maven matrix, wait-msi-artifact and the Analyze jobs — with no
failure among the ones that have finished. No database here either; the dialect behaviour is read off the code
and the engine defaults, not exercised.

[maximthomas]

118c8e86 on top of e325f6f4. One change, and no code in it: the file is gone and the rule that should have kept it out is in.

Requested change

1. build.log committed at the root of the repository — removed, and *.log ignored

Confirmed as described: 3125 lines, 200,608 bytes, added by e325f6f4dc, carrying /private/tmp/opendj-885-ddl and /Users/maximthomas. It is a local Maven run redirected to a file and was never part of this change.

git rm for the file, and *.log into the Maven section of .gitignore — next to target/, where the output of a build belongs — so the next redirect is not offered up for staging. Confirmed there was nothing matching it before (*.class, target/, hs_err_pid* and the IDE entries, and no rule for a log), and that no other .log file is tracked on this branch or on master, so the rule takes nothing else out.

One correction on the risk as stated, since it changes nothing about the fix but does change what the fix is for: a squash merge takes the diff of this branch's tree against master, so a removal made on the branch does reach it — no merged commit on master will carry build.log, whether it is removed here or not. What the removal cannot reach is e325f6f4dc itself, which stays behind refs/pull/936/head on this repository after the merge. Out either way, because it has no business in the branch — not because the squash would otherwise keep it.

CodeQL 1283 — the NumberFormatException is caught, by a broad RuntimeException

Same reading. Long.valueOf(rows.getString(1).trim()) (:1541) runs inside the lambda handed to boundedSessionCall, which is try { return call.run(); } finally { … } and catches nothing, so the exception leaves sessionValue through catch (SQLException | RuntimeException e) at :1544 — the comment on that catch is this case and nothing else. NumberFormatException is an IllegalArgumentException, and the same catch also takes the NullPointerException from a null column value that the rule does not mention. The rule wants one of the two named.

It is worth closing as a false positive on those grounds; I do not have the permission on this repository to dismiss it from here.

Nothing to change on the rest

The three that landed differently and the postgres question all resolve to the code as it stands, so nothing moved: gaveUpOnTheLock's upper guard (:1630), the early return for an already-tighter session sitting outside the try that renames a failure (:1395), the restore in the finally, and the postgres readback left unpaid with the asymmetry stated at the constant.

CI restarted on the new head with this push, so there is nothing to report on it yet either.

[vharseko]

Approving. build.log is out and the rule that should have kept it out is in, which was the whole of the request — the code itself was already where I would approve it last round.

The removal, checked against the tree rather than taken on the word of it

118c8e86 drops the file and puts *.log in the Maven section of .gitignore, next to target/. Confirmed here: nothing matching is tracked on this branch or on master, so the rule takes nothing else out; there is no negation in .gitignore for it to collide with; and the branch is three commits on 995d54a8 with nothing unrelated carried in.

Your correction on the risk stands and mine does not. A squash takes the diff of this branch's tree against master, so a removal made here does reach it and no merged commit would have carried the file either way. What the removal cannot reach is e325f6f4dc behind refs/pull/936/head — which is reason enough on its own, and is the reason I should have given.

CodeQL 1283 is dismissed as a false positive, so nothing of this round is left open on the repository after the merge.

I read the head again rather than only this commit: the headers, the state and vendor number the rewrite carries over, the permit accounting behind keepOutOfThePool() (poolable is read in one place, and its false branch is the pool.destroy(this) that gives the permit back), and the invariants #866, #877 and #882 wrote down — the innodb_lock_wait_timeout this does not touch among them. Nothing moved that should not have.

Recorded for a follow-up, and not a condition of this approval

withDdlLockBound() returns early and says nothing where the dialect is unknown:

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 1377 to 1386 in 118c8e8

	*/
	<T> T withDdlLockBound(Connection con, Dialect dialect, Execution<T> action) throws SQLException {
	final int seconds=ddlLockBoundSeconds();
	// Asked first with nothing displaced yet, which is what tells an engine this bound is never put
	// on - oracle, and one none of these settings fit - from an engine it is put on. What the session
	// actually carries is read below, and can take the bound off again all by itself.
	if (dialect==null || seconds<=0 || dialect.ddlLockBoundSql(seconds, null)==null) {
	return action.run();
	}
	final String query=dialect.ddlLockBoundQuery();

dialectOf() keys on the driver class name, so MariaDB Connector/J — or a Percona- or Aurora-branded driver — against a live MySQL answers null. That is a session whose lock_wait_timeout is a year, which is the wait this PR exists to end, and a deployment that set ddl.lock.timeout there is told nothing about it: reportTheWaitIsLeftUnbounded fires on a SQL failure and never on this return, and the property javadoc names the oracle exemption alone.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 952 to 960 in 118c8e8

	/**
	* The bound on the wait of a DDL of this backend for a lock another session holds, in seconds. A
	* value of {@code 0}, or a negative one, leaves it waiting for as long as the engine lets it,
	* which is what this backend did before this bound existed (#885).
	* <p>
	* A bound of the statement is the wrong tool for this, which is why the DDL of this backend is
	* {@link StatementBound#BULK} and stays there: a query timeout cannot tell a statement that is
	* <em>working</em> - a create index of a populated table - from one that is <em>queued</em> behind

This is item 5 of my review of #934, one property later — "Being conservative there is defensible … but the silence is not, given that the strict parsing of the value exists so that a deployment that asked for a bound and misspelled it is not left with none quietly." reportUnknownDialect closed it there, but it speaks of the connect bound and pool.timeout and keys off the url rather than the driver, so it does not reach this bound. The conservative behaviour is right and testAnEngineThisBackendDoesNotKnowIsLeftAlone pins it deliberately; it is only the silence that is worth a line, here or in a follow-up of its own.

CI I cannot report on yet: most of the matrix is still pending on this head, with no failure among the jobs that have finished.

[vharseko]

Code review

The merge only (6752ecc3e8). The approval of 118c8e86b7 stands and nothing below is a condition of it.

Checked first that the resolution is what the commit message says it is, and it is. Master's drop loop is transplanted verbatim into dropCatalogTables() — same skip, same three counts, same order (catalogTables() still puts the catalog last), same single commit, same rollback at the removeStorageFiles() level; the empty-catalog early return commits exactly where master's zero-iteration loop did. TestCase.java is adjacency alone, and the two container cases are no more order-dependent than before: testTheStandingReadBoundReachesTheSocket restores readTimeoutMillis and drains the pool on the way out. CachedConnection.java merged with no conflict reported and none hidden either: poolable still has exactly one reader (close(), choosing pool.destroy(this)) and one writer added by this branch, #934 adds neither, and each boundedSessionCall() takes a fresh Backstop, so state.previous is never read under a lift and nothing of #934's standing read bound is left off. .gitignore, the five license headers and the five-file footprint all survive — git diff 2a7bb9d7ed 6752ecc3e8 reverts nothing master shipped, and no .log came back with the merge.

Two things did not survive it.

Found 2 issues:

1. The loop's own lookup is now inside the bound but outside the rewrite, so a lock this bound ends is reported as the bare vendor error. dropCatalogTables() puts master's whole loop under withDdlLockBound(), and that loop calls isExistsTable() once per row — a statement neither parent ran inside the bound: master had no bound at all, and the branch's dropTables() ran only the drop table statements. isExistsTable() turns every failure into a StorageRuntimeException, and withDdlLockBound() renames only what arrives as a SQLException, so a 55P03 / 1205 / 1222 that the lookup gave up with never reaches gaveUpOnTheLock(). The asymmetry is inside one iteration: the drop is renamed and names the property, the lookup deciding whether to issue that drop is not, and an operator meets exactly the unexplained state this PR exists to stop shipping.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2835 to 2843 in 6752ecc

	final TreeName catalogTree=getCatalogTree();
	return withDdlLockBound(con, dialectOf(con), () -> {
	final ClearCounts counts=new ClearCounts();
	for (final Map.Entry<TreeName,String> tree : trees.entrySet()) {
	final String tableName=tree.getValue();
	final boolean isCatalog=catalogTree.equals(tree.getKey());
	if (!isExistsTable(con, scope, tableName)) { // a row of the catalog outliving its table
	reportClearLine(LocalizableMessage.raw(
	"jdbc: backend %s names tree %s, whose table %s is not there: nothing to drop for it",

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2718 to 2723 in 6752ecc

	}
	return false;
	});
	} catch (Exception e) {
	throw new StorageRuntimeException(e);
	}

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2115 to 2121 in 6752ecc

	final long startedAt=nanoTime();
	try {
	return action.run();
	}catch (SQLException e) {
	throw gaveUpOnTheLock(e, dialect, seconds, startedAt);
	}finally {
	if (restore!=null) {

2. dropTable()'s javadoc was left behind by the resolution. ClearCounts and dropCatalogTables() were inserted between the comment and the method it documents, so the block now stands in front of ClearCounts — "Drops one table of a clear. It is a method of its own", of a counts holder, followed immediately by that class's own one-line javadoc — while dropTable() 60 lines below carries none. It is the copy of the ordering reasoning that sat at the drop itself; catalogTables() still states the ordering, so nothing is lost outright, but the comment now describes neither the class it precedes nor the loop that moved.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2790 to 2800 in 6752ecc

	/**
	* Drops one table of a clear. It is a method of its own so that the order {@link
	* #removeStorageFiles()} drops in can be watched from a test: what names the trees has to outlive
	* them, and that guarantee is the loop's - it holds because the loop walks the catalog's map in
	* the order that map was built in, and a test asserting on the map instead would go on passing
	* over a loop that had stopped doing so.
	*/
	/** What the drop loop of a clear did, which {@link #reportClearOutcome} accounts for. */
	static final class ClearCounts {
	/** Tables dropped, the catalog of the backend among them. */

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2859 to 2863 in 6752ecc

	}
	void dropTable(Connection con, String tableName) throws SQLException {
	try (final PreparedStatement statement = con.prepareStatement("drop table " + tableName)) {
	// bulk, as #882 made every drop of this backend: nobody waits on a clear, and what it takes

---

Checked and not raising. The bound sitting around the whole loop rather than around each drop, and what that does to gaveUpOnTheLock()'s slack window once the loop's earlier drops are work of their own: that shape is the branch's own, it is argued at LOCK_BOUND_SLACK_MILLIS and it was approved last round — the merge only enlarged what runs inside a lambda that already had it. The unknown-dialect silence recorded last round is untouched and still a follow-up. Both new cases earn their place: testAClearWithNoTableToDropIsGivenNoBound fails with the early return deleted (the set local is issued into an empty loop), and testTheDropLoopOfARemovedBackendIsBoundedOnce fails on the exact-list equality if the bound moves per drop, so "once" is really pinned and not just "a bound appears".

CI I cannot report on: every job of this head — the build-maven matrix, wait-msi-artifact and the four Analyze jobs — is still pending, with no failure among them. No database here either; the dialect behaviour above is read off the code and the engine defaults rather than exercised.

[vharseko]

Requesting changes on the two findings of my review of this merge a few minutes ago, which supersedes the approval of 118c8e86b7 until they are answered. The reasoning, the links and what I checked and am not raising are all there and are not repeated here — this only changes the state of the round, and nothing new was found since.

Both are of the merge (6752ecc3e8) and of nothing that was approved last round:

1. isExistsTable() is inside the bound and outside the rewrite (JDBCStorage.java:2841). dropCatalogTables() runs master's whole loop under withDdlLockBound(), and that loop's lookup wraps every failure in a StorageRuntimeException, which catch (SQLException e) { throw gaveUpOnTheLock(...) } (:2118) cannot see. A 55P03 / 1205 / 1222 the lookup gave up with therefore reaches an operator bare, in the same iteration whose drop would have named the property. Neither parent ran a statement of that kind inside the bound, so this arrived with the resolution.

2. dropTable()'s javadoc was left behind (JDBCStorage.java:2791-2797). ClearCounts and dropCatalogTables() went in between the comment and its method; the block now introduces a counts holder as "a method of its own", and dropTable() (:2861) has no javadoc at all.

The second is a line to move. For the first, whatever closes it is fine by me — the lookup answering with the SQLException it was given, the rewrite reaching an unchecked failure the way removeStorageFiles()'s own catch (Exception e) already has to, or the lookup taken back out of the bound — as long as a lock this bound ended does not come out of that loop unnamed. If you conclude it should stay as it is, say why and I will take it.

[maximthomas]

Both closed in 9cc5a0efa0, on top of the merge you reviewed.

1. The lookup inside the bound and outside the rewrite

Confirmed as described: isExistsTable() answers with a StorageRuntimeException whatever it is given, withDdlLockBound() caught SQLException alone, and the drop loop is the one place where a statement of that kind runs inside the bound — openTree()'s own isExistsTable() guard sits outside it, in front of the commitStatement() that opens one.

Of the three closures you offered I took the second, the rewrite reaching an unchecked failure:

}catch (RuntimeException e) {
    throw gaveUpOnTheLock(e, dialect, seconds, startedAt);
}

with an overload that reads the chain for the engine's own way of saying the lock was not available — firstLinkMatching(e, WITHOUT_THE_RELEASE, EVERY_LINK, ...), the same walk lockNotAvailable() uses, so the rename asks the question the class's other classifiers ask — and puts that link through the existing gaveUpOnTheLock(SQLException, ...). The slack window is therefore the same one, and a wait far past the bound is still left alone wherever it arrived from. A failure the rename does not apply to is given back exactly as it arrived, class and stack intact; one it does apply to is wrapped in StorageRuntimeException, which is what removeStorageFiles() already reads to decide what it rethrows.

Why not the other two:

• The lookup answering with the SQLException it was given is not a signature this bound can afford to change. isExistsTable(Connection, TableScope, String) is what WriteableTransactionImpl.isExistsTable(TreeName) calls, and that one is read from openTree(), deleteTree(), isCatalogTableOpened() and the stamp sweep — none of which is on a throws SQLException path, and the test fixture overrides exactly that signature. The change would ripple far past the loop this finding is about.
• The lookup taken back out of the bound means naming every table first and dropping them all after, and that is a regression of the loop master shipped: its skip decides between leaving a catalog row where it is and dropping the table it names, and it has to keep being answered by a database that has already seen this loop's earlier drops.

The catch covers any future statement placed under a bound too, which is the shape the finding is really about.

2. dropTable()'s javadoc

Moved back in front of the method, and the one link it carries now names dropCatalogTables() rather than removeStorageFiles() — the loop whose order it is about is the one that moved. ClearCounts keeps its own one-line javadoc.

Tests

Two cases in JDBCDdlLockBoundTestCase, both over the drop loop with the metadata lookup failing:

• testALockTheLookupOfAClearGaveUpOnNamesTheProperty — a 55P03 out of the lookup arrives as a SQLTimeoutException naming org.openidentityplatform.opendj.jdbc.ddl.lock.timeout, with the engine's failure chained.
• testAFailureOfTheLookupThatWasNoLockWaitIsLeftExactlyAsItIs — a 42P01 comes out as the same object it went in as.

Each kills its own mutant and only its own: with the catch (RuntimeException) deleted the first fails (the lookup's failure was left as the engine reported it) and the second passes; with the link==null guard deleted the second fails (a failure that was no lock wait was renamed) and the first passes.

Verified

JDBCDdlLockBoundTestCase 39, CachedConnectionTestCase 100, JDBCStorageRetryTest 99, JDBCStatementBoundTestCase 44, ImportConnectionsTestCase 18, CatalogConnectionTestCase 13, StampConnectionTestCase 5 — 318 run / 0 failures / 0 skipped. No database here either: the container suites are not part of that run, so the dialect numbers above are still read off the code and the engine defaults rather than exercised.

[vharseko]

Approving. Both findings of my review of the merge 6752ecc3e8 are closed, and I checked each against the head rather than against the answer.

What this round contains

Your comment puts 9cc5a0efa0 "on top of the merge you reviewed". It sits on top of a second one - 69afe2d552, seventeen minutes earlier - which is also new since the last round and brings #891, #904, #901, #908 and #916 into the branch. Its own commit message says exactly what it resolved, so nothing was hidden; the comment just does not name it, and a reader of the comment alone would take this round for one commit. I reviewed both.

The merge

I reconstructed git's own auto-merge of 6752ecc3e8 with 5d176c6915 and diffed the committed tree against it. The whole hand resolution is the two things the commit message names and nothing else: master's AtomicInteger import kept beside this branch's AtomicLong, and the two cases holding a failure the bound renamed to the classification it had before the bound existed, moved from isRetryableConflict(...) to conflictVerdict(...).conflict. No third edit, and nothing of either side dropped.

Those two assertions are faithful and not merely green: on the pre-merge code MySQL 40001/1205 was isConflict -> true and SQL Server S0001/1222 -> false, which is what Conflict.AFTER_LOCK_WAIT and Conflict.NONE say under the new enum, and no call to isRetryableConflict survives anywhere in the tree.

What git resolves silently is where a merge this size actually goes wrong, so: master adds no DDL statement site; execute, bounded, boundedSessionCall, Backstop and applyBackstop are untouched by it; and it changes neither poolable nor keepOutOfThePool() - its only CachedConnection changes are isUnboundedWait()/boundedWait() and getNonNegativeProperty going package-private for #891. An import connection of #891 is borrowed from the same pool with the same setAutoCommit(false), so it meets this bound on the same terms as any other connection and inATransactionBlock() reads it the same way.

One warning for whoever reads this merge after me: git diff 6752ecc3e8 5d176c6915 is not the diff to read. 6752ecc3e8 is not on master's line - the base is 2a7bb9d7ed - and that naive pairing makes it look as though master had deleted keepOutOfThePool() and made poolable final. It did neither.

1. The lookup inside the bound and outside the rewrite - closed

Of the three closures I offered you took the second, and it is the one I would have taken: the other two are the signature change and the loop regression you describe, and both cost more than the finding.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2188 to 2200 in 9cc5a0e

	try {
	return action.run();
	}catch (SQLException e) {
	throw gaveUpOnTheLock(e, dialect, seconds, startedAt);
	}catch (RuntimeException e) {
	// Not every statement running under this bound answers with the SQLException it was given:
	// the lookup deciding each drop of a clear wraps whatever it sees in a
	// StorageRuntimeException (isExistsTable), and it runs inside the same bound as the drop it
	// decides. Without this, a lock this bound ended reaches an operator as the bare vendor error
	// one line away from the drop that would have named the property.
	throw gaveUpOnTheLock(e, dialect, seconds, startedAt);
	}finally {
	if (restore!=null) {

The rename reaches an unchecked failure through the same classifier and the same slack window as the checked one - firstLinkMatching(..., WITHOUT_THE_RELEASE, EVERY_LINK, isLockTimeout), then the existing gaveUpOnTheLock(SQLException, ...) - so nothing new decides what counts as a lock this bound ended.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2415 to 2435 in 9cc5a0e

	/**
	* The same rename where the failure arrives unchecked, which is how a statement of the action that
	* is not the DDL itself answers: {@link #isExistsTable}, asked once per row by the drop loop of a
	* clear, gives back a {@link StorageRuntimeException} holding what the engine said. The chain is
	* read for the engine's own way of saying the lock was not available, and that link is put through
	* the rename above - so the same wait is named the same way whichever statement of the action was
	* the one waiting.
	* <p>
	* A failure the rename does not apply to is given back exactly as it arrived, keeping its class and
	* its stack. One it does apply to is wrapped again, in the class every unchecked failure of this
	* storage carries and the class {@link #removeStorageFiles()} reads to decide what it rethrows.
	*/
	RuntimeException gaveUpOnTheLock(RuntimeException e, Dialect dialect, int seconds, long startedAt) {
	final SQLException link=firstLinkMatching(e, WITHOUT_THE_RELEASE, EVERY_LINK,
	failure -> isLockTimeout(failure, dialect));
	if (link==null) {
	return e;
	}
	final SQLException renamed=gaveUpOnTheLock(link, dialect, seconds, startedAt);
	return (renamed==link) ? e : new StorageRuntimeException(renamed);
	}

The parts that could have gone wrong quietly:

• Nothing is swallowed. link==null and renamed==link both give the original object back by reference, so a failure that was no lock wait - and one that arrived far past the bound - leaves this method exactly as it entered it. The renamed==link test is sound because the checked overload has no third outcome: it returns its own parameter or a new object.
• The classification is preserved. The rename is built from link rather than from the wrapper, so the state and the vendor number are the ones a caller read before; removeStorageFiles()'s e instanceof StorageRuntimeException still holds, and its rollback still runs.
• The other call site stays inert. Everything in commitStatement()'s lambda is throws SQLException, so the new catch has nothing to take there - which is what makes this a change to the clear path and to nothing else.
• EVERY_LINK is no new risk. walkLinks() terminates on its identity set whatever the budget, and this is the budget lockNotAvailable() has used since the round that added it.

Both new cases earn their place, and each kills its own mutant and only its own, as you say. The assertSame in both is a real pin rather than an accident: givingUpOnTheLookup() re-stubs getMetaData() after TableScope.of() has used the original, and the mock's elapsed time is nowhere near the slack window.

2. dropTable()'s javadoc - closed

Back in front of the method, and the {@link} now names dropCatalogTables(), which is where the loop it is about lives. The insertion-order guarantee the block asserts still holds - readCatalogRows builds a LinkedHashMap. ClearCounts reads correctly with its own one-line javadoc.

OpenDJ/opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java

Lines 2954 to 2963 in 9cc5a0e

	/**
	* Drops one table of a clear. It is a method of its own so that the order {@link
	* #dropCatalogTables} drops in can be watched from a test: what names the trees has to outlive
	* them, and that guarantee is the loop's - it holds because the loop walks the catalog's map in
	* the order that map was built in, and a test asserting on the map instead would go on passing
	* over a loop that had stopped doing so.
	*/
	void dropTable(Connection con, String tableName) throws SQLException {
	try (final PreparedStatement statement = con.prepareStatement("drop table " + tableName)) {

Recorded, and not a condition of this approval

The description no longer describes the code. It was rewritten for e325f6f4dc and has not moved since, which is two rounds: it says JDBCDdlLockBoundTestCase, 36 cases and 230 tests where your own last comment says 39 and 318; it names "the drop table loop of removeStorageFiles()" as the second call site, which the merge moved into dropCatalogTables(); and it presents the rewrite as the one around the DDL, with no mention of the lookup or of the unchecked path this round added. Nothing in it is wrong about the design and the commit messages carry the rest - but it is the document a reader meets first, and last round I said of it that it was genuinely rewritten rather than patched, which I could not say of it now.

The unknown-dialect silence recorded two rounds ago is untouched and still a follow-up.

CI I cannot report on: every job of 9cc5a0efa0 - the build-maven matrix, wait-msi-artifact and the four Analyze jobs - is still pending, with no failure among them. No database here either, so the dialect behaviour above is read off the code and the engine defaults rather than exercised.