[#873] Give each backend its own compressed schema trees

[vharseko]

Fixes #873

Problem

The two trees holding the compressed schema definitions were named from a literal, so they carried no backend qualifier while every other tree of a backend is named from EntryContainer.getTreePrefix() and so carries its base DN:

// PersistentCompressedSchema.java, before
private static final TreeName adTreeName = new TreeName("compressed_schema", DB_NAME_AD);
private static final TreeName ocTreeName = new TreeName("compressed_schema", DB_NAME_OC);

That is harmless where a storage holds a single backend — JE and PDB give each its own directory, and CASStorage.getTableName() names its table after the backend id — but JDBCStorage derives its table name from the tree name alone, so the two names were constants for every JDBC backend of every server sharing a database:

/compressed_schema/compressed_attributes     -> opendj_e2294f6da66249788dd98ef0eaa7e2e1ac1ecee36e0379978aaba584
/compressed_schema/compressed_object_classes -> opendj_2d5bf8029431e9bfc9219e314541368cb2e08a08ce8271ff032c79b8

Two such backends therefore shared one token space. CompressedSchema.exclusiveLock is an instance field, and a token is allocated from the size of the instance's own decode map, so two backends that loaded the same table at open held the same next token: backend A allocated N for cn and wrote row N → cn, backend B allocated N for sn and overwrote it. PersistentCompressedSchema.store() issues a bare txn.put, an upsert with no condition, so the last writer won silently.

Both kept serving from memory, so nothing showed until a restart. After it, decodeAttribute returns whatever the row holds and raises ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN only when the token is absent altogether — a token resolving to the wrong attribute description decodes without any error at all.

Nothing validates getDBDirectory() for uniqueness, so the configuration is accepted without a word. The test suite already worked around the collision (jdbc/TestCase#dropStaleTrees).

Fix

The trees are now named /compressed_schema_<backendId>/..., the qualifier being the backend id rather than an entry container's tree prefix: PersistentCompressedSchema is built once per RootContainer, before any entry container exists, and config.getBaseDN() is a set, so a backend holding two base DNs has no single prefix to borrow.

Migration lives in load(), not in the upgrade tooling. Written against the SPI, it covers JE, PDB, JDBC and Cassandra with one piece of code and moves two small trees per backend. On its first open a backend upgraded from a version that shared the pair copies across the records the shared trees hold and its own do not, then loads from its own. Specifically:

• The copy is fail-closed, but does not hide the storage. If it cannot complete, the open fails with ERR_COMPSCHEMA_CANNOT_MIGRATE, naming the pair of trees that failed. Carrying on with no definitions would restart token allocation from zero and mis-decode every entry already written, reporting nothing. A failure the storage reports as its own is left with the type it was given: the migration runs inside the WriteOperation of RootContainer.open(), and PDBStorage.write() decides by type what to do with what leaves it, so a RollbackException arriving there wrapped would fail the open permanently where the same conflict used to be replayed.
• The shared trees are read, never emptied. On a shared database they may still be the only copy a backend that has not been upgraded yet has.
• Only missing keys are copied, never overwriting a definition the backend already owns. That makes the migration safe to re-run after it was interrupted — which is how a storage without transactions recovers from a copy that stopped halfway — and safe against a legacy tree a backend of an earlier version is still writing to. It is also what keeps the pre-upgrade collision from spreading: where the shared tree holds another backend's definition under a token this backend has already used, the definition its own entries were encoded against is the one that survives.
• A read-only open cannot migrate, so export-ldif and verify-index read the shared trees where they lie, loading them before the backend's own so that anything already migrated wins for the same token. That settles the decode map only — CompressedSchema.loadAttributeToMaps keys the encode map by attribute description, so a displaced legacy description stays in it — which is harmless because nothing encodes during a read-only open, and the comment now says that rather than claiming more. It asks openTree() of nothing either: JEStorage.openTree ignores createOnDemand and reaches env.openDatabase() with setAllowCreate(true), so asking at all would leave two empty databases behind an offline tool run against a backend that has not migrated yet.
• The migration is reported once, and only if it happened. RootContainer.open() logs NOTE_COMPSCHEMA_MIGRATED after the transaction commits and only where records were copied, so a copy a rollback undid — or a PersistIt replay — no longer reports one. The message names both pairs of trees in full.

Deciding all of that needs a question no read can answer: a storage may materialize a tree on first access — JE opens its databases with setAllowCreate(true), so a cursor would create the very tree whose absence is being tested — or reject the access outright, as JDBC does when no table of that name exists. So ReadableTransaction gains treeExists(), implemented by the four storages and delegated by TracedStorage; the two importer transaction adapters throw UnsupportedOperationException, as they already do for the operations they cannot serve.

A rejected query is not an absent tree. The Cassandra backend keeps every tree of a backend as a partition of the one table named after the backend id, so it can only answer whether that partition holds a record — and where the table itself has not been created, the driver rejects the query rather than returning nothing. InvalidQueryException carries the driver's whole INVALID protocol code, so two conditions have to hold before a rejection is read as an absent tree, reporting a populated tree absent being the very corruption this PR is about:

• the message has to name an absent table or keyspace. A table that is there with a shape this backend did not write — an undefined column, say — is not an absent tree;
• and only a read-only transaction may answer it at all, which is what a read-only open of a never-written backend needs, since openTree() creates nothing there. A writeable transaction has just created the table through openTree(), so a rejection there is a fault and fails the open, as the unconditional cursor of the old load() did. That is also where a table a coordinator has not caught up with lands — what a rolling upgrade produces, since schema agreement is never reached in a mixed-version cluster — and it fails loudly rather than passing for a table that was never created.

Which name a JDBC statement takes says who owns the tree it names. listTrees() reads the tree2table cache and removeStorageFiles() drops every table it names, so reading another backend's tree must not put it up for removal — otherwise import-ldif --clearBackend on one backend would drop the shared trees this migration promises to leave alone. A path that creates or writes a tree — openTree(), clearTree(), deleteTree(), put(), update(), delete(), and a cursor's delete() — takes the enrolling getTableName(); a path that only reads takes the new readTableName(). Guarding the existence probe alone would not have held: the migration counts the shared tree and copies it out, so the next statement would have enrolled it again.

readTableName() answers from that same cache where the tree is in it, and computes the name — a SHA-224 digest — without entering it there otherwise. Every tree the backend owns passes through openTree(name, true) as it is opened, so read(), the per-entry hot path of every search, stays a map lookup rather than a JCA provider lookup and a digest per call; only the shared tree, twice per open of the backend, is computed. listTrees() still names the complete owned set.

UpgradeTasks is deliberately untouched. Its rename step (UpgradeTasks.java:860) migrates 2.x local-db backends to pluggable JE by renaming JE databases through a JE Environment directly, and never runs for PDB or JDBC; what it produces is the shared pair, which the first open then migrates like any other.

Not fixed by this

An installation where two backends have already been sharing the tables is damaged before the upgrade. Migration hands each of them the same, already-corrupted snapshot: it stops the damage growing and prevents any further collision, but it cannot repair entries written earlier. Those need an export/reimport from whichever side is intact. A workaround remains available for anyone not yet upgraded — the tables are unqualified by schema, so giving each JDBC backend its own database, or its own schema/user in the URL, keeps them apart.

Naming these two trees after the backend id has a price of its own: alone among the trees of a backend they do not follow the entries when a JDBC backend is deleted and re-created under another id over the same database, every other tree being named from its base DN and found again. Token allocation would then restart at zero over entries encoded with the definitions of the old id, so changing a backend id over populated storage needs an export and a re-import — as it always has on Cassandra, where the one table of a backend is named after the id and the entries do not survive the rename either. ds-cfg-backend-id is read-only in configuration, so this takes deleting and re-creating the backend; the javadoc of treePrefix() records it.

Two adjacent defects of the JDBC backend surfaced during review and are tracked on their own rather than folded in here: isExistsTable() asked the catalog for every table of the database, with neither a catalog nor a schema filter (#887, since fixed on master and merged into this branch), and removeStorageFiles() drops only the tables the running process has already touched, so an offline import-ldif --clearBackend clears nothing (#888). Both predate this change, although the migration is the first caller whose decision rests on what isExistsTable() answers.

Verification

PersistentCompressedSchemaTest — 8 cases against an in-memory transaction standing for one database addressed by several backends, so it runs without a container. The fixture models two behaviours of a real engine on purpose: it materializes a tree whenever one is asked for, whatever createOnDemand says, which is what JEStorage does, and a write can be made to fail.

case	asserts
backendsSharingOneDatabaseDoNotShareTheTokenSpace	both backends opened before either writes — as at server start, which is what makes them hold the same next token — then reopened; each decodes its own attribute
definitionsWrittenBeforeTheUpgradeAreMigrated	a store made to look pre-upgrade decodes correctly afterwards, and the record counts match
theSharedTreesSurviveTheMigration	the shared trees still hold their records
anInterruptedMigrationIsFinishedByTheNextOpen	one record removed from a completed migration is restored by the next open
aReadOnlyOpenReadsTheSharedTreesWhereTheyLie	a read-only open decodes correctly and writes nothing — not even the empty trees an openTree() would leave behind
aDefinitionOfThisBackendIsNeverOverwrittenByTheSharedOne	the shared trees hold another backend's definition under a token this backend has used: its own survives, and the definition it lacked is still copied
aMigrationThatCannotCompleteFailsTheOpen	a failing write fails the open with ERR_COMPSCHEMA_CANNOT_MIGRATE, naming the backend and both trees
aStorageFailureIsLeftForTheStorageToRecognize	a StorageRuntimeException leaves the open with its own type, so the storage's retry can still recognize it

Each guard was checked against the change it exists to catch, by taking that change out and watching the right test fail: without the backend qualifier, backendsSharingOneDatabaseDoNotShareTheTokenSpace reports expected:<"cn"> but was:<"sn"> — backend A's entry decoding as backend B's attribute, the corruption itself; without the never-overwrite guard, aDefinitionOfThisBackendIsNeverOverwrittenByTheSharedOne reports the same; with openTree(adTreeName, shouldCreate) back in the read-only branch, aReadOnlyOpenReadsTheSharedTreesWhereTheyLie finds the trees created; with the storage failure wrapped again, aStorageFailureIsLeftForTheStorageToRecognize catches an InitializationException; and with no fail-closed at all, aMigrationThatCannotCompleteFailsTheOpen sees the raw failure go by.

Four cases added to jdbc/TestCase, inherited by all four engine suites:

• testTreeExistsAnswersForAMissingTable — from a writeable and from a read-only transaction, and after a deleteTree
• testCompressedSchemaTableIsQualifiedByBackendId — end to end on a real database: after the suite's backend has been opened and populated, the backend-qualified table exists and the shared one does not
• testProbingATreeDoesNotPutItUpForRemoval — a second storage on the same database runs every read the migration runs against a tree it never opened (exists, count, read, cursor), then removeStorageFiles(); the tree is listed for removal by none of them and the table is still there
• testDeletingThroughACursorPutsTheTreeUpForRemoval — the other side of the same rule: a cursor that deletes writes to the tree, so that one is listed

One case added to cassandra/TestCase:

• testTreeExistsAnswersForAMissingTable — a table openTree() has not created answers false to a read and throws out of a writeable transaction; then, once created, the tree is there only while it holds a record

DummyWriteableTransaction.openTree now honours createOnDemand and no longer replaces an already open tree with an empty one.

Test runs, all green:

suite	result
PersistentCompressedSchemaTest	8/8
JETestCase, EncryptedJETestCase	34/34 each
PDBTestCase, EncryptedPDBTestCase	34/34 each
PDBStorageTest	3/3
DefaultIndexTest	5/5
OnDiskMergeImporterTest	29/29
PgSqlTestCase	57/57
MySqlTestCase	57/57
cassandra/TestCase	39/39

MsSqlTestCase and OracleTestCase were not run locally — no dialect-specific SQL was added, the path is shared with the two engines above — and are left to CI.

[maximthomas]

Nice change — the per-backend prefix is the right shape for #873, the migration is idempotent and crash-safe, and I checked that tokens never reach the replication protocol, the changelog, LDIF export or a backup archive, so this cannot break cross-server decoding. Two things I'd like fixed before merge, plus some smaller notes.

Cassandra treeExists() fails open where the old code failed closed (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java:262

}catch (RuntimeException e) {
    for (Throwable cause=e; cause!=null; cause=cause.getCause()) {
        if (cause instanceof InvalidQueryException) {
            return false;
        }
    }
    throw e;
}

InvalidQueryException is the driver's bucket for the whole INVALID protocol code, not a "table absent" signal — keyspace absent, undefined column and unconfigured-table all arrive as the same type. Returning false for any of them means a populated tree is reported absent, and load() then skips it:

// PersistentCompressedSchema.java:289 / :315
if (txn.treeExists(ocTree)) { ... }

with CompressedSchema.getAttributeId allocating from id = mappings.adDecodeMap.size(), i.e. restarting at 0 and overwriting live definitions. That is the same silent mis-decoding this PR exists to fix. The two guards are independent, so one bad probe is enough to corrupt one token space on its own.

Before this PR loadTrees opened the cursor unconditionally, so a rejected query propagated out of RootContainer.open and the backend failed to open — fail-closed. Cassandra is now the only implementation of the new SPI method that fails open; JE, PDB and JDBC all throw StorageRuntimeException.

The realistic trigger is a coordinator whose schema does not yet contain the table. The driver waits for schema agreement only on SchemaChange responses, its checker completes with false (not an error) on the 10s timeout, CASStorage never calls isSchemaInAgreement(), and a QueryValidationException is never retried on another node. The driver's own reference.conf documents exactly this ("getting an unconfigured table error for a table that you created right before"), and notes agreement never succeeds in a mixed-version cluster — i.e. during a rolling upgrade.

Suggested fix — ask the driver's local metadata snapshot instead of inferring absence from an exception, and let everything else propagate:

if (!session.getMetadata().getKeyspace(keyspace)
        .flatMap(ks -> ks.getTable(table)).isPresent()) {
    return false;   // the backend's table was never created
}
return execute(...).one() != null;   // any rejection now fails the open

That mirrors what JDBCStorage.isExistsTable already does with con.getMetaData().getTables(...). If you prefer to keep the exception shape, at minimum drop the cause-chain walk and catch InvalidQueryException directly.

toTableName() does not cover the paths it was written for (minor)

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

The new comment promises that probing a tree will not enrol it:

toTableName() rather than getTableName(): asking whether a tree is there must not enrol it in tree2table.

but only isExistsTable was switched. The caller re-enrols the tree on the very next statement, because both of these still use the populating getTableName():

// :261  needsLegacyDefinitions -> recordCount -> getRecordCount
"select count(*) from "+getTableName(treeName)
// :516  copyMissingRecords / loadTrees -> CursorImpl
this.tableName=getTableName(treeName);

listTrees() returns tree2table.asMap().keySet() and removeStorageFiles() drops every table it names, so the shared legacy tables still end up on the drop list of any backend that migrates — on every open, forever, since the legacy pair is deliberately never deleted.

To be clear on severity: this is not a regression. On master isExistsTable also did tree2table.get(treeName), and the legacy names were the backend's own names, so master enrolled and dropped them too. The defect is that the guard is incomplete and its comment and new test (testProbingATreeDoesNotPutItUpForRemoval, which only exercises treeExists) claim a protection that isn't delivered.

Minimal fix: keep the populating getTableName() on paths that create or write a tree (openTree, clearTree, deleteTree, put/delete), and use the pure toTableName() on read-only paths (read :243, getRecordCount :261, CursorImpl :516). Every tree the backend owns passes through openTree(name, true) during startup, so listTrees() still sees the complete owned set.

Nits

• Read-only open creates the new trees on JE: load() calls txn.openTree(adTreeName, shouldCreate) even when read-only, and JEStorage.openTree ignores createOnDemand — it reaches env.openDatabase(null, name, dbConfig()) with setAllowCreate(true). Since the names changed, an offline export-ldif/verify-index on an unmigrated backend now leaves two empty databases behind. Harmless, but aReadOnlyOpenReadsTheSharedTreesWhereTheyLie asserts this against a DummyWriteableTransaction, so it passes while the real engine does something else.
• A brand-new backend inherits the legacy snapshot: needsLegacyDefinitions() is true for any backend whose own trees are smaller than the legacy pair, including one created fresh after the upgrade on a shared JDBC/Cassandra database. It has no entries at all, yet copies every foreign definition and starts its token space at N. Gating on the own trees being absent would avoid it.
• isExistsTable probes every catalog and schema: con.getMetaData().getTables(null, null, null, {"TABLE"}) — null catalog and null schema. On MySQL that spans every database on the server. treeExists can then answer true for a table that the unqualified select count(*) from opendj_<hash> in getRecordCount cannot resolve, throwing out of needsLegacyDefinitions, which sits outside the migration's try/catch. Pre-existing, but this PR makes a new decision depend on it.
• The cause-chain walk is dead code: the comment says "the statement cache may hand back the loader's failure wrapped", but Caffeine rethrows a loader RuntimeException unwrapped (catch (RuntimeException e) { throw e; }) and only wraps checked exceptions — which session.prepare cannot throw. The walk only widens what gets swallowed.
• treeExists contradicts the SPI javadoc it ships with: ReadableTransaction.java:56 says "This is not the same question as whether the tree is empty"; CASStorage.java:253 redefines it as "exists here means holds at least one record". Harmless for today's only caller, a trap for the next.
• No Cassandra test for the new treeExists: JDBC got three new tests; the Cassandra TestCase gained none, so neither the fail-open branch nor the empty-tree semantics is exercised.
• CodeQL comment on JDBCStorage.java:130: false positive re-triggered by the code move — SHA-224 is unchanged from master and only derives a deterministic table name, no security property. Worth dismissing explicitly rather than leaving open.
• Downgrade defeats the count heuristic: needsLegacyDefinitions compares record counts only. After a downgrade the old binary resumes allocating in the legacy trees, and on re-upgrade legacyCount == ownCount makes migration a no-op while the two disagree. Downgrade isn't a supported path, but the javadoc's "trees only ever grow" invariant holds only one-way and could say so.

[vharseko]

Thanks — the fail-open was a real hole, and the incomplete guard was worse than the comment it shipped with. Both are fixed in 9472182, together with three of the nits; the PR description is updated to match. Where I went a different way than suggested, the reasoning is below.

Cassandra treeExists() — fixed, but not through the driver metadata

Agreed on the finding: InvalidQueryException is the driver's whole INVALID bucket, master failed closed through the unconditional cursor, and skipping a populated tree restarts the token allocation from zero over live definitions.

I did not take the session.getMetadata() route, for two reasons:

• schema metadata can be turned off (advanced.metadata.schema.enabled = false), and with an empty snapshot getKeyspace() returns Optional.empty() for a table that is right there — so treeExists() would answer false always, which is the same corruption made deterministic;
• getKeyspaceName() and getTableName() build quoted identifiers, so the lookup would need CqlIdentifier.fromCql() on both halves to match at all.

Instead the swallow is kept but bounded to where nothing can follow from the answer:

}catch (InvalidQueryException e) {          // no cause-chain walk any more
    if (accessMode.isWriteable()) {
        throw e;
    }
    return false;
}

The only legitimate reason to answer "absent" is a read-only open of a backend that was never written, where openTree() creates nothing. A writeable transaction has just run CREATE TABLE IF NOT EXISTS through openTree(), so a rejection there is a fault and now fails the open exactly as it did before treeExists() existed. The unconfigured-table window you describe — a coordinator behind on schema during a rolling upgrade — lands in the writeable case, where it now fails loudly instead of silently mis-allocating.

Worth noting where the mode comes from: CASStorage.read() always builds its transaction with AccessMode.READ_ONLY, and write() passes the storage's own mode, so the compressed schema — which runs inside write() — is governed by how the backend was opened, which is the distinction that matters here.

The cause-chain walk went with it: you are right that Caffeine rethrows a loader RuntimeException unwrapped and only wraps checked ones, which session.prepare cannot throw.

New cassandra/TestCase#testTreeExistsAnswersForAMissingTable covers both branches — a table openTree() never created answers false to a read and throws out of a writeable transaction — and then pins the "at least one record" semantics: after openTree() the tree is absent until a put, and absent again after deleteTree.

toTableName() — extended to every read path

Fixed as suggested: read(), getRecordCount() and CursorImpl now take the pure toTableName(), and openTree(), clearTree(), deleteTree(), put(), update() and delete() keep the enrolling getTableName(). The rule is written down on toTableName() itself so the next statement added has somewhere to look, and testProbingATreeDoesNotPutItUpForRemoval now runs every read the migration runs — exists, count, read, cursor — instead of only the first.

On severity, one thing to add rather than to argue: listTrees() is a per-process cache that nothing seeds — open() only takes a connection — and the one caller that reads it before the root container exists is removeStorageFiles(), from BackendImpl.importLDIF(clearBackend). In the offline import-ldif tool nothing has touched a tree by then, so the drop loop is skipped entirely and nothing is cleared; in the online import task the same call does drop tables, because there the storage instance has been serving traffic and close() does not invalidate the cache. So the leftovers the incomplete guard could produce were real but narrower than the drop list suggests — and the underlying inconsistency is its own defect, filed as #888.

Read-only open creating the trees on JE — fixed

load() now asks openTree() only where the trees may be created:

if (shouldCreate)
{
  txn.openTree(adTreeName, true);
  txn.openTree(ocTreeName, true);
}

JEStorage.openTree ignores createOnDemand and reaches env.openDatabase() with setAllowCreate(true), so passing the flag through was the bug; nothing below needs the trees open, since every read is guarded by treeExists(). That also makes the assertion in aReadOnlyOpenReadsTheSharedTreesWhereTheyLie mean what it says against a real engine and not only against the dummy transaction.

isExistsTable probing every catalog and schema — agreed, tracked separately

Right, and it is worse than a correctness question — the method walks the entire catalog once per tree. That is #887, which already asks for the table name to be passed as the pattern and for the identifier case to come from storesUpperCaseIdentifiers()/storesLowerCaseIdentifiers(); the catalog and schema filters belong in the same change. Folding it in here would duplicate that work, so this PR leaves it alone.

SPI javadoc, downgrade — documented

ReadableTransaction.treeExists() now says what a storage whose trees have no existence of their own answers, so the Cassandra semantics are part of the contract instead of contradicting it, and needsLegacyDefinitions() records that "trees only ever grow" holds in one direction only and that downgrading across the separation is not a supported path.

A brand-new backend inheriting the legacy snapshot — kept, deliberately

This is the one I would like to keep as it is. Gating on the backend's own trees being absent would break the case the method exists for: after an interrupted migration the own trees are present and half-copied, the gate would answer "nothing to migrate", and the backend would then allocate tokens from a map smaller than the definitions its entries were encoded with — the corruption this PR is about, reached by the recovery path.

What a fresh backend inherits, by contrast, is a complete and self-consistent token-to-definition mapping; the cost is one copy of a few hundred rows at its first open, and nothing decodes wrongly because of it. I have written that trade-off into the javadoc of needsLegacyDefinitions() so it reads as a decision rather than an oversight. Happy to revisit if you see a case where inheriting is actually harmful.

CodeQL on JDBCStorage.java:130

False positive, as you say: the digest is unchanged from master and only derives a deterministic table name, with no security property resting on it. I will dismiss the alert as such in code scanning unless you would rather it stayed open for the record.

Test runs

suite	result
PersistentCompressedSchemaTest	5/5
PgSqlTestCase	40/40
cassandra/TestCase	39/39
JETestCase	34/34
PDBTestCase	34/34
PDBStorageTest	3/3

[maximthomas]

Please resolve conflicts

[vharseko]

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

Conflicts and how they were resolved:

• isExistsTable(): this branch moves it into ReadableTransactionImpl (that is what treeExists() is answered from) and names the table with toTableName() so that a probe is not enrolled in tree2table; master meanwhile rewrote its body to ask the catalog for one name (storedIdentifier()) instead of listing every table. Both kept: master's body, in the readable transaction, over toTableName(). The copy left in WriteableTransactionTransactionImpl is gone - it inherits the one above.
• ReadableTransactionImpl.read(): toTableName() from here with master's hashParam(con).
• jdbc/TestCase.java: master's testTableOfATreeIsFoundByName and the three tests of this PR (plus the isExistingTable helper) all kept.

getTableName() is left in place in commentTable() and updateTableStatistics(), the paths master added: both run over trees this backend owns and has already opened, which is the side of the split this PR draws.

mvn -pl opendj-server-legacy test-compile passes, PersistentCompressedSchemaTest 5/5. The container suites are left to CI.

[vharseko]

Please resolve conflicts

fixed

[maximthomas]

The separation itself looks right, and the round-1 fixes hold up: toTableName() is now used on every JDBC read path, the read-only branch no longer calls openTree(), and the Cassandra narrowing in f82de940 does what it says. One blocker and three issues below, then nits.

Migration catch-all defeats the PersistIt retry (blocker)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java:255-262

catch (final Exception e)
{
  throw new InitializationException(ERR_COMPSCHEMA_CANNOT_MIGRATE.get(...), e);
}

The migration runs inside RootContainer.open()'s WriteOperation, i.e. inside the storage retry loop. PersistIt matches by type on what leaves operation.run(this) (opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:653):

catch (final RollbackException e)
{
  // retry after random sleep

A RollbackException from copyMissingRecords arrives there wrapped, misses that clause, falls into catch (final Exception e) { txn.rollback(); throw e; }, and the backend open fails permanently — where the same conflict would previously have been replayed. Please rethrow the conflict types before the catch-all.

Only PersistIt is affected: JDBC's isRetryableConflict walks the cause chain (MAX_CAUSE_HOPS = 16) so #867's retry still fires through the wrapper, and JEStorage.write() has no retry loop.

Every JDBC read recomputes a SHA-224 table name (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:190, called from :1038 read(), :1056 getRecordCount(), :1073 isExistsTable(), :1330 CursorImpl ctor.

static String toTableName(TreeName treeName) {
    final MessageDigest md = MessageDigest.getInstance("SHA-224");
    final byte[] messageDigest = md.digest(treeName.toString().getBytes());
    ...

The split from getTableName() is correct — listTrees() is tree2table.asMap().keySet(), so a probe must not enrol — but toTableName() has no memo, and txn.read() is the per-entry hot path (DN2ID:127, ID2Entry:485, DefaultIndex:274, VLVIndex:778, State:96). A 1000-entry search now does 1000 JCA provider lookups plus digests where it previously did 1000 map gets. A second, non-enrolling cache keyed by TreeName keeps the split and removes the regression.

Cassandra treeExists() treats any InvalidQueryException as "absent" (major)

opendj-server-legacy/src/main/java/org/opends/server/backends/cassandra/CASStorage.java:260-279

}catch (InvalidQueryException e) {
    if (accessMode.isWriteable()) { throw e; }
    return false;

InvalidQueryException is the driver's whole INVALID bucket, not "no such table". A backend table that exists with a different shape (no key column) makes every treeExists() answer false, so needsLegacyDefinitions() sees zero counts, loadTrees() loads nothing, and the server starts with an empty token table and decodes existing entries as the wrong attributes — the #873 failure mode from the other direction.

The accessMode guard does not limit this as intended, because CASStorage.read() builds its transaction as new TransactionImpl(AccessMode.READ_ONLY) unconditionally: under read() the rethrow is unreachable however the storage was opened. Suggest matching "unconfigured table"/"does not exist" (or checking keyspace metadata) and failing loudly on everything else.

Three guard tests cannot fail (major)

opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/PersistentCompressedSchemaTest.java

Never-overwrite is untested. makeStoreLookPreUpgrade() seeds the legacy trees from the backend's own trees, so both sides always hold identical bytes:

private void makeStoreLookPreUpgrade(String backendId) throws Exception
{
  copyTree(ownTree(backendId, AD), LEGACY_AD);

Delete the if (txn.read(to, key) == null) guard in copyMissingRecords and all five tests stay green. Seeding one legacy token with a different value and asserting the own value survives would close it.

The read-only test cannot fail for the bug it guards. aReadOnlyOpenReadsTheSharedTreesWhereTheyLie asserts treeExists(ownTree("backendA", AD)).isFalse(), but the fixture in opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/DefaultIndexTest.java:259 does nothing when createOnDemand is false:

public void openTree(TreeName name, boolean createOnDemand)
{
  if (createOnDemand)
  {
    storage.putIfAbsent(name, new TreeMap<ByteString, ByteString>());
  }
}

The JE defect being guarded is precisely that JEStorage creates the database for false. Re-introduce txn.openTree(adTreeName, shouldCreate) in the read-only branch and the suite stays green while a real offline export-ldif again leaves two empty databases behind.

Fail-closed is untested. ERR_COMPSCHEMA_CANNOT_MIGRATE appears nowhere under src/test. DummyWriteableTransaction makes a throwing put easy to inject; fine as a follow-up.

The migration NOTE can report a migration that did not happen (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java:251-253

final long copied = copyMissingRecords(txn, LEGACY_AD_TREE_NAME, adTreeName)
    + copyMissingRecords(txn, LEGACY_OC_TREE_NAME, ocTreeName);
logger.info(NOTE_COMPSCHEMA_MIGRATED, copied, backendId, LEGACY_TREE_PREFIX, adTreeName.getBaseDN());

Logged unconditionally (fires with copied == 0), before the transaction commits (a later throw in openAndRegisterEntryContainers rolls the copy back and the line still stands), and again on every PersistIt replay with a full count. It is the only evidence the migration path emits. Suggest logging only when copied > 0, after commit.

Read-only "own wins" holds only for the decode map (minor)

opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/PersistentCompressedSchema.java:206-212 — the comment says loading legacy first makes an already-migrated definition win for the same token. loadAttributeToMaps (opendj-server-legacy/src/main/java/org/opends/server/api/CompressedSchema.java:505) writes two maps keyed differently:

mappings.adEncodeMap.put(ad, id);      // keyed by description
...
mappings.adDecodeMap.set(id, ad);      // keyed by token

The second load overwrites decodeMap[id] but leaves the displaced legacy pair in encodeMap. A backend created after the fix allocates from 0, so ids collide by construction with legacy definitions from another backend: legacy 5 = cn then own 5 = sn gives decodeMap[5] = sn with cn -> 5 still standing. Encoding cn then yields a token that decodes to sn. Whether that is reachable depends on whether anything encodes during a read-only open — if nothing does, the comment just claims more than the code delivers.

Nits

• Compressed schema keyed by backend id: PersistentCompressedSchema.java:129-138 — every other tree is named from the base DN, so on JDBC they survive a config rebuild; these do not. Delete and re-create the backend under a new id over the same database and token allocation restarts at 0 over entries encoded with the old mapping. Narrow, since backend-id is read-only in config.
• CursorImpl.delete() bypasses enrolment: JDBCStorage.java:1330 caches toTableName(treeName) and :1417 writes through it, contradicting the new javadoc rule that write paths take getTableName(). Latent — every owned tree is also opened with openTree(name, true). Taking getTableName() in the ctor costs one call per cursor.
• Two javadocs contradict on downgrade: PersistentCompressedSchema.java:224 says "Downgrading across the separation is not a supported path"; :268-269 says leaving the legacy tree "is what makes a downgrade possible".
• treePrefix() javadoc misstates TreeName: it says TreeName "splits its string form on '/' and states that no component may contain one". TreeName.valueOf() splits at the last /, so a baseDN containing one round-trips, and TreeName states no such rule. The escaping is fine; the reason given is not.
• Messages 618/619 name prefixes, not trees: LEGACY_TREE_PREFIX is compressed_schema and adTreeName.getBaseDN() is compressed_schema_<id> — neither appears in backendstat list-raw-dbs. The object-class tree is never named, including in the failure message where knowing which of the two failed is the point.
• JDBC read-only comment describes an unreachable path: JDBCStorage.java:1068-1071 justifies moving isExistsTable() into ReadableTransactionImpl with a JDBC read-only open, but RootContainer.open() always uses storage.write() and WriteableTransactionTransactionImpl's ctor throws ReadOnlyStorageException on a non-writeable storage. The move is right — the live caller is the writeable migration probing the legacy tree — the stated reason is not.
• needsLegacyDefinitions() probes forever: two treeExists plus up to four getRecordCount on every open of every backend, including installs that never had legacy trees. On JDBC that is two catalog probes and up to four select count(*). No "migration done" marker lets it stop.

[vharseko]

Thanks — the blocker was real, and the three untestable guards were the more useful finding of the two. Everything is fixed in d1dd75e and the PR description is updated to match. Two places where I went a different way, and one correction, are below.

The migration catch-all — fixed

Agreed on all of it. copyMissingRecords now runs under migrateTree(), which rethrows StorageRuntimeException untouched and wraps only the rest:

catch (final StorageRuntimeException e)
{
  // Left with the type the storage gave it. PDBStorage.write() decides by type what to do
  // with what leaves operation.run(this): a transaction conflict reaches its retry as a
  // RollbackException only, so wrapping it here would turn a conflict that used to be
  // replayed into a permanent failure of the open.
  throw e;
}

Nothing is lost on the fail-closed side: a storage failure that is not a conflict still fails the open, through newRootContainer's ERR_OPEN_ENV_FAIL, and the transaction rolls back either way. aStorageFailureIsLeftForTheStorageToRecognize pins it — with the rethrow removed it catches an InitializationException instead.

While there, ERR_COMPSCHEMA_CANNOT_MIGRATE now takes the pair of trees actually being copied rather than a prefix and a base DN, so the message says which of the two failed:

... could not be migrated from the shared tree '/compressed_schema/compressed_attributes'
to '/compressed_schema_backendA/compressed_attributes': ...

toTableName() on the read path — fixed, with a memo

Right, and it is worse than a lookup: MessageDigest.getInstance is a provider lookup per call, on txn.read(). The split stays, but the read paths now go through readTableName(), which answers from the same cache without populating it:

String readTableName(TreeName treeName) {
    final String enrolled=tree2table.getIfPresent(treeName);
    return enrolled!=null ? enrolled : toTableName(treeName);
}

Every tree the backend owns is enrolled by openTree(name, true) as it is opened, so the hot path is a map get again, and only the shared tree — twice per open — is digested. No second cache to grow, and listTrees() is untouched.

Cassandra treeExists() — narrowed, but the corruption you describe is not reachable

Narrowed as suggested: a rejection now has to name an absent table or keyspace, matched across the wordings the server has used, and anything else in the INVALID bucket propagates.

The specific scenario I do not think holds, and would rather have on record than quietly fix around. "The server starts with an empty token table" needs a writeable open, and there the rethrow does fire: RootContainer.open() always goes through storage.write(), and CASStorage.write() passes the storage's own mode, which is READ_WRITE for every online open (BackendImpl.java:200,681). Your point about read() hard-coding READ_ONLY is correct, but the only caller of treeExists() today runs inside write(); READ_ONLY reaches it only from getReadOnlyRootContainer(), i.e. an offline export-ldif/verify-index, where nothing is written and a token that was not loaded raises ERR_COMPRESSEDSCHEMA_UNRECOGNIZED_AD_TOKEN — loudly, not as a wrong attribute.

So the change is worth having for the reason you give — the type is a bucket, and the guard should not rest on a caller's access mode alone — but it closes a robustness gap rather than a live corruption. cassandra/TestCase#testTreeExistsAnswersForAMissingTable still passes against cassandra:latest, so the wording of a real "no such table" is among the ones matched.

The three guards that could not fail — all three fixed

• Never-overwrite. You were right that makeStoreLookPreUpgrade() makes both sides identical. The new aDefinitionOfThisBackendIsNeverOverwrittenByTheSharedOne builds the state the issue actually leaves behind: backendA holds cn under its first token, the shared trees hold backendB's sn under the same token plus one definition backendA lacks, so the counts send the migration on its way. With the guard removed it reports expected:<"cn"> but was:<"sn"> — the same failure as the headline test, reached through the recovery path — and it also asserts that the definition backendA did not have was copied, so it cannot pass by the migration not running.
• The read-only test. Fixed at the fixture rather than at the assertion: DummyWriteableTransaction is subclassed by one that materializes a tree whenever it is asked for a tree at all, whatever createOnDemand says, which is what JEStorage does. Putting txn.openTree(adTreeName, shouldCreate) back now fails the suite.
• Fail-closed. aMigrationThatCannotCompleteFailsTheOpen injects a failing put and asserts the message names the backend and both trees; with the try/catch removed the raw IllegalStateException goes by and it fails.

Each of the five guards was checked by removing the change it exists to catch and confirming that exactly the intended test fails — the runs are listed in the PR description.

The NOTE — fixed

Agreed on all three counts. The count is kept on the instance and RootContainer.open() reports it after storage.write() returns and only when it is non-zero, so a copy a rollback undid and a PersistIt replay both report nothing. Both pairs of trees are named in full.

Read-only "own wins" — comment corrected

You are right that loadAttributeToMaps keys the two maps differently and only the decode map is settled by load order. Nothing encodes during a read-only open — export-ldif and verify-index decode — so the behaviour stands; the comment no longer claims more than that, and says which map is left holding the displaced pair.

Nits

nit	outcome
keyed by backend-id, not base DN	Kept, and written down: treePrefix() now records that these two trees alone do not follow the entries when a JDBC backend is re-created under another id, and the "Not fixed by this" section says an id change over populated storage needs an export and a re-import — as it always has on Cassandra, where the entries do not survive it either.
CursorImpl.delete() bypasses enrolment	Fixed, but not in the constructor: enrolling there would put the shared tree into tree2table on the very cursor the migration reads it with, and re-break testProbingATreeDoesNotPutItUpForRemoval. The cursor reads through readTableName() and resolves the enrolling name lazily on its first delete(), so a cursor that never deletes costs nothing. New testDeletingThroughACursorPutsTheTreeUpForRemoval covers the write side.
two javadocs contradict on downgrade	Fixed — leaving the shared trees is for a backend that has not been upgraded, not for going back; both places say that now.
treePrefix() javadoc misstates TreeName	Half right, and reworded accordingly: valueOf() does split at the last /, as you say, but TreeName's class javadoc does state the rule — "Note: This class assumes name components don't contain a '/'" (TreeName.java:24). So the escape rests on a documented assumption; the javadoc now gives that as the reason instead of the round-trip claim.
messages 618/619 name prefixes, not trees	Fixed; both name both trees in full, and 619 names the pair that failed.
JDBC read-only comment describes an unreachable path	Fixed — the comment now gives the live reason: the writeable migration probing a tree it must not create or enrol.
needsLegacyDefinitions() probes forever	Left as it is, and documented: two existence probes per open, and the counts only where the shared trees are still there. A "migration done" marker would be this backend's own record in a database it may be sharing, which is the thing this change is trying to stop doing.

Test runs

suite	result
PersistentCompressedSchemaTest	8/8
PgSqlTestCase	57/57
MySqlTestCase	57/57
cassandra/TestCase	39/39
JETestCase, EncryptedJETestCase	34/34 each
PDBTestCase, EncryptedPDBTestCase	34/34 each
PDBStorageTest	3/3
DefaultIndexTest	5/5
OnDiskMergeImporterTest	29/29