Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import

[vharseko]

Problem

Troubleshooting the JDBC backend on the database side is needlessly hard, as discussion #859 showed while investigating #860:

• Table names are opaque SHA-224 hashes of the tree name (opendj_89664c…), so telling dn2id apart from id2entry required recomputing hashes by hand — with the normalized-DN quirks (reversed RDN order) that entails.
• The bulk-imported dn2id table there had never been analyzed: pg_stat showed a row estimate of 1,104 against the real 13,908, leaving the planner free to misestimate the where k>? order by k cursor batches.

Change

Tree name stamped as the table comment. openTree() stores the tree name on the table, visible in \dt+ / the information schema:

Database	Mechanism
PostgreSQL	COMMENT ON TABLE … IS E'…' (the E'' form keeps backslash semantics independent of standard_conforming_strings)
Oracle	COMMENT ON TABLE … IS '…' (backslash is never an escape character there)
MySQL	ALTER TABLE … COMMENT '…', escaped according to the @@sql_mode of the session that parses the literal (NO_BACKSLASH_ESCAPES turns backslash into an ordinary character)
MS SQL Server	MS_Description extended property (idempotent add/update, class=1), value and table name passed as bind parameters

Engines outside these four are left unstamped — mirroring the statistics path — rather than fed untested DDL on every open.

The stamp runs on a connection of its own, outside the pool, never on the transaction that opened the tree: comment statements are DDL (an implicit commit on MySQL and Oracle), and a failing sp_addextendedproperty rolls the whole transaction back on SQL Server — either would corrupt work pending on the caller's connection, such as the trusted flag DefaultIndex.afterOpen() writes between openTree() calls. It is outside the pool because the statement needs session settings — the lock timeout below — and CachedConnection.close() only rolls back, so a pooled connection would carry them over to whoever borrows it next.

That connection is shared by every tree of one storage.write(), and the readback runs on it too. An open costs one physical connect, not one per tree — whether it stamps a whole backend (about 25 trees for a stock suffix) or finds every comment already in place. The readback used to borrow a second connection from the pool, which the thread doing an open cannot afford: it is inside a transaction holding a pooled connection already, and a pool that cannot open another one waits for a peer to return one — which there is the very thread it is blocking. The dialect is taken from the caller's own connection for the same reason: finding it out must not cost a borrow either.

The whole login attempt of that connection is bounded, not only the socket connect inside it, because not one of the four drivers covers both phases with a single property. Each dialect declares the property that bounds the connect and the property that bounds the reads behind it — the prelogin handshake, TLS, authentication — which is exactly the phase a proxy at its connection limit or a moved VIP leaves unanswered after accepting the TCP connection:

Driver	Connect	Reads behind it
PostgreSQL	connectTimeout, plus loginTimeout for the login pgjdbc runs on a thread of its own (s)	socketTimeout (s)
MySQL	connectTimeout (ms)	socketTimeout (ms)
Oracle	oracle.net.CONNECT_TIMEOUT (ms) — its own reference says it "doesn't include user authentication"	oracle.jdbc.ReadTimeout (ms)
MS SQL Server	loginTimeout (s)	socketTimeout (ms)

The SQL Server row is the one that is easy to get wrong: loginTimeout reads as if it covered the login, but TDSChannel.open() hands the socket min(what is left of loginTimeout, socketTimeout) and socketTimeout defaults to 0 — "wait forever" — so the read of the prelogin answer is left open. Bounded, the open of a tree leaves a table unstamped instead of hanging dsconfig create-backend-index, which opens one on a running server. DriverManager.setLoginTimeout() is deliberately not used: it is JVM-global and would change every other DriverManager user in the process.

Comment statements also take locks (a metadata lock on MySQL, a schema modification lock on SQL Server, a DDL lock on Oracle) and openTree() runs on every backend open, so:

• the stored comment is read back from the catalog first, which takes no lock, and the statement is only issued when it is absent or stale. Steady-state opens cost one catalog SELECT per tree and one connection for the sweep — no DDL, no lock;
• when a statement is issued, the connection first bounds its lock wait (lock_wait_timeout on MySQL, LOCK_TIMEOUT on SQL Server, lock_timeout on PostgreSQL, ddl_lock_timeout on Oracle). MySQL waits a year and SQL Server waits forever by default, so an unbounded stamp could queue behind an unrelated transaction of another session on the same database — and on MySQL park every other query on that table behind itself. A diagnostic aid must never be able to do that; giving up is the correct outcome, and the failure path already degrades to "table left unstamped". The bound is committed as soon as it is set: PostgreSQL undoes a plain SET when the transaction that ran it is rolled back, and a failed stamp rolls back a connection that goes on to serve the rest of the sweep.

Existing deployments get stamped on the first read-write open after an upgrade, without reimporting (read-only tools such as export-ldif/backendstat never stamp).

Where the value must be spliced into a DDL literal (no bind parameters there), sqlLiteral() escapes and verifies in one place: quotes are doubled, backslashes are doubled on the dialects where they are escape characters inside literals, and a paired-characters scan of the result guarantees a regression in the escaping throws instead of reaching SQL. This keeps CodeQL java/concatenated-sql-query alerts 1267 and 1268 closed.

A failed stamp never fails the backend — it is logged at warning, as its sibling updateTableStatistics() does — and what happens next depends on what the failure says, which is a three-way question rather than the two-way one it looks like:

Scope	What failed	What follows
TREE	the database rejected the statement (an account without the privilege to comment its tables)	remembered: it would be rejected again for this tree on every open. The rest of the sweep is stamped as usual — the privilege may well be missing for one table alone
MOMENT	another session held the table and the statement gave up on the bound above	nothing is remembered, and the sweep goes on: the lock belongs to that table, and one permanently contended table must not cost a backend all of its comments
SESSION	the connection is gone, or was never established	the sweep ends — every tree behind it needs that same connection and would pay the same connect attempt again, ~25 of them in a row. Nothing is remembered, so the next open tries again

Both exception chains are walked when classifying, getCause() and getNextException(): a driver reports the vendor error of a failed statement as the next exception of a generic one at least as often as it reports it as the cause, and reading only one of the two classifies a lock timeout as a rejection — leaving a tree unstamped until the next start over a moment of contention. A remembered rejection is forgotten when the storage closes, so disabling and re-enabling the backend is enough to retry once the privilege has been granted; a server restart is not required.

Optimizer statistics refreshed after bulk load. Importer.close() — covering import-ldif, online import tasks, rebuild-index and replication total-update initialization via OnDiskMergeImporter — refreshes statistics per dialect: ANALYZE (PostgreSQL), ANALYZE TABLE (MySQL — problems reported as a result row surface as failures), dbms_stats.gather_table_stats with the table name bound (Oracle), UPDATE STATISTICS (MS SQL). Only the trees the import actually wrote — tracked through put()/clearTree() — are refreshed, so rebuilding a single index does not trigger a full-scan statistics pass over the whole backend. A full import does cover every tree, since AbstractTwoPhaseImportStrategy.beforePhaseOne clears them all before the first record is written. deleteTree() and removeStorageFiles() invalidate the tree-to-table cache so dropped trees are never analyzed later, and the importer returns its pooled connection in a finally. Best-effort throughout: every failure of a table — the guard against an unknown dialect included — is logged as a warning and leaves the other tables to be refreshed, never failing the import that produced the data, while the method reports failure so tests catch rejected SQL.

An import that failed or was cancelled refreshes nothing: its trees hold an incomplete import that is going to be run again, and on Oracle gathering statistics of them would delay the report of the failure — or of the cancellation — by a full scan per table. Importer gained a default void aborted() for that; OnDiskMergeImporter.doImport() reports it from the one catch around the whole import (so the InterruptedException of a cancelled import is covered too) and rethrows, TracedStorage forwards it, and the JE, PDB and Cassandra importers inherit the no-op. A failure of the notification itself is suppressed into the Throwable that caused the abort rather than replacing it — concrete for the motivating case, an import killed by an OutOfMemoryError, where notifying the storage allocates.

What a completed import does refresh is bounded and optional, since dbms_stats defaults to AUTO_SAMPLE_SIZE — a full scan of a table whose blob column holds the entries:

• org.openidentityplatform.opendj.jdbc.statistics.timeout — setQueryTimeout per table, 600 s by default, 0 for no limit;
• org.openidentityplatform.opendj.jdbc.statistics=false — no refresh at all,

in the style of the …jdbc.fetchsize and …jdbc.ttl properties this backend already has.

This half is defence in depth rather than the fix for #859's live-traffic symptoms — #863 addressed the measured problem there, and PostgreSQL autoanalyze closes the post-import window on its own within about a minute. A deterministic post-bulk-load refresh still makes the first post-import query plans predictable and covers deployments with auto-stats disabled.

Tests

StampConnectionTestCase is new and needs no database at all, which is the point: the container suites skip themselves whole when no docker is reachable, so a bound only they exercise is a bound that can be deleted without a single test going red.

• testEveryDriverGivesUpOnASilentServer — a ServerSocket that accepts connections and answers nothing, the shape of a proxy at its connection limit; each of the four real drivers must give up on it well inside the ceiling. The four are attempted concurrently, so the suite pays the bound of the slowest rather than the sum: 31 s. Deleting the one line that hands the bounds to the driver makes it fail rather than pass in 31 s.
• testStampConnectionHandsItsBoundsToTheDriver — a recording java.sql.Driver: every declared property must arrive, and as a copy, since a driver is free to write into the map it is passed.
• testEveryDialectDeclaresBothBounds — both phases are declared, with positive values, for every dialect.
• testFailureScopeTellsTheThreeApart / testFailureScopeWalksBothChains — the classification above, including a vendor code reachable only through getNextException(), and a self-referring chain that must not loop the walk.

