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, Oracle	COMMENT ON TABLE … IS '/dc=com,dc=example/dn2id' (also the untested default for other engines)
MySQL	ALTER TABLE … COMMENT '…'
MS SQL Server	MS_Description extended property (idempotent add/update, class=1), value and table name passed as bind parameters

Comment statements are DDL — a metadata lock on MySQL, a DDL lock on Oracle — so the stored comment is read back from the catalog first and the statement is only issued when it is absent or stale: repeated backend opens cost one catalog SELECT per tree, no DDL, no extra commit. 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), quotes are doubled, backslashes are escaped for MySQL — backslash is live in its literals, and the index-id half of a tree name is not constrained by config — and a paired-quotes guard verifies the result cannot terminate the literal. This clears CodeQL java/concatenated-sql-query alerts 1267 and 1268. A failure to stamp is logged at debug and never fails the backend.

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). deleteTree() invalidates the tree-to-table cache so dropped trees are not analyzed later. 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 (the backslash fails on the previous MySQL escaping).
• 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 39/39, MySql 39/39, Oracle 39/39, MsSql 39/39 — 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.