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 '…'
MySQL	ALTER TABLE … COMMENT '…'
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 dedicated pooled connection, 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. Comment statements also take locks (a metadata lock on MySQL, a DDL lock on Oracle) and openTree() runs on every backend open, so the stored comment is read back from the catalog first and the statement is only issued when it is absent or stale: steady-state opens cost one catalog SELECT per tree — no DDL, no lock. 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 failure to stamp is logged at debug, never fails the backend, and can no longer disturb anything else.

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. 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: a failure is logged as a warning and never fails the import, but the method reports success so tests catch rejected SQL.

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

• 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 is skipped when the stored comment matches (a readback that always reported "absent" fails the test), and a stale comment is re-stamped.
• testCommentFailureLeavesTransactionIntact — a failing stamp leaves a write pending in the caller's transaction untouched; fails against the previous code on MySQL, Oracle and MS SQL.
• 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), sys.dm_db_stats_properties(…).last_updated set (MS SQL) — plus a direct assertion that the dialect-specific refresh statement is accepted.

All four container test suites pass locally: PgSql 42/42, MySql 42/42, Oracle 42/42, MsSql 42/42 — no skips.

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.