In the container suites:

• testTreeNameStoredAsTableComment — reads the comment back from each database's catalog and compares it to the tree name; a single quote and a backslash in the base DN exercise the literal escaping.
• testCommentStampSkippedWhenAlreadyStored — a second stamp attempt reports UP_TO_DATE (and not FAILED: the outcome is an enum, so "skipped" and "rejected" cannot be confused), and a stale comment is re-stamped.
• testCommentStampsShareOneConnection — three freshly created trees stamped in one storage.write() cost exactly one physical connect, and the open that follows — every comment in place, readback included — costs exactly one more.
• testCommentStampGivesUpOnLock — another session holds an uncommitted row of the table while the stamp is issued (on SQL Server the stored property is dropped first, so the statement is sp_addextendedproperty rather than the update): the call must return well inside the 5 s bound it is given, and if it reports success the comment must really be stored. It is also what catches an invalid session-setting statement — the PostgreSQL SET lock_timeout form was wrong at first and this suite failed on it.
• testCommentFailureLeavesTransactionIntact — a failing stamp leaves a write pending in the caller's transaction untouched (the pending write targets another tree: a statement left pending on the very table being stamped would make the comment statement wait for the caller's own lock, a shape no production path has), and the failure is not reissued.
• testTransientStampFailureIsRetried — a stamp that failed with a connection exception is attempted again rather than remembered, unlike the rejected statement of the test above.
• testConnectionFailureEndsTheSweep — a lost connection is paid once for the whole open, not once per tree, and the open that follows tries again.
• testContendedTableDoesNotEndTheSweep — one table reporting its dialect's lock-timeout failure leaves the trees behind it stamped, and is itself stamped by the next open.
• testDeleteTreeForgetsTree — a dropped tree disappears from the tree-to-table cache.
• testImportRefreshesTableStatistics — asserts freshness on all four databases: pg_class.reltuples > 0 (PostgreSQL), user_tables.num_rows set (Oracle), mysql.innodb_table_stats.n_rows > 0 (MySQL, with STATS_AUTO_RECALC=0 set on the table beforehand so InnoDB's background recalculation cannot satisfy the assertion by itself), sys.dm_db_stats_properties(…).last_updated set (MS SQL) — plus a direct assertion that the dialect-specific refresh statement is accepted.
• testAbortedImportSkipsStatistics — an import told it was aborted refreshes nothing, while the one that follows does.
• testStatisticsRefreshCanBeTurnedOff — with …jdbc.statistics=false the refresh reports that nothing was refreshed and issues no statement.

All four container suites pass locally with 51/51 and zero skips — PgSql, MySql, MsSql, Oracle — as does StampConnectionTestCase (5/5, no database). The pluggable suites that share the importer SPI (OnDiskMergeImporterTest, DefaultIndexTest, StateTest, DN2IDTest) pass as well.

Follow-up to #860 / #863; closes the diagnostics gap from discussion #859.

[maximthomas]

The table-comment idea is well motivated — in #859 you had to hand the reporter an openssl dgst -sha224 loop just to map a table name to a tree — and the comment round-trip is properly tested on all four databases. The statistics hook point is correct too, and broader than the description claims: it also covers online import tasks, rebuild-index, and replication total-update initialization.

But there is one blocker: the comment escaping is injectable on MySQL, and CodeQL agrees — the gate is red on this PR (java/concatenated-sql-query, severity high, alerts 1267 and 1268), while it passes on #863, #858 and #854.

Note the currently-green checks prove nothing: -P precommit is set only if: runner.os == 'Linux' (.github/workflows/build.yml:93-97) and failsafe exists only in that profile (opendj-server-legacy/pom.xml:1223-1297), so the macOS/Windows legs ran zero tests. The ubuntu legs are still queued.

MySQL comment escaping is injectable via a VLV index name (blocker)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:151 escapes only single quotes:

final String comment=treeName.toString().replace("'","''");
...
sql="alter table "+tableName+" comment '"+comment+"'";

The base-DN half is safe (DN.toNormalizedUrlSafeString() percent-encodes), but the index-id half is not. opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java:118 builds it from raw config:

super(new TreeName(entryContainer.getTreePrefix(), "vlv." + config.getName()));

and opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/BackendVLVIndexConfiguration.xml:172-185 declares that property as a bare <adm:string /> with no <adm:pattern> — unlike sort-order right above it. Under MySQL's default sql_mode, backslash escapes are live inside '…', so a VLV index named x\', add column zz int -- yields:

alter table opendj_… comment '/dc=example,dc=com/vlv.x\'', add column zz int -- '

The literal ends at the doubled quote and the rest becomes part of the ALTER TABLE. It needs config-write privilege, so it is not remotely exploitable, but it crosses into arbitrary SQL on the backing database — and even with no attacker, a name containing \ corrupts the comment or throws a syntax error swallowed at TRACE. PostgreSQL (standard_conforming_strings=on), Oracle and T-SQL are unaffected.

Bind the value as a parameter where the dialect allows, or escape backslashes for MySQL, and/or constrain the VLV name property. This must clear both CodeQL alerts.

The comment is re-stamped on every backend open (major)

JDBCStorage.java:415 calls commentTable unconditionally, outside the create branch, so every openTree(_, true) issues a comment statement plus a commit — roughly 25 per open for a default single-suffix backend (2 compressed-schema + 5 system + 18 default index trees), and again on every dsconfig create-backend-index.

That is DDL on MySQL and Oracle. On MySQL it takes an exclusive metadata lock with lock_wait_timeout defaulting to a year; on Oracle a DDL lock with ddl_lock_timeout defaulting to 0. Both are startup-hang or silent-failure risks on a shared database.

Guarding it — stamp only when the table was just created, or when the stored comment differs — also disposes of two smaller concerns in the same edit: the new mid-transaction con.commit(), and the con.rollback() at JDBCStorage.java:170 running inside the caller's storage.write(). That rollback is mostly harmless as written (PostgreSQL has already committed at JDBCStorage.java:383-389 and in fact needs the rollback to escape 25P02; MySQL/Oracle implicitly commit before DDL), but on SQL Server it can discard a pending TRUSTED flag write, and swallow-plus-rollback inside someone else's transaction is the wrong shape regardless.

updateTableStatistics analyzes tables that no longer exist (minor)

listTrees() returns tree2table.asMap().keySet() — every TreeName ever hashed in this JVM. deleteTree() at JDBCStorage.java:441-450 drops the table but never invalidates the cache:

for (final TreeName treeName : listTrees()) {
    final String tableName=getTableName(treeName);

So after a dsconfig delete-backend-index, the next import in that process runs analyze <dropped_table>, and every dead tree produces a logger.warn at JDBCStorage.java:207-208 plus allRefreshed=false — an error-level line about a table the admin deliberately removed. A tree2table.invalidate(treeName) in deleteTree() fixes it.

The statistics test asserts nothing on MySQL and SQL Server (minor)

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:371-374 returns early:

} else {
    // mysql/mssql maintain their estimates on their own: nothing distinguishable to assert
    return;
}

and the direct assertTrue(storage.updateTableStatistics(con)) cannot fail on MySQL either, because ANALYZE TABLE reports problems as a result-set row (Msg_type='Error') rather than a SQLException, and executeAny() discards the result set.

Concretely: delete the updateTableStatistics(con) call from ImporterImpl.close() and PgSql and Oracle fail, MySql and MsSql still pass. "39/39 on all four suites" is accurate but is not four-database evidence for this half. If you want real coverage, MySQL exposes mysql.innodb_table_stats.n_rows (updated by ANALYZE TABLE) and SQL Server exposes sys.dm_db_stats_properties(object_id('…'), stats_id).last_updated.

Nits

• else returns from inside the loop: JDBCStorage.java:196-198 returns allRefreshed for an unrecognized driver from within the for. Behaviour-equivalent today since driverName is loop-invariant, but it reports "all refreshed" having done nothing, and it will swallow recorded failures the moment dialect selection becomes per-tree. Hoist the switch out of the loop.
• H2 is claimed but absent: both new comments cite H2 support; there is no H2 driver dependency, test case or doc anywhere in the repo, so comment on table and the statistics else branch are untested. Either drop the claim or note it as untested.
• SQL Server probe omits class = 1: major_id in sys.extended_properties is unique only within a class, so major_id=object_id(...) and minor_id=0 and name='MS_Description' at JDBCStorage.java:158 can match a non-table property and route to sp_updateextendedproperty on a table that has none (error 15217, swallowed).
• Comment failure is TRACE-only: JDBCStorage.java:172 — a diagnostic that silently fails to be written is hard to notice. debug or info would fit better, given the statistics path already uses warn.
• Read-only opens are never stamped: commentTable sits inside if (createOnDemand), and EntryContainer.open passes shouldCreate = accessMode.isWriteable(). "Existing deployments get stamped after an upgrade" holds only for read-write opens, not for export-ldif / verify-index / backendstat.
• Statistics half vs #859: worth softening the framing. The reporter's load is live LDAP traffic (4000 searches / 750 adds / 250 modifies per 5 min) with no import, so this hook would never have fired for them; #863 fixed the measured symptom; your own remaining diagnosis in that thread was dead-tuple bloat needing VACUUM (ANALYZE), which this does not do; and PostgreSQL autoanalyze closes the post-import window within about a minute. A deterministic post-bulk-load ANALYZE is still good practice and covers autovacuum=off — just not the fix for #859.

[vharseko]

Thanks for the thorough review — everything is addressed in the latest push, point by point:

MySQL escaping / CodeQL (blocker). Fixed at the root and where the dialect allows it, replaced with binds:

• MS SQL passes the comment value and table name as bind parameters to sp_addextendedproperty/sp_updateextendedproperty and object_id(?) — no splicing left at all;
• Oracle statistics bind the table name in dbms_stats.gather_table_stats(user, ?);
• the two statements that take no binds (COMMENT ON TABLE, MySQL ALTER TABLE … COMMENT) double quotes, additionally escape backslashes on MySQL, and a requireQuotesPaired() guard verifies the escaped literal cannot be terminated — so a regression in the escaping throws instead of reaching SQL.
  The comment test DN now carries a backslash next to the quote (o=comment'te\st) and fails against the old escaping on MySQL. Both CodeQL alerts (1267, 1268) should close with this push. Constraining the VLV name pattern in config is worth doing too, but separately — escaping had to be correct regardless.

Re-stamped on every open (major). commentTable() now reads the stored comment back from the catalog (obj_description / information_schema.tables / user_tab_comments / sys.extended_properties) and issues the DDL only when it is absent or stale. Steady-state opens cost one catalog SELECT per tree — no DDL, no metadata/DDL lock, no mid-transaction commit; the commit/rollback pair now runs only on first stamp (right after create table, which itself commits) or on an actual rename.

Stale ANALYZE after deleteTree (minor). deleteTree() now does tree2table.invalidate(treeName), so updateTableStatistics() no longer analyzes dropped tables or warns about them.

Statistics test asserts nothing on MySQL/SQL Server (minor). Both suggestions taken: the test asserts mysql.innodb_table_stats.n_rows > 0 and sys.dm_db_stats_properties(…).last_updated is not null. On top of that, updateTableStatistics() now reads the ANALYZE TABLE result row and treats Msg_type=error as a failure, so the direct assertTrue(updateTableStatistics(con)) is meaningful on MySQL as well. Removing the updateTableStatistics call from ImporterImpl.close() now fails the test on all four databases.

Nits. All taken: the dialect switch is hoisted out of the per-tree loop (unknown dialects return before doing anything); both sys.extended_properties probes constrain class=1; the comment-failure log went from trace to debug; the H2 claim is dropped from the code comments and the PR text (the COMMENT ON branch is marked as the untested default for other engines).

Read-only opens / #859 framing. PR description updated: stamping happens on the first read-write open, and the statistics half is described as defence in depth (deterministic first plans, covers disabled auto-stats) rather than the fix for #859's live-traffic symptoms, which #863 addressed.

Local runs after the changes: PgSql 39/39, MySql 39/39, Oracle 39/39, MsSql 39/39 — zero skips.

[maximthomas]

Stamping tables with their tree name is a genuinely useful diagnostic, and refreshing statistics after import is the right fix for #859. The new tests really do execute (39 tests, 0 skipped) against postgres, mysql and mssql containers.

One blocker though: the comment stamp performs transaction control on a connection it does not own.

commentTable() commits / rolls back the caller's transaction (blocker)

commentTable() is called at the end of openTree() — i.e. on the caller's in-flight storage.write(...) connection, not a private one:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:503
commentTable(con, treeName);

// JDBCStorage.java:198-210
    executeAny(statement);
    con.commit();               // <-- commits the caller's pending work
  }
}catch (SQLException|RuntimeException e) {
  try {
    con.rollback();             // <-- discards the caller's pending work
  } catch (SQLException e2) {}
  logger.debug(...);            // and the caller is never told
}

