[#877] Bound every statement of the JDBC backend by the class of its call site

[vharseko]

Problem

Not one statement of the JDBC backend was given a setQueryTimeout: nineteen sites in JDBCStorage prepared a statement and waited for the database indefinitely. A row locked by an unrelated session, a table waiting for a metadata lock, or a database that stops answering mid-query parked the worker thread that issued it for good, with nothing in the log to say so.

This is the half of #872 that lives behind a successful login. #876 bounded the establishment of a connection; a statement on a connection that is already through was, until now, the one unbounded phase left — and deliberately so, since the read bound of the login is lifted once the login is over, because leaving it in place would fail every statement slower than it.

Change

The bound goes on the statement, not on the session. Setting the engine's own statement_timeout / MAX_EXECUTION_TIME / LOCK_TIMEOUT once per connection would be cheaper, but a pooled connection cannot carry a session setting: CachedConnection.close() only rolls back, so whatever one operation set would apply to whoever borrows the connection next — the same reason #866 gives its comment statements a connection outside the pool. All execution sites already went through execute() / executeResultSet(), so that is where the bound is applied.

One value cannot serve every call site, so the bound is per class of statement:

Class	Sites	Property	Default
OPERATION	the entry read, put/update/delete, the batches a cursor walks for a client, the VLV offset of positionToIndex(), the catalog lookups of openTree()	org.openidentityplatform.opendj.jdbc.query.timeout	120 s
BULK	select count(*), the delete from of clearTree(), the order by k desc behind positionToLastKey(), every statement an import issues, every batch of a cursor opened for a walk of a whole tree, create table, create index, drop table	org.openidentityplatform.opendj.jdbc.bulk.timeout	0 (no bound)
the statistics refresh	analyze / dbms_stats.gather_table_stats / update statistics after an import	org.openidentityplatform.opendj.jdbc.statistics.timeout	600 s

0, or a negative value, leaves a class unbounded, exactly as this backend ran before; a value that is not a number is ignored in favour of the default, as Integer.getInteger() has it, so a typo cannot silently unbound a class.

The split is not theoretical: BackendImpl.openBackend() logs NOTE_BACKEND_STARTED with getEntryCount(), which is a select count(*) over id2entry on every backend start, and AbstractTwoPhaseImportStrategy.beforePhaseOne calls clearTree() for every tree before an import writes its first record. positionToLastKey() belongs to the same class for the same reason: it has no key to seek on, so it is an order by k desc over the whole table — a scan and a sort of it on SQL Server, where k is a varbinary(max) that cannot be an index key — and RootContainer runs it once per base DN through EntryContainer.getHighestEntryID() on every open of a backend, outside the try/catch of BackendImpl.openBackend(). A single 120-second default would have broken the start of a large backend and every import-ldif.

The default of the operation class also sits above the lock timeout of the engines, which matters for the transaction replay of #867: MySQL surfaces contention as class 40 through innodb_lock_wait_timeout (50 s) and that has to stay a replayable conflict rather than become a cancelled statement.

The class follows the work, not the shape of the statement. An import issues the same select and the same upsert a client operation does; what differs is that nobody is waiting on it, and that on SQL Server it works the table unindexed. So the class belongs to the transaction: the two an ImporterImpl holds are bulk, and with them every statement an import issues — put() through upsert(), read(), and the batches of the cursor phase one walks (OnDiskMergeImporter.ID2EntrySource). Only the catalog lookups of openTree() keep the operation class whoever runs them: they read a data dictionary rather than the data, so a wait there is another session's metadata lock, which is one of the waits this bound exists to end.

A walk of a whole tree asks for it through the SPI. ReadableTransaction.openBulkCursor() is a default method answering exactly as openCursor(), so every engine that bounds nothing — JE, PersistIt, Cassandra — inherits it unchanged, and only the JDBC backend gives it a class of its own. What asks for it is what walks a tree whole with no client waiting: ExportJob, the whole-tree passes of VerifyJob, the tree and index dumps of dbtest, the load of the compressed schema, and the read that checks id2entry is there. Reading the class off the statement instead would not do: the opening batch of every cursor is the same unconditioned order by k that positionToLastKey() issues, so it would either unbound the first batch of every search or bound the walk of an export as if a client were waiting on it.

Two of those have nobody at a command line, which is what makes this a bound that has to be right rather than a preference:

• ID2Entry.afterOpen() reads the first batch of a cursor over id2entry on every open of a backend (EntryContainer.open() ← RootContainer.openAndRegisterEntryContainers() ← BackendImpl.openBackend());
• LDAPReplicationDomain.loadGenerationId() computes the generation ID of a domain the first time it starts — computeGenerationId() → exportLDIF() → ExportJob, a walk of the whole of id2entry.

On SQL Server, where every batch of such a walk is a scan and a sort of the table, both of those failed at two minutes on a backend large enough.

The rows are read inside the bound. executeResultSet() hands the rows to its caller instead of returning a live ResultSet, so the transfer runs while the bound is still armed. A driver hands rows over as they are asked for, and setQueryTimeout covering ResultSet.next() is optional in the JDBC contract ("drivers may also apply this limit"): PostgreSQL and MySQL buffer a result whole and are not affected, but Oracle prefetches ten rows against batches of a thousand and SQL Server buffers adaptively, so a drain outside the bound is a wait with nothing bounding it — on SQL Server that is #877's own symptom, since a select under READ COMMITTED really does block on a row another session holds.

Two layers, because the first one is not answered everywhere. setQueryTimeout cancels the statement and keeps the connection — every driver implements it by cancelling, not by a socket timeout: pgjdbc opens a connection of its own to send a CancelRequest, mysql-connector-j issues KILL QUERY, and ojdbc and mssql-jdbc send a break on the same socket. Behind it, a socket read timeout is armed for the duration of the statement and released afterwards, so the wait ends even when the cancel is not acted upon.

Oracle is why that second layer exists, and the container suite is what found it: with setQueryTimeout(5) the blocked write ran for the full 600 s of the test harness, parked in SocketDispatcher.read0 under OracleStatement.doExecuteWithTimeout — the timeout was armed and never arrived, because a session blocked in a row-lock enqueue does not process the break its driver sends. Reaching the second layer costs the connection (the driver closes it), which is the price of a wait the database was never going to end on its own.

That second layer belongs to the connection, not to the statement, so it is arbitrated between the statements running on one. A socket read timeout is a property of the socket, and this backend does share a connection: an Importer holds a single one for the whole of an import and writes to it from every phase-one worker and every phase-two task. Armed and released per statement there, the first statement to finish would take the backstop away from every statement still in flight, and a statement of a class carrying no bound would run under whatever value a concurrent one happened to arm — dying at a bound it was never given, and naming no property for it, since such a statement never reaches the classification below.

So the value armed is the loosest of the bounds of the statements in flight, a statement with no bound of its own takes the backstop off for as long as it runs, and what the connection carried before is put back when the last of them is through. The state is kept per physical connection, by identity: CachedConnection.prepareStatement() hands the statement to the connection it wraps, so that is the one a statement reports, while the catalog lookups hold the wrapper of that same connection — both unwrap to the same entry, and an entry lives only while statements are running on its connection.

It only ever tightens: it is armed when the connection carries no read timeout at all - 0, "no timeout" in the JDBC contract - or one looser than itself, and where nothing is changed nothing is put back afterwards. Being the cancel's bound plus a margin, it is the looser of the two by construction, so setting it unconditionally would have replaced a read timeout a deployment gave its connections (the setting #885 asks for) exactly while a statement was running.

It is also the only layer the catalog lookups of openTree() can be given: DatabaseMetaData.getTables() and getIndexInfo() take no query timeout, and they run once per tree on every open, behind the same locks as the create table they guard.

The statistics refresh keeps a property of its own, under both layers. It is not a class of StatementBound: what it takes follows the size of the table it describes, so a class would put 120 s over a statement its own property allows 600. It does need the second layer, and on the engine that most needs it — on Oracle this is dbms_stats.gather_table_stats, and it runs at the very end of a successful import, where a cancel that is not acted upon would park import-ldif with the data already committed and nothing left to report.

A driver that will not take one of the two layers keeps working, and says which one it is. setQueryTimeout raising SQLFeatureNotSupportedException degrades to the socket read timeout behind it, with one warning, rather than failing every statement. A driver with no network timeout at all says so the same way, and that is remembered for the storage — the scope of a driver — while a connection that merely failed the call, which is most often one on its way out, is remembered only while its own statements run and never speaks for the connections that are healthy. Each cause has a warning of its own.

A failure before the bound is passed through untouched, so a lock wait reported in class 40 stays the conflict a caller can replay. One that arrives at the bound is wrapped in a SQLTimeoutException naming the property that produced it — no driver knows why it was cancelled, and every one of them reports a cancellation differently (PostgreSQL 57014, Oracle ORA-01013, and neither of them as a SQLTimeoutException), so the bound is recognized by the time the statement took, measured on the monotonic clock, rather than by the class or the state of its failure. Where the cancel is not in force — a DatabaseMetaData lookup takes no query timeout, and a driver may refuse one — the statement is measured against what the socket read timeout really allows it, its bound plus the margin, instead of against a property that bounded nothing. The SQL state and the error number are carried over, and the failure being replaced is chained. The statement itself is left out of the message: a driver renders it with its parameters bound, and those are entry data.

Tests

JDBCStatementBoundTestCase — 25 tests, no database, 4 s: the defaults, that each class follows its own property, that a value which is not a bound leaves the statement unbounded while one that is not a number falls back to the default, that the bound reaches the statement and that an unbounded class costs no call at all, that the rows are read while the bound is still armed and a failure during that transfer is measured against the bound, that a driver without a query timeout keeps working under the backstop alone while one without a network timeout is asked once and a connection that failed the call is asked again, that a catalog lookup is bounded and that a failure inside the margin of the layer bounding it is passed through, that the scan behind the highest entry id is bulk while the batches a client walks are not, that a cursor opened for a walk of a whole tree takes bulk batches, that every statement of an import is bulk, that the statistics refresh runs under its own bound and the backstop, and both classification branches (a failure inside the bound arrives as the very instance thrown; one at the bound names the property and keeps its SQL state).

Five of them are about a connection carrying more than one statement at a time, which is what an import does — all on two threads, since that is the only way one statement outlives another: that the backstop is armed and put back, that it never loosens a tighter bound the connection already carries, that a statement of an unbounded class takes it off while it runs and gives it back afterwards, that it outlasts the statement that armed it, that with two bounds in flight the loosest is what is armed, and that a looser bound joining re-arms it and the tighter one gets its own back when the looser statement leaves.

ID2EntryTest — the read that checks the tree is there when a backend opens asks for a bulk cursor, the call site of this branch that no operator is standing at.

TestCase.testWriteBlockedByAnotherSessionGivesUpAtItsBound and testBulkStatementGivesUpAtItsOwnBound — another session holds every row of the tree in an uncommitted transaction, and the operation under test has to give up inside its bound. Only the class being tested is bounded and the other is set to 0, so a pass cannot be credited to the wrong property. The bulk case goes through Importer.clearTree(), which is where the delete from <table> of that class is reachable. Both assert the window of the bound — from the bound itself to four times it, or to the bound plus the margin of the second layer on Oracle, where that is what ends the wait — and that the failure names the property that produced it, so a run that gives up at MySQL's own 50-second innodb_lock_wait_timeout, or falls over at once for an unrelated reason, fails the test. The window is measured on the monotonic clock, as timedOut() measures the bound.

All four container suites pass with no skips — PgSql 55/55, MySql 55/55, MsSql 55/55, Oracle 55/55. The blocked write, given a 5-second bound, gives up at it on PostgreSQL, MySQL and SQL Server, and at the bound plus the 30-second margin of the second layer on Oracle — the cancel being ignored there and the socket read timeout ending the wait. No measurable overhead across the change: the suites stay within container noise of where they were.

Out of scope

Two refinements are not covered here, rather than stretched into a branch that cannot reach them. They are filed as #885:

• a read timeout for an established connection as a setting of its own — it belongs in CachedConnection.relaxReadBound(), which only exists on the branch of [#872] Bound the connect of the JDBC pool and report a connect it cannot make #876, so a branch off master cannot touch it without conflicting. The second layer above covers the same failure for the duration of a statement, which is when it matters;
• a lock timeout for the pooled connections — it has to be set once, when a connection is established (CachedConnection again), or it costs a round trip per transaction. With the statement bounded the hang is already over; what remains is a more precise error and, on the engines that report a lock wait in class 40, a replayable one.

commit() takes no bound of any kind and is left as it stands. So are the two statements that run on a stamp connection — the comment statement of #866 and the session setting (set lock_timeout / alter session set ddl_lock_timeout) issued when that connection is established: a stamp connection is given a lock timeout of its own and a socket read timeout in its connect properties.

Follow-up to #872 / #876.

Fixes #877

[vharseko]

@maximthomas one commit added since the review was requested — 3da33ea, worth a look before you start rather than after.

The socket read timeout armed behind the cancel of a statement was being set unconditionally. It is the cancel's bound plus a margin, so it is the looser of the two by construction, which means a connection that already carried a read timeout of its own had it replaced — with a weaker one — for the duration of every statement. Nothing in this repository sets such a timeout today, but #885 asks for exactly that setting, and it would have been silently ignored while a statement was running, which is when it matters.

armBackstop() now arms only when there is something to gain: when the connection carries no bound at all (0, "no timeout" in the JDBC contract) or one looser than the backstop. Where it changes nothing, it puts nothing back either.

Two tests came with it, so JDBCStatementBoundTestCase is 9 rather than 7: the backstop is armed and put back in order, and it is not armed at all in front of a tighter bound. The suites were re-run on that commit — the docker-free one 9/9 and PgSql 39/39; the other three dialects are unchanged by it, since the guard is reached only when a connection carries a read timeout and none of them does in the suites.

The "Two layers" section of the description now says this too.

[maximthomas]

Reviewed 58f2874 and 3da33ea. The OPERATION/BULK split is a sound design, and the "only ever tighten" guard in 3da33ea is correct: previous == 0 arms, 0 < previous <= backstop is left alone, previous > backstop is tightened and restored, and the -1 sentinel cannot collide because getNetworkTimeout() is non-negative by contract.

Two issues should be addressed before merge; the rest are minor.

Row transfer runs outside both timeout layers (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:148-163 releases the backstop before the caller reads a single row:

try {
    return execution.run();                  // returns a live ResultSet
}catch (SQLException e) {
    throw timedOut(e, bound, seconds, startedAt);
}finally {
    releaseBackstop(statement, backstop);    // socket read timeout back to 0
}

The drain happens afterwards — read() at :376-378, and fetchBatch() at :658-662 which pulls up to fetchsize (default 1000) rows.

setQueryTimeout covering ResultSet.next() is explicitly optional per the JDBC javadoc ("drivers may also apply this limit to ResultSet methods"). Of the four supported drivers:

• PostgreSQL / MySQL — fully buffered at execute (setFetchSize is never called; useCursorFetch defaults false). Not affected.
• Oracle (ojdbc8) — defaultRowPrefetch is 10 against batches of 1000, and OracleStatement.fetchMoreRows calls beginTimeout() only when serverCursor == true, which is false by default. Roughly 99 of every 100 round trips run with neither timeout armed, plus the LOB round trips for v blob.
• SQL Server (mssql-jdbc) — responseBuffering=adaptive is the default, and TDSCommand.startResponse cancels TDSTimeoutTask right after the first readPacket(); cancelQueryTimeout defaults to -1.

On SQL Server this means #877's own symptom is only nondeterministically fixed: default READ COMMITTED takes shared locks, so a select really does block on a row another session holds, and whether that block lands inside execute() (covered) or after the first 8 KB packet (uncovered) depends on where the locked row sits in the batch. Meanwhile the javadoc at :134-144 states that "A statement of this backend has to end" and that the socket read timeout "ends the wait even when the cancel is not acted upon" — not delivered on half the supported engines. That assurance also discourages the one mitigation that does cover the drain (a connection-level socket timeout), which on Oracle is not even reachable through the URL; it needs oracle.net.READ_TIMEOUT.

Suggested fix: hold the backstop for the statement's life — release it where the ResultSet/statement is closed rather than when executeQuery returns.

positionToLastKey() is bounded as OPERATION on the backend-open path (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:782-784:

public boolean positionToLastKey() {
    if (fetchBatch(null,null,0,true,1)) {

With no condition this emits select k,v from <table> order by k desc offset ? rows fetch next ? rows only through the single-argument executeResultSet (:108), i.e. OPERATION, 120 s.

It runs on every backend open, unconditionally per base DN:
BackendImpl.java:200 newRootContainer(...) — outside the only try/catch, which starts at :206 and wraps getEntryCount() — → RootContainer.java:225 ec.getHighestEntryID(txn) → EntryContainer.java:660-665 cursor.positionToLastKey() on id2entry.

On SQL Server k is varbinary(max) (:428) and cannot be an index key, as the comment at :478 already notes, so this is a full scan plus a top-1 sort. A large mssql backend that previously opened slowly now fails to open, and the only escape is disabling the bound globally.

This also contradicts the PR's own taxonomy: getRecordCount already uses BULK (:392), whose javadoc reads "a whole table at once" (:88) — which is exactly what an unfiltered order by k desc is.

One line at :658 covers this, positionToIndex, and the mssql first-batch case:

executeResultSet(statement, condition==null ? StatementBound.BULK : StatementBound.OPERATION)

Nits

• setQueryTimeout is unguarded (JDBCStorage.java:153): it sits outside any try, unlike armBackstop(), which catches and degrades. The JDBC spec allows SQLFeatureNotSupportedException, and this backend accepts an arbitrary URL with an ANSI fallback dialect — on such a driver every statement now fails where it previously worked.
• timedOut() misattributes late failures (:224): classification is purely by elapsed time, so a connection reset at 121 s is reported as "raise org.openidentityplatform.opendj.jdbc.query.timeout". System.currentTimeMillis() also means an NTP step backwards hides a real timeout and a step forwards manufactures one — System.nanoTime() is the right clock for a duration.
• backstopWarned is static (:172): one warning per JVM, shared across instances. With a driver lacking setNetworkTimeout it fires once during startup and is then permanently silent; a second JDBC backend never reports it at all.
• Two execution sites bypass bounded() (:410, :484): getMetaData().getTables(...) and getIndexInfo(...) are unbounded in both phases and run once per tree on every open. The commit message's "all nineteen execution sites" is accurate for those nineteen, but there are 21 places this backend sends work to the database — and the commit names "a table waiting for a metadata lock" as a motivating hazard, which is precisely what these two are exposed to.
• positionToIndex is O(offset) on every engine (:796): offset ? rows fetch next ? is driven straight from a client-supplied VLV position (VLVIndex.java:701), so a deep VLV request now errors at 120 s instead of answering slowly.
• BULK defaults to 0 (:89): one of the three hangs named in the method's own javadoc — a table waiting for a metadata lock — stays unfixed under stock settings, e.g. the MySQL create index in openTree() (:458). The trade-off is reasonable, but the description reads as if the whole class is closed.
• ImporterImpl.close() has no finally (:838): con.commit(); con.close(); — and the bulk bound makes this reachable, since a throwing clearTree() closes the importer and a throwing commit() then skips con.close(), leaking the connection with its transaction and locks.

[vharseko]

Thank you — both major findings were real, and every line reference in the review checked out. All of it is addressed in 1485984, and the description of the PR is updated where it claimed more than the branch delivered.

Row transfer runs outside both timeout layers

Confirmed and fixed. executeResultSet() no longer returns a live ResultSet: it takes what the caller makes of the rows and runs that while the bound is still armed, so the transfer, the classification of a failure and the release of the backstop all happen where the statement does. A ResultSet proxy would have been the smaller diff, but it puts a reflective call on every next() of a thousand-row batch, so the four call sites — read(), getRecordCount(), fetchBatch(), positionToKey() — pass a handler instead; each of them already consumed the rows inside a narrow try-with-resources, so nothing else moved.

Your driver survey matches what is in the file: setFetchSize is never called, so PostgreSQL and MySQL buffer whole and were never affected, and Oracle and SQL Server were the ones running the drain unarmed.

positionToLastKey() is bounded as OPERATION on the backend-open path

Confirmed and fixed, and the path is exactly as you traced it: BackendImpl.openBackend():200 → newRootContainer() outside the try/catch that starts at :206 → RootContainer.openAndRegisterEntryContainers() → EntryContainer.getHighestEntryID():660 → positionToLastKey() on id2entry, once per base DN on every open.

I did not take the one-liner at :658, though. condition==null is also true for the first batch of every cursor (next() passes currentKeyDb==null?null:">"), and with BULK unbounded by default that would put the opening batch of every search back outside any bound — on mssql the unindexed one. So fetchBatch() receives the class from its caller now: positionToLastKey() is BULK, cursor iteration and positionToKeyOrNext() stay OPERATION.

Nits

• positionToIndex is O(offset) — kept as OPERATION, deliberately, and the reasoning is now in a javadoc on the method. The offset comes from a client's VLV request, so it is on a search path: answering a deep offset with an error after two minutes is the outcome I want over parking a worker thread on it for as long as the walk takes. It is the one point of the review I did not follow; happy to revisit if you read it the other way.
• setQueryTimeout unguarded — fixed. A driver that raises SQLFeatureNotSupportedException now degrades to the socket read timeout behind it, with one warning, instead of failing every statement.
• timedOut() and the wall clock — the clock is System.nanoTime() now. The classification stays time-based for the reason the comment already gave, and the comment says plainly what it cannot tell apart: a failure of another kind arriving after the bound. That one is chained, not swallowed.
• backstopWarned is static — it is per storage now, and so is the new flag for the query-timeout warning above it.
• getTables() / getIndexInfo() bypass bounded() — both go through the bound now. DatabaseMetaData takes no query timeout, so they get the socket read timeout alone, as OPERATION: they ask a data dictionary rather than doing work of their own, so a wait there is another session's metadata lock. Note this touches isExistsTable(), which [#885] Ask the catalog for the table of a tree by name #886 rewrites — whichever of the two lands second will need a small manual merge there.
• BULK defaults to 0 — behaviour kept, wording fixed. The javadoc of the class now says outright that it ships unbounded and that a create index waiting for a metadata lock waits for as long as the engine lets it, and the description no longer reads as if the class were closed. The lock timeout that actually covers that case is JDBC backend: the DDL of openTree waits for a lock with no bound, and an established connection has no read bound #885.
• ImporterImpl.close() has no finally — fixed: the connection goes back whatever the commit does, and the storage the importer opened is closed whatever the connection does.

One more, which the review did not catch

seconds() claimed that a value which is not a number leaves a class unbounded. Integer.getInteger(name, default) falls back to the default on an unparsable value, which is what the test asserted all along under a name saying the opposite. The javadoc, the test name and the PR description are corrected; the behaviour is the safe one, so nothing changed in the code.

Verification

JDBCStatementBoundTestCase is 15 tests now (was 9), still with no database: the rows being read while the bound is armed, a failure during the transfer being measured against it, a driver without a query timeout still running under the backstop, a catalog lookup being bounded, and the class of both kinds of cursor batch.

All four container suites pass with no skips on the amended branch — PgSql 39/39, MySql 39/39, MsSql 39/39, Oracle 39/39 — plus the JDBC EncryptedTestCase 34/34.

[vharseko]

Rebased onto master (0b9c0f6), which had moved on under the JDBC backend since this branch was cut (#886 catalog lookup, #866 table stamping and statistics, #867 SQL Server upsert).

Conflicts and how they were resolved:

• isExistsTable(): master's catalog lookup by name (storedIdentifier()) runs inside the bounded(con, StatementBound.OPERATION, ...) this PR wraps it in.
• CursorImpl.positionToKey(): master's hashParam(con) with the final byte[] value this PR lifts out of the try-with-resources.
• ImporterImpl.close(): master's version kept - it commits, refreshes the statistics and returns the connection in a finally that also closes the stamp session, which is what this PR was fixing there.
• Imports and jdbc/TestCase.java: both sides kept.

One thing beyond the conflict markers: this PR replaces executeResultSet(statement) with the handler form that reads the rows while the bound is still armed, and master added three call sites of the old overload. They were moved over - isMysqlBackslashEscape() and the table comment readback now take executeResultSet(statement, rs -> ...), i.e. the OPERATION class. The mysql analyze table readback in updateTableStatistics() deliberately does not: that statement carries the bound of the statistics refresh (...jdbc.statistics.timeout, 600s by default), and a StatementBound would put a 120s one over it, so it reads its rows directly with the trace line kept.

mvn -pl opendj-server-legacy test-compile passes, JDBCStatementBoundTestCase 15/15 (no database). The container suites are left to CI.

[maximthomas]

Reviewed at b7c74213279a3816ef1337858e7563265450104e against master 0b9c0f63f5. The rebase itself is clean — I checked the four seams you listed and found no loss: isExistsTable() propagates a timeout instead of returning false (no spurious create table), the lifted final byte[] value keeps hashParam(con), all four #867 hashParam sites survive, and timedOut() copies SQLState and vendor code so #867's retry classifier still sees a class-40 conflict as retryable and a cancel (57014 / ORA-01013 / HY008 / 70100) as not. One blocker, two majors, one minor below.

The backstop is connection-wide, but the importer shares one connection across all import threads (blocker)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:246-252, :310-312, :222-224

armBackstop() sets Connection.setNetworkTimeout() — a property of the socket, not of the statement. ImporterImpl holds exactly one Connection (:1729, assigned :1758) and gives it to both transactions:

txr = new ReadableTransactionImpl(con);
txw = new WriteableTransactionTransactionImpl(con);

OnDiskMergeImporter drives that single Importer from nbThreads phase-one workers (OnDiskMergeImporter.java:903/:921/:943) and one phase-two task per tree (:1286 invokeParallel). Two consequences:

(a) ImporterImpl.clearTree (:1803-1805 → txw.clearTree :1387, BULK) short-circuits at :222-224 and arms nothing, but runs on a socket a concurrent OPERATION set to 150 000 ms. Pooled connections start at networkTimeout 0 (the dialect connect properties go only to newStampConnection, :582-585), so the arm always takes effect. A multi-minute delete from <table> dies at 150 s, the driver closes the connection, and import-ldif fails where it previously completed slowly — and since BULK never entered bounded(), timedOut() never runs, so the error names nothing.

(b) With N concurrent statements only the first arms; the rest hit

if (previous > 0 && previous <= backstop) {
    return -1;
}

and arm nothing, then the first restores 0 at :252 while they are still in flight. During any import most statements run with the socket layer off — the layer the javadoc at :215-218 says exists because Oracle "does not process the break its driver sends".

Cheapest fix: skip the backstop on the importer's connection; it is the only shared one, every other is borrowed per read()/write(). Thorough fix: move arm/release into CachedConnection behind a lock with an outstanding-statement count, arm to the tightest value requested, restore only at zero, and let BULK register "no bound" instead of short-circuiting before armBackstop.

The cursor batch bound aborts rebuild-index on SQL Server (major)

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

The first batch of a cursor has no seek predicate, so fetchBatch (:1549-1560) issues an unconditioned order by k over the whole table, and next() gives it OPERATION (120 s):

if (buffer.isEmpty() && !fetchBatch(currentKeyDb==null?null:">", currentKeyDb, 0, false,
        adaptiveBatchSize(), StatementBound.OPERATION)) {

On mssql k is not indexable — your own comment at :1364 says so, and the create index (k) at :1332-1360 covers postgres/mysql/oracle only:

// mssql: k is varbinary(max), which cannot be an index key column - cursor batches stay unindexed there

so SQL Server scans and TOP-N sorts the whole table. rebuild-index reaches this path: OnDiskMergeImporter.java:1086 importer.openCursor(id2Entry.getName()) in ID2EntrySource.processAllEntries, constructed at :551 under rebuildIndex(...), entered from BackendImpl.java:786. Past 120 s the driver cancels, fetchBatch throws, and the rebuild aborts — master ran the same scan unbounded (master's fetchBatch took no bound and used the untimed executeResultSet(statement)). No client is waiting, so the OPERATION rationale does not apply here.

Fix: take next()'s class from the caller the way fetchBatch already does elsewhere — importer/rebuild cursors are BULK, client-search cursors stay OPERATION. Same pattern you already applied to positionToLastKey (:1694).

The statistics refresh gets the query timeout but not the backstop (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:959, :977

statement.setQueryTimeout(timeoutSeconds); // 0: wait without limit
...
executeAny(statement);

executeAny() (:354-359) just calls statement.execute() — no bounded(), so armBackstop never runs, and the connection is a pooled one with networkTimeout 0. This is the only statement in the file with layer 1 and not layer 2. It runs on Oracle, as dbms_stats.gather_table_stats, inside ImporterImpl.close() — after the data is committed. By the premise of your own javadoc at :215-218, if the cancel is not acted upon the 600 s bound never arrives and import-ldif parks forever at the end of a successful import, holding the pooled connection, with no error.

Not a regression (master had neither layer here), but the PR title says every statement is bound and this is the counterexample, on the named engine. The tool already exists: the bounded(Connection, StatementBound, Execution) overload isExistsTable uses arms only the socket backstop, so wrapping these statements in it keeps the 600 s bound and closes the gap.

The container bound test cannot fail for the reason it exists (minor)

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:254, :282-286

final int boundSeconds = 5;
...
} catch (Exception expected) {
    // the bound was reached and the transaction rolled back
}
final long elapsed = System.currentTimeMillis() - startedAt;
assertTrue(elapsed < 120000, "gave up only after " + elapsed + " ms");

The bound is 5 s and the asserted ceiling is 120 s. On MySQL the blocked statement runs on a pooled connection and lockTimeoutSql reaches only stamp connections (:582, :588), so InnoDB's own 50 s innodb_lock_wait_timeout ends the wait inside the ceiling — both testWriteBlockedByAnotherSessionGivesUpAtItsBound and testBulkStatementGivesUpAtItsOwnBound pass with bounded() deleted. And any exception satisfies the catch, so a run failing instantly for an unrelated reason passes at t≈0 too.

Assert against the bound (elapsed >= boundSeconds * 1000 and elapsed < boundSeconds * 4 * 1000) and assert the exception is the one the bound produces — SQLTimeoutException, or that the message names the property, which timedOut() puts there. Setting the untested classes to "0" and clearing every property in the finally is right and worth keeping.

Nits

• Vacuous BULK assertion: JDBCStatementBoundTestCase.java:117-118 sets BULK's property to a non-number and asserts BULK.seconds() == 0, but BULK's default is already 0 — it passes whether the fallback works, the value parses as 0, or the property is never read. Set a numeric value first, then a non-numeric one. Only the OPERATION half at :114-115 currently pins the fallback.
• Javadoc claims coverage the suite does not have: JDBCStatementBoundTestCase.java:143-144 says the backstop is put back "so a bulk statement sharing the connection is not cut by the bound of an entry read", but the test drives one mock connection from one thread in sequence. That is exactly the property the blocker above shows the code lacks. (The rest of the suite is load-bearing: removing the tighten-only guard fails :167-177, dropping the release fails the inOrder at :158/:248, misassigning positionToLastKey/next fails :325-340.)
• Comment on the wrong method: the "The connection goes back whatever the commit does…" block landed above aborted(), which only sets a flag. It describes close(), which is where the nested finally it explains actually is.

[vharseko]

Thank you — the blocker and both majors were real, and every line reference checked out again. All of it is addressed in c1508e0, and the description of the PR is updated where it claimed more than the branch delivered.

The backstop is connection-wide, but the importer shares one connection

Confirmed, and taken the thorough way rather than by skipping the importer's connection: the socket read timeout is now arbitrated between the statements running on one connection instead of being armed and put back per statement.

A Backstop holds, per physical connection, the bounds of the statements in flight, a count of those running with no bound of their own, and what the connection carried before. What is armed is the loosest bound in flight, and a statement of an unbounded class takes the backstop off for as long as it runs. That closes both halves of your finding:

• (a) clearTree()'s delete from no longer inherits the 150 000 ms an OPERATION armed beside it. It now announces itself through bounded() as "no bound" instead of short-circuiting before armBackstop, which also means a failure of a BULK statement finally goes through timedOut() rather than arriving anonymous;
• (b) the first statement to finish no longer takes the backstop away from those still in flight — it goes back when the last of them is through.

The state is keyed by identity on the connection of the driver, since CachedConnection.prepareStatement() hands the statement to the connection it wraps while the catalog lookups hold the wrapper of that same connection; a wrapper is unwrapped on the way in so both find the same entry, and an entry lives only while statements are running on its connection.

Your trace of where the overlap comes from holds, and it is narrower than "phase one runs on N threads": importStrategy.beforePhaseOne(container) is called from inside processEntry, lazily on the first entry of a container (OnDiskMergeImporter.java:1236-1250), so during a rebuild-index the caller thread is walking id2entry through the importer's cursor while a worker thread is running deleteDatabase(importer) → clearTree() on the same connection. With several base DNs, one container's setTrust(...) puts overlap another's clearTree() the same way.

One corollary the review did not name, which the shared release also fixes: releaseBackstop() used to put back the value the releasing statement had read, so with two different bounds in flight the restore order could leave a stale networkTimeout on the connection — and on the importer's connection that is the very connection the statistics refresh in close() runs on next.

The cursor batch bound aborts rebuild-index on SQL Server

Confirmed and fixed as you proposed — the class comes from the caller, not from the shape of the statement. ReadableTransactionImpl.openCursor() stays OPERATION; the new openBulkCursor() beside it is what ImporterImpl.openCursor() calls, and CursorImpl.batchBound carries that into next(), positionToKeyOrNext() and positionToIndex(). positionToLastKey() stays BULK however the cursor was opened, since it is whole-table work either way.

That also answers the objection I had to the one-liner last round: the opening batch of a search cursor keeps its bound, because the class no longer follows condition == null.

The statistics refresh gets the query timeout but not the backstop

Confirmed and fixed, with one correction to the remedy: bounded(Connection, StatementBound, Execution) takes its seconds from the class, so wrapping the refresh in it would have put OPERATION's 120 s — and a 150 s backstop — over a statement whose own property allows 600. There is now a bounded(con, property, seconds, execution) overload that both layers go through, so the refresh keeps ...jdbc.statistics.timeout and timedOut() names that property when it is what was reached. setQueryTimeout there goes through the guarded helper as well, so a driver without one degrades instead of failing the refresh.

The one statement left outside both layers is now the comment statement of #866, deliberately: it runs on a stamp connection, which is given a lock timeout of its own and a socket read timeout in its connect properties. The javadoc of executeAny() says so, and the description says so under "Out of scope".

The container bound test cannot fail for the reason it exists

Fixed both ways you asked for. The window is asserted from the bound itself to four times it — and on Oracle to the bound plus the margin of the second layer, since that is what ends the wait there rather than the cancel — so a run that gives up at InnoDB's own 50 s no longer passes. And the failure has to be the one the bound produced: the assertion walks the cause chain for the property timedOut() names, so a run failing instantly for an unrelated reason fails the test at t≈0 instead of passing there. Setting the untested class to "0" and clearing every property in the finally is kept.

Nits

• Vacuous BULK assertion — fixed: the value that is not a number now follows one that was, so the assertion sees the fallback rather than the default it happens to equal.
• Javadoc claims coverage the suite does not have — the javadoc no longer claims it, and the suite now has it: three tests drive one connection with two statements in flight (one of them across two threads with latches, so the release really is concurrent) and pin the unbounded veto, the outliving release, and the loosest-bound-wins arbitration.
• Comment on the wrong method — moved to close(), where the nested finally it describes is.

Verification

JDBCStatementBoundTestCase is 20 tests now (was 15), still with no database. All four container suites pass with no skips on the amended branch — PgSql 55/55, MySql 55/55, MsSql 55/55, Oracle 55/55 — plus JDBCStorageRetryTest 26/26 and StampConnectionTestCase 5/5.

[maximthomas]

Reviewed at c1508e0fc68cc24f64c31f9d804931cc1eda045f against 0b9c0f63f5.

The Backstop redesign is right. I tried hard to break it and could not: the "a failed restore leaves
a stale networkTimeout on a pooled connection" theory is unreachable, because on all three
non-postgres drivers every throwing path in setNetworkTimeout implies the connection is already
closed, and a dead one fails isValid(0) on borrow. The "the restore strips the stamp connection's
connect-property read timeout" theory is also false — all four drivers do report that value from
getNetworkTimeout() (ojdbc via T4CConnection's override, not PhysicalConnection), so
previous is 30000, the tighten-only guard returns, and nothing is armed there at all. Fixes (3),
(4), (5) and (6) check out.

Fix (2) does not. It went in one level too low, and it is the one that makes a server fail to start.

The bulk cursor class was added below the SPI, so only the importer can reach it (blocker)

openBulkCursor() is package-private on ReadableTransactionImpl and is not on the SPI, which
declares only openCursor:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1376
public Cursor<ByteString, ByteString> openCursor(TreeName treeName) {
    return new CursorImpl(isReadOnly, con, treeName, StatementBound.OPERATION);
}
...
// :1387 — not on org.opends.server.backends.pluggable.spi.ReadableTransaction
Cursor<ByteString, ByteString> openBulkCursor(TreeName treeName) {
    return new CursorImpl(isReadOnly, con, treeName, StatementBound.BULK);
}

Its only caller is ImporterImpl.openCursor() (:1970). Every other holder of a
ReadableTransaction lands on :1376 and keeps 120 s per batch — and on SQL Server every batch is a
full scan and sort, as the file's own comment at :1500 says, because k is a varbinary(max) and
create index (k) covers only postgres/mysql/oracle (:1470/:1479/:1491).

That is still rebuild-index's problem, plus export-ldif, verify-index and dbtest. But the path
that matters is not a command line:

LDAPReplicationDomain.computeGenerationId()            :3191-3193
  -> exportBackend(null, true) -> backend.exportLDIF(exportConfig)
  -> BackendImpl.java:624  new ExportJob
  -> ExportJob.java:175    txn.openCursor(id2entry.getName())     // OPERATION, 120 s per batch

computeGenerationId() is called at LDAPReplicationDomain.java:3326, on the if (!found) branch of
loadGenerationId() — the first start of a replicated domain, with no operator involved (also at
:3605 after a failed import, and via initializeRemote for a total update). Master had no bound on
any cursor, so this server started before the PR. Now, on a large enough SQL Server backend, the
generation ID is never computed and the domain does not come up.

Putting the choice on the SPI fixes all of these at once; covering only
ExportJob/VerifyJob/BackendStat would leave the four cursor call sites in DN2URI, VLVIndex,
PersistentCompressedSchema and ID2Entry unclassified — I did not trace those to their callers.

The importer's own writes and reads keep the 120 s bound, and the new comment says they do not (major)

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1965
// Bulk, like everything else an import does: this walks a whole tree with no client waiting

Only clearTree (:1521) and deleteTree (:1533) are BULK. ImporterImpl.put (:1955) reaches
upsert, whose every dialect branch calls the one-argument overload:

// :191
int execute(PreparedStatement statement) throws SQLException {
    return execute(statement, StatementBound.OPERATION);
}

and ImporterImpl.read (:1960) reaches the two-argument executeResultSet at :1369, also
OPERATION. Master issued both raw, with no setNetworkTimeout anywhere and no socketTimeout on
pooled connections, so every row an import writes gains a 120 s ceiling it did not have.

Reachability is narrower than the cursor case and I want to be accurate about it: this needs a
concurrent writer, not merely a large import. The importer's own threads share one Connection
(:1907), so they are one session and cannot lock-block each other. But h is the primary key on
every dialect and the default lock wait is forever on mssql, postgres and oracle, so an upsert blocked
by an LDAP write on the same table during an online ImportTask or rebuild-index does sit until
120 s and then fails the import. MySQL escapes only because its own 50 s innodb_lock_wait_timeout
fires first.

Either give put/read BULK — which is what the comment already claims — or leave them OPERATION
deliberately and correct the comment, which is currently false about the code directly beneath it.

The new concurrency tests pin re-entrancy, not concurrency (minor)

Two of the three issue their second statement from inside a Mockito Answer, on the same thread:

// opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java:205
when(operation.executeUpdate()).thenAnswer(new Answer<Integer>() {
    public Integer answer(InvocationOnMock invocation) throws Throwable {
        storage.execute(bulk, StatementBound.BULK); // what another thread of the import is doing
        return 1;
    }
});

Backstop's synchronized blocks are reentrant, so this takes both monitors trivially — a lost update
on bounds/unbounded/holders under real threads would still pass. The one genuinely two-threaded
test (:227) gives both statements OPERATION "7", so two distinct bounds are never merged or
decremented concurrently.

testTheBackstopFollowsTheLoosestBoundInFlight (:281) also arms the loose bound first and lets the
tight one join, so wanted never changes and applyBackstop's re-arm branch is never entered with
armed > 0 && wanted > 0. Nothing tests the reverse order (OPERATION in flight, BULK joins, must
re-arm 37000 -> 130000) or the tighten-back-down when the looser statement finishes first.

And testTheBatchesOfAnImportCursorAreBulk (:478) passes BULK to CursorImpl by hand;
openBulkCursor appears nowhere in either test file, so reverting ImporterImpl.openCursor to
txr.openCursor() — the blocker above — passes the whole suite.

The cross-thread test hangs the build instead of failing it (minor)

JDBCStatementBoundTestCase:227 has three untimed waits — mayFinish.await() (:239),
running.await() (:259), concurrent.join() (:268) — and no timeOut on the method or the class,
unlike TestCase.java:208/:229. running.countDown() is inside the mock's Answer (:238), so if
storage.execute(lingering) throws before reaching it, the main thread parks at :259 forever.

The background throwable is also dropped:

// :243
public void run() {
    try { storage.execute(lingering); }
    catch (SQLException e) { throw new RuntimeException(e); }   // nothing captures this
}

join() does not rethrow, and releaseBackstop runs in a finally, so a failure after the
countdown still leaves the InOrder verification passing.

The container test times with the wall clock (minor)

// opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:293
final long startedAt = System.currentTimeMillis();
...
assertTrue(elapsed >= boundSeconds * 1000L, ...);   // :316, no slack

timedOut()'s own comment says the production measurement is taken from the monotonic clock "which a
step of the wall clock can neither lengthen nor shorten"; the test that checks it uses the wall clock,
with zero slack on the floor.

Round 4's finding is otherwise genuinely fixed — namesTheBound (:306) works, because timedOut()
only puts the property in the message once the bound has elapsed, so an instant unrelated failure can
no longer pass. What it does not exclude is a post-bound unrelated failure, since timedOut()
relabels any SQLException arriving after seconds as that property's breach.

Nits

• unsupported does not survive, so its comment is wrong: the comment at JDBCStorage.java:437
  says a driver with no network timeout "is not asked again", but unsupported is a field of
  Backstop, and releaseBackstop drops the entry whenever --holders <= 0 (:388). With one
  statement at a time the latch dies at every release, so such a driver is asked, throws and is caught
  once per statement forever.
• The one-shot warning is spent by the wrong cause: backstopWarned (:294) is CAS'd at :440
  inside the single catch its own comment says serves both causes. The common one — a connection on
  its way out — permanently consumes the one shot and silences the genuine "this driver has no network
  timeout" warning for the life of the storage.
• "Only the comment DDL is left outside both layers" is not exhaustive:
  executeSessionStatement — the set lock_timeout / alter session set ddl_lock_timeout issued from
  newStampConnection — is outside both layers too. Harmless for the same reason (it runs on a stamp
  connection), but worth naming under "Out of scope".
• timedOut() names a property the Connection overload never set: bounded(Connection, StatementBound, Execution) sets no query timeout, yet a failure after seconds is rewritten to
  "did not finish within the 120s of ...jdbc.query.timeout: raise that property". On a pooled
  connection the only layer in force there is the backstop at bound+30 s, so a catalog lookup that
  fails at 121 s from a reset connection points the operator at the wrong knob.
• positionToKey() and delete() never got batchBound (:1812, :1767) and stay OPERATION on
  a bulk cursor. Unreachable today — ImporterImpl.openCursor declares SequentialCursor, and
  delete() throws on the read-only transaction — so this is consistency only.

[vharseko]

Thank you — the blocker, the major and every minor were real, and each line reference checked out again. All of it is addressed in ed42c17, together with one call site the review named as untraced and one it did not reach.

The bulk cursor class was added below the SPI

Confirmed and fixed the way you proposed: openBulkCursor(TreeName) is on ReadableTransaction now, as a default that answers exactly as openCursor() — every engine that bounds nothing inherits it unchanged (JE, PersistIt, Cassandra), TracedStorage traces it, and the JDBC backend overrides it. The walks that no client operation waits on ask for it: ExportJob, the five whole-tree passes of VerifyJob (two of them through new bulk variants on Index and ID2ChildrenCount), the two of BackendStat, and PersistentCompressedSchema.load().

I traced the four call sites you left open, and two of them turned out to matter:

• ID2Entry.afterOpen() — this is the one the review did not reach, and it is worse than the export path: open() does txn.openCursor(id2entry).next(), which is the same unconditioned first batch positionToLastKey() was made bulk for, and it runs on every open of a backend (EntryContainer.open():523 ← RootContainer.openAndRegisterEntryContainers() ← BackendImpl.openBackend()). So a large enough mssql backend stopped opening at all, with no replication involved. It asks for a bulk cursor now;
• PersistentCompressedSchema (:149, :172) — both trees are read whole while the backend opens: bulk;
• DN2URI (:272, :554, :611) — containsReferrals(), targetEntryReferrals() and returnSearchReferences() all run under a client operation: they stay operations;
• VLVIndex (:516, :549, :699) — all three answer a VLV request of a search: operations too.

Your generation-ID path is exactly as you traced it — loadGenerationId():3326 on the if (!found) branch → computeGenerationId():3193 → exportBackend → ExportJob:175 — and it is fixed with the rest of them.

The importer's own writes and reads keep the 120 s bound

Confirmed, and taken the first of the two ways you offered: the comment is now true of the code. The class is a property of the transaction rather than of a call site, so ImporterImpl holds a bulk ReadableTransactionImpl/WriteableTransactionTransactionImpl and every statement an import issues takes it — put() through upsert(), read(), and the batches of openCursor(), which no longer needs a bulk cursor of its own. Your reachability analysis is what settled it: the importer's threads share one session and cannot block each other, but an upsert of an online ImportTask or rebuild-index blocked by an LDAP write on the same table sat until the bound of an entry read and then failed the import.

The catalog lookups of openTree() deliberately keep the operation class whoever runs them, and the field javadoc says why: they read a data dictionary rather than the data, so a wait there is another session's metadata lock, which is one of the waits this bound exists to end.

The tests

• the two re-entrant tests are two-threaded now, and so is the arbitration test; testALooserBoundRearmsTheBackstopAndTheTighterOneGetsItBack covers the order nothing covered — an operation in flight, a bulk statement joining it, the re-arm to the looser value and the tightening back when the looser one leaves;
• the wiring you could revert without failing anything is pinned: testEveryStatementOfAnImportIsBulk drives a real ImporterImpl over a mock connection through openCursor(), read() and put(), and ID2EntryTest pins the bulk cursor of the open path;
• the cross-thread test cannot hang the build any more: timeOut on the class, every wait bounded, and the background throwable captured and rethrown by joinOrFail() instead of being dropped by join();
• the container test measures with System.nanoTime(), like timedOut() does, with a quarter of a second of slack under the bound for the coarse timer of a driver.

Nits

• unsupported did not survive — split by cause. A driver with no network timeout at all says so with SQLFeatureNotSupportedException, and that is remembered per storage, which is the scope of a driver; a connection that failed the call is remembered only while its statements run, which is the scope of a dying connection. Each has a warning of its own, so the common cause no longer spends the one shot the real one needs;
• timedOut() named a property the Connection overload never set — a statement bounded by the backstop alone is now measured against what that layer really allows it, the bound plus its margin, and the message says so instead of pointing at a query timeout that was never armed. A driver that refuses setQueryTimeout puts a statement in the same position, and it is classified the same way now;
• the session statement — named under "Out of scope" with the comment statement, and its own comment says why it is safe there;
• positionToKey() and delete() — both take the class of their cursor now.

Verification

JDBCStatementBoundTestCase 25/25 (was 20) and the new ID2EntryTest 1/1, both without a database, plus JDBCStorageRetryTest 26/26, StampConnectionTestCase 5/5, DefaultIndexTest, ID2ChildrenCountTest, DN2IDTest, StateTest and OnDiskMergeImporterTest 29/29 for the pluggable side. All four container suites pass with no skips - PgSql 55/55, MySql 55/55, MsSql 55/55, Oracle 55/55 - plus the JDBC and Cassandra EncryptedTestCase at 34/34 each.