[#877] Bound a statement of the JDBC backend by the class of the work it belongs to

[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 together with the reads that walk makes, 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. A value above 24.9 days is taken down to it rather than read as another way of saying "no bound": that is what a socket read timeout can hold — setNetworkTimeout takes milliseconds of an int — so Integer.MAX_VALUE, the usual "no bound" idiom, bounds a statement at the ceiling instead of unbounding it. Clamping down rather than out is the direction that never takes a bound away from a deployment that asked for one, and at 24.9 days it cancels nothing a database will not have ended first.

So not every statement ships bounded, and the title says the class rather than the statement for that reason. The bulk class ships at 0 and a class with no bound takes neither layer: what such a statement legitimately needs follows the size of the backend and the speed of its database, neither of which can be guessed here. Three of those run with no operator watching — PersistentCompressedSchema and ID2Entry.afterOpen() walk their trees while start-ds opens the backend, and ExportJob walks the whole of id2entry behind the generation ID a replicated domain computes the first time it starts — so on a blocked table they wait as this backend waited before any of these bounds existed. That is the deliberate half of the trade: the only other value there was to give them is the bound of a client operation, and bounding them that way stopped a large backend from opening at all. A deployment that wants them bounded sets the bulk property, which bounds the import statements with them; a bound of their own is follow-up material rather than a knob added here on a guess.

The split is not theoretical: AbstractTwoPhaseImportStrategy.beforePhaseOne calls clearTree() for every tree before an import writes its first record, and select count(*) is a scan of the whole table on every engine here for whoever asks — dbtest through BackendStat, and the counts verify-index reports. (It is not the count behind NOTE_BACKEND_STARTED: that one is RootContainer.getEntryCount(), which sums id2childrenCount and never reaches getRecordCount. An earlier revision of this description said otherwise.) 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.

The same holds inside a walk. verify-index walks dn2id whole and reads the children count of every DN it passes (VerifyJob → ID2ChildrenCount.getCount → ShardedCounter), then walks the counter tree whole and asks ID2Entry.containsEntryID() of every record in it. Those are one cursor per DN and one per record, inside a job nobody is waiting on, so they take the class of the walk rather than of a client operation — while the same counter read made by a search asking for numSubordinates, or by a VLV index reporting its size, stays exactly what it is (a delete and a modify DN reach neither form: they go through removeCount). containsEntryID() has no caller but that walk, so it is bulk outright; getCount() is told which of the two it is.

And so is the count that sizes that walk. ProgressTask reads a total before the first record is verified, and on the --clean path it reads the record count of the tree being verified, which is bulk by the tree it counts. The other path — a plain verify-index, with or without an index named, which is what an operator runs — read getNumberOfEntriesInBaseDN0() as a client operation: one cursor on the counter tree, and on SQL Server a scan and a sort of it, under the 120 s of an entry read. It is built outside the try that guards the walk, so reaching that bound aborted the whole job before its first record — the job #877 exists to protect. getTotalCount() is now told which of the two it is, the same way getCount() is, and cn=monitor, GroupManager and SubentryManager keep the client class they should have.

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 and the reads they make as they go, 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. What it does arm, it always gives back: a connection whose driver refuses the call mid-flight, and one still armed when the storage gives this layer up altogether, are both put back to what they carried before. A pooled connection that kept ours would go back to the pool wearing it as its own, and the next borrower — which only ever tightens — would read it as a value of a deployment and keep it from then on. 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 armed for it, rather than against what the property says that layer would have been. Asking for a layer is not having it: a driver with no network timeout, a connection that failed the call, one already carrying a tighter timeout of a deployment's own, and a statement of an unbounded class running beside this one each leave it unarmed, and a statement neither layer bounded reached no bound of ours at all. Its failure is the driver's own and is passed through exactly as it is — naming a property that armed nothing sends an operator to raise a value that changes nothing about the wait they watched. 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.

Two things the move of the rows inside the bound touched, put back as they were. positionToKey() now wraps the row inside the handler rather than after it, so null keeps meaning "no such key" and only that: read outside, a row whose v is null - which the schema allows, however this backend writes it - reported a key that exists as absent instead of failing the way read() still fails on it. Both go through valueOfRow(), which names that failure rather than leaving it the bare NullPointerException of ByteString.wrap — a RuntimeException still, so that a corrupt row is never weighed against the bound of the statement that read it and reported as a timeout of a property that would have changed nothing. So does the third reader of a value, the batch of a cursor, and there the check is made as the rows come off the statement rather than as they are handed out: a batch is buffered whole, so left to advanceFromBuffer() the same row would have failed outside the bound, outside the catch of the batch that read it, and as exactly the bare NPE this replaces. And startImport() gives back what it borrowed on every path that does not build an importer to hold it — through a finally rather than a catch, so an Error is covered as ReadOnlyStorageException is: the connection an import would have kept for its whole duration was leaving the pool for good with the transaction it had already begun. The storage goes back with it, on the branch where this method opened it: ImporterImpl.close() is the only thing that closes a storage an import opened, so a failure between the open and the importer that would have held it left it open forever — which the borrow was already covered against and the build was not.

A bound is clamped to what the second layer can hold, 24.9 days, since setNetworkTimeout takes milliseconds of an int and a bound past that has no value of that layer to be given. Not for the arithmetic: backstopMillis() does the multiply in long under a Math.min, and only adding the margin in int seconds, which needs a property within 30 of Integer.MAX_VALUE, can overflow it into the negative timeout every driver refuses. The ceiling is a ceiling of the layer, not a guard of that sum — worth saying, since a reader who later drops the Math.min must not read the clamp as covering them.

Tests

JDBCStatementBoundTestCase — 37 tests, no database, 8 s, one storage per test so that a latch meant to be one-shot for the life of a storage cannot silence the assertions of the tests after it: 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 the 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, one reported a few milliseconds under the bound — a driver keeps its timer in whole seconds — is still the bound and names it too while one a millisecond further out is not, and the message reports the time the statement really took rather than the bound it reached, which on Oracle is a margin later than the property that armed it. Those last two run on a clock the test drives rather than on a sleep: the classification turns on a few milliseconds either side of the bound, and a sleep that a loaded box lengthens pins nothing while staying green.

Three of them are about the layers being told apart: that a statement neither layer bounded is passed through untouched, that a connection whose driver refuses the call is given back what it carried, and — with the rows of the delta this round — that a row whose v is null fails rather than reporting the key as absent — in positionToKey(), in read() and in a batch of a cursor alike, and by the named failure of valueOfRow() rather than by whatever null reaches first. And that startImport() returns its connection when the importer cannot be built, and closes the storage it opened on the branch where it opened one: the connection half pins the designed path rather than the widening to finally, since ReadOnlyStorageException is a RuntimeException and the catch it replaces covered it already, while the storage half goes red without the delta of this round.

Four more come with this round: that a bound above what the second layer can hold is taken down to the ceiling rather than read as no bound at all — pinned on the value really armed, so that dropping either Math call is caught — and that a failure exactly a slack under the bound is still the bound, which is the point the two cases either side of it leave free.

Six of them are about a connection carrying more than one statement at a time, which is what an import does — the import test among them, since its statements now report the connection they run on and so really reach the second layer instead of stopping at the first — 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.

BulkCursorTest — 12 tests, no database, 3 s: which cursor each walk of a whole tree asks for, pinned twice — that the bulk cursor is what it asks for, and that it asks for no cursor of an operation at all. ReadableTransaction.openBulkCursor() is a default answering exactly as openCursor() does, so a call site turned back would compile, run, and stay invisible on every engine but this one.

Six call sites are pinned through their own caller: ExportJob (the generation-ID walk), the id2entry, dn2id and VLV walks of VerifyJob, and both trees of PersistentCompressedSchema. The dn2id one walks a tree holding a record, so the walk really runs its body and the counter read inside it is pinned with it — over an empty tree the loop stops before its first row and every never() below it passes on a run that reached nothing. Three more are pinned through the delegation rather than the caller, which is all that is available for them: DefaultIndex, ID2ChildrenCount/ShardedCounter and ID2Entry.containsEntryID(). The count that sizes the progress report of a verify is pinned one hop above its cursor — on the container, ProgressTask reading it through getNumberOfEntriesInBaseDN0() — with the cursor of that read pinned both ways below it: bulk for the walk, an operation for the client that reads the same total. And VerifyJob.iterateID2ChildrenCount() is pinned by the compiler: ID2ChildrenCount exposes no cursor but the bulk one — the dead openCursor() overload it kept is deleted — so that revert does not build.

Three call sites are left uncovered and named in the suite rather than passed over: the attribute index of verify-index, whose MatchingRuleIndex is final and cannot be handed to a mock, and the two of dbtest. All three walk a tree only on the command line of an operator. Named with them is what no call site pins at all: the single-row ReadableTransaction.read() those same walks make, once per row of dn2id and of a VLV index, which has no bulk form in the SPI and takes the class of the transaction it is made through — a gap of the SPI rather than a call site anyone can revert, and one that risks a lock wait rather than a walk cut short.

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 what the bound really allows a statement, which is the second layer rather than the property, since holdBackstop() arms the socket read timeout at the bound plus its margin on every engine and not only on Oracle — 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. They also assert which layer ended the wait, because a ceiling wide enough for the second one cannot tell a working first layer from a missing one: a driver that stops taking setQueryTimeout degrades to the backstop by design and silently, ends there, and would otherwise be scored as the bound doing its work. timedOut() names the layer, and the suite reads it off the message through a constant shared with it. The window is measured on the monotonic clock, as timedOut() measures the bound.

All four container suites pass with no skips — PgSql, MySql, MsSql and Oracle 62/62 each on the current head (the counts have grown with the suites the merges below brought in). 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

Merged with master (#876, #880, #883)

The three PRs this branch was waiting behind have landed, and the merge had to decide two things
rather than only separate two edits:

• The statement class moved into commitStatement(). [#879] Skip the validation of a pooled JDBC connection returned a moment ago #883 (JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879) replaced the
  prepareStatement/execute/commit of openTree(), clearTree() and deleteTree() with a single
  commitStatement(sql, ddl) that raises partlyCommitted on the side each engine commits on. Those
  call sites are exactly the list this PR names as overridden downwards — the create table and the
  three create index of openTree(), the delete from of clearTree(), the drop table of
  deleteTree() — so the class is given inside commitStatement() (execute(statement, StatementBound.BULK)) instead of at each site. Nobody waits on any statement that reaches it.
• The importer keeps its constructor and takes the validated connection. ImporterImpl(con, StatementBound.BULK) stands as this PR wrote it, so both transactions of an import are still bulk by
  construction; startImport() borrows through getValidatedConnection(), which JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 asks of an
  import for the same reason it asks it of an open — the statements come far from the borrow.

checkReadOnly() of #880 (#874) and the two constructors of this PR meet in the write transaction:
super(con, bound) captures isReadOnly, and each mutating operation is guarded per call rather than
the transaction being refused outright. That is right for a transaction and wrong for an import, and
the suite said so: with the refusal gone from the constructor, startImport() handed out an importer for
a storage that is not writeable, and the two tests of that path failed on the merge (build-maven (ubuntu-latest, 21) of run 33504253623). 0f1b576 puts the refusal where the importer is built - an
import writes by definition, so ImporterImpl refuses such a storage there, which is where it was
refused before #874 - while the write transaction keeps exactly what #874 gave it. What reaches that
refusal is a storage that was already open: import-ldif and rebuild-index both close the storage
first, and startImport() opens a closed one READ_WRITE, so the refusal stands where the old one
stood rather than where an import of any storage of this server would meet it.

The same commit takes both borrows of the storage through one method. #883 (#879) moved
startImport() onto getValidatedConnection(), which does not go through getConnection(), so the
stand-ins those two tests install for the pool stopped being reached and the tests borrowed for real -
against the jdbc:postgresql://localhost/test that a mock configuration answers with. getConnection()
and getValidatedConnection() now both delegate to getConnection(boolean trusted), which is the one
place a stand-in has to intercept.

One line of Out of scope above has gone stale with #876 landing: a read timeout for an established
connection is no longer unreachable from a branch off master — CachedConnection.relaxReadBound() is
in master now. It stays out of this PR all the same, as #885.

mvn -pl opendj-server-legacy test-compile is green, and so is JDBCStatementBoundTestCase (37/37)
with JDBCStorageRetryTest, CachedConnectionTestCase and StampConnectionTestCase beside it
(135/135). The four engine suites were not re-run at the time of that merge; they have been since — see the section below.

Merged with master (#881, #894)

#881 (#873) gives each backend its own pair of compressed schema trees; #894 (#890) persists a token
before handing it out. #894 came through cleanly. Every conflict was against #881 — four in
JDBCStorage, two in PersistentCompressedSchema, one interleaved block in jdbc/TestCase, and one
the merge did not mark at all.

• A read path takes the non-enrolling table name and keeps its class. [#873] Give each backend its own compressed schema trees #881 splits the name a
  tree maps to in two: getTableName() enrols the tree in tree2table, which is what
  removeStorageFiles() drops, while readTableName() answers without enrolling — so a tree this
  backend does not own, the shared pair the migration of JDBC backends sharing a database URL share one pair of compressed-schema tables #873 reads, is not put up for removal by
  being read. read(), getRecordCount() and CursorImpl are the three sites both PRs edit, and
  the two edits are independent: one decides which table the statement names, the other how long it
  may take. Neither side's line carried both, so each was rewritten to. Master's getRecordCount()
  would not have compiled here in any case — this branch removed the single-argument
  executeResultSet() that returns a live ResultSet, which is what puts the row transfer inside
  the bound.
• isExistsTable() keeps master's place and this branch's bound. [#873] Give each backend its own compressed schema trees #881 moved it from the
  writeable transaction up to the readable one, so the probe of JDBC backends sharing a database URL share one pair of compressed-schema tables #873 — made from the writeable
  transaction of RootContainer.open() — can neither create nor enrol the shared tree. The bound
  goes with it, and stays hard-coded OPERATION rather than taken from the transaction that happens
  to ask: it reads a data dictionary rather than the data, so a wait there is another session's
  metadata lock whoever runs it. That is the rule this description already states for the catalog
  lookups of openTree().
• The walks of the compressed schema stay bulk, and the migration walk becomes one. [#873] Give each backend its own compressed schema trees #881 turned
  load() into loadTrees(txn, ocTree, adTree), called twice and each walk now guarded by
  treeExists(); both keep the bulk class this PR gave them. copyMissingRecords() — the walk [#873] Give each backend its own compressed schema trees #881
  adds, which copies the legacy definitions into the backend's own pair — takes one it never had: it
  reads a whole legacy tree while the backend opens, with nobody waiting on it, which is the rule the
  bulk class exists for. Left at the operation class it would be the same failure this description
  names for PersistentCompressedSchema above, on the one open where there is most to read. This is
  the merge extending a rule to code that did not conflict with it, and is called out here for that
  reason.
• One conflict the merge did not mark. BulkCursorTest.testTheCompressedSchemaIsLoadedWithBulkCursors
  is new on this branch and PersistentCompressedSchema's constructor is changed on master: a
  textual conflict in neither file, and a compile error in the merge, [#873] Give each backend its own compressed schema trees #881 having added the
  backendId that qualifies the trees. The test also had to stub treeExists() — the walks it pins
  are guarded by it now and a mock answers false — so it fails outright without the stub rather
  than passing on nothing. With the stub, reverting either openBulkCursor in loadTrees() reddens
  it and only it (12 run, 1 failure).

jdbc/TestCase needed no decision: both sides appended methods at the same point and both sets are
kept — this branch's 194 lines onto master's file, nothing dropped either way.

CI is green on the merge (bdcbea9, run 33610234970): 32160 tests, 0 failures, 0 skips across
the whole matrix. The four engine suites ran against real containers and pass 62/62 each — PgSql,
MySql, MsSql, Oracle — with EncryptedTestCase 35/35, JDBCStatementBoundTestCase 37/37,
JDBCStorageRetryTest 66/66, CachedConnectionTestCase 64/64, StampConnectionTestCase 5/5,
BulkCursorTest 12/12 and PersistentCompressedSchemaTest 8/8 beside them.

[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.

[maximthomas]

Verdict: ship with fixes — two of them before merge. Nothing here is a regression against master
and there is no defect in the production change. What is missing is the test that keeps round 5's
blocker fixed (blocking) and a test ceiling that scores a working backstop as a failure (blocking).
Everything else is follow-up material.

---

praise: round 5's blocker is genuinely fixed.

openBulkCursor is on the SPI as a default, and all four paths the last round named reach it —
export-ldif and the replication generation-ID walk via ExportJob:179, verify-index via VerifyJob
(×5), dbtest via BackendStat (×2), import/rebuild via ImporterImpl. The one in-repo decorator,
TracedStorage, overrides both wrappers (:303/:390). The importer's put/read took the first
of the two options offered, and the javadoc now matches: ImporterImpl exposes only
aborted/close/clearTree/put/read/openCursor, no openTree.

---

issue (blocking): the fix is pinned at 1 call site out of 12.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/ExportJob.java:179

Pinned: ID2Entry.java:389 (via ID2EntryTest:48/52) and the JDBCStorage override itself
(JDBCStatementBoundTestCase:586). Unpinned: ExportJob:179, VerifyJob:349/446/529/611/659,
BackendStat:1121/1290, PersistentCompressedSchema:151/174, DefaultIndex:142,
ID2ChildrenCount:69, ShardedCounter:84, TracedStorage:303/390.

Reverting any of the eleven is invisible twice over — off JDBC because
spi/ReadableTransaction.java:62 is a default that calls openCursor, and on JDBC because no test
in src/test/.../backends/jdbc/ (9 files) reaches the pluggable layer at all. A later refactor of
ExportJob, or a merge conflict resolved in it, silently restores the 120 s bound on the generation-ID
walk — round 5's blocker — with CI green.

This is round 5's finding [3] recurring on the fix for round 5's finding [1].

Marked blocking despite the code at HEAD being correct, and the reason is the recurrence rather than the
severity: the same revert-and-still-pass gap was raised last round, fixed for one call site, and is back
at 2-of-12. Deferring it to a follow-up is the same bet that already lost once, and what it protects is
blocker-severity behaviour.

Two Mockito assertions in the shape ID2EntryTest already uses — one on ExportJob, one on a
VerifyJob path — make a revert visible. Two of eleven is enough.

---

issue (blocking): the container-test ceiling is below the backstop it arms, on 3 of 4 engines.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:319-320

final long ceilingSeconds = getJdbcUrl().startsWith("jdbc:oracle")
    ? boundSeconds + JDBCStorage.BACKSTOP_MARGIN_SECONDS + 10 : boundSeconds * 4L;

boundSeconds = 5, so the non-oracle ceiling is 20 s. But BACKSTOP_MARGIN_SECONDS = 30 and
backstopMillis() arms the socket layer at (5+30)*1000 = 35 s — and holdBackstop() is not
dialect-gated, so 35 s is armed on all four engines.

On pg/mysql/mssql, a run where the driver's cancel does not land ends at ~35 s. namesTheBound passes,
then the ceiling assertion fails with "gave up only after 35xxx ms, past the 20 s this bound of 5 s
allows". The second layer doing its job is scored as the bound failing. The test contradicts its own
subject.

Blocking because it is a test that fails when the code works: the next red on those suites reads as a
product bug. One line, test-side only.

Give the other three the oracle branch's shape (bound + margin + 10 = 45 s). Still under MySQL's
50 s innodb_lock_wait_timeout, which is what the ceiling exists to catch.

I could not measure how often this fires — it depends on whether pgjdbc's and Connector/J's out-of-band
cancel can fail to land under this fixture, which I did not check.

---

issue (non-blocking): CLOCK_SLACK_MILLIS is unreachable — the round-5 fix rescues nothing.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:311 runs before
the new slack at :322:

assertTrue(namesTheBound(failure, bound), ...);              // :311 — runs first
...
assertTrue(elapsed >= boundSeconds * 1000L - CLOCK_SLACK_MILLIS, ...);   // :322 — the new slack

and opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:494 has no
slack of its own:

if (System.nanoTime()-startedAt < endsAfter*1_000_000_000L) {
    return e;                       // raw driver exception, unlabelled
}

A driver reporting the cancel at bound*1000 - 5 ms gets the raw exception. namesTheBound looks for
bound.property, a string only timedOut()'s wrapper emits, so :311 fails with "does not name".
The window [bound*1000-250, bound*1000) is exactly what :311 rejects.

Widened further: the test's startedAt (:298) precedes prepareStatement, while timedOut()'s
starts inside bounded() — the test can measure over the bound while timedOut() measures under it.

Put the slack in timedOut() at :494, not only in the test. Otherwise the production message is
missing for exactly the case this fix identified.

---

note: after this fix, three server-driven paths wait forever — so "bound every statement" no longer
describes the PR. Deviation, not a defect; your call.

ID2Entry.java:389, PersistentCompressedSchema.java:151/:174, ExportJob.java:179.

BULK is 0 at both layers, and applyBackstop with wanted==0 && armed==0 returns without touching the
driver. Reachable unattended:

• start-ds → BackendImpl.openBackend:196 → RootContainer.open:130 → PersistentCompressedSchema
  ctor :94 (two whole-tree walks) and AbstractTree.open:43 → ID2Entry.afterOpen:389
• LDAPReplicationDomain.loadGenerationId:3326 → computeGenerationId:3191 → ExportJob:179, on
  first start of a replicated domain

On a blocked table, start-ds parks with no message and no timeout.

This is a return to master, not a regression — at base, fetchBatch used the untimed
executeResultSet and the pool carries no read timeout (the socketTimeout properties are the #866
stamp connection's). The 120 s these paths carried at the previous head was the new behaviour, and it
was the blocker. So the trade is right for this PR.

Either narrow the claim in #877, or open a follow-up for a separate, wider startup bound.

---

suggestion (non-blocking): the import test pins layer 1 but never enters layer 2.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java:602

It is a real pin, not a vacuous one — put() reaches execute(.., BULK) twice through the ANSI
branch, read() and the cursor reach executeResultSet(.., BULK), and flipping any one to OPERATION
fires setQueryTimeout(7) so never() fails. But:

final Connection parent = mock(Connection.class);
when(parent.prepareStatement(anyString())).thenReturn(statement);
// statement.getConnection() is NOT stubbed here (cf. lingering() at :107)

connectionOf (:282) returns null → holdBackstop (:362-364) returns null. So the test pins
"an import sets no query timeout" but not "an import takes the backstop off" — the half that decides
whether an import can hang on a dead TCP peer.

Separately: the mock's class name matches no dialect, so the ANSI else (:1652) is the only upsert
branch any test executes. :1625/:1632/:1639/:1646 are covered by nothing. Pre-existing gap,
noted not blocking.

---

nitpick (non-blocking): timedOut() understates the elapsed time by the margin, in the case you
document yourself.

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

final long endsAfter = cancelArmed ? seconds : seconds+(long)BACKSTOP_MARGIN_SECONDS;

On Oracle setQueryTimeout succeeds, so cancelArmed is true — but the session ignores the break and
the wait ends at the socket layer 150 s later. Your own test comment says so:

"Oracle is given the second layer as well — a session blocked in a row-lock enqueue does not act on
the break its driver sends, so the wait there ends at the socket read timeout."

The message then reports "did not finish within the 120s" for a statement that waited 150 s.
cancelArmed cannot tell "armed" from "armed and obeyed".

Not a misdirection — backstopMillis() derives the socket layer from the same seconds, so the property
named governs both layers and raising it is the right remedy. Message accuracy only.

Report the observed elapsed, or word it "at least".

---

nitpick (non-blocking): the class-level timeOut is inert, and the comment above it is false.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java:67

// timeOut on the class, not on the waits inside a test: a statement of another thread that never
// arrives has to fail this suite rather than hang the build waiting for it.
@Test(groups = { "precommit", "jdbc" }, sequential = true, timeOut = 120000)

Every method carries a bare @Test. Measured on testng-6.14.3 (javap -c TestNGMethod.init): the
class lookup runs only when the method annotation is absent, so a bare method @Test wholly replaces
the class one and timeOut resolves to 0. groups survives only because initGroups(Class) merges
separately.

It does not matter — awaitOrFail (30 s) and joinOrFail (join(30 s) + assertFalse(isAlive)) bound
every wait, so round 5's [4] is fixed, just not by the declared mechanism. The comment now describes
neither TestNG's behaviour nor the code beneath it.

Drop the attribute and the comment; the bounded waits are the real protection.

---

chore (non-blocking): leftovers from the conversion.

• ID2ChildrenCount.java:60 — openCursor(ReadableTransaction) is dead; VerifyJob:529 was its only
  caller and now uses openBulkCursor. It keeps ShardedCounter.java:76 alive as dead code too (sole
  caller is ID2ChildrenCount:62). Delete both.
• Index.java:50 — openBulkCursor is abstract with no default, the one interface in the change set
  not using the compatibility pattern the SPI change relies on. Harmless today (package-private,
  DefaultIndex the only implementor); a future implementor gets a compile error, not a fallback.
• JDBCStorage.java:1453 — getRecordCount hardcodes BULK and ignores the transaction's new bound
  field, the only place the class is overridden downward:

  return executeResultSet(statement, StatementBound.BULK, rc -> rc.next() ? rc.getLong(1) : 0);

  All callers today are admin/import; the hazard is the next one, since it is reachable through the
  public Tree.getRecordCount(ReadableTransaction).
• BackendStat.java:1121 carries no comment while its twin at :1290 does.

[vharseko]

Thanks — both blockers are fixed in a652083, and so is everything below them. Details, including the two places I did not follow the suggestion and why.

The fix is pinned at 1 call site out of 12 (blocking)

Fixed, at 10 of 12 by test and one more by the compiler.

New BulkCursorTest (7 tests, no database, 2 s) pins each call site twice — that the bulk cursor is what it asks for, and that no cursor of an operation is asked for at all: ExportJob (the generation-ID walk), VerifyJob ×3 (id2entry, dn2id, a VLV index), PersistentCompressedSchema (both trees), DefaultIndex, ID2ChildrenCount/ShardedCounter. I checked the pins bite rather than assuming it: reverting ExportJob:179 and PersistentCompressedSchema:151 to openCursor fails exactly the two tests that cover them, and nothing else.

VerifyJob.iterateID2ChildrenCount() is now pinned by the compiler instead of by a test — your chore about the dead ID2ChildrenCount.openCursor() overload turned out to be the better tool for it. With the overload deleted (and ShardedCounter.openCursor() with it), that revert does not compile.

Two are left uncovered and named in the suite's javadoc rather than passed over: the attribute index of verify-index, whose MatchingRuleIndex is final and so cannot be handed to a mock, and the two sites of BackendStat. Both walk a tree only on the command line of an operator.

Two corrections to the reasoning, neither of which changes your conclusion:

• "no test in src/test/.../backends/jdbc/ reaches the pluggable layer at all" — jdbc/TestCase.java:66 extends PluggableBackendImplTestCase, which does call backend.exportLDIF() and backend.verifyBackend() against a real JDBC backend. The container suites do reach those paths; what they cannot do is notice the class of the bound, because their tables hold a handful of rows and 120 s is never reached. So the revert is invisible for a different reason than the one given, and a test at that level would not have closed the gap either.
• "two Mockito assertions in the shape ID2EntryTest already uses" — ExportJob.exportContainer and the VerifyJob.iterate* walkers are private, and VerifyJob's trees are set inside verifyBackend0(). Driving them through the public entry points with a recording transaction is not available either: EntryContainer.sharedLock is a field (final Lock sharedLock = lock.readLock()), so a mock leaves it null and verifyBackend0() NPEs before the first cursor. Pinning these cost four package-private seams and two package-private fields in VerifyJob, each carrying a comment saying what it is for. If you would rather have the production classes untouched and those five sites unpinned, say so and I will take the seams back out — but I would rather pay this than leave blocker-severity behaviour on the honour system for a third round.

The container-test ceiling is below the backstop it arms (blocking)

Fixed. The ceiling is now bound + BACKSTOP_MARGIN_SECONDS + 10 on every engine, not only on Oracle — you are right that holdBackstop() is not dialect-gated and 35 s is armed everywhere, so a run where the driver's cancel does not land was being scored as the bound failing. 45 s is still under MySQL's 50 s innodb_lock_wait_timeout, which is what the ceiling exists to catch.

CLOCK_SLACK_MILLIS is unreachable

Fixed on the production side, which is where you said it belonged. CLOCK_SLACK_MILLIS now lives on JDBCStorage and timedOut() classifies with it, so a driver reporting the cancel a few milliseconds under the bound still gets the property named; the container test reuses the same constant instead of declaring its own. New test testAFailureJustUnderTheBoundStillNamesTheProperty pins it.

After this fix, three server-driven paths wait forever (note)

Taken as a scope correction rather than deferred. The PR title now reads "Bound a statement of the JDBC backend by the class of the work it belongs to", the description gained a paragraph naming those three paths and why they ship unbounded, and StatementBound.BULK's javadoc says the same thing next to the default that causes it. bounded()'s javadoc no longer opens with a promise the bulk class does not keep.

The import test pins layer 1 but never enters layer 2 (suggestion)

Fixed. statement.getConnection() is stubbed to the physical connection, as CachedConnection.prepareStatement() has it, and the test now runs an entry read on that same connection from another thread — so it pins the half that matters: while a bounded statement is in flight, the import takes the backstop off and the connection is not cut at 37 s. The gap in the upsert branches is real and pre-existing; I have left it out of this branch rather than widen it again.

timedOut() understates the elapsed time (nitpick)

Fixed. The message reports the time measured rather than the bound reached — "the statement took N ms, reaching the 120s of ..." — so the Oracle case reads as the 150 s it was. testAFailureAtTheBoundReportsTheTimeItReallyTook pins it.

The class-level timeOut is inert (nitpick)

Fixed — attribute and comment dropped. The comment now says what actually bounds those waits (awaitOrFail, Background.joinOrFail).

Chores

• dead ID2ChildrenCount.openCursor() and ShardedCounter.openCursor() — deleted, and the javadoc of the surviving method records that being the only cursor over those trees is now a compile-time guarantee;
• BackendStat:1121 — comment added, matching its twin;
• Index.java:50 — kept abstract, with the reason written down: this interface is package-private with one implementor, and a second one silently inheriting openCursor's behaviour is the failure the SPI default is a compromise for, not something to reproduce here. A compile error is the better answer at this level;
• JDBCStorage.getRecordCount — kept BULK, also with the reason written down: select count(*) is a scan of the whole table on all four engines, so what it takes follows the size of the backend rather than the work of whoever asked, and BackendImpl.openBackend() logs it for every backend it starts. It is a deliberate downward override, and it now says so.

Three more, found while doing the above

• CursorImpl.positionToKey() read a row whose v is null as a missing key. Moving the rows inside the bound had turned ByteString.wrap(rc.getBytes("v")) into a raw byte[] that null could stand in for, so a row that exists reported as absent where master failed and where read() still fails. The wrap is back inside the handler, so null means "no such row" and only that.
• startImport() leaked the connection it had just borrowed when new ImporterImpl(...) threw — which is a designed path, since the transaction of a read-only storage throws ReadOnlyStorageException there and the comment above it says as much. The connection an import holds for its whole duration was leaving the pool for good, with the transaction it had begun. Same defect you had me fix in ImporterImpl.close() in round 1, on the sibling path; it is on master too.
• the container bound test skipped blocker.rollback() when an assertion failed, and the cleanup that follows drops the table — a bulk statement, unbounded there — so one failed assertion could park the build on the lock the blocker still held until the harness timed out. The rollback is in a finally now.

Tests

JDBCStatementBoundTestCase 27/27, BulkCursorTest 7/7, PDBTestCase 34/34 (export and verify end to end, after the visibility changes and the deletions), plus ID2EntryTest, DN2IDTest, DefaultIndexTest, ID2ChildrenCountTest.

All four container suites, run after every change above, with no skips: PgSql 55/55, MySql 55/55, MsSql 55/55, Oracle 55/55.

[maximthomas]

praise: Every item from the last round landed as described, and three of them landed better than asked.

• The counter-class fix is threaded correctly at every hop — ID2ChildrenCount:167 → ShardedCounter:145 → openCursor0:94, both 2-arg overloads delegating false, removeCount hard-pinned, and VerifyJob:525 the only true in the repo.
• BACKSTOP_ALONE as a constant shared between the production message and the assertion is the right shape: a reworded message moves both halves together instead of quietly un-pinning the test.
• The one-record BulkCursorTest fixture is genuinely load-bearing, not decoration — VerifyJob:492-499 pushes counters inside the cursor loop and only the post-loop drain reaches getCount(txn, parent.entryID, true), so an empty tree would have yielded zero openBulkCursor calls and the pin would have passed on nothing.
• Reverting each fix to confirm exactly the expected test goes red is the discipline that makes the rest of this reviewable.
• Saying outright that testStartImportGivesTheConnectionBackWhenTheImporterCannotBeBuilt would not have gone red against the catch (RuntimeException) it replaces, rather than claiming coverage it doesn't have.
• The import connection is uniformly BULK by construction — ImporterImpl:2110-2112 builds both transactions that way, and clearTree/getRecordCount hard-code it. That uniformity is what makes a whole class of shared-connection timeout races unreachable, even at ~25 concurrent holders through one ImporterImpl.con. Worth keeping deliberate.

---

issue (blocking): VerifyJob:1041 is the one branch of its if/else left at the operation class.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VerifyJob.java:1041

The if (indexIterator) arm has four branches, all pinned BULK through getRecordCount (JDBCStorage:1588-1590). The else arm is not:

else if (!vlvIndexList.isEmpty()) {
  totalCount = vlvIndexList.get(0).getRecordCount(txn);   // BULK
}
} else {
  totalCount = rootContainer.getEntryContainer(verifyConfig.getBaseDN())
                 .getNumberOfEntriesInBaseDN0(txn);        // -> getTotalCount -> getCount(txn,key) -> false
}

txn comes from verifyBackend():133 storage.read(...) — ReadableTransactionImpl(con), pinned OPERATION at JDBCStorage:1534-1536 — and ProgressTask is built at :247 outside the inner try.

On MS SQL Server k is varbinary(max) and cannot be an index key: getTableDialect:1650 declares primary key(h) only, and :1700 gives postgres/mysql/oracle a create index on <t>(k) and mssql none. So the first batch is a scan and a sort of the whole table under ~120 s:

select k,v from opendj_<hash> where k>=? order by k offset ? rows fetch next 32 rows only

Blocking because this is a regression, not a gap left open. Base 0b9c0f63f5 has one setQueryTimeout in the entire backend (:743, the statistics refresh) and no query.timeout property, so this read is unbounded today and a large-backend verify completes. After the merge it aborts before its first record — the job #877 exists to protect.

Fix is the machinery already built:

// VerifyJob:1041
totalCount = rootContainer.getEntryContainer(verifyConfig.getBaseDN())
               .getNumberOfEntriesInBaseDN0(txn, /* partOfAWholeTreeWalk */ true);
// -> getTotalCount(txn, true) -> getCount(txn, key, true)

Keep the 2-arg form for the client callers — LocalBackendMonitor:95/:109 (cn=monitor), GroupManager:532, SubentryManager:275, AciListenerManager:348 all reach the same read with a client waiting, where OPERATION is correct. A BulkCursorTest pin on the else arm would match the four siblings that already have one.

---

question (blocking): What is the upgrade story for the 120 s default?

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

Base bounds nothing; OPERATION ships 120. So every client statement in an existing JDBC deployment goes from unbounded to 120 s on upgrade, and the only lever is:

OPERATION("org.openidentityplatform.opendj.jdbc.query.timeout", 120)
// seconds() -> clampSeconds(Integer.getInteger(property, defaultSeconds))

Integer.getInteger only — no dsconfig, no cn=config, and no admin-guide change in this PR. Any deployment with operations that legitimately run past two minutes today starts failing after the upgrade with nothing documented to turn it off.

Not a code defect, and the PR states the default openly — but it is the largest exposure here by blast radius, and it should be an answered question rather than an implicit one before merge.

---

issue (non-blocking): MAX_BOUND_SECONDS javadoc is wrong twice, and the clamp is untested.

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

* ... Clamped rather than refused, since this is 24 855 days and anything past it was meant as "no bound".
static final int MAX_BOUND_SECONDS = Integer.MAX_VALUE/1000 - BACKSTOP_MARGIN_SECONDS;

2147483647/1000 - 30 = 2147453 s = 24.855 days, off by a factor of 1000. 24 855 days would be Integer.MAX_VALUE seconds. The difference matters: 25 days is a ceiling a deployment could plausibly configure and then silently receive a finite bound.

The stated reason is also false over almost the range it excludes — backstopMillis already does the multiply in long under a Math.min:

private static int backstopMillis(int seconds) {
  return (int) Math.min(Integer.MAX_VALUE, (seconds+BACKSTOP_MARGIN_SECONDS)*1000L);
}

Nothing below seconds = 2147483618 can overflow that; below the ceiling it simply returns Integer.MAX_VALUE, a valid timeout. The real reason to clamp is that setNetworkTimeout takes an int of millis — worth saying, since a reader who later removes the Math.min will believe clampSeconds still covers them.

No test touches either Math call; deleting both leaves the suite green. One assertion closes it:

assertEquals(JDBCStorage.clampSeconds(Integer.MAX_VALUE), JDBCStorage.MAX_BOUND_SECONDS);

---

issue (non-blocking): startImport's new finally gives back the connection but not the storage.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2196 vs :2214

The getConnection() catch got the storage close; the finally added in the same hunk did not, though the same sentence applies:

catch (Exception e) { if (!wasOpen) { close(); } throw new StorageRuntimeException(e); }   // :2196
...
finally { if (!built) { try { con.close(); } catch (...) {} } }                            // :2214 — no !wasOpen close()

An Error or RuntimeException out of the two transaction constructors on the !wasOpen path leaves the storage this method opened open forever, since ImporterImpl.close() is the only thing that would close it. That is exactly the class of failure the widening to finally was for. (ReadOnlyStorageException cannot co-occur — open(READ_WRITE) at :642-646 sets accessMode first.)

Also untested in either direction: testStartImportGivesTheConnectionBackWhenTheImporterCannotBeBuilt stubs getStorageStatus() to working(), so wasOpen == true and the :2196 branch runs in no test.

finally {
  if (!built) {
    try { con.close(); } catch (...) {}
    if (!wasOpen) { close(); }
  }
}

---

suggestion (non-blocking): The null-value pin catches an incidental NPE.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java:979

catch (NullPointerException expected) { /* ... */ }

The NPE comes out of ByteString.wrap(rc.getBytes("v")) (JDBCStorage:2016), not from anything the production code raises deliberately. Any unrelated NPE later introduced into positionToKey satisfies it, and if ByteString.wrap ever tolerates null the test flips from pinning to failing for an unrelated reason. Assert on the message or cause, or give the production path a named failure.

---

suggestion (non-blocking): The CLOCK_SLACK_MILLIS boundary itself is unpinned.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStatementBoundTestCase.java:523

The two cases sit at 749 and 875 against a 750 threshold; neither sits at 750, so < vs <= in

if (elapsedMillis < endsAfterMillis - CLOCK_SLACK_MILLIS) { return e; }   // JDBCStorage:602-607

survives a flip. Moving one case to exactly the boundary closes it. Separately, the suite javadoc implies the 40001/1205 SQLState matters on this path; it doesn't — the classification is on elapsed time alone.

---

nitpick (non-blocking): getRecordCount's javadoc closes an enumeration that is wrong by six sites.

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

One of the three places the class of the transaction is overridden downwards, the others being openBulkCursor(TreeName) and CursorImpl.positionToLastKey().

StatementBound.BULK is also hard-coded at :1297, :1660, :1671, :1680, :1692 (openTree create table / create index ×3), :1723 (clearTree) and :1734 (deleteTree) — all inside WriteableTransactionTransactionImpl, whose bound is OPERATION on the write() path. Drop the count or name the DDL sites; the next reader auditing where an operation-class transaction can disarm the shared backstop will trust the list.

---

nitpick (non-blocking): ShardedCounter.getCount's javadoc names callers that call removeCount.

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/ShardedCounter.java:125-129

A client operation reads a counter of its own — numSubordinates of a search, a delete, a modify DN — and takes the bound of one.

EntryContainer:1778 (delete) and :2208 (modify DN) call removeCount, not getCount. The actual 2-arg getCount callers are EntryContainer:750, ID2ChildrenCount:177 (getTotalCount) and VLVIndex:531 — and the list reads as exhaustive while omitting the last two, which is how the getTotalCount path stays invisible to whoever next decides the flag.

---

chore (non-blocking): Two PR-body corrections.

1. The body still carries the justification this delta retracts:

BackendImpl.openBackend() logs NOTE_BACKEND_STARTED with getEntryCount(), which is a select count(*) over id2entry on every backend start.

The new javadoc at JDBCStorage:1578-1582 says the opposite and is correct — that count is RootContainer.getEntryCount():410 → getNumberOfEntriesInBaseDN0:2381 → id2childrenCount.getTotalCount(txn), which never reaches getRecordCount.

2. MAX_BOUND_SECONDS is a new behavioural ceiling the body doesn't mention; it states only that "0, or a negative value, leaves a class unbounded". A property set to Integer.MAX_VALUE — the usual "no bound" idiom — now yields a 24.9-day bound instead. Either say so, or map anything above the ceiling to 0.

[vharseko]

Thank you — the blocking issue was real, every non-blocking one was, and each line reference checked out again. All of it is in 6ebf27e, with the description corrected where this round retracts it. The blocking question is answered here rather than coded around.

VerifyJob:1041 — the count that sizes a verify

Real, and the exposure is wider than the report has it: cleanMode is set only where --clean named the indexes (VerifyJob:162-185), so the four BULK-pinned branches are the --clean path alone. A plain verify-index, with or without --index, comes to the else arm — the one left at OPERATION. The rest is as described: getNumberOfEntriesInBaseDN0 → ID2ChildrenCount.getTotalCount → ShardedCounter.getCount(txn, key) → openCursor0(txn, false), with ShardCursor.next() opening on positionToKeyOrNext — a scan and a sort of the counter tree where k cannot be an index key. Built at :247, outside the try at :251, so reaching the bound ends the job rather than the progress report.

Fixed with the machinery already built: getNumberOfEntriesInBaseDN0(txn, true) → getTotalCount(txn, true) → the existing getCount(txn, key, true). The 1-arg forms stay for the client callers — LocalBackendMonitor:95/109, GroupManager:532, SubentryManager:275.

Three pins rather than one, since the read is two hops from a cursor: that getTotalCount(txn, true) asks for a bulk cursor, that getTotalCount(txn) asks for an operation one, and that ProgressTask(false, txn) asks the container for the walk form — verify(entryContainer).getNumberOfEntriesInBaseDN0(txn, true) with a never() on the 1-arg beside it. Reverting :1041 reddens exactly testTheProgressCountOfAVerifyIsReadAsPartOfItsWalk and nothing else. ProgressTask and its constructor lost private for that pin, which is the visibility id2entry / dn2id / the iterate* methods already carry for this suite.

One adjacent site is left as it is, deliberately, and now says so in javadoc. RootContainer.getEntryCount():410 makes the same read behind NOTE_BACKEND_STARTED on every backend start, with nobody waiting on it either. It is not threaded because it arrives through the public BackendImpl.getEntryCount() that cn=monitor, GroupManager and SubentryManager share, and it can afford the client class: BackendImpl:331-341 catches everything and answers -1, so what reaching the bound costs there is a log line reporting -1 entries, not a failed open. That is a different trade from a job that aborts, and it is recorded at EntryContainer.getNumberOfEntriesInBaseDN0 rather than left for the next reader to rediscover. If you would rather it were threaded too, it is a boolean through one public method and its four callers.

The upgrade story for the 120 s default

A fair question, and the honest answer is that nothing here is documented because nothing in this backend is: fetchsize, fetchsize.initial, ttl and statistics/statistics.timeout are all System.getProperty with no dsconfig and no admin-guide entry, and JDBCBackendConfiguration.xml carries exactly one property, db-directory. The JDBC backend is configured through the wiki page it is documented on. So this is not a new gap in kind — but it is the largest by blast radius, as you say, and it should be answered rather than implied. Three things narrow it:

• the default sits above the server's own client-facing ceiling. GlobalConfiguration.time-limit is 60 s by default, so a search that legitimately runs past 120 s is already twice past what the server allows a search out of the box. What is left exposed is writes, which have no such limit, and paged or VLV reads on a slow database;
• the failure names its own remedy. timedOut() produces ... reaching the 120s of org.openidentityplatform.opendj.jdbc.query.timeout: raise that property, or set it to 0 for no bound. An operator who meets this on upgrade is told the property and told that 0 restores exactly the previous behaviour, without a document to find first;
• it is one JVM property, not a schema change — OPENDJ_JAVA_ARGS / java.args, effective at the next restart, no dsconfig and no config migration.

What I have not done is ship OPERATION at 0. That is the one alternative that removes the exposure entirely, and it also removes the fix for everyone who does not read the release notes — a hung statement parking a worker thread forever is the failure #877 was opened for, and it is silent. I would rather ship the bound, say so loudly in the release notes and the wiki page, and let a deployment that needs longer set one number. If you would rather have the reverse default for one release, say so and I will flip it — it is a one-token change and the property carries it either way.

MAX_BOUND_SECONDS — the javadoc was wrong twice, and the clamp was untested

Both correct, to the digit. 2147483647/1000 - 30 = 2147453 s is 24.9 days, not 24 855; and backstopMillis cannot overflow below seconds = 2147483618, since the Math.min is over a long multiply and only the int addition of the margin can wrap. The javadoc now says the ceiling is a ceiling of the layer — setNetworkTimeout takes milliseconds of an int, so a bound past it has no socket read timeout to be given — and says explicitly that it is not what keeps that sum in range, so a reader who later drops the Math.min is not covered by it.

On the second half of your chore item, I kept the clamp rather than mapping above-ceiling to 0, and said so in both the javadoc and the description: clamping down never takes a bound away from a deployment that asked for one, while reading a large value as "no bound" does, and at 24.9 days the clamp cancels nothing a database will not have ended first. 0 remains the way to say "no bound".

testABoundLargerThanTheBackstopCanHoldIsTakenDownToIt closes the gap, on the value really armed rather than on clampSeconds alone: it asserts your assertEquals(clampSeconds(MAX_VALUE), MAX_BOUND_SECONDS), that the property resolves to the ceiling, that setQueryTimeout receives it, and that the socket read timeout armed is (MAX_BOUND_SECONDS + 30) * 1000 — a positive int, which is the whole point. Removing either Math call reddens it.

startImport — the finally gave back the connection but not the storage

Correct, including that ReadOnlyStorageException cannot co-occur on that branch. Fixed exactly as written:

finally {
  if (!built) {
    try { con.close(); } catch (SQLException ignored) { }
    if (!wasOpen) { close(); }
  }
}

And your point about the direction that was untested is now covered by testStartImportClosesTheStorageItOpenedWhenTheImporterCannotBeBuilt, which runs the wasOpen == false path: storage not working, open() recorded, the build failing, then con.close() and close() both asserted. Dropping the two new lines reddens it and nothing else.

The null-value pin caught an incidental NPE

Right, and I took the second of your two options: the production path now names the failure. valueOfRow() is shared by read(), by positionToKey() and by the batch of a cursor — every place a v becomes a value — and throws StorageRuntimeException("jdbc: a row of <table> is present with no value"). A RuntimeException rather than an SQLException on purpose: an SQLException raised inside the handler runs through timedOut(), and a corrupt row met 130 s into a statement would have been reported as a timeout of a property that would have changed nothing. The key is left out of the message for the reason the statement is left out of timedOut()'s.

The pin now asserts the message and the table, and two more assert that read() and a batch of a cursor fail the same way — the batch being the reader that would otherwise fail furthest from the row, since it is buffered whole and unwrapped in advanceFromBuffer(), outside the bound and outside the catch of the statement that read it. Reverting valueOfRow to ByteString.wrap reddens both.

The CLOCK_SLACK_MILLIS boundary

Correct — 749 and 875 both survive a < → <= flip. testAFailureExactlyASlackUnderTheBoundIsStillTheBound sits at exactly 750 against the 1 s bound and fails on that flip, verified by making it. The literal is deliberate for the reason the neighbouring case gives: a test computing its input from the constant it pins follows that constant anywhere. The suite javadoc no longer implies the SQL state matters on this path.

The two javadoc enumerations

Both wrong as reported. getRecordCount now names the classes of override rather than counting them — openBulkCursor, positionToLastKey, and the DDL a write transaction issues (create table, the three create index, clearTree, deleteTree) — and says why the number is left out: the next hard-coded BULK would make it stale again. One note on your list: :1297 is removeStorageFiles(), which is a method of the storage rather than of WriteableTransactionTransactionImpl, so the sites inside a transaction are the six you counted.

ShardedCounter.getCount now names the callers it has — EntryContainer.getNumberOfChildren and VLVIndex.getEntryCount for the client form, ID2ChildrenCount.getTotalCount told which it is by its own caller — and says outright that a delete and a modify DN reach neither, going through removeCount. The getTotalCount path being invisible in that list is exactly how the blocking issue above stayed invisible.

The description

Both corrections applied. The NOTE_BACKEND_STARTED sentence is replaced by what getRecordCount really serves, with the retraction stated rather than quietly dropped, and the ceiling now appears where the property semantics are given.

One more, found while re-reading the readers of a value

The batch of a cursor is the third place a v becomes a value, and it was not covered by the fix above: fetchBatch() buffered rc.getBytes(2) unchecked and advanceFromBuffer() unwrapped it a row at a time afterwards, so the same null row failed as a bare NPE — and from further away than either of the two you flagged, since advanceFromBuffer() runs outside the bound and outside the catch of the batch that read it. Checked now where the rows come off the statement, with a pin of its own.

Suites: JDBCStatementBoundTestCase 37/37, BulkCursorTest 12/12, ID2EntryTest and ID2ChildrenCountTest unchanged and green — 49/49 through -Pprecommit verify. Each fix of this round was reverted in turn to confirm the intended test, and only it, goes red.

The container suites were not re-run for this delta: no statement's SQL changed, only which class bounds one and how a row without a value is reported. Say so if you would rather have all four before merge and I will run them.

[vharseko]

Merged with master

#876, #880 and #883 have all landed, and this branch is merged with them. Two of the conflicts were
decisions rather than text, so they are worth reading rather than taking on the summary's word:

• The bulk class moved into commitStatement(). [#879] Skip the validation of a pooled JDBC connection returned a moment ago #883 replaced the
  prepareStatement/execute/commit of openTree(), clearTree() and deleteTree() with
  commitStatement(sql, ddl), which raises partlyCommitted on the side each engine commits on. Its
  call sites are exactly the list this PR names as overridden downwards — create table, the three
  create index, delete from, drop table — so the class is now given once inside
  commitStatement() (execute(statement, StatementBound.BULK)) rather than at each site. Every
  statement that reaches it is one nobody waits on, so the two lists cannot drift apart at a later
  call site.
• ImporterImpl(con, StatementBound.BULK) stands, so the uniformity you called out above is
  unchanged — both transactions of an import are bulk by construction. What moved is the borrow:
  startImport() now takes getValidatedConnection(), which JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 asks of an import for its own
  reason (the statements come far from the borrow).

checkReadOnly() of #880 and the two constructors of this PR meet in the write transaction:
super(con, bound) captures isReadOnly, and each mutating operation is guarded per call rather than
the transaction being refused when the storage is read-only.

Also worth flagging: one line of Out of scope in the description had gone stale — a read timeout
for an established connection is no longer unreachable from master, relaxReadBound() having landed
with #876. It stays out of this PR as #885, and the description now says so.

mvn -pl opendj-server-legacy test-compile is green; the four engine suites have not been re-run
since the merge.

The blocking point of the last round — VerifyJob:1041, the else arm reaching
getNumberOfEntriesInBaseDN0(txn) at the operation class while the other four branches are pinned
BULK — was answered by 6ebf27ecb1, which is on the branch ahead of this merge and is what the
re-requested review would be reading. The merge changes nothing about it.

[vharseko]

@maximthomas the merge of the previous comment broke two tests, and 0f1b576 fixes them — worth reading before the next round rather than after, since one half of it walks back a line of that merge.

build-maven (ubuntu-latest, 21) of run 33504253623 ended Tests run: 32065, Failures: 2, both in JDBCStatementBoundTestCase, both on startImport(). Neither failure was a textual conflict; both are the merge deciding something it did not look like it was deciding.

1. The import path lost its refusal

The merge note said the write transaction "takes the per-operation read-only check of #874 in place of the refusal it replaced". That is right for a transaction — RootContainer.open(AccessMode) asks for a write transaction even in read-only mode, which is the whole of #874 — and wrong for an import, which is the one caller that writes by definition. With ReadOnlyStorageException gone from the constructor, ImporterImpl was built on a storage that is not writeable and would take a connection, begin its transaction and fail at the first tree AbstractTwoPhaseImportStrategy.beforePhaseOne clears, rather than at its start. The two tests failed exactly there, at their fail(...).

0f1b576 puts the refusal in ImporterImpl's constructor — where it was until #874, and where the connection and the storage are still given back by the finally this PR added. The write transaction keeps everything #874 gave it; TestCase.testReadOnlyTransactionReadsButRefusesWrites is untouched by the change.

Said plainly, because it is the first thing worth attacking: that refusal is reachable only for a storage that is already open read-only. OnDiskMergeImporter.rebuildIndex() closes the storage at :533 and BackendImpl.importLDIF() at :682, so both production callers arrive with it closed and startImport() opens it READ_WRITE — as an import of any storage of this server reopens it, JEStorage.startImport() included. So this restores a defensive refusal rather than fixing a live import path, which is precisely what it was before #874, and the comment in the constructor now says so rather than implying an operator can meet it.

2. The stand-ins for the pool stopped being reached

Independently of the above, #883 (#879) moved startImport() from getConnection() to getValidatedConnection(), and the two methods do not go through one another — each calls CachedConnection.getConnection(...) itself. Both tests install their connection by overriding getConnection(), so after the merge they reached past the double and borrowed for real: mockCfg(JDBCBackendCfg.class) answers getDBDirectory() with the definition default, jdbc:postgresql://localhost/test. That would have failed these tests on their own, refusal or no refusal.

getConnection() and getValidatedConnection() now both delegate to getConnection(boolean trusted), and that is what the two tests override — one seam instead of two, so the next move between the trusted and the validated borrow cannot silently walk past a double again. The trusted flag itself is unchanged in every path.

Four things found while fixing this, none of them touched here

Named rather than fixed, because none is a regression of this branch and each is a decision of its own:

• close() (JDBCStorage.java:737) resets storageStatus and unstampableTrees but not accessMode, so a storage that an import opened stays READ_WRITE on the instance afterwards. Every read-only check of the class, checkReadOnly() included, reads that field.
• startImport() does not close a storage that open() left half-open: open() sets storageStatus = working() inside the try-with-resources, so a failure of the implicit con.close() throws after the storage is marked open, and the catch there rethrows without closing.
• removeStorageFiles() drops every table of the backend with no read-only check at all. It borrows a connection directly and never went through a transaction, so it was not covered before Offline export-ldif, verify-index and backendstat cannot open a JDBC backend #874 either.
• CASStorage.ImporterImpl (CASStorage.java:575) has the same shape and the same gap as the one fixed here — that storage has checked read-only per operation from the start.

Happy to file them as an issue if you would rather they did not sit in a comment.

Runs

JDBCStatementBoundTestCase 37/37, and JDBCStorageRetryTest, CachedConnectionTestCase and StampConnectionTestCase 135/135 beside it, all without a database. mvn -pl opendj-server-legacy test-compile green. The four engine suites have not been re-run since the merge. Re-requesting review at 0f1b576.

[vharseko]

@maximthomas #881 (#873) and #894 (#890) have landed and this branch is merged with them — bdcbea9. #894 came through cleanly; every conflict was against #881, and three of them were decisions rather than text. The description carries the full account; the short form, and the one thing worth attacking, are here.

readTableName() and the statement class are independent, and all three shared sites take both

#881 splits the table name in two: getTableName() enrols the tree in tree2table, which is what removeStorageFiles() drops, and readTableName() answers without enrolling — so reading a tree this backend does not own, the shared compressed schema pair of #873, does not put it up for removal. read(), getRecordCount() and CursorImpl are edited by both PRs, and the edits decide different things: which table the statement names, and how long it may take. Each now carries both.

Master's getRecordCount() line would not have compiled here in any case — this branch removed the single-argument executeResultSet() that returns a live ResultSet, which is what puts the row transfer inside the bound. Worth saying because taking "their" side there would have looked like the conservative resolution and was not one.

isExistsTable() keeps master's place and this branch's bound

#881 moved it from the writeable transaction up to the readable one, so the probe of #873 — made from the writeable transaction of RootContainer.open() — can neither create nor enrol the shared tree. The OPERATION bound moved with it and stays hard-coded rather than taken from the transaction that happens to ask: it reads a data dictionary rather than the data, so a wait there is another session's metadata lock whoever runs it. That is the rule this PR already states for the catalog lookups of openTree(), so the move changes where it lives and not what it is.

copyMissingRecords() is bulk, and that is the merge extending a rule to code that did not conflict with it

#881 turned load() into loadTrees(txn, ocTree, adTree) guarded by treeExists(); both walks keep the bulk class this PR gave them. The migration walk #881 adds takes one it never had. It reads a whole legacy tree while the backend opens with nobody waiting on it, which is the rule the bulk class exists for, and left at the operation class it would be exactly the failure this PR's own description names for PersistentCompressedSchema — on the one open where there is most to read, since a migration only runs where the legacy pair is still populated.

Flagged rather than folded in, because nothing in #881 conflicted with it and the merge is not the place to decide such things quietly. Say so if you would rather it were reverted to openCursor and filed instead.

One conflict git did not mark, and it is the one that would have gone unnoticed

BulkCursorTest.testTheCompressedSchemaIsLoadedWithBulkCursors is new on this branch; PersistentCompressedSchema's constructor changed on master. No textual conflict in either file — a compile error in the merge, #881 having added the backendId that qualifies the trees.

It also had to stub treeExists(): the two walks it pins are guarded by it now and a mock answers false. Checked rather than assumed, both ways — without the stub the test fails (12 run, 1 failure) rather than passing on nothing, and with it, reverting either openBulkCursor in loadTrees() reddens that test and only it.

jdbc/TestCase needed no decision: both sides appended methods at the same point, both sets are kept — this branch's 194 lines onto master's file, nothing dropped either way.

State

mvn -pl opendj-server-legacy test-compile is green. BulkCursorTest, PersistentCompressedSchemaTest and DefaultIndexTest pass 25/25; JDBCStatementBoundTestCase, JDBCStorageRetryTest, CachedConnectionTestCase and StampConnectionTestCase 172/172. The four engine suites have not been re-run since this merge — CI is running them now.

The blocking point of the last round — VerifyJob:1041, the else arm reaching getNumberOfEntriesInBaseDN0(txn) at the operation class — was answered by 6ebf27e, which is behind both merges since. Neither merge touches it. Re-requesting review.

[vharseko]

CI has finished on the merge — run 33610234970 on bdcbea9, green across the whole matrix: 32160 tests, 0 failures, 0 skips.

The four engine suites did run against real containers rather than skipping, which is the part of the previous comment that was still a promise:

Suite
PgSqlTestCase	62/62
MySqlTestCase	62/62
MsSqlTestCase	62/62
OracleTestCase	62/62
EncryptedTestCase	35/35
JDBCStatementBoundTestCase	37/37
JDBCStorageRetryTest	66/66
CachedConnectionTestCase	64/64
StampConnectionTestCase	5/5
BulkCursorTest	12/12
PersistentCompressedSchemaTest	8/8

Two stale lines in the description are corrected with it: the engine counts under Tests were still the 55/55 of before these merges, and the note closing the #876/#880/#883 section said the suites had not been re-run — they have been now, and against everything since.