There is pending caller work at that point. AbstractTree.open() is openTree() then afterOpen(), and afterOpen() writes:

// opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/DefaultIndex.java:105-109
if (createOnDemand && !trusted && entryContainer.isEmpty(txn))
{
  setTrusted(txn, true);   // -> State.addFlagsToIndex -> txn.update, uncommitted
}

so the next index's openTree() reaches commentTable() with that update still pending. EntryContainer.java:512-513 has the same shape for VLV indexes.

Running the PR's exact SQL against live engines with one uncommitted row pending:

engine	comment succeeds	comment fails
MS SQL 2022	committed early	row silently lost
MySQL 9.2	committed early	committed early (implicit commit fires before ALTER TABLE is even checked)
PostgreSQL 16	committed early	row lost — but unreachable, see below

Trigger: table already exists + comment not yet stamped + empty container — i.e. the first restart after upgrading an empty or post-aborted-import backend. If the DB account cannot ALTER the tables, the readback never matches, so this repeats on every openTree() forever, visible only at debug level. Result on mssql: indexes silently stay untrusted.

Two things that make the obvious fixes not work:

• On MS SQL, sys.sp_addextendedproperty / sp_updateextendedproperty contain an unqualified ROLLBACK TRANSACTION, so a failure takes @@TRANCOUNT 1 → 0 before the catch block runs. A savepoint does not help, and neither does deleting the rollback().
• On MySQL and Oracle the comment DDL implicitly commits anyway, so con.commit() is not really the problem — issuing the statement on the caller's connection is.

PostgreSQL is exempt today only by accident: create index if not exists + con.commit() (JDBCStorage.java:471-477) runs unconditionally just before, flushing anything pending.

Suggested fix — stamp only when the table is created, where create table already owns the commit, and let commentTable() stop touching the transaction:

// JDBCStorage.java:459-467
if (!isExistsTable(treeName)) {
    try (final PreparedStatement statement=con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")){
        execute(statement);
        commentTable(con, treeName);   // no readback, no commit(), no rollback()
        con.commit();
    }catch (SQLException e) {
        throw new StorageRuntimeException(e);
    }
}

If retro-stamping tables created by older versions matters, do that on a connection of its own (getConnection() in try-with-resources), never on con.

readStoredComment() conflates "no comment" with "no readback" (minor)

// JDBCStorage.java:231-233
}else {
    return null;   // unknown dialect
}

null also means "no comment stored", so the caller stamps unconditionally — for an unknown dialect that means DDL plus a transaction boundary on every openTree(), forever. updateTableStatistics() already gets this right by returning early for unrecognised drivers (JDBCStorage.java:253-255); commentTable() should do the same, or use a distinct sentinel.

Only reachable with a driver outside postgres/mysql/oracle/microsoft. H2 is such a case that otherwise works end-to-end with this backend; MariaDB/SQLite/HSQLDB/Derby are already broken by getTableDialect(), so for them it is theoretical.

rebuild-index re-analyzes every table in the backend (minor)

