Skip to content

[#888] Name the trees of a JDBC backend from a catalog in the database - #893

Open
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:issues/888-jdbc-tree-catalog
Open

[#888] Name the trees of a JDBC backend from a catalog in the database#893
vharseko wants to merge 2 commits into
OpenIdentityPlatform:masterfrom
vharseko:issues/888-jdbc-tree-catalog

Conversation

@vharseko

@vharseko vharseko commented Aug 20, 2026

Copy link
Copy Markdown
Member

Fixes #888

Problem

listTrees() answered from tree2table, a cache a tree enters the first time this process names a table for it. Nothing seeds it — open() takes a connection and sets the storage status — so a process which has opened nothing names no trees:

// JDBCStorage.java, before
public Set<TreeName> listTrees() {
	return tree2table.asMap().keySet();
}

removeStorageFiles() is the one caller running before the root container is open, and it drops exactly what listTrees() names. In the offline import-ldif the backend is configured and never opened (BackendToolUtils.getBackends() calls configureBackend() alone), so the set was empty, the drop loop was skipped, and import-ldif --clearBackend cleared a JDBC backend of nothing, without a word in the log.

Online the same command does drop the tables: ImportTask disables the backend and calls importLDIF on the Backend object it already holds, whose JDBCStorage has been serving traffic with a fully populated cache — close() does not invalidate it. JE and PDB enumerate the environment itself, so both honour the contract whatever the process did earlier.

What survived was not only leftovers

The importer clears an entry container when its first entry arrives (OnDiskMergeImporter.doImport()beforePhaseOne(container)), so a base DN configured in the backend but absent from the imported LDIF was cleared by nothing at all: it kept its entries and went on serving them, where the same command on JE removes the whole backend directory. The option is documented as "Remove all entries for all base DNs in the backend before importing".

The rest of what stayed behind: the table of a base DN or of an index no longer configured, and the compressed-schema tables, all of them surviving an operation documented to clear everything, with the same command behaving differently offline and online.

Fix

The trees of a backend are recorded in the database, in a tree of their own: one row per tree, keyed by the tree name, with the table holding it as its value.

The catalog is per backend and named /opendj_catalog/<backendId>, so its table name follows from the configuration without asking the database anything — which is exactly what a process that has opened nothing needs — and so that backends sharing one database URL (#873) never name each other's trees. Being an ordinary tree it needs no dialect of its own: the same create table, the same upsert switch and the same statements serve it on all four engines. It is created without the index openTree() gives a tree, which serves the where k>? order by k cursor batches the catalog never runs, and without a comment (#866), which would only repeat what its rows say in plain text — that also keeps the cost of a stamp the database rejects where it was, one attempt per tree of the backend and not one more.

  • openTree(createOnDemand) enrols, and nothing else does. Naming a tree in order to read it must never put it up for removal. The row is written on every open rather than only when a table is created, so that a backend upgraded from a version without a catalog fills it in at its first read-write open instead of waiting for its trees to be created again.
  • A row is written before its table and taken out before its table is dropped, so that the commit of the DDL carries the row with it. Of the two ways a half-done openTree can end, a row naming a table that is not there is the one the removal is ready for — it skips such a row and logs it — while a table nothing names is adopted with its stale rows by the next open of that tree and is dropped by no clear ever after. deleteTree() is the sharper case: the drop commits the enclosing transaction, so a row taken out after it would be all that a later terminal failure — write() replays a class 40 conflict and rethrows everything else — could still roll back, over a table already gone, and nothing would put it right, since a deleted tree is not opened again.
  • The compressed schema trees are the exception. Named from a literal, they are the same pair for every backend of a database (JDBC backends sharing a database URL share one pair of compressed-schema tables #873), so they are never enrolled: a backend must not offer for removal a tree another one may be the only owner of, and that pair is deliberately left where it lies ([#873] Give each backend its own compressed schema trees #881). The trees the fix of JDBC backends sharing a database URL share one pair of compressed-schema tables #873 names after the backend id are enrolled like any other.
  • What a tool is shown is not what a clear may drop. catalogTables() answers the removal — the catalog's rows and the catalog itself, the table taken from the row rather than recomputed. listTrees() answers dbtest, and adds the shared compressed schema trees whose tables are there, so that list-raw-dbs counts them and dump-raw-db --dbName compressed_schema/… goes on resolving their names, as it did before the catalog.
  • removeStorageFiles() drops the catalog last and skips a table that is not there. Dropping a table is DDL, which mysql and oracle commit as they go, so an attempt which fails halfway is finished by the next one instead of leaving behind tables nothing names any more. A skipped row is logged, and a clear which dropped nothing at all while there was something to drop is logged on top of that — that silence was the whole of JDBC backend: removeStorageFiles() drops only the tables this process has touched, so an offline import-ldif --clearBackend clears nothing #888.
  • What a clear did not remove is reported, never guessed at. Once everything the catalog named is gone, the opendj tables still standing in the catalog and schema of the connection are counted and logged: a table is named after the hash of its tree name, so nothing about one says whether it holds a tree of a backend sharing the database (JDBC backends sharing a database URL share one pair of compressed-schema tables #873), or was left by a version keeping no catalog, or by a tree taken out of the configuration while the backend was disabled. The count is scoped to getCatalog()/getSchema() because Connector/J 8 answers a null catalog for every database of the server.

isExistsTable() moved to the storage itself, unchanged, since the removal needs it outside a transaction now; the catalog lookup of #885 is untouched.

Not fixed by this

Enrolment covers the trees the current configuration opens read-write, and those alone. A table left by a tree that is no longer configured — an attribute index removed while the backend was disabled, or anything at all predating the upgrade — is named by no catalog, cannot be attributed to a backend, and is therefore reported after every clear rather than dropped. Re-adding such an index adopts the surviving table with its pre-clear rows, exactly as it did before this change; only the report is new.

The compressed-schema tables of a backend are cleared once #881 gives each backend its own pair — this should land after #881 for that reason, and because the two touch the same methods.

Tests

Five cases in jdbc/TestCase, inherited by all four engine suites:

case asserts
testABackendIsClearedByAProcessThatNeverOpenedIt a storage which never opened the backend names its tree and drops its tables — and the tables of another backend of the same database stay where they are
testADeletedTreeIsNoLongerNamedByTheCatalog a dropped tree leaves the catalog with its table, and the clear that follows does not stumble over it
testAClearSkipsACatalogRowWhoseTableIsGone a row whose table was dropped behind the catalog's back neither fails the clear nor stops it dropping the rest
testReadingATreeDoesNotPutItUpForRemoval openTree(tree, false) enrols nothing: a tree of another backend, read but not owned, survives this one's clear
testTheSharedCompressedSchemaTreesAreNamedButNeverCleared the shared pair is named by listTrees() and left standing by a clear which drops the backend's own trees

The first fails on master: the offline storage names no tree at all and the clear drops nothing.

PluggableBackendImplTestCase.testImportLDIF already finalizes the backend and imports with setClearBackend(true) on master, but it keeps the same JDBCStorage instance with tree2table intact, so the clear it runs was never the offline one. It is unchanged here, and the five cases above cover that path alone.

suite result
PgSqlTestCase 58/58, no skips
MySqlTestCase 58/58, no skips
MsSqlTestCase 58/58, no skips
OracleTestCase 58/58, no skips

…talog in the database

listTrees() answered from tree2table, a cache a tree enters the first time this
process names a table for it. Nothing seeds it - open() takes a connection and
sets the storage status - so a process which has opened nothing names no trees.

removeStorageFiles() is the one caller running before the root container is
open, and it drops exactly what listTrees() names. In the offline import-ldif
the backend is configured and never opened, so the set was empty, the drop loop
was skipped, and "import-ldif --clearBackend" cleared a JDBC backend of nothing,
without a word in the log. Online the same command does drop the tables:
ImportTask calls importLDIF on the Backend object it already holds, whose
storage has been serving traffic with a fully populated cache. JE and PDB
enumerate the environment itself, so both honour the contract whatever the
process did earlier.

What survived was not only the tables of trees the import does not rebuild. The
importer clears an entry container when its first entry arrives, so a base DN
configured in the backend but absent from the imported LDIF was cleared by
nothing at all: it kept its entries and went on serving them, where the same
command on JE removes the whole backend directory. The option is documented as
"Remove all entries for all base DNs in the backend before importing".

The trees of a backend are recorded in the database now, in a tree of their own:
one row per tree, keyed by the tree name, with the table holding it as its
value. The catalog is per backend and named after the backend id alone, so its
table name follows from the configuration without asking the database anything -
which is what a process that has opened nothing needs - and so that backends
sharing one database URL (OpenIdentityPlatform#873) never name each other's trees. Being an ordinary
tree it needs no dialect of its own; it is created without the index openTree()
gives a tree, which serves cursor batches the catalog never runs, and without a
comment, which would only repeat what its rows say in plain text.

openTree(createOnDemand) enrols, and nothing else does: naming a tree in order
to read it must never put it up for removal. The row is written on every open
rather than only when a table is created, so a backend upgraded from a version
without a catalog fills it in at its first read-write open. deleteTree() takes
the row out together with the table.

The compressed schema trees are the exception. Named from a literal, they are
the same pair for every backend of a database (OpenIdentityPlatform#873), so they are never
enrolled: a backend must not offer for removal a tree another one may be the
only owner of, and that pair is deliberately left where it lies (OpenIdentityPlatform#881). The
trees the fix of OpenIdentityPlatform#873 names after the backend id are enrolled like any other.

removeStorageFiles() drops the catalog last and skips a table that is not there:
dropping a table is DDL, which mysql and oracle commit as they go, so an attempt
which fails halfway is finished by the next one instead of leaving behind tables
nothing names any more. Where no catalog is there yet, the opendj tables the
connection can reach are counted and reported rather than dropped - a table is
named after the hash of its tree name, so nothing about it says which backend of
a shared database it belongs to.

Two cases in jdbc/TestCase, inherited by all four engine suites:

* testABackendIsClearedByAProcessThatNeverOpenedIt - a storage which never
  opened the backend names its tree and drops its tables, while the tables of
  another backend of the same database stay where they are
* testADeletedTreeIsNoLongerNamedByTheCatalog - a dropped tree leaves the
  catalog with its table, and the clear that follows does not stumble over it

PgSqlTestCase and MySqlTestCase pass 55/55 with no skips. MsSqlTestCase and
OracleTestCase were not run locally.
@vharseko
vharseko requested a review from maximthomas August 20, 2026 13:38
@vharseko vharseko added bug jdbc tests Test suites: fixing, enabling, un-disabling labels Aug 20, 2026

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix for #888 is real and well scoped: the catalog makes an offline --clearBackend name the trees it has to drop, the catalog-last ordering is right, and the two new cases cover the path that was broken. Two things to settle before merge — one consistency hole in the catalog itself, one sentence in the description that is not true as written.

Stale catalog row outlives its table (major)

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

deleteTree() commits the drop table immediately, then leaves the matching catalog row to the caller's transaction:

try (final PreparedStatement statement=con.prepareStatement("drop table "+getTableName(treeName))) {
    execute(statement);
    con.commit();          // the table is gone here
}
...
unenrolFromCatalog(treeName);   // -> delete(catalog, ...): rolls back with the enclosing write()

Delete an index (dsconfig delete-backend-index, a base DN removal, EntryContainer.clear()) and let anything later in the same transaction fail terminally — write() replays only class-40 conflicts and rethrows the rest unreplayed — and the drop stands while the row rolls back.

The row is then permanent: a deleted tree is never opened again, so nothing re-enrols or re-deletes it. listTrees() returns a TreeName whose table does not exist, and BackendStat.listRawDBs opens a cursor per listed tree, so dbtest list-raw-dbs fails with a StorageRuntimeException for good. removeStorageFiles() survives only because it has the isExistsTable skip at :862; listTrees() has no such guard.

Two lines: commit the unenrol with the drop, the way :1214 and :1307 already commit their DDL.

An uncatalogued table is never dropped, and after the first open never reported either (major)

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

final Set<TreeName> trees=listTrees(con);
if (trees.isEmpty()) {
    reportUncataloguedTables(con);
} else { ...drop loop... }

listTrees(Connection) returns an empty set only while the catalog table is absent — the moment it exists, :1679 unconditionally does trees.add(catalog). So the report branch is dead from the first read-write open on, and enrolment only ever covers the trees the current configuration opens (enrolInCatalog is called from openTree(createOnDemand) alone).

An attribute index removed from the configuration while the backend was disabled, or before the upgrade, leaves an opendj_<hash> table the catalog never learns of. Every later import-ldif --clearBackend drops the catalogued trees, takes the else branch, and leaves that one untouched and unmentioned. If the index is re-added, openTree only creates a table when !isExistsTable (:1210), so it adopts the survivor with its pre-clear rows — the index then returns entry IDs the reimported id2entry does not have.

This makes the description's "the first read-write open after the upgrade makes the next clear complete" false for any tree the current configuration does not open. Correct the sentence at least; better, fire the report whenever the connection can see opendj tables the catalog does not name, not only when the catalog table is missing.

The compressed-schema trees disappear from listTrees() (minor)

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

Not dropping their tables is deliberate (#881), but the enrolment skip also removes them from the tool-facing listing. On master they were in tree2table after any open, read-only included, because PersistentCompressedSchema.load cursors them. Now dbtest dump-raw-db --dbName compressed_schema/compressed_attributes fails name resolution in BackendStat.getStorageTreeName and list-raw-dbs silently undercounts. They are computable from the constant — add them to the listTrees() result without enrolling them.

A clear that drops nothing still says nothing (minor)

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

if (!isExistsTable(con, tableName)) { // a row of the catalog outliving its table
    continue;
}

No log line, and removeStorageFiles() returns normally. If every derived table name misses, --clearBackend again clears nothing without a word in the log — the exact failure mode of #888 — and BackendImpl.importLDIF goes on to import into surviving data. Log the skip, or count the skips and warn once when nothing at all was dropped.

Nits

  • openTree has the same asymmetry as deleteTree (JDBCStorage.java:1214 vs :1286): the create table commits, the upsert(catalog, ...) does not. A terminal failure mid-open leaves committed tables and an empty catalog table — and since listTrees adds the catalog itself, the clear takes the else branch and drops only the catalog. Self-heals at the next successful open, since enrolment runs on every open, which is why this is a nit and deleteTree is not.
  • The catalog's value column is written and never read (JDBCStorage.java:1286 vs :1680): listTrees does select k only and the drop loop recomputes getTableName(treeName). The one fact that would make the catalog robust to a change of the naming function is recorded and ignored. Either read v, or stop writing it and drop "the table holding that tree as its value" from the javadoc.
  • The uncatalogued count is unscoped (JDBCStorage.java:928): getTables(null, null, "opendj%", ...) — null catalog and null schema counts other backends' tables on a shared URL (#873), their catalogs, and on Connector/J 8 (nullCatalogMeansCurrent=false) every database on the server. Pass con.getCatalog()/con.getSchema(), and say in the message that the count spans the connection.
  • The description credits itself with a test change that is not in the diff: "PluggableBackendImplTestCase now really exercises the drop" — that file is untouched, and finalizeBackend() + setClearBackend(true) already exist on master at :1005-1016. That case also keeps the same JDBCStorage instance with tree2table intact, so it never covered the offline path. The two new jdbc cases cover it alone.
  • The three guards the fix rests on are covered by no test: the stale-row skip at :862 is unreachable from testADeletedTreeIsNoLongerNamedByTheCatalog because deleteTree unenrols first — the test passes with the skip deleted; nothing asserts that openTree(tree, false) does not enrol; nothing asserts the compressed-schema skip. The last two are what stop one backend offering another's trees for removal.
  • The new tests leak tables on failure (opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:1389, :1425): neighbour.removeStorageFiles() is the last statement of the body, after five assertions, outside any try/finally. dropStaleTrees runs only in @BeforeClass and cleanUp() drops nothing, so one failure leaves opendj_ tables alive for the rest of the class. Move both clears into finally.
  • Two stale artefacts (JDBCStorage.java:1357, :910): the comment "forget the mapping so listTrees() consumers (updateTableStatistics) skip the dropped table" is no longer true — listTrees() does not read tree2table and updateTableStatistics is only called with writtenTrees; and if (trees.contains(catalog)) is always true at its only call site.

Not covered here: nothing was run against an engine. MsSql and Oracle are unexercised on both sides, and the suites skip rather than fail when a container does not start — worth confirming the new create table <catalog> and select k from <catalog> on those two before this lands.

…it that drops its table

The catalog row of a deleted tree was left to the enclosing transaction while
the drop committed at once, so a terminal failure later in that transaction
rolled the row back over a table already gone, and nothing ever put it right -
a deleted tree is not opened again. It is taken out before the drop now, so
the commit of the DDL carries it. openTree writes its row before the create
table for the same reason the other way round: a table nothing names is
adopted with its stale rows by the next open and is dropped by no clear.

The uncatalogued table report was unreachable past the first read-write open,
listTrees() having added the catalog itself unconditionally. It runs after
every clear now, counting the opendj tables of the connection's own catalog
and schema that no catalog of this backend names; the shared compressed schema
pair is left out of that count, being kept on purpose.

listTrees() no longer answers the removal. catalogTables() does, from the rows
alone, taking each table name from its row rather than recomputing it, while
listTrees() adds the shared compressed schema trees whose tables are there, so
that dbtest names them again as it did before the catalog.

A catalog row whose table is gone is logged where it was skipped in silence,
and so is a clear which dropped nothing while there was something to drop -
that silence was the whole of OpenIdentityPlatform#888.

Three cases cover the guards nothing covered: the skip of a row whose table
was dropped behind the catalog's back, openTree(tree, false) enrolling
nothing, and the shared pair being named but never cleared. Both existing
cases clear their backends in a finally.
@vharseko

Copy link
Copy Markdown
Member Author

Thank you — every point is addressed in e49fce2, and all four engines are now run. Two of the fixes are not the ones suggested; the reasoning is below, together with one consequence I think was overstated.

Stale catalog row outliving its table (major) — fixed, by reordering

Confirmed exactly as described: con.commit() after the drop table commits the whole enclosing transaction, so the delete of the row was the only part of deleteTree still owed to it, and write() replays a class 40 conflict and rethrows everything else unreplayed.

Rather than committing the unenrol after the drop, unenrolFromCatalog now runs before it, so the commit of the DDL carries the row with it:

unenrolFromCatalog(treeName);
if (isExistsTable(treeName)) {
    try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) {
        execute(statement);
        con.commit();
    }
    ...
}

One statement moved instead of one added, and it behaves better in the case the suggestion does not cover: if the drop itself fails, the pending row delete goes back with the transaction, leaving the tree named and its table standing — both still there — where a commit placed after the drop would have had nothing to commit anyway.

One correction to the consequence. dbtest list-raw-dbs does not fail: listRawDBs calls appendStorageTreeStats, which catches Exception and fills the row with appendStatsNoData (BackendStat.java:806-809), so a stale row shows as a tree with dashes rather than an error. dump-raw-db --dbName <that tree> does fail and return 1. The row is permanent as described, and the fix stands on that alone.

An uncatalogued table is never dropped, nor reported past the first open (major) — report fixed, the drop deliberately not

Both halves confirmed: catalogTables() (was listTrees(con)) adds the catalog unconditionally, so the old trees.isEmpty() branch was dead from the first read-write open, and openTree adopts an existing table (if (!isExistsTable)), stale rows included.

reportUncataloguedTables is now reportClearOutcome, and it runs after every clear, once everything the catalog named is gone: whatever opendj table is still standing is by definition named by no catalog of this backend. It is scoped to con.getCatalog()/con.getSchema() — your Connector/J 8 point — and the shared compressed schema pair is excluded from the count, since reporting it after every clear would be asking an administrator to remove the one thing this code goes out of its way to spare.

The table itself is still not dropped, and I do not think it can be: nothing about opendj_<hash> says whose it is, and on a shared database (#873) guessing would drop another backend's data. That is now stated plainly in Not fixed by this rather than implied away, and the false sentence you flagged is gone.

The compressed schema trees disappear from listTrees() (minor) — fixed, but not by adding them to listTrees()

The regression is real. The suggested fix is not safe as written, though: removeStorageFiles() iterates exactly what listTrees() returns, so adding the pair there would make --clearBackend drop them — precisely what the enrolment skip exists to prevent.

Split instead:

  • catalogTables(Connection) — the rows of the catalog and the catalog itself, nothing else. This is what the removal reads.
  • listTrees() — the same, plus the shared pair, each added only when isExistsTable says its table is there (so that once [#873] Give each backend its own compressed schema trees #881 gives each backend its own pair, an installation holding neither table does not start naming two trees that do not exist).

list-raw-dbs counts them again and dump-raw-db --dbName compressed_schema/compressed_attributes resolves again, with the clear leaving them where they lie.

A clear that drops nothing still says nothing (minor) — fixed

The skip logs the tree and the table it could not find, and a clear which dropped nothing at all while there was something to drop warns on top of that. The second condition matters: a first-ever --clearBackend on an empty backend legitimately drops nothing, and warning there would be noise.

Nits

  • openTree asymmetry — fixed the same way as deleteTree, mirrored: enrolInCatalog now runs before the create table, so the DDL commit carries the row. Of the two ways a half-done open can end, a row naming a missing table is the one the removal is ready for; a table nothing names is the one adopted with stale rows and dropped by no clear ever after.
  • The value column written and never read — now read. catalogTables does select k,v and the drop loop uses the recorded table name, falling back to getTableName(treeName) for an empty v.
  • The uncatalogued count unscoped — scoped, as above. Note that isExistsTable still passes null, null: that is master's code moved verbatim, it runs per tree per open, and narrowing it changes table detection on four engines for a hazard that predates this PR. Worth its own change; I did not want it hidden in this one.
  • The description crediting a test change not in the diff — removed. The section now says what is true: PluggableBackendImplTestCase.testImportLDIF already does this on master, keeps the same JDBCStorage with tree2table intact, and therefore never covered the offline path.
  • The three uncovered guards — three cases added: testAClearSkipsACatalogRowWhoseTableIsGone (the table is dropped behind the catalog's back, which no code path of the backend does), testReadingATreeDoesNotPutItUpForRemoval (a tree of another backend, read but not owned, survives this one's clear), testTheSharedCompressedSchemaTreesAreNamedButNeverCleared.
  • Tests leaking tables on failure — both existing cases clear in a finally now, through a clearQuietly helper that never lets the failure of a clear replace the failure being reported.
  • The two stale artefacts — both gone. catalogDroppedLast went with them: the ordering now comes from the LinkedHashMap catalogTables builds, so there is no always-true contains left to be always true. The deleteTree comment is rewritten; since tree2table is a pure SHA-224 of the tree name, invalidating it was never more than dropping a memo, and the comment now says so instead of claiming listTrees() consumers depend on it.

Engines

All four run locally, none skipped:

suite result
PgSqlTestCase 58/58, no skips
MySqlTestCase 58/58, no skips
MsSqlTestCase 58/58, no skips
OracleTestCase 58/58, no skips

53 on master, 58 here — the two cases of the first push plus the three above, on every engine.

@vharseko
vharseko requested a review from maximthomas August 21, 2026 08:13

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-traced all eleven of the previous round against e49fce2 rather than taking the reply's word: they are genuinely addressed, and your correction about list-raw-dbs is right — appendStorageTreeStats catches and prints dashes, only dump-raw-db exits 1. Three things the new commit introduces or leaves.

A successful clear tells the operator to hand-remove another live backend's tables (major)

reportClearOutcome now runs after every clear, not only when the catalog table was missing, and counts every opendj% table in con.getCatalog()/con.getSchema() minus only the shared compressed-schema pair.

Table names are a bare SHA-224 of the tree name with no backend id (JDBCStorage.java:186-192), and the catalog table /opendj_catalog/<backendId> is itself hashed (:238-240). So on the shared-database layout of #873 the count covers backend B's ~25 live tables plus B's own catalog, and the warning ends these have to be removed by hand.

The PR's own test reproduces it. createBackendCfg gives every backend the same URL (opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:128-134), and testABackendIsClearedByAProcessThatNeverOpenedIt asserts at :1408 that the neighbour's table is still standing after the clear — so leftBehind >= 2 and the warning fires inside a green test, with nothing asserting on the log.

The provenance you need already exists: #866 stamps every table with its tree name in the table comment, which getTables() returns as REMARKS. Skip a table whose stamp names a tree of a base DN this backend does not serve, and the count becomes true. Failing that, drop the removed by hand clause.

The reordering's guarantee holds on two engines of four (major)

if the drop itself fails, the pending row delete goes back with the transaction

That is PostgreSQL and SQL Server. MySQL and Oracle implicitly commit the pending transaction before executing DDL — the file says so itself at JDBCStorage.java:1796-1799, "DDL, which mysql and oracle commit as they go". There the DELETE lands first and the drop runs as a separate transaction:

unenrolFromCatalog(treeName);                 // DELETE, pending
if (isExistsTable(treeName)) {
    try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) {
        execute(statement);                   // mysql/oracle: commits the DELETE, THEN drops
        con.commit();
    }
}

On Oracle drop table needs an exclusive lock and ddl_lock_timeout defaults to 0, so a concurrent transaction on the tree gives ORA-00054 at once. That is SQLState 61000, and isConflict (:1121-1136) replays only SQLState 40* plus ORA-00060 — so write() rethrows it unreplayed, the row is committed-deleted and the table stands. By your own comment at :1272-1275 that is the worse half: a table nothing names, adopted with its stale rows by the next open and dropped by no clear ever after. Reachable from AttributeIndex.deleteIndex (AttributeIndex.java:1019-1024) and per-tree inside EntryContainer.clear()'s loop (EntryContainer.java:2560-2568). MySQL's equivalent is 1205 / SQLState 40001, so it replays and self-heals; Oracle is the live case.

To be clear, the bug this PR was reopened for — a terminal failure later in the same write() rolling the row back over a dropped table — is fixed on all four engines. What is wrong is the invariant asserted around it. Either give the unenrol its own con.commit() after the drop's, which is correct everywhere and converges on retry, or say in the comment at :1412-1420 and at TestCase.java:1458 that the guarantee is PostgreSQL/SQL Server only.

The first offline clear after an upgrade still drops nothing (major)

BackendImpl.java:670-673 clears the storage before it opens the root container at :681. catalogTables() returns Collections.emptyMap() when the catalog table is absent (JDBCStorage.java:1774), and nothing enrols earlier — enrolInCatalog is reached only from openTree's createOnDemand branch (:1275), which the read-write open after the clear takes.

So on an installation whose tables predate this PR — the state #888 is filed about — an offline import-ldif --clearBackend iterates zero rows and drops zero tables, exactly as on master. The residue is every tree the import does not rewrite: another base DN in the same backend, an index not rebuilt. It self-heals from the second clear on, and it does now warn.

This is a documentation change, not a code one. "Not fixed by this" currently names only trees the config no longer serves; it should also say that a backend upgraded in place must be started once before its first --clearBackend import.

The skip that keeps a clear going is unreliable on MySQL (minor)

The drop loop's guard passes a null catalog while the report ten lines below is scoped:

if (!isExistsTable(con, tableName)) { missing++; continue; }   // :874 -> getTables(null, null, ...)
...
metaData.getTables(catalog, schema, storedIdentifier(metaData, "opendj%"), ...);   // :947

opendj-server-legacy/pom.xml:294-303 pins mysql-connector-j 9.2.0, whose nullDatabaseMeansCurrent defaults to false and databaseTerm to CATALOG — a null catalog searches every database on the server and schemaPattern is ignored. Two OpenDJ databases on one MySQL server with the same backend id and suffix produce identical opendj_<hash> names, so a stale row whose local table is gone matches the twin next door, the skip is not taken, the unqualified drop table throws 1051, and the catch at :887 aborts the whole --clearBackend — deterministically, on every retry. That is the designed skip-and-warn turning into a hard failure, and testAClearSkipsACatalogRowWhoseTableIsGone cannot see it in a single-database container.

Agreed that narrowing isExistsTable in general belongs in its own change; this one call site now decides between "skip" and "drop", which the others do not.

Nits

  • The fix is pinned by no test: testADeletedTreeIsNoLongerNamedByTheCatalog (TestCase.java:1424-1456) commits its write() cleanly and injects no failure after deleteTree, so it passes with the reordering reverted; the mirrored openTree reorder is asserted by nothing at all. A case that throws after deleteTree inside the same WriteOperation would pin it.
  • Two branches still commit nothing: deleteTree's skip branch (:1421, table already gone) leaves the DELETE to the enclosing write() — pre-existing, but the fix passes right by it, and AttributeIndex.deleteIndex is not masked by a later tree in a loop. In openTree, enrolInCatalog is committed on every open only on postgres, where create index if not exists + commit sits outside the !isExistsTable block (:1288-1294); mysql/oracle commit only when the index is actually created (:1295-1318) and mssql has no index branch (:1319), so a reopen leaves ~25 upserts pending on three engines.
  • The "dropped nothing" warning prints the wrong number: if (dropped==0 && (leftBehind>0 || missing>0)) logs only missing, so the #888 state — empty catalog, tables standing — reads the clear dropped no table at all, 0 of the trees its catalog names having lost their table already. The condition that fired was leftBehind, which is never printed. Also, the noise suppression you describe holds only on a database that holds no other opendj% table.
  • clearQuietly swallows the code under test: TestCase.java:157-164 catches every Exception from removeStorageFiles(), and in testAClearSkipsACatalogRowWhoseTableIsGone and testTheSharedCompressedSchemaTrees... it is the only clear in the case. The neighbour's clear also lost the assertion it had before. Worth catching in the cleanup-only position and asserting where the clear is the subject.
  • Half the shared pair is untested: testTheSharedCompressedSchemaTreesAreNamedButNeverCleared uses SHARED_COMPRESSED_SCHEMA_TREES.get(0) only. Element 1 is a hand-copy of a PersistentCompressedSchema private, and a wrong literal there would silently un-name and un-spare that tree.
  • The v fallback is unexercised: every catalog row a test writes has v == getTableName(k), so neither the recorded-name path nor the empty-v fallback at :1790-1793 is covered — a swapped k/v would pass the suite. Same for the LinkedHashMap catalog-last ordering at :1800-1801, which matters only on a half-failed removal no test induces.
  • unenrolFromCatalog lacks the guard enrolInCatalog has: :1342 returns early for SHARED_COMPRESSED_SCHEMA_BASE_DN, :1378 does not. No live caller today, but the asymmetry is what would let a deleteTree drop the shared pair.
  • dbtest on an upgraded, never-started installation: listTrees() now names only the shared pair until the first read-write open, so dump-raw-db --dbName /dc=example,dc=com/id2entry exits 1 where the base version resolved it via the tree2table memo. Narrow, and it disappears after one start.
  • Test setup still leaks: the setup half of testABackendIsClearedByAProcessThatNeverOpenedIt (:1390-1391) has a finally that only closes, so a throw there exits before the second try/finally. And the new test helper isExistsTable (:136-146) repeats getTables(null, null, null, ...), walking every accessible schema once per assertion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug jdbc tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JDBC backend: removeStorageFiles() drops only the tables this process has touched, so an offline import-ldif --clearBackend clears nothing

2 participants