// JDBCStorage.java:257
for (final TreeName treeName : listTrees()) {

listTrees() is the live key set of the tree2table cache — every tree this JVM has hashed, not the trees the import wrote. Rebuilding one attribute index therefore analyzes id2entry, dn2id and ~25 other tables. Negligible for import-ldif (which does write them all), but on Oracle dbms_stats.gather_table_stats with default AUTO_SAMPLE_SIZE is a full scan of each. Tracking the trees actually written in ImporterImpl.put/clearTree would keep the benefit without the collateral cost.

Nits

• requireQuotesPaired() is unreachable: both call sites pass an already-escaped string (.replace("'","''")), so every quote run is even-length and the check can never throw. Harmless, but it is not the guard the comment claims — it also ignores backslashes, which matter on postgres with standard_conforming_strings = off.
• removeStorageFiles() does not invalidate the cache: deleteTree() now calls tree2table.invalidate(treeName) (JDBCStorage.java:539), but removeStorageFiles() (JDBCStorage.java:312-317) drops every table without invalidating, so a later updateTableStatistics() warns once per missing table.
• ImporterImpl.close() can leak a pooled connection: updateTableStatistics(con) sits between con.commit() and con.close() (JDBCStorage.java:863-866) and only catches SQLException; a RuntimeException from driverNameOf() or the cache loader would skip con.close().
• Test gaps: opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java covers the happy path only. Nothing asserts that a second open skips the DDL (a readStoredComment() that always returned null would still pass), that deleteTree() invalidates the cache, or that a failing comment statement leaves the caller's transaction intact — which is the case that matters most.

[vharseko]

Thanks for the second pass — all points addressed in the latest push.

commentTable() performing transaction control on the caller's connection (blocker). Fixed with the variant you suggested for retro-stamping, applied across the board: commentTable() now runs on a dedicated pooled connection (getConnection() in try-with-resources) and never touches the connection that opened the tree — no commit(), no rollback(), no DDL on it. That covers both hazards at once: the implicit commit of comment DDL on mysql/oracle, and SQL Server's sp_addextendedproperty internal ROLLBACK — both now land on a connection with nothing pending, and CachedConnection.close() rolls back whatever a failed attempt left before returning the connection to the pool. Stamping a freshly created table goes through the same path (the create branch commits before the stamp, so the dedicated connection sees the table), and retro-stamping of tables created by older versions is preserved.

The scenario from your table is now a test: testCommentFailureLeavesTransactionIntact injects a failing readStoredComment() and asserts that a write pending in the caller's transaction survives an openTree() whose stamp fails. Against the previous code it fails on mysql, oracle and mssql (postgres stays accidentally exempt via the index-creation commit, as you noted).

readStoredComment() conflating "no comment" with "no readback" (minor). commentTable() now recognizes the four dialects up front and returns before any readback or DDL for anything else — the same shape as updateTableStatistics(). The unreachable return null branch became an explicit SQLException.

rebuild-index re-analyzing every table (minor). Taken as suggested: ImporterImpl tracks the trees written through put()/clearTree() and close() passes exactly that set to updateTableStatistics(con, trees). Rebuilding one index now analyzes that index's trees only.

Nits. All taken:

• requireQuotesPaired() is folded into sqlLiteral(value, backslashIsEscape), which escapes and verifies in one place — quotes and, where backslash is live, backslashes. The postgres comment now uses the E'' form, so backslash semantics no longer depend on standard_conforming_strings.
• removeStorageFiles() invalidates the tree-to-table cache after dropping the tables.
• ImporterImpl.close() returns the pooled connection in a finally, so a RuntimeException from the statistics path can no longer leak it.
• Test gaps: testCommentStampSkippedWhenAlreadyStored asserts a second stamp attempt is skipped (a readStoredComment() that always returned null now fails the test) and that a stale comment is re-stamped; testDeleteTreeForgetsTree covers the cache invalidation; the transaction-intact case is the test above.

Local runs after the changes: PgSql 42/42, MySql 42/42, Oracle 42/42, MsSql 42/42 — zero skips (39 previous plus the 3 new tests per suite). CodeQL alerts 1267/1268 remain closed. PR description updated to match.

[maximthomas]

The round-2 blockers are genuinely fixed. I re-verified the escaping (java/concatenated-sql-query closed), the tree2table invalidation in deleteTree()/removeStorageFiles(), the writtenTrees scoping, and the finally in ImporterImpl.close(). I also round-tripped every dialect's stamp and readback against live PostgreSQL 17, MySQL 9.2, SQL Server 2019 and Oracle Free 23 with a tree name containing both ' and \ — all four store and read back exactly. Resource handling is clean on every path, including both early returns and the catch.

One major issue left, plus nits. No blocker.

Comment DDL can wait forever on a lock (major)

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

The dedicated connection removes the self-inflicted variant of this — on MySQL ≥ 8.0.3 the driver defaults useInformationSchema=true, so getTables()/getIndexInfo() on the caller's connection take no metadata lock, and the stamp completes in ~50 ms with the caller's transaction still open. Measured. What is left is everyone else:

• MySQL — any other session holding an open transaction that touched the table blocks alter table … comment on MDL_EXCLUSIVE. @@lock_wait_timeout defaults to 31536000 (one year) and MDL deadlock detection does not see it. While the ALTER is queued, every other query on that table blocks behind it — measured with a plain select count(*) from a third session.
• SQL Server — another session's uncommitted INSERT on the table blocks sp_addextendedproperty, and @@lock_timeout is -1.
• PostgreSQL and Oracle are fine (ddl_lock_timeout=0 fails fast; PG is saved by the unconditional con.commit() at JDBCStorage.java:498).

This is reachable on the first read-write open after upgrading to this build, when the stamp is issued once per tree, and a stalled stamp freezes the table for all sessions. A diagnostic aid should never be able to do that. The failure is already swallowed and logged, so a timeout degrades exactly to "table left unstamped":

// before the DDL, on the stamping connection
if (mysql)     { exec("set session lock_wait_timeout=5"); }
if (microsoft) { exec("set lock_timeout 5000"); }
if (postgres)  { exec("set local lock_timeout='5s'"); }  // oracle already fails fast

CI cannot catch this: TestCase.setUp → dropStaleTrees drops every opendj_% table, so each openTree is a fresh create and the retro-stamp path is never taken.

The MySQL statistics assertion has no teeth (minor)

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

sql = "select n_rows from mysql.innodb_table_stats where database_name=database() and table_name='" + tableName + "'";
...
assertTrue(rows > 0, "statistics of " + tableName + " look stale: " + rows);

InnoDB's innodb_stats_auto_recalc is ON by default and refreshes n_rows in the background, so this is non-zero without any ANALYZE TABLE. Measured with updateTableStatistics() stubbed to a no-op: n_rows=2, assertion passes, stable across retries. So contrary to the PR description, deleting the updateTableStatistics(con, writtenTrees) call from ImporterImpl.close() does not fail this test on MySQL — the other three dialects do have teeth (PG -1→40, Oracle NULL→40, MSSQL 0→1). assertEquals(rows, 40) or a last_update check would fix it.

A failed stamp repeats on every open, invisibly (minor)

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

if (treeComment.equals(readStoredComment(con, tableName))) {
    return false;
}

The guard suppresses the repeat only when the stamp previously succeeded. A MySQL account granted CREATE, INDEX, SELECT, INSERT, UPDATE, DELETE but not ALTER runs this backend perfectly yet fails the stamp on all ~25 trees on every open, forever — and logger.debug at JDBCStorage.java:225 is below the default error, warning, so nothing is ever visible. Its sibling updateTableStatistics() uses logger.warn for the same class of best-effort failure. Remembering the failure per JVM, or matching the warn level, would close it.

Nits

• updateTableStatistics() reports success having done nothing (JDBCStorage.java:274): it returns true for an unrecognised driver, while commentTable() returns false in the same situation (JDBCStorage.java:189). Since the direct assertion is assertTrue(storage.updateTableStatistics(con, …)) (TestCase.java:479), any engine outside the dialect switch passes it vacuously.
• The test's SQL Server readback omits class = 1 (TestCase.java:316): production was fixed to include it (JDBCStorage.java:247) because major_id is unique only within a class; the test helper still matches on major_id/minor_id/name alone, so it can assert on a non-table extended property.
• assertFalse(storage.commentTable(tree)) cannot tell "skipped" from "failed" (TestCase.java:365): commentTable() returns false both when the comment matches and from the catch (Exception) at JDBCStorage.java:224, so an implementation that always threw would satisfy it.
• commentTable() swallows InterruptedException and drops the interrupt (JDBCStorage.java:224): the catch (Exception e) covers getConnection() → LinkedBlockingQueue.poll(...), which throws and clears the interrupt status. Every other getConnection() caller in the file propagates or wraps; this one should re-assert with Thread.currentThread().interrupt().
• sqlLiteral(value, true) corrupts under NO_BACKSLASH_ESCAPES (JDBCStorage.java:157): doubling backslashes stores a\\b for a\b, which never matches the readback and re-stamps forever. Only reachable through a VLV index name, since AVA.toNormalizedUrlSafe percent-encodes \ as %5C and BackendVLVIndexConfiguration.xml:172-185 still leaves name an unconstrained <adm:string/> — the same freedom behind the earlier injection finding. Reading the backslash rule from @@sql_mode would settle both.
• "Only the trees the import wrote" is every tree for a full import (JDBCStorage.java:893): AbstractTwoPhaseImportStrategy.beforePhaseOne calls entryContainer.delete(...), which routes to importer.clearTree (OnDiskMergeImporter.java:4184-4187), so an import-ldif puts all ~25 trees in writtenTrees before a record is written. Correct for a full import, but it also runs on the abort path (the importer is closed by try-with-resources at OnDiskMergeImporter.java:233), uninterruptible and unbounded — worth a note, since the method comment claims to avoid exactly this on Oracle.
• testCommentFailureLeavesTransactionIntact enshrines a shape the code cannot survive (TestCase.java:404-410): it does txn.put(tree, …) then txn.openTree(tree, true) in one transaction — an uncommitted write followed by the comment DDL on the same table, which measured as a hard block on both MySQL and SQL Server. It only passes because the subclass stubs readStoredComment() to throw, bailing out before the DDL. No production path has that shape today, but the test presents it as supported.

[vharseko]

Thanks — all of round 3 is addressed in a6bec79. Details, including one place where my measurements disagree with yours.

Comment DDL can wait forever on a lock (major) — fixed, with a caveat about the suggested form

The stamp now bounds its lock wait before issuing the statement: set session lock_wait_timeout=5 (MySQL), set lock_timeout 5000 (SQL Server), set lock_timeout = 5000 (PostgreSQL), alter session set ddl_lock_timeout=5 (Oracle — its default of 0 already fails fast, but a DBA can raise it globally).

Two things had to change around your suggestion for it to be safe:

• The setting cannot go on a pooled connection. CachedConnection.close() only rolls back and puts the object back in the queue (CachedConnection.java:149-152), so a session setting survives into whoever borrows it next. On SQL Server that is a real regression risk: LOCK_TIMEOUT applies to ordinary row-lock waits too, so a later write on that connection could fail with error 1222. The stamp therefore runs on a connection of its own, outside the pool (newStampConnection()), opened only when a table actually has to be stamped. The readback stays on a pooled connection, so the steady-state cost is unchanged: one catalog SELECT per tree, no extra connect.
• The setting must be sent as a plain batch. The SQL Server driver runs a PreparedStatement through sp_executesql, and a SET made there is reverted when that call returns — before the statement it is meant to protect. It goes through createStatement() for that reason.

Your rationale for PostgreSQL is out of date, by the way: the con.commit() at the old line 498 stopped mattering once the stamp left the caller's connection. PostgreSQL is low-risk for a different reason — COMMENT ON TABLE takes SHARE UPDATE EXCLUSIVE, which does not conflict with DML — but its lock_timeout is 0 (wait forever) and it does conflict with a concurrent ANALYZE/VACUUM/CREATE INDEX, including this PR's own statistics refresh, so it gets a bound as well.

I could not reproduce the block itself on any engine. The new testCommentStampGivesUpOnLock leaves an uncommitted row of the table in another session and then stamps: it comes back at once on MySQL 9.2, SQL Server 2019-CU30 (I dropped the stored property first so the statement is sp_addextendedproperty and not the update path) and Oracle Free 23. On MySQL 9.2 ALTER TABLE … COMMENT is a metadata-only change that does not take the exclusive MDL. So on these versions the wait you measured does not happen — but the defaults you quote are real, the bound costs one statement, and a timeout degrades exactly to "table left unstamped", so it stays in.

That test earned its place immediately: it caught my first attempt at the PostgreSQL form (set lock_timeout '5s' — PostgreSQL needs = or TO), which failed the stamp on all 25 trees.

A failed stamp repeats on every open, invisibly (minor) — fixed

Logged at warning now, matching updateTableStatistics(), and remembered per JVM, so an account without ALTER reports the problem once per tree instead of silently retrying it forever. deleteTree() and removeStorageFiles() drop the memo so a recreated table gets a fresh attempt. An InterruptedException from the pool's blocking queue is caught separately, re-asserts the flag, and is not remembered — an interrupt says nothing about the table.

The MySQL statistics assertion has no teeth (minor) — fixed

The test now runs alter table … stats_auto_recalc=0 before the import, so InnoDB's background recalculation cannot satisfy the assertion on its own and n_rows stays 0 unless analyze table runs. (The direct assertTrue(storage.updateTableStatistics(...)) did already have teeth on MySQL after round 1 — Msg_type='Error' is turned into a SQLException — it was the end-to-end assertion that was weak, as you said.)

Nits

• commentTable() returns an outcome — STAMPED / UP_TO_DATE / UNSUPPORTED / FAILED — so "skipped" and "rejected" can no longer be confused, in the code or in the test. updateTableStatistics() returns false for an engine outside the dialect switch, where it refreshes nothing, so assertTrue(...) there is no longer vacuous.
• The test's SQL Server readback matches class = 1, as production does.
• sqlLiteral(value, true) under NO_BACKSLASH_ESCAPES: the MySQL escaping now follows the connection's @@sql_mode, read once on the stamp path.
• testCommentFailureLeavesTransactionIntact no longer models an uncommitted write on the table being stamped: the pending write goes to another tree, and the failure is injected at the statement (newStampConnection()) rather than at the readback, so the path the test claims to cover is the path it runs. It also asserts the failure is not reissued.
• writtenTrees covering every tree for a full import, including the abort path: noted at the field and in the PR description. clearTree() commits immediately, so nothing is held while this happens, but the abort-path cost is real and worth stating.

All four container suites: PgSql 43/43, MySql 43/43, MsSql 43/43, Oracle 43/43, no skips.

[maximthomas]

Round 3 holds up: sqlLiteral's doubling + paired scan is sound in both modes, the dialect branch order is right in commentTable() and updateTableStatistics(), the new resources are all in try-with-resources, ImporterImpl.close() fixes the connection leak and the missed JDBCStorage.close(), and deleteTree()'s tree2table.invalidate() is meaningful. I also checked the "no production path leaves a write pending on the table being stamped" claim and it is true - state is the only tree written during an open sweep and it is opened at EntryContainer.java:489, before the first write to it; and because commentTable() runs inline on the caller's thread, no lock cycle can form, so the caller can never be the deadlock victim. One substantive request below; the rest is polish.

Statistics refresh is unbounded, and runs after aborted imports (major)

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

public void close() {
    try {
        try {
            con.commit();
            updateTableStatistics(con, writtenTrees);

ImporterImpl.close() is the try-with-resources exit at OnDiskMergeImporter.java:233, so it also runs when the import throws or is cancelled (OnDiskMergeImporter.java:1242, throw new InterruptedException("Import processing canceled.")). And AbstractTwoPhaseImportStrategy.beforePhaseOne clears every tree before the first record, so writtenTrees is already the whole backend at that point. An import that dies one second in still gathers statistics on all ~25 tables - describing a backend that was just truncated. The comment at JDBCStorage.java:944 acknowledges this, so it looks deliberate, but it is worth reconsidering: those statistics are wrong by construction, and on Oracle producing them is the most expensive part of the failed run.

Cost, separately: dbms_stats.gather_table_stats(user, ?) defaults to AUTO_SAMPLE_SIZE, a full table scan since 11g, and with ENABLE STORAGE IN ROW on v blob that reads most of the entry payload. There is no setQueryTimeout anywhere in the repo and no opt-out - no system property in the style of CachedConnection's org.openidentityplatform.opendj.jdbc.ttl, no JDBCBackendCfg attribute. So import-ldif on a multi-million-entry Oracle backend logs its final status and then scans every table with nothing to cap it, and an operator has no way to turn that off. Postgres/MySQL/MSSQL are all sampled and cheap - this is Oracle-specific.

Suggest: skip the gather when the import body threw (or clear writtenTrees on the exceptional path), plus a setQueryTimeout and a system-property opt-out.

The stamp connection is un-pooled and has no connect timeout (minor)

JDBCStorage.java:211:

Connection newStampConnection() throws SQLException {
    final Connection con=DriverManager.getConnection(config.getDBDirectory());

For the cold start this is not new - RootContainer.open() calls storage.open() before storage.write(), and that already connects with no login timeout, so an unreachable DB hangs before commentTable() is reached. But the live-server path is new: EntryContainer.applyConfigurationAdd (EntryContainer.java:207, and :292 for VLV) runs index.open(txn, true) inside storage.write() on a running server. Pre-PR that made zero new physical connects; now each stamped tree makes one, un-pooled and un-timed-out. If pooled connections still work but new connects blackhole (VIP moved, conntrack drop, proxy connection limit), dsconfig create-backend-index hangs indefinitely. First open after upgrade also goes from 1 new connect to ~27. DriverManager.setLoginTimeout() - or just documenting connectTimeout in the db-directory URL - covers both.

One physical connection per tree on the first open after upgrade (minor)

commentTable() opens its own connection per tree, serially, inside the single storage.write() that opens the backend. That is ~25 connects for a stock userRoot (5 core + 18 default-index + 2 compressed-schema trees), once per deployment - the UP_TO_DATE early return at :259 suppresses it afterwards. Worth hoisting one newStampConnection() for the whole sweep, but the magnitude is modest next to what openTree already does per tree per open: isExistsTable() is an unfiltered getMetaData().getTables(null,null,null,...) enumeration of every table in the database, and on Postgres an unconditional create index if not exists + commit. For what it's worth, on Postgres that pre-existing CREATE INDEX IF NOT EXISTS takes a ShareLock (conflicts with DML, waits unbounded) while the new COMMENT ON TABLE takes only ShareUpdateExclusiveLock and is capped at 5 s - the new statement is the safer of the two.

@@sql_mode is probed on a different session than the one parsing the literal (minor)

try (final Connection con=getConnection()) {            // pooled, possibly hours old
    sql="alter table "+tableName+" comment "+sqlLiteral(treeComment,isMysqlBackslashEscape(con));
}
try (final Connection con=newStampConnection()) {       // brand-new session parses it

Pooled connections are reused for the life of the JVM (CachedConnection expires the per-URL queue, not individual connections, and close() only rolls back), so a runtime SET GLOBAL sql_mode can leave the two sessions disagreeing. Consequence if they do: the stamp succeeds storing the wrong text, returns STAMPED (never memoized), and every later openTree() re-issues the ALTER - the per-open DDL the readback was added to remove.

Low likelihood in practice: both connections use the same URL, so ?sessionVariables=... and init_connect apply identically, NO_BACKSLASH_ESCAPES is non-default, and base DNs cannot contain a backslash at all (AVA.toNormalizedUrlSafeString URL-encodes values, so \ becomes %5C). The only reachable source is a VLV index deliberately named with a backslash - VLVIndex.java:118 uses the raw, un-escaped config.getName(). Unlike driverNameOf, the @@sql_mode probe is a plain select with no CachedConnection cast, so moving it onto the stamp connection is a 3-line change that removes the cross-session assumption entirely.

The failure memo makes transient failures permanent (minor)

}catch (Exception e) {
    unstampableTrees.add(treeName);

This memoizes every non-interrupt failure - including the 5 s lock timeout this round introduced, and connection blips. The bound's rationale is "give up now, try again later", but there is no later in this JVM: a server that started during one contended moment stays unstamped until restart. Memoizing only permanent-looking failures (or not memoizing lock timeouts) keeps the "no ALTER privilege" case cheap without making contention sticky.

Nits

• newStampConnection() leaks on setAutoCommit: if con.setAutoCommit(false) throws, the connection is never closed - the try-with-resources resource expression threw, so the variable was never bound. Same shape pre-exists in CachedConnection.java:99, where it is worse because the catch recurses with doubling backoff.
• testCommentStampGivesUpOnLock tolerance: assertTrue(elapsedMs < 60000, ...) against a 5 s bound, where the result is allowed to be either STAMPED or FAILED, means timing is the only real assertion and it has a 12x margin. It stays green if COMMENT_LOCK_TIMEOUT_SECONDS regresses to 30. ~20 s keeps the cross-engine slack with teeth.
• "a write left pending" understates MySQL: the comment in testCommentFailureLeavesTransactionIntact says a write on the stamped table is the trigger, but MySQL holds a shared MDL for any table a transaction has merely touched - and isExistsIndex() touches it on the caller's connection just before the stamp, on exactly the branch that does not commit. "still holding a lock (on mysql, a metadata lock)" would be accurate.

[vharseko]

Thanks — all of round 4 is addressed. Details below, including the two places where I disagree with the reasoning but took the change anyway.

Statistics refresh is unbounded, and runs after aborted imports (major) — fixed

Three changes:

• An aborted import no longer refreshes anything. Importer gained a default void aborted(); OnDiskMergeImporter.doImport() calls it from the one catch that wraps the whole import (so the cancellation path at OnDiskMergeImporter.java:1242 is covered too) and rethrows, and the JDBC importer skips the refresh in close(). Nothing else in the SPI changes: the JE, PDB and Cassandra importers inherit the no-op, and TracedStorage.TracedImporter forwards it.
• The refresh of a table is bounded: setQueryTimeout, org.openidentityplatform.opendj.jdbc.statistics.timeout seconds, 600 by default, 0 for no limit.
• It can be turned off entirely: org.openidentityplatform.opendj.jdbc.statistics=false, in the style of the fetchsize/ttl properties this backend already has.

One correction to the reasoning, which does not change the outcome: the statistics of an aborted import are not "wrong by construction" — they describe exactly what the tables hold at that moment. And the cost runs the other way round from the example: an import that dies one second in has just had every tree cleared, so gathering statistics of those empty tables is trivial; the expensive gather is the one after a long import, where the data is real. What makes it worth skipping is that the operator is going to run the import again, and that on Oracle the gather delays the report of the failure — or of a cancellation — by its own duration.

New test: testAbortedImportSkipsStatistics (aborted import refreshes nothing, the next one does), testStatisticsRefreshCanBeTurnedOff.

The stamp connection is un-pooled and has no connect timeout (minor) — fixed

newStampConnection() now passes the driver property that bounds the connect attempt, 10 s in every dialect's own unit: connectTimeout (postgres, seconds / mysql, milliseconds), loginTimeout (sql server, seconds), oracle.net.CONNECT_TIMEOUT (oracle, milliseconds). I did not use DriverManager.setLoginTimeout() — it is JVM-global and would silently change every other DriverManager user in the process, including the connection pool.

One physical connection per tree on the first open after upgrade (minor) — fixed

The stamps of one storage.write() share a StampSession: one connection for the whole sweep, opened lazily and closed by write() (and by ImporterImpl.close()). The steady state opens nothing at all, since every readback matches. New test testCommentStampsShareOneConnection asserts both: one connect for three freshly created trees, zero for the open that follows.

@@sql_mode is probed on a different session than the one parsing the literal (minor) — fixed

The mysql statement is now built after the stamp connection is open, and the probe runs on that connection, so the session that answers select @@sql_mode is the session that parses the literal.

The failure memo makes transient failures permanent (minor) — fixed

isTransientFailure() decides whether a failure is worth remembering: a connection exception (SQL state 08…), a SQLTransientException, or the dialect's lock timeout (postgres 55P03, mysql 1205, sql server 1222, oracle ORA-00054/ORA-04021) is a property of the moment and is retried; anything else — a rejected statement, no ALTER privilege — is remembered as before. New test testTransientStampFailureIsRetried.

Nits

• newStampConnection() leaks on setAutoCommit: closed, and the same shape in CachedConnection.java:99 is closed too — the connection is closed before the catch recurses.
• testCommentStampGivesUpOnLock tolerance: 60 s → 20 s.
• "a write left pending" understates MySQL: reworded to "a statement left pending … a write, or on mysql any statement, since a transaction holds a shared metadata lock on every table it touched". Note this is in tension with your own round-3 measurement — you found that with useInformationSchema=true (the driver default since 8.0.3) getIndexInfo() takes no metadata lock and the stamp completes in ~50 ms with the caller's transaction open. The comment now describes the stricter rule, which is the safe thing for a test comment to claim.

Local runs after the changes: PgSql 47/47, MySql 47/47, Oracle 47/47, MsSql 47/47 — zero skips (43 before, plus the four new tests). Because the fix for the major touches the shared importer SPI, OnDiskMergeImporterTest, DefaultIndexTest, StateTest and DN2IDTest were run as well: 49/49. PR description updated to match.

[maximthomas]

Round 4 is genuinely addressed: aborted() reaches every import entry point, the statistics refresh is bounded and optional, the stamp connection is shared and its connect attempt bounded, isTransientFailure() keeps contention from being sticky, and the CachedConnection leak is closed. The shared connection introduced one new defect that is worse than anything round 4 raised.

PostgreSQL loses the lock bound after the first failed stamp (blocker)

newStampConnection() sends the lock bound as the first statement of an uncommitted transaction — opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:269-283:

final Connection con=DriverManager.getConnection(config.getDBDirectory(), properties);
try {
    con.setAutoCommit(false);
    executeSessionStatement(con, dialect.lockTimeoutSql); // "set lock_timeout = 5000"
}

StampSession.reset() rolls back after any failure and keeps the connection (JDBCStorage.java:303-311):

void reset() {
    if (con!=null) {
        try {
            con.rollback();
        }catch (SQLException e) {
            close();
        }
    }
}

PostgreSQL reverts a plain SET when the surrounding transaction rolls back. Measured on postgres:17 with the pinned pgjdbc 42.7.12:

after SET (same txn):   5s
forced statement error, then ROLLBACK
after ROLLBACK:         0          <-- wait forever
COMMENT ON TABLE behind a conflicting SHARE UPDATE EXCLUSIVE lock:
  bound intact:         1004 ms (55P03)
  after the rollback:   20003 ms, cancelled by the watchdog -- never returned

55P03 leaves the connection usable, so reset() takes the rollback branch rather than close(), and the same connection is reused. One StampSession serves every tree of every suffix, because RootContainer.java:135 wraps the whole backend open in a single storage.write(). So tree #1's failure — including one caused by the 5 s bound doing its job — disarms the bound for trees #2..N, reinstating the unbounded wait COMMENT_LOCK_TIMEOUT_SECONDS exists to prevent, on the startup and dsconfig create-backend-index path.

MySQL, Oracle and SQL Server are unaffected: their session settings are not transactional.

No test covers it. testCommentFailureLeavesTransactionIntact and testTransientStampFailureIsRetried both stub newStampConnection() to throw, so session.con stays null and reset() is a no-op — the branch where a real connection survives a rollback is never exercised.

Suggest committing the setting inside newStampConnection(), or reissuing dialect.lockTimeoutSql after any reset() that keeps the connection.

The connect bound only covers the TCP connect on three of four dialects (major)

JDBCStorage.java:177-190:

POSTGRES("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, ...),
MYSQL("connectTimeout", STAMP_CONNECT_TIMEOUT_SECONDS*1000, ...),
ORACLE("oracle.net.CONNECT_TIMEOUT", STAMP_CONNECT_TIMEOUT_SECONDS*1000, ...),
MICROSOFT("loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, ...);

The names, units and values are all correct, but pgjdbc and mysql-connector-j apply connectTimeout only to socket.connect(); reads during TLS and authentication are governed by socketTimeout, which defaults to 0 and is never set here. Oracle's own JDBC reference says oracle.net.CONNECT_TIMEOUT "doesn't include user authentication" — that needs oracle.jdbc.ReadTimeout. Only mssql-jdbc's loginTimeout covers the entire login phase.

The scenario the PR description names — "a proxy at its connection limit" — usually accepts the TCP connection and then stalls, which is exactly the case still unbounded. openTree() on a running server would hang rather than leave the table unstamped.

A sweep-wide failure is paid once per tree (minor)

StampSession.connection() assigns con only on success (JDBCStorage.java:293-298):

Connection connection(Dialect dialect) throws SQLException {
    if (con==null) {
        con=newStampConnection(dialect);
    }
    return con;
}

so a failed connect leaves it null and every following tree reconnects — the "one physical connect per sweep" property holds only on the success path. isTransientFailure() memoizes neither 08… nor lock timeouts, so the whole cost repeats on every open until the trees are stamped.

MySQL FLUSH TABLES WITH READ LOCK (mysqldump, xtrabackup) blocks every table's alter table … comment at once: ~25 trees x 5 s is about 125 s added to a backend open, for a diagnostic aid. Giving up on the remaining trees after the first lock timeout of a sweep keeps the retry-next-open semantics without the linear cost.

The connect-failure variant is largely masked by the pre-existing unbounded recursion in CachedConnection.getConnection(), which would hang on the pooled readback first.

Nits

• @@sql_mode probed once per tree: JDBCStorage.java:377 calls isMysqlBackslashEscape(con) for every tree of the sweep, but the connection is now shared and the value cannot change within that session — ~25 avoidable round-trips. Cache it on StampSession.
• doImport catches Exception, not Throwable: OnDiskMergeImporter.java:1196-1209 — an import killed by an Error (OOM on an oversized entry) skips aborted() and still refreshes statistics on partially-written tables. Wasted work only, not incorrectness.
• default: means a different dialect in each switch: Oracle at JDBCStorage.java:392, Microsoft at :459, :491 and :548. Correct today, but a fifth Dialect constant would silently get Oracle DDL plus SQL Server readback, statistics and error codes instead of a compile error.
• mssql-jdbc property precedence: mergeURLAndSuppliedProperties gives the supplied Properties precedence over the URL, the opposite of pgjdbc and Oracle. Inert while that map carries only the timeout key, but a footgun if credentials are ever added to it.

Verified clean, not worth re-raising

aborted() reaches every import entry point (startImport() has exactly two callers, both always calling doImport; import-ldif, the online ImportTask, rebuild-index and replication total-update all funnel through it, and TracedImporter forwards it). All four new tests have teeth. Failsafe runs forkCount=1, parallel=none, so the global …jdbc.statistics=false cannot race a sibling test. setQueryTimeout is honoured by all four drivers, including Oracle's PL/SQL block. put()/clearTree() never reach openTree(), so StampSession is never touched concurrently by import threads. updateTableStatistics() commits or rolls back per tree, so one failure does not poison the rest on PostgreSQL. The CachedConnection close-on-setup-failure is a clean leak fix. No open CodeQL alerts on the touched files.

[vharseko]

Thanks — all of round 5 is addressed. Details below, including one place where the blocker is narrower than described and one thing the fix does not reach.

PostgreSQL loses the lock bound after the first failed stamp (blocker) — fixed

newStampConnection() commits the session setting right after making it, so the bound is no longer part of the transaction a failed stamp rolls back. The other three dialects' settings are not transactional; there the commit ends an empty transaction.

Two notes on the analysis, neither of which changes the fix:

• The window is the first stamp of a connection, not any failure: a stamp that succeeds commits the SET along with its own COMMENT, and a later rollback cannot undo a committed SET. That window is exactly the case you name, though — the retro-stamping sweep after an upgrade, where the first tree can be the one that hits the lock the bound exists for, and it then disarms trees Fix build with closed dependency #2..N.
• reset() re-establishing the setting would work as well, but committing costs one round trip per sweep instead of one per failure, and it also covers the branch where reset() closes the connection: the replacement goes through newStampConnection() again.

New test testLockBoundSurvivesAFailedStamp: it reads what a freshly bounded stamp connection reports, fails a real stamp on the session (a table that was never created, so the statement fails on a connection that stays usable — the branch neither existing failure test reached, as you noted), and asserts the session's connection still reports the same bound. Against the previous code it fails on PostgreSQL with expected [5s] but found [0], which is your measurement. The assertion is skipped on Oracle, where ddl_lock_timeout lives in v$parameter and the application user cannot select from it.

The connect bound only covers the TCP connect on three of four dialects (major) — fixed

Every dialect now carries a second property covering the reads of TLS and authentication:

Database	Connect attempt	Reads after it
PostgreSQL	connectTimeout (10 s)	loginTimeout (10 s)
MySQL	connectTimeout (10 s)	socketTimeout (30 s)
Oracle	oracle.net.CONNECT_TIMEOUT (10 s)	oracle.jdbc.ReadTimeout (30 s)
MS SQL Server	loginTimeout (10 s) — covers the phase on its own	—

PostgreSQL gets loginTimeout rather than socketTimeout because pgjdbc implements it as a bound on the whole login: Driver.connect() reads it (falling back to DriverManager.getLoginTimeout()), runs makeConnection() on a daemon thread and abandons it when the bound expires. The read bounds are 30 s: unlike loginTimeout they outlive the login phase and apply to the comment statement too, so they stay well clear of the 5 s lock bound — the statement gives up on a contended lock long before the socket gives up on the server.

testEveryDialectBoundsItsLoginAttempt guards the declaration; a server that accepts a connection and then goes quiet is not something a container can stage.

One thing this does not reach, and I would rather not stretch this PR to cover: the pooled readback runs before the stamp, and CachedConnection.getConnection() connects with no properties at all and retries forever on failure (CachedConnection.java:83-111), so your proxy scenario still hangs there first — as your own minor #3 says. That is pre-existing and belongs in the pool: filed as #872.

A sweep-wide failure is paid once per tree (minor) — fixed

StampSession gives up for the rest of the sweep when a stamp fails for a reason of the moment — the same predicate that decides not to remember it, so a connect exception and a lock timeout both end the sweep while a rejected statement still only marks its own tree. The trees left behind are not remembered, so the next open tries them again; the give-up is logged at debug per table, after the one warning that reported the failure.

New test testTransientStampFailureEndsTheSweep: three trees in one storage.write(), an injected 08006 on the stamp connection, one attempt (not three), and one more attempt on the next open.

Nits

• @@sql_mode probed once per tree: cached on StampSession and forgotten with the connection it describes. testSqlModeProbedOncePerSweep asserts exactly one probe on MySQL for a three-tree sweep, and none on the other three engines.
• doImport catches Exception: now catch (Throwable t) with a precise rethrow, so an import killed by an Error reports the abort as well.
• default: means a different dialect in each switch: all four switches name every constant. The three that build SQL throw IllegalStateException on an unknown dialect instead of inheriting another one's statement; in isTransientFailure() the default means "no lock timeout code known for this dialect", which is the conservative answer (remember the failure) rather than SQL Server's codes.
• mssql-jdbc property precedence: noted at the field, together with why the map is copied before it reaches the driver.

Local runs after the changes: PgSql 51/51, MySql 51/51, MsSql 51/51, Oracle 51/51 — zero skips (47 before, plus the four new tests). PR description updated to match.

[maximthomas]

Round 5 is genuinely fixed — I checked rather than took it: on a live PostgreSQL, show lock_timeout now stays 5s after a failed stamp where it previously went to 0. Build is green on JDK 11 and 26, and the 51/51 × 4 with zero skips is real; I ran all four dialects.

Two things left, both the same hazard the PR itself set out to bound.

The SQL Server stamp connection has no bound at all (major)

Dialect.MICROSOFT (opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:198-200) carries one property, and mssql-jdbc discards it. TDSChannel.open():

int socketTimeout = con.getSocketTimeoutMilliseconds();   // SOCKET_TIMEOUT default = 0
if (!con.isConnected()) socketTimeout = Math.min(con.timerRemaining(con.timerExpire), socketTimeout);
tcpSocket.setSoTimeout(socketTimeout);                    // min(10000, 0) == 0 == forever

loginTimeout bounds the socket connect, never a read. Measured against mssql/server:2022 behind a proxy that keeps established connections working and accepts-then-never-answers new ones — the state named at JDBCStorage.java:157-163 ("a moved vip, a proxy at its connection limit"). Reproduced twice, with independent harnesses:

MASTER  openTree():  RETURNED after 0,15s / 0,40s
HEAD    openTree():  *** STILL BLOCKED at the 300s and 600s caps ***
        :432 newStampConnection(url, {loginTimeout=10})
        at com.microsoft.sqlserver.jdbc.SQLServerConnection.prelogin(SQLServerConnection.java:4578)
        Thread.interrupt() -> thread alive=true

encrypt is irrelevant — the block precedes TLS. One property fixes it, measured at 10,08 s with SQLState 08S01, which isTransientFailure() already routes into giveUp():

MICROSOFT("set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000),
	"loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS,
	"socketTimeout", STAMP_READ_TIMEOUT_SECONDS*1000);   // ms

PostgreSQL needs socketTimeout too for its post-login reads (blocked 60 s on a blackholed socket), but mind the unit — pgjdbc's is in seconds, so STAMP_READ_TIMEOUT_SECONDS raw, not *1000. Its login is already bounded at 10,8 s, so PostgreSQL alone would be minor. setQueryTimeout is not an alternative: it unblocks none of pgjdbc, mssql-jdbc or Oracle — pgjdbc's cancel travels on a second connection, and postgres logged canceling statement due to user request while the client stayed blocked the full 45 s. Oracle and MySQL are correctly bounded (Oracle measured 30,01 s; do not "simplify" oracle.jdbc.ReadTimeout to oracle.net.READ_TIMEOUT, which is inert via Properties — measured 93 s).

The readback connection is unbounded, and can self-deadlock (major)

commentTable takes a second pooled connection at JDBCStorage.java:421, reaching opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:100:

final Connection conNew = DriverManager.getConnection(connectionString);   // no Properties: no bound
...
} catch (SQLException e) { // max_connection server error: try recursion for reuse connection
	return getConnection(connectionString, (waitTime == 0) ? 1 : waitTime * 2);
}

Not a rare path. Measured with the real CachedConnection: storage.open() creates one connection and returns it, write():702 takes that same object, so the pool is empty and :421 connects physically on every read-write backend open:

open() borrowed parent=5ed828d | returned it -> pool=1
write():702 borrowed parent=5ed828d (same object? true) -> pool=0
:421 tree#1 NEW PHYSICAL CONNECT? true

Warm pool of 1, new connects silenced, PostgreSQL: blocked >120 s, Thread.interrupt() at 60 s had no effect, while the transaction's own select 1 kept working. Master issues no connect here at all. And a role at CONNECTION LIMIT 1 deadlocks permanently:

MASTER open()+write():702+openTree() completed in 0,473 s (openTree issues no connect)
--- t=90 s :421 TIMED_WAITING recursive getConnection frames=19

The recursion waits for a peer to return a pooled connection, but the only one in existence is the transaction connection held by the thread it is blocking. Master's read()/write() share that loop, but there a peer really does return one; at :421 that is violated by construction.

Fix: take the dialect from the transaction connection — openTree already does exactly that at :809 — and run readStoredComment on session.connection(dialect), which is bounded. Not on the transaction connection: a failed statement aborts the PostgreSQL block (25P02, as StampSession.reset() notes), and commentTable's catch (Exception e) would swallow that and continue the open on a doomed transaction.

The test guarding the bounds cannot detect their absence (minor)

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:825-830 mirrors the enum literal:

assertEquals(dialect.connectProperties.size(), dialect == JDBCStorage.Dialect.MICROSOFT ? 1 : 2, ...

Deleting properties.putAll(dialect.connectProperties) at JDBCStorage.java:291 kills the whole feature — no bound reaches any driver — and still gives Tests run: 51, Failures: 0, Skipped: 0. It also asserts the buggy shape as the contract, so the fix above cannot land without editing it. Its javadoc says the scenario "is not something a container can stage"; two agents staged it with a ~40-line TCP proxy in under a minute.

The statistics default: throw fails the import (minor)

JDBCStorage.java:621 — the switch sits before the try (…) catch (SQLException e), so IllegalStateException propagates out of updateTableStatistics() and ImporterImpl.close(), against the contract stated 20 lines above. The same throws in commentTable (:454) and readStoredComment (:558) are inside a catch and degrade correctly.

One contended table costs the whole backend its comments (minor)

JDBCStorage.java:476 keys the sweep-wide giveUp() on isTransientFailure(), which also fires for a lock on a single table (PG 55P03, MySQL 1205, ORA-00054, MSSQL 1222). Trees open in fixed order, so one permanently contended table means nothing is ever stamped, on any open. Before round 6 the other ~24 still got their comments for one 5 s bound.

aborted() can swallow the Throwable that caused the abort (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/OnDiskMergeImporter.java:1202 — if importStrategy.aborted() throws, throw t never runs. Concrete for the motivating case: an OOM-killed import calls aborted(), which for TracedImporter formats strings.

Nits

• The comment states the opposite of what holds: JDBCStorage.java:165-173 says the read bound "outlives the login phase and covers the comment statement too" — true only for MySQL and Oracle; :198 says SQL Server's loginTimeout needs "no read bound of its own", which is the belief the major above rests on.
• MySQL's 30 s read bound is really ~60 s: measured 60,09 s with socketTimeout=30000, 30,02 s with sslMode=DISABLED. NetworkResources.forceClose() skips its shutdownInput() fast path on secure sockets, so SSLSocketImpl.close() drains input waiting for close_notify and pays SO_TIMEOUT twice. Connector/J defaults to sslMode=PREFERRED; 15000 gives a true 30 s.
• pgjdbc connectTimeout=10 is a no-op: 10 is already the PGProperty.CONNECT_TIMEOUT default.
• testSqlModeProbedOncePerSweep is vacuous on 3 of 4 dialects: the expectation is literally 0 and backslashIsEscape() is never reached, so it contributes 1 of the 51 on Pg/MsSql/Oracle without testing anything. Real teeth on MySQL.
• A green "51/51" can mean nothing: Testcontainers 1.20.6 negotiates docker-java API v1.32, Docker Engine 29.6.2 has MinAPIVersion: 1.40, so isDockerAvailable() goes false and every test throws SkipException at TestCase.java:82 — Tests run: 53, Skipped: 53, BUILD SUCCESS, EXIT=0. Pre-existing; -Dapi.version=1.43 is needed for the counts to mean what they say.

Verified clean, not worth re-raising: the commit-after-SET fix and its test (fails on the previous revision with expected [5s] but found [0], PostgreSQL only, as designed); the enum rewrite (compiled and run — connectProperties non-null in both constructors, every name, unit and value correct, no mutable-map leak); gaveUp is per-transaction (stampSession is a field of WriteableTransactionTransactionImpl at :786, fresh per write(), closed at :713); the mysqlBackslashEscape stale-cache path does not exist, since close() clears it outside the if (con!=null) guard; StampSession is single-threaded (ImporterToWriteableTransactionAdapter.openTree throws UnsupportedOperationException); catch (Throwable t) compiles under precise rethrow; no deadlock from the second pooled connection, because openTree commits its DDL at :826/:837/:846/:858 first — I reproduced the SQL Server LCK_M_SCH_S block in the hypothetical (15000 ms → Msg 1222) and measured the real path at 0 ms; testLockBoundSurvivesAFailedStamp and testTransientStampFailureEndsTheSweep both have teeth; and deferring the CachedConnection retry loop to #872 is right, since the gaveUp check sits before the pooled getConnection().

[vharseko]

Round 6 is addressed — and both majors were right, including the part I would have argued with a round ago. Thank you for measuring rather than reasoning about TDSChannel.open(); min(timerRemaining, 0) == 0 is not something the property name suggests.

The SQL Server stamp connection has no bound at all

Fixed, and PostgreSQL with it. Every dialect now declares both phases, and the two-constructor split you reviewed last round survives — one constructor for the three drivers whose connect property and read property bound the login between them, one for PostgreSQL, which also wants loginTimeout for the login pgjdbc runs on a thread of its own:

MICROSOFT("set lock_timeout "+(COMMENT_LOCK_TIMEOUT_SECONDS*1000),
    "loginTimeout", STAMP_CONNECT_TIMEOUT_SECONDS, "socketTimeout", STAMP_READ_TIMEOUT_SECONDS*1000);

Units as you gave them: mssql-jdbc's socketTimeout in ms, pgjdbc's in seconds. oracle.jdbc.ReadTimeout left alone — the note about oracle.net.READ_TIMEOUT being inert via Properties is now in the code, so nobody "simplifies" it later.

The comment at :165-173 no longer says the opposite of what holds, and the :198 line that the belief rested on is gone. The MySQL doubling under sslMode=PREFERRED is documented at the constant rather than compensated for in the value: halving it would be a lie in the other direction the moment somebody sets sslMode=DISABLED.

The readback connection is unbounded, and can self-deadlock

Fixed as prescribed: the readback runs on session.connection(dialect), and the dialect comes from the transaction's own connection at the openTree() call site. No pooled borrow is left in the comment path.

One detail worth recording, since it is the reason the change is slightly larger than "swap the argument": readStoredComment() opened with dialectOf(con), which is ((CachedConnection) con).parent — handing it the stamp connection is a ClassCastException, not a fix. The dialect is a parameter now, on both readStoredComment() and commentTable().

This does change what a steady-state open costs, and the PR text now says so instead of claiming zero: one connection per open, shared by every tree of it, rather than one pooled borrow per open (which, as you measured at :421, was a physical connect anyway with the pool empty). No DDL and no lock when the comments are in place — that part is unchanged. testCommentStampsShareOneConnection asserts the new number, 1 per open for 3 trees, rather than being quietly relaxed.

The test guarding the bounds cannot detect their absence

You were right that this was the finding that mattered most, because it is why the other two survived five rounds. Replaced by StampConnectionTestCase — no database, no docker, so it runs in every build:

• testEveryDriverGivesUpOnASilentServer — your ~40-line TCP proxy, minus the proxy: a ServerSocket that accepts and answers nothing. All four real drivers, attempted concurrently, so the suite pays the slowest bound rather than the sum. 31 s.
• testStampConnectionHandsItsBoundsToTheDriver — a recording java.sql.Driver; every declared property must arrive, and as a copy.
• testEveryDialectDeclaresBothBounds — what the old test did, minus the assertion of the buggy shape.

I checked it has teeth the way you asked me to rather than assuming: deleting properties.putAll(dialect.connectProperties) — the one line that kills the whole feature — turns it red. That is the check the old test could not make.

One contended table costs the whole backend its comments

Fixed, and you are right that round 6 made this worse than round 5. isTransientFailure() was a two-way answer to a three-way question; it is FailureScope now:

	what failed	what follows
TREE	the database rejected the statement	remembered; the rest of the sweep is stamped as usual
MOMENT	the statement gave up on a lock	nothing remembered, sweep goes on — the lock belongs to that table
SESSION	the connection is gone	sweep ends; nothing remembered

testContendedTableDoesNotEndTheSweep covers it per dialect (each reports its own lock-timeout code), and testTransientStampFailureEndsTheSweep is now testConnectionFailureEndsTheSweep, which is what it always tested.

The statistics default: throw fails the import

Fixed — the statement is chosen inside the try, and the catch is Exception, so the guard degrades to "this table was not refreshed" like every other failure there.

aborted() can swallow the Throwable that caused the abort

Fixed with addSuppressed, so the OOM still arrives and the notification failure travels with it.

One you did not raise

failureScope() walked getCause() only. Drivers report a vendor error as the next exception of a generic one at least as often as they report it as the cause, and on that path a lock timeout was classified as a rejection — the tree left unstamped until the next start over a moment of contention, which is the exact outcome isTransientFailure() existed to prevent. Both chains are walked now, cycle-safe; testFailureScopeWalksBothChains covers it.

Also: a remembered rejection is cleared in close(), so disabling and re-enabling the backend is enough once the privilege is granted. "Until the next start" meaning a server restart was a harsher contract than intended.

Nits

• connectTimeout=10 on pgjdbc is indeed already the default — kept, deliberately: the four dialects read as one table, and a reader should not have to know which driver's default happens to coincide. It costs nothing.
• testSqlModeProbedOncePerSweep stays as it is. It has teeth on MySQL and costs a no-op assertion elsewhere; splitting it per dialect buys less than it complicates.
• The Testcontainers/api.version point is real and pre-existing, and it is the reason the new bound tests were written to need no docker at all — a bound only the container suites exercise is a bound that a silent skip deletes. Fixing the negotiation itself belongs in its own change.

Not in this PR, on purpose

#872 generalizes exactly this connect-bounding into CachedConnection.ConnectDialect — same four drivers, same two-phase shape, and it reaches the same conclusion about SQL Server independently. Once it lands, JDBCStorage.Dialect.connectProperties should go and the stamp connection should bound itself through the shared helper, which also brings declaredInUrl() with it: this PR's putAll overrides a connectTimeout an administrator put in the URL, which #872 treats as a bug and I would rather fix there than duplicate the fix here. Same for the driverName.contains(...) chains still in getTableDialect(), openTree() and upsert() — worth folding into Dialect, but as a change of its own rather than inside this one.

Re-run on all four: PgSql, MySql, MsSql and Oracle at 51/51, zero skips, plus StampConnectionTestCase 5/5 without a database.

One note on the local numbers, since you check them: the first PostgreSQL run of this revision came back 53 run, 1 failure, 52 skipped — EmbeddedDirectoryServer could not bind 0.0.0.0:65532, "Address already in use". Not this change and not the container: TestCaseUtils.findFreePorts(4) binds free ports, closes them, and hands the numbers to the server, so a second JVM starting in that window takes one. Six of them were running on the machine at the time. It passed on the retry, and it is worth knowing the shape of it before it is read as a flaky test of this PR.