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

[vharseko]

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 it is stamped with its tree name like any other table (#866): a clear reports what it did not drop, and on a database several backends address the catalog of a neighbouring backend is otherwise the one table such a report can attribute to nobody. The stamp costs one attempt per open of the storage and not one per tree, being issued behind the very flag that keeps the catalog from being opened twice.

• 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 whenever the catalog does not already record the tree at the table this version records it at, 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 — and so that a row of a version naming its tables otherwise is rewritten rather than trusted, a removal dropping the table the row records and no other. What the catalog holds is read once when the storage first opens it, so an open of a backend whose catalog is complete — every open after the first — writes nothing to it at all.

• A row is written before its table is created and taken out after its table is dropped, which is deliberately not a mirror image. Of the two ways a half-done change 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. openTree() therefore enrols first, and the catalog table is read and written on a connection of its own — opened straight through the driver, like the stamp connection of Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import #866, and closed with the transaction; no other connection touches that table. It has to be committed: on postgres and sql server the caller's create table would carry it and on mysql and oracle that DDL commits it as it begins, but the open which fills the catalog of a backend upgraded from a version keeping none creates no table at all, and a transaction failing after the enrolment would take the whole of it back. And that commit must not be the caller's: RootContainer.open() is one write() over ~25 openTree() calls, replayReason() reads partlyCommitted before it asks anything else, and a deadlock at tree N would fail the backend open where master replayed it. A connection of its own is what makes the two compatible — the row commits, and the catalog raises partlyCommitted nowhere, so a caller's attempt is exactly as replayable as it was on master. deleteTree() unenrols on that same connection, and drops first for the reason read the other way round: an unenrolment left pending before the drop would be committed by the drop itself on mysql and oracle and would stand even where the drop then failed — ORA-00054 on a tree another session holds, say, which write() rethrows unreplayed. The catalog table is created there too, and the table is opened once per storage under a lock, so two transactions opening trees at the same time cannot both find it absent and both create it.

• 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. The lookup deciding between the skip and the drop is narrowed to the database and the schema of the connection: asked with a null catalog, Connector/J answers for every database of the server, and a table of the same name next door would turn a skip that keeps the clear going into an unqualified drop table of a table that is not here, failing the whole clear on this attempt and on every one after it.

• What a clear did not remove is reported, and reported as what it is. A table is named after the hash of its tree name, so its name says nothing about whose it is — but the comment Stamp JDBC backend tables with their tree name and refresh optimizer statistics after import #866 stamps it with says exactly that, in plain text. Once everything the catalog named is gone, the opendj tables still standing in the catalog and the schema of the connection are read for that stamp: one naming a tree of a base DN this backend does not serve belongs to a backend sharing the database (JDBC backends sharing a database URL share one pair of compressed-schema tables #873) and is passed over in silence; one naming a tree of this backend — an index taken out of the configuration while the backend was disabled, say — is reported as this backend's own, and so as removable by hand; a table carrying no stamp at all can be attributed to nobody and is reported as exactly that; and a stamp the database would not give up is reported apart from all three, since it states nothing either way and calling it an absent stamp would turn a connection that died halfway into a confident line about tables this backend may well own. An engine with no comment readback of its own answers for every one of its tables that way, nothing having been asked of them. The scan is scoped to getCatalog()/getSchema() because Connector/J 8 answers a null catalog for every database of the server, and the schema of each row is compared besides — a schema reaches getTables() as a pattern, where _ is a single-character wildcard.

  The line for a clear which dropped nothing is decided by what the clear itself did, so a database which would not say what is standing silences the lists and not it — that silence is the whole of JDBC backend: removeStorageFiles() drops only the tables this process has touched, so an offline import-ldif --clearBackend clears nothing #888 — and it is printed before them: on a backend upgraded in place every table is unstamped and lands in the list of what can be attributed to nobody, and the reason has to reach the operator ahead of the names.

isExistsTable() moved to the storage itself, since the removal needs it outside a transaction now, and takes the scope of the connection it asks on: the database it works in, and the schemas an unqualified name of it resolves in — current_schemas(true) on postgres, the default schema and then dbo on sql server, the current schema on oracle, and the database itself on mysql, whose schema is its catalog. The database is the half that has to narrow: Connector/J reads a null catalog as any database of the server, and two backends of the stock backend id in two databases of one server name their tables alike, so a foreign answer turns a skip into an unqualified drop table of a table that is not here (1051 on mysql, no class 40 state, so write() rethrows it and EntryContainer.clear() fails now and on every attempt after it) and leaves an openTree naming a tree whose table is not in this database. The schema is the half that must not narrow to one name: the statements it guards are unqualified and resolve across the whole path, so a lookup asking about current_schema() alone would be the stricter question of the two and would report every table of an installation created in public absent the moment a schema is put in front of it — the clear dropping nothing, which is #888 over again, and the next open creating a second, empty set of tables that shadows the populated ones from that commit on. Every lookup goes through the scope, the create of openTree, the drop of deleteTree and the getIndexInfo of the cursor index included; a listing is asked with no schema pattern at all and read back through the path, which is also what closes the _-as-wildcard hole. A transaction asks its connection once — and asks again where the connection would not answer, rather than latching a lookup as wide as the whole server for its whole life; both failures are logged. 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 and is left where it is 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.

What the report can say of such a table depends on what its stamp names, and the stamp names the tree and never the backend the tree belonged to. An index of a base DN this backend still serves is reported as its own, and so as removable by hand. A tree of a base DN taken out of the configuration is indistinguishable from a tree of a backend sharing the database (#873), which a clear must not offer for removal, so it is passed over in silence: what is left of a removed base DN is found by its stamp and removed by hand. Attributing those apart would take a backend id in the stamp, which is a change to #866's format and not to this.

A backend upgraded in place has to be started once before its first offline --clearBackend. BackendImpl.importLDIF clears the storage before it opens the root container, and nothing enrols a tree earlier, so on an installation whose tables predate this change that first offline clear finds no catalog and drops nothing — as on master, except that it now says so in the log rather than going by in silence. Every clear after the first read-write open is complete. That first read-write open also needs the privilege to create one table, where a version keeping no catalog issued no DDL at all on an installation whose tables were already there: a DML-only account fails the open, and the failure names the table and says it holds the catalog rather than reaching the operator as a bare SQL error inside ERR_OPEN_ENV_FAIL.

A table this connection cannot reach unqualified is not found. The lookup answers for the database of the connection and for the schemas an unqualified name of it resolves in, which is the same question every statement of this backend answers: it creates its tables unqualified, reads them unqualified, and a table it cannot reach that way is not one it can be said to hold. Two things stay outside that: a table of another database of the same server (deliberately — that is the narrowing this scope exists for) and, on oracle, a table reached through a synonym, which no path enumerates. An open recreates such a table empty in the schema the connection does work in, as it did before this change.

The compressed-schema tables of a backend are cleared now that #881 has given each backend its own pair: that pair is named after the backend id, is under no literal prefix, and is enrolled like any other tree. The legacy pair /compressed_schema/… is the one PersistentCompressedSchema migrates from and never writes to again, and a clear leaves it exactly where it lies — the definitions of a backend that has not been started since the upgrade are still in it.

Tests

Eleven 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 — while the tables of another backend of the same database stay where they are, and are named in nothing this clear reports
testADeletedTreeIsNoLongerNamedByTheCatalog	a dropped tree leaves the catalog with its table, and the clear that follows does not stumble over it
testADeletedTreeStaysOutOfTheCatalogWhenItsTransactionFails	a write() throwing after deleteTree leaves the table dropped and the row gone: the delete is owed to no transaction that could still roll it back
testADeletedTreeStaysOutOfTheCatalogWhenItsTableIsAlreadyGone	the same where there is no table to drop, and so no commit of a drop to carry the row out with it — the branch the ordering of deleteTree actually turns on, and the one case that fails on the ordering of the round before it
testAReopenedTreeStaysInTheCatalogWhenItsTransactionFails	a write() throwing after an openTree of a tree whose table is already standing leaves the row where it is — the enrolment that fills the catalog of an upgraded backend creates no table, so nothing of the caller's transaction would commit it. Green on all four engines for one reason now: the row is committed on the catalog's own connection
testARowRecordingAnotherTableIsEnrolledAgain	a row recording a table this version would not create is written again, where one recording the right table is left alone: what a removal drops is the table the row records, so trusting such a row would leave the real table standing and named by nothing
testAClearSkipsACatalogRowWhoseTableIsGone	a row whose table was dropped behind the catalog's back neither fails the clear nor stops it dropping the rest
testAClearDropsTheTableTheCatalogRecords	the row records the table holding its tree, and a row recording none falls back to the name derived from the tree
testAClearDropsTheCatalogAfterEveryTreeItNames	the order removeStorageFiles() actually drops in, recorded from the drops themselves: the catalog goes last, after every tree it names. Asserting on the map the loop walks would hold of any loop at all, that map being built with the catalog put last by hand
testAClearReportsTheTablesItCanAttributeToThisBackend	an uncatalogued table of a base DN this backend serves is reported as its own, and nothing of a neighbouring backend's — its trees or its catalog — is reported at all
testReadingATreeDoesNotPutItUpForRemoval	openTree(tree, false) enrols nothing: a tree of another backend, read but not owned, survives this one's clear
testTheSharedCompressedSchemaTreesAreNamedButNeverCleared	both trees of the shared pair are named by listTrees() and left standing by a clear which drops the backend's own trees
testAClearFindsATableOfAnotherSchemaOfTheSearchPath (postgres only)	a backend whose connections resolve in a schema put ahead of the one its tables are in still names those trees and still drops them: the lookup answers for the search path, not for current_schema(). Fails on the round before this one, where the clear dropped nothing

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

JDBCStorageRetryTest — master's own mock suite, which #883 gave the cases pinning what an openTree may commit — carries one more, testFillingTheCatalogOfAnUpgradedBackendLeavesTheAttemptReplayable: a backend whose tables are all there and whose catalog is not is opened, and the write that creates the catalog and fills it is asserted to be replayed and to have issued no statement on the caller's connection. Its fixtures now model a database of named tables (the catalog's among them) and hand out the connection the catalog is written on; every assertion of those cases is unchanged.

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 cases above cover that path alone.

suite	where	result
JDBCStorageRetryTest, CachedConnectionTestCase, StampConnectionTestCase	locally, -P precommit verify	136/136, no skips
aggregate javadoc (verify javadoc:aggregate, the gate of #899)	locally, whole reactor	green
MsSqlTestCase, MySqlTestCase, OracleTestCase	CI, run 33630195947 on 58c6876	72/72 each, Skipped: 0, on every one of the five ubuntu jobs
PgSqlTestCase	the same run	72 of its 73; the 73rd is the search-path case, which never reached its first statement — see below

The four engine suites are stated from CI and not from a local run: the head they last passed on locally
is three rounds old, and the run that matters is the one on this head. That run also turned up the one
thing a local run of the same four suites would have turned up just as well and no earlier — the section
below is what it cost and what it did not.

The harness rule the first CI round of this head turned up

Every ubuntu job of run 33630195947 failed, all five on one case and the same one, and on nothing a database did:

RuntimeException: The @Test annotation for class PgSqlTestCase must include sequential=true
  to ensure that tests for a single class are run together
    at TestListener.enforceTestClassTypeAndAnnotations(TestListener.java:641)
    at TestListener.onTestStart(TestListener.java:356)

TestListener resolves that annotation from the class declaring the case rather than from the one
running it, and it checks each declaring class once. Until this branch every case of the four engine
suites was declared in the abstract TestCase, whose nearest class-level annotation is the
@Test(groups = ..., sequential = true) of PluggableBackendImplTestCase; PgSqlTestCase declared no
case of its own, so its bare @Test was never the one consulted.
testAClearFindsATableOfAnotherSchemaOfTheSearchPath is the first case declared in PgSqlTestCase
itself, and with it that bare annotation becomes the one that answers: sequential defaults to false
and the case dies in onTestStart, before its first statement. OracleTestCase has carried
sequential = true since it grew a case of its own (test_issue_496_2); this is the same one word, with
the reason written down beside it this time.

So the search-path case has not run yet, on this head or on any other, and the table above says exactly
what that run does support: 72 of 72 on the other three engines and 72 of PgSql's 73, Skipped: 0, on
all five jobs. Nothing else moves with the word: the annotation names no groups on this branch either,
and the precommit profile selects by the failsafe includes (**/*TestCase.java) and never by group.
MySqlTestCase, MsSqlTestCase and EncryptedTestCase carry the same bare @Test, are green for as
long as they declare no case of their own, and are left alone rather than changed on speculation.

Merged with master (#880, #883, then #881 and #894)

master has since taken the read-only transactions of #874 (#880) and the connection validation and
replay accounting of #879 (#883). Four points of contact, all of them in JDBCStorage:

• The enrolment writes, so it stands behind the read-only check. openTree(createOnDemand) calls
  checkReadOnly() before enrolInCatalog(), and deleteTree() calls it before the drop and the
  unenrolment both. A read-only storage may open an existing tree and read it; it may not put a row
  in the catalog.
• removeStorageFiles() reads the catalog over getValidatedConnection(). JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 names the removal
  as one of the paths that issue their statements far from the borrow and compensate a dropped
  connection in no other way, so a connection dropped inside the alive window would surface out of the
  clear rather than be retried.
• Every commit the catalog issues of its own raises partlyCommitted. JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 takes an attempt out
  of the conflict replay at the moment part of its work is committed, and the catalog commits three
  times on paths of its own: the create table of the catalog table now goes through
  commitStatement(sql, true) like every other DDL of a write transaction, and the enrolment and the
  unenrolment raise the flag in front of their con.commit(). Left unraised, write() would replay
  an attempt whose catalog row is already committed.
• isExistsTable(TreeName) keeps the catalog- and schema-scoped form of this branch, the copy
  master grew in the write transaction being the unscoped one this PR replaces.

The merge is Merge remote-tracking branch 'origin/master' into issues/888-jdbc-tree-catalog; nothing
of the review rounds above is changed by it.

Then #881 landed, which this branch was to follow, and #894 with it. Both conflicts are in
JDBCStorage and neither is only textual:

• isExistsTable(TreeName) keeps the scope of this branch, in the place [#873] Give each backend its own compressed schema trees #881 moved it to. [#873] Give each backend its own compressed schema trees #881
  moved the lookup to the readable transaction so that the compressed schema migration can probe the
  legacy tree through treeExists() without creating or enrolling it; this branch had narrowed the
  same method to the database and the schema path of its connection. It now lives in
  ReadableTransactionImpl, narrowed, with tableScope/takeTableScope() beside it — the writeable
  transaction inherits both — and it asks about the non-enrolling readTableName of [#873] Give each backend its own compressed schema trees #881.
• The two names of [#873] Give each backend its own compressed schema trees #881 are followed wherever this branch names a table. The owner takes the
  enrolling getTableName (the create of openTree, the drop of deleteTree, the name a catalog row
  records); the asker takes readTableName (the legacy pair in listTrees() and leftoverTables(),
  the fallback of a row recording no table). The comment on tree2table says what that memo is now
  that listTrees() answers from the catalog rather than from it.
• isOwnTree() knows the pair [#873] Give each backend its own compressed schema trees #881 names after the backend id, so a clear reports two tables whose
  stamp carries this backend's id as its own rather than as tables nothing can be said about.

Two cases of #881 needed the catalog taken into account, and one of them was failing:

• testProbingATreeDoesNotPutItUpForRemoval gave both storages the same backend id, which was two
  backends only for as long as ownership was a map in the process. A catalog is named after the backend
  id and outlives the open that filled it, so the second storage was being shown — correctly — the tree
  its own earlier open had enrolled, and the case failed on its assertion and then dropped the table it
  goes on to require. The second storage is a backend of its own now, which is what the case says it is.
• testDeletingThroughACursorPutsTheTreeUpForRemoval passes unchanged and says why: what puts a tree up
  for removal is the row its backend's catalog holds, written by the openTree the case runs first and
  outliving the storage that made it.

testTheSharedCompressedSchemaTreesAreNamedButNeverCleared creates the legacy pair and asserts a clear
leaves it standing. Since #881 no backend of the class makes that pair, so those tables are the case's
own and it drops them when it is done: left behind they would fail
testCompressedSchemaTableIsQualifiedByBackendId, which asserts the database holds no legacy pair,
whenever TestNG ran it second.

Merged with master (#882)

#877 (#882) has landed and this branch is merged with it. One file conflicts,
JDBCStorage.java, and every conflict is the same shape: a statement #882 gave a class
to is a statement this branch rewrote. The class and the rewrite decide different things

• how long a statement may take, and which table it names or how it is issued - so each
  takes both.

• removeStorageFiles() keeps the catalog-driven loop of this branch. [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 bounded
  the drop table of the loop it replaces as BULK; that class is given once now,
  inside dropTable(), so the drops of a clear cannot drift apart from it at a later
  call site.
• The readable transaction keeps its delegation to the scoped lookup. [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 moved
  isExistsTable up to the readable transaction and wrapped its body in
  bounded(con, OPERATION, …); this branch had already moved the body out to
  isExistsTable(Connection, TableScope, String). The bound moves with the body: it
  reads a data dictionary rather than the data, so a wait there is another session's
  metadata lock whoever asks - hard-coded rather than taken from the transaction, which
  is the rule JDBC backend: no statement is given a query timeout, and the read bound of an established connection is gone too #877 states for the catalog lookups of openTree().
• isExistsIndex() takes both - the scope narrowing of this branch and the operation
  bound of [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882.

Two the merge did not mark, and they are the ones worth reading:

• readCatalogRows() did not compile. It read from a live ResultSet through the
  one-argument executeResultSet(), which [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 removed - that removal is what puts the
  row transfer inside the bound. The rows are read inside the handler now, exactly as
  [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 converted every other such site, and take the same operation class those took.
  No textual conflict in either branch; a compile error in the merge.
• createCatalogTable() would have been bounded as a client operation. Its
  create table went through execute(statement), which carried no bound at all before
  [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882 and silently takes OPERATION after it. Master has no bare execute() left -
  every site there names its class - and JDBC backend: no statement is given a query timeout, and the read bound of an established connection is gone too #877 puts every create table of this backend
  at BULK, that being DDL nobody waits on. It is BULK here for the same reason.
  This is the merge choosing for a statement neither branch conflicted over, so it is
  called out rather than folded in: say so if you would rather it were left at the
  default and filed.

mvn -pl opendj-server-legacy test-compile is green, and so are
JDBCStatementBoundTestCase (37/37), JDBCStorageRetryTest (66/66),
CachedConnectionTestCase (64/64), StampConnectionTestCase (5/5), BulkCursorTest
(12/12) and PersistentCompressedSchemaTest (8/8) - 193/193 together. The four engine
suites have not been re-run since this merge and are left to CI.

The sixth review round answered (66579d3)

• The catalog connect takes the bound the operator configured, org.openidentityplatform.opendj.jdbc.connect.timeout, read through one method both this connect and every borrow of the pool now go through, and 0 is honoured as the operator asking for no bound of the connect. A deployment which had raised that property because its login is slower than the default met a second, tighter bound here, and 08001 is no conflict write() replays: the backend stopped opening on an installation that opened before this connection existed. What is not taken from the pool is the deadline of the borrow — this connect waits for no peer to return a connection — and the javadoc says what that costs at a property of 0 (a login the database never finishes parks inside the lock openCatalog() holds) rather than claiming parity.
• A row of the catalog the read passes over is reported by the clear. A row naming no tree, one naming something that is not a tree name, and one recording a table outside the namespace this backend names its tables in are each skipped rather than turned into a drop table of a value read back out of a table — and until now no line of the report named them: what such a row records is outside the opendj names the leftover scan walks, and the row itself is dropped by nothing either. They are counted and listed now. Nothing this version writes makes such a row, so the line is the account of a database written into by something else; it is no term of the "dropped nothing" line, a catalog whose table is there always naming itself.
• The postgres case covers the shadowing create as well as the lookup. openTree creates a table where its lookup answers there is none, and an unqualified create table lands in current_schema() — the schema ahead of the tables. The case now opens the tree on the ahead-resolving storage and asserts that no second table appeared there and that the populated one in public is untouched, which is the more destructive of the two halves and was asserted by nothing. Asked of information_schema with the schema and the name bound, since getTables() takes the schema as a pattern where _ is a single-character wildcard.
• A comment [#873] Give each backend its own compressed schema trees #881 had made false is rewritten: it said the shared pair holds the compressed schema of the backend the class opens in setUp(), which since [#873] Give each backend its own compressed schema trees #881 no backend of the class makes. Believed, it argues for deleting the hand-drop of that case, and testCompressedSchemaTableIsQualifiedByBackendId then fails whenever TestNG runs it second.

The catalog connection is under test now

CatalogConnectionTestCase (5 cases, no database) covers the establishment of that connection, which no test reached at all — the probe driver is answered a postgresql url pgjdbc cannot parse, so DriverManager falls through to it while ConnectDialect still reads the prefix as postgres, which is what makes the connect fill in bounds in the first place. Its two cases about the bound go red on the previous head (connectTimeout … expected [120] but found [30]). JDBCStorageRetryTest asserts the enrolment on that connection — the create, the row and the commit — where each of its assertions had held just as well of a storage that enrolled nothing whatever. TestCase covers the skipped row on all four engines.

On "the catalog connection has no deadline of any kind"

That round read 58c6876, one merge behind: f305785 brought #882 in. On this head the select of the catalog, its upsert and its delete are issued through the transaction of CatalogSession, which carries StatementBound.OPERATION — 120 s by default, with the socket read timeout of #882 behind it — so the enrolment blocked behind a clear that holds the catalog table ends at that bound naming the property, rather than parking a start-up with nothing in the log. The one catalog statement left unbounded is the create table, at BULK, which is the class #877 gives every DDL of this backend; say so if the catalog's should be the one that differs.

The session lock timeout is still deliberately absent (a catalog row is the state a clear reads, and it queues for its lock rather than dying on a bound), and the read bound of the login is still lifted once the login is through (#872), the backstop of #882 arming per statement over it.

One instance of that point was real and is fixed: TableScope.schemaPathOf()'s select unnest(current_schemas(true)) carried no bound at all — the one statement of this class outside the scheme of #877 — and it now takes OPERATION like the lookups the scope it builds narrows. The savepoint and the getSchema() fallback beside it answer for a query an engine refuses, not for one it never answers.

A code review of the same head

• reset() of both sessions, enrolInCatalog(), readEnrolledTrees() and createCatalogTable() caught SQLException alone, where unenrolFromCatalog() had always taken both and says why. An unchecked failure skipped the rollback and left the connection every remaining tree of that open writes its row on in postgres 25P02, failing the twenty-odd enrolments behind it with a cause nowhere near the one that started it — and in createCatalogTable it skipped the tolerance of a table another session had just created, turning a benign race into a failed open.
• A failed catalog connect is reported through CachedConnection.reported(), which is what keeps the password of a connection string a driver echoed back out of ERR_OPEN_ENV_FAIL.
• The backend id goes into the name of the catalog tree escaped, the way PersistentCompressedSchema escapes it into its own prefix. TreeName.valueOf splits on the last slash, so an id carrying one named a tree that does not survive being read back — and a clear reading the stamp of its own catalog table could not recognize it. An id of the ordinary shape is unchanged, so this renames no table of any installation.
• listTrees() borrows a validated connection. Answering from the catalog made it one of the paths that issue their statements far from the borrow and compensate a dropped connection in no other way — the very list getValidatedConnection() documents.
• readEnrolledTrees() asks through the non-enrolling name of [#873] Give each backend its own compressed schema trees #881: reading what the catalog records is not taking an interest in the tree it names, and a row it decides not to trust must not put that tree in the memo. Two statements about tree2table this branch had made stale are corrected with it.

Still open, and named rather than folded in

• The catalog connect makes one attempt. CachedConnection.getConnection() waits out a database at its connection limit or still recovering (53300, 57P03 and the equivalents of the other three) for up to the pool timeout; this connect does not, and it is on the critical path of the first openTree(createOnDemand) of every read-write open. Pooling it is what the javadoc argues against — the caller is holding a pooled connection already — so what it wants is the pool's retry, not its deque. Left for the next round.
• A clear that dropped only its catalog table does not print why. The line explaining the upgrade is gated on dropped==0, and a catalog table that is there while its rows are not — a restored backup older than the tables — makes dropped one. The condition wants to be "nothing but the catalog", and the text to say "a catalog that is not there, or one that names nothing".
• unenrolFromCatalog() takes the tree out of the memo before it deletes the row. A concurrent openTree of that same tree in the window between the two finds it absent from the memo, writes and commits its row, and the delete then removes what it had just written: a table nothing names, which is the state this whole change exists to prevent. The memo entry wants to go after the commit.
• leftoverTables() can name one table twice, where two schemas of the search path hold the same table name: the listing is deliberately made with no schema pattern, so both rows pass the scope, and the single stamp read resolves whichever the path reaches first.

Tests of this round

suite	where	result
CatalogConnectionTestCase, JDBCStorageRetryTest, StampConnectionTestCase, CachedConnectionTestCase, JDBCStatementBoundTestCase	locally, -P precommit verify	178/178, Skipped: 0
the two new cases about the bound of the catalog connect	locally, against the previous head	red, as they have to be
PgSqlTestCase	locally, against a container	76/76, Skipped: 0 — the search-path case among them, which had never run before this round
MySqlTestCase	locally, against a container	75/75, Skipped: 0
MsSqlTestCase, OracleTestCase	left to CI on this head	—

[maximthomas]

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.

[vharseko]

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.

[maximthomas]

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.

[vharseko]

Thank you — all three majors are addressed in 87d3bb5, and so are the nits bar two I disagree with, with the reasoning below. All four engines run again.

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

Confirmed, and the case in this very PR reproduces it exactly as you describe: the neighbour keeps two tables (its tree and its own catalog), so the old leftBehind counted 2 and the line ended these have to be removed by hand inside a green test.

The provenance you point at is the right one, but not through getTables(): the MS SQL Server driver returns no REMARKS for a table at all, and the Oracle driver returns it only with remarksReporting=true, so the readback would have been true on two engines and blind on the other two. readStoredComment() — the per-dialect readback #866 already carries — is used instead, once per table still standing, which on a clear that left nothing behind is no query at all.

What the report says now, per table:

• a stamp naming a tree of a base DN this backend does not serve → a table of a backend sharing the database (JDBC backends sharing a database URL share one pair of compressed-schema tables #873): passed over in silence;
• a stamp naming a tree of this backend → reported as this backend's own, and only there is removing it by hand suggested;
• no stamp at all → reported as attributable to nobody, and left exactly where it is.

One deliberate reversal. The catalog table is now stamped like any other table. A neighbouring backend's catalog is otherwise the one table such a report can attribute to no one, and it would then be reported as "unattributable" after every clear on a shared database — the same false alarm in a quieter voice. It costs one stamp attempt per open of the storage rather than one per tree, being issued behind the very flag that keeps the catalog from being opened twice. The description of the catalog in the PR text is corrected accordingly.

testAClearReportsTheTablesItCanAttributeToThisBackend asserts both halves: an uncatalogued table of a base DN this backend serves is reported as its own, and testABackendIsClearedByAProcessThatNeverOpenedIt now asserts that neither the neighbour's tree nor the neighbour's catalog is named in what the clear reports.

The reordering's guarantee holds on two engines of four (major) — fixed, by the order you named

Confirmed, and the invariant I wrote around it was wrong for mysql and oracle for exactly the reason you give. deleteTree() now drops, commits, unenrols and commits again:

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

Which is what the rule always was, once stated without the word "mirror": a row is written before its table is created and taken out after its table is dropped. openTree() enrolling first and deleteTree() unenrolling last are the same rule read in the two directions — the state to avoid is a table nothing names, and the state the removal is ready for is a row whose table is gone. The comments at both sites say that now, and no longer claim a symmetry that would be wrong.

The unenrol carries its own commit for the bug this was reopened for, so both halves hold on all four engines: a failed drop leaves tree and table both there (nothing is pending before it), and a failure after the drop leaves at worst a row the clear skips and logs. deleteTree's "table already gone" branch commits its delete too, which it did not before.

The first offline clear after an upgrade still drops nothing (major) — documented, in two places

Confirmed: BackendImpl.java:669-673 clears before newRootContainer at :681, and nothing enrols earlier. Not fixed by this now says that a backend upgraded in place has to be started once before its first offline --clearBackend, and the log line for a clear that dropped nothing says it too — that is where an administrator meets the situation.

The skip that keeps a clear going is unreliable on MySQL (minor) — fixed at the call sites that decide a drop

isExistsTable got an overload taking a catalog and a schema, and the removal passes con.getCatalog()/con.getSchema() — in the drop loop, in catalogTables() (a twin catalog next door would have sent the select k,v at a table that is not here) and in the shared-pair lookup of listTrees(). openTree's per-tree call is untouched, as agreed: there a false "it is there" only skips a create table, which is the hazard that predates this PR.

Nits

• The fix pinned by no test — testADeletedTreeStaysOutOfTheCatalogWhenItsTransactionFails added: a WriteOperation that deletes a tree and then throws. It pins the half that a refactor could undo, the delete being owed to no transaction that could still roll it back; the mysql/oracle half is now pinned by construction, nothing being pending before the drop at all.
• Two branches committing nothing — deleteTree's skip branch fixed as above. In openTree the enrolment still rides the enclosing transaction where no table and no index are created, which is the reopen of an unchanged backend: the rows are already there, so what a rollback loses is a rewrite of what it rewrote, and the next open writes them again.
• The "dropped nothing" warning printing the wrong number — it prints all three now: rows whose table was already gone, tables of this backend named by no catalog, and tables attributable to nobody.
• clearQuietly swallowing the code under test — I do not think it does. testAClearSkipsACatalogRowWhoseTableIsGone calls storage.removeStorageFiles() in the body at TestCase.java:1487, and testTheSharedCompressedSchemaTrees… at :1564; the clearQuietly in the finally of each is the cleanup the previous round asked to be moved there ("Move both clears into finally"). The neighbour's clear is cleanup in the same sense — the case asserts on the neighbour's table before it, not on the clear.
• Half the shared pair untested — both trees now, in the open, in the listTrees() assertion and in the assertion that a clear left them standing.
• The v fallback unexercised — testAClearDropsTheTableTheCatalogRecords asserts that a row records the table holding its tree (a swapped k/v fails it), then empties every v behind the catalog's back and asserts the fallback answers with the derived name and the clear still drops the table.
• unenrolFromCatalog lacking the guard — added, with a comment saying what it is: symmetry, not protection. It could not have stopped a deleteTree from dropping the shared pair, the drop happening whatever the catalog says, and there is no row of that pair for the delete to find.
• dbtest on an upgraded, never-started installation — I checked this one and do not think it is a regression. BackendStat opens through getReadOnlyRootContainer() (:917), i.e. AccessMode.READ_ONLY, and this backend's openTree(treeName, false) does nothing whatsoever — it does not so much as call getTableName. So on master tree2table is empty at that point too and listTrees() returns nothing there either. Under this PR the same run returns the shared compressed schema pair, which is one tree more than before, not one less.
• Test setup leaking — the setup half of the offline clear case clears both backends when it throws, before the case's own finally is ever reached. The test helper isExistsTable still lists what the connection can see, deliberately: it is there so that no folding rule of the backend under test is trusted by the assertions.

Engines

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

58 on the previous push, 61 here — the three cases above, on every engine.

[maximthomas]

issue (blocking): the narrowing missed the call site that decides a drop table

JDBCStorage.java:1557, :1490. Your reply says the fix went to "the call sites that decide a
drop", exempting only openTree's create. Both of these pass a TreeName, so both bind to the
inner 1-arg helper at :1368 and end at isExistsTable(con, null, null, name):

// :1368 — the inner helper, still unqualified
boolean isExistsTable(TreeName treeName) {
    return JDBCStorage.this.isExistsTable(con, getTableName(treeName)); // -> (con, null, null, name)
}

// :1557 — this decides a drop, not a create
if (isExistsTable(treeName)) {
    try (final PreparedStatement statement = con.prepareStatement("drop table " + getTableName(treeName))) {

Narrowed in this commit: :898, :1906, :1927. Not narrowed: :1490, :1557.

Two backends in separate databases of one MySQL server with the default backendId and the same
base DN hash to identical opendj_<sha224> names. Connector/J 8 reads a null catalog as any
database, so :1557 sees the neighbour's table and issues an unqualified drop table for a table
absent here → 1051 → StorageRuntimeException out of the callback. Not a class-40 state, so
write() does not replay: EntryContainer.clear() fails now and on every retry. Your own new
javadoc at :840-851 states this outcome verbatim.

At :1490 the same false positive skips the create and enrolInCatalog reaches the upsert at
:1475 unconditionally — the backend open fails instead.

Fix: route both through isExistsTable(con, catalogOf(con), schemaOf(con), name).

Not an issue at :1517 — catalogTableOpened || short-circuits ahead of it on every reachable path.

---

issue (non-blocking): a leftover of a removed base DN is now reported by nothing

JDBCStorage.java:1019-1024. Two sinks, no else:

final TreeName stamp = stampedTree(con, dialect, tableName);
if (stamp == null) {
    unattributed.add(tableName);
} else if (isOwnTree(stamp)) {
    ours.add(tableName);
}                                  // <- a parsed stamp that is not ours falls out of both

Remove a base DN while the backend is disabled, then clear: the tables are stamped, isOwnTree
iterates the current config.getBaseDN() and returns false, and they appear in no line at all.
Round 2 counted them into leftBehind — over-broadly, which is what I raised, but it named them.

Worse, the ours text at :966-969 advertises exactly this case ("a tree taken out of the
configuration while the backend was disabled") and closes with "re-adding the base DN … adopts
it" — both unreachable, since reaching ours already requires the base DN to be served. Only a
removed index of a still-served base DN can land there.

The silence for another live backend's tables is correct under #873 and not in scope here.

---

issue (non-blocking): two of the three new tests are green on e49fce21fb

TestCase.java:1619-1657, :1665-1693. Round-2 deleteTree was:

unenrolFromCatalog(treeName);          // DELETE still pending...
if (isExistsTable(treeName)) {
    execute(statement); con.commit();  // ...and this commit carried it, on all four engines
}

testADeletedTreeStaysOutOfTheCatalogWhenItsTransactionFails opens the tree read-write just
before deleting it, so isExistsTable is always true and the drop branch always taken — same
outcome on r2 as on head. The branch this commit actually changed, table already gone, is
reached by no test. testAClearDropsTheTableTheCatalogRecords asserts the v fallback, which is
byte-identical in r2. Revert the whole reorder and the suite stays green; "pinned by construction"
holds on r2 too.

reportClearOutcome and its three warning strings are asserted by nothing — every new assertion
calls leftoverTables() directly on the test's own connection.

Suggested: drop the table behind the backend first, then deleteTree it and fail the write.

---

issue (non-blocking): after an upgrade the report calls the backend's own live tables unattributable

JDBCStorage.java:970-972. #866's stamp is commit 3800973a69; git tag --contains 3800973a69
is empty against 94 tags (latest 5.1.2). It is in no release.

So on an upgraded installation nothing is stamped and nothing stamps before the clear —
removeStorageFiles()'s open(READ_WRITE) at :875-881 opens no tree. dropped==0,
missing==0, every own table → unattributed, and the operator is told they "may hold the trees
of a backend sharing this database" about this backend's own live data, at the one moment when all
of it is certainly its own.

Suggested: when dropped==0 && missing==0 and the catalog table does not exist, infer "predates
the catalog" — print only the :975-976 upgrade line, which already carries the right remedy.

---

suggestion (non-blocking): schema reaches getTables as an unescaped pattern

JDBCStorage.java:855-865. storedIdentifier only case-folds and is applied to the table name;
the loop compares TABLE_NAME alone, never TABLE_SCHEM/TABLE_CAT. _ is a single-character
wildcard, so on Postgres with app_data and appXdata in one database the narrowed call at
:898 still answers for the wrong schema. Escape it, or compare TABLE_SCHEM in the loop.

suggestion (non-blocking): a failed readback is reported as a confident "no stamp"

JDBCStorage.java:1040-1053. stampedTree catches SQLException | RuntimeException → null →
unattributed. Because it swallows, leftoverTables's own "the database would not say" path can
no longer fire, so a connection dying after the metadata scan yields a confident warning naming
this backend's own tables. Count read failures apart from absent stamps.

suggestion (non-blocking): leftovers == null suppresses the "dropped nothing" line

JDBCStorage.java:963-976. The early return precedes the dropped==0 check. dropped and
missing are known at :896-907; decide that line before consulting leftovers. (Pre-existing
at r2, not a regression.)

suggestion (non-blocking): the tests bypass the normalisation this round added

TestCase.java. Tests call leftoverTables(con, con.getCatalog(), con.getSchema()); production
calls it with catalogOf/schemaOf, which map "" to null. emptyToNull could be deleted and
every test stays green. Same file: assertReportsNothingOf is a pure absence assertion — it
passes vacuously if the scan enumerates nothing; assert the neighbour's table is listed by the
neighbour's own leftoverTables.

nitpick (non-blocking): an assertion that cannot fail

TestCase.java — assertTrue(recorded.containsKey(storage.getCatalogTree())). catalogTables()
ends with an unconditional trees.remove(catalog); trees.put(catalog, catalogTable); after its
only early return, so a non-empty map always contains that key.

nitpick (non-blocking): the rest

• createBackendCfg(String) leaves getBaseDN() unstubbed, so ours is vacuous in every test but one.
• The catalog's own stamp is issued behind if (!catalogTableOpened), so a catalog created by r2
  stays unstamped and still trips a neighbour's report until every backend is reopened on this build.
• dropTableIfExists's catch (SQLException ignored) {} is the sole cleanup for the table one test
  deliberately leaves standing.
• readStoredComment's javadoc still says "this runs on the stamp connection, which is not a pooled
  one" — leftoverTables now calls it on the pooled clear connection, and those post-commit reads
  are neither committed nor rolled back before the connection returns to the cache.
• catalogOf/schemaOf's javadoc still says "a count too wide, not a wrong one"; since this commit
  the same value feeds the drop loop at :898, where too wide is exactly wrong.
• The ours advice ("re-adding the base DN or the index it belongs to adopts it") is wrong for the
  catalog table, which isOwnTree special-cases to true. Text only — unreachable, since the catalog
  is dropped last and a failed drop rethrows before the report runs.

[vharseko]

All of round 3 is addressed in f51a1c1407. Re-traced each point against the code rather than taking the summary's word for it, and two of them come out other than as written — said so below.

The narrowing missed the call site that decides a drop table (blocking)

Right, and the fix went one level deeper than the two call sites you name: the inner one-argument helper is gone, and isExistsTable(TreeName) now asks with the catalog and the schema of its own connection. That covers :1490 and :1557, and also openTree's create, which you exempted as the older hazard - with a catalog it is no longer only that. A foreign table answering there skips the create after enrolInCatalog has written the row, so the catalog names a tree whose table is not in this database, and the first write to it fails. :1517 is covered by the same change; you are right that catalogTableOpened || short-circuits ahead of it on every path reachable today, but the flag is reset by removeStorageFiles() itself, so it costs nothing to have it asked correctly.

The catalog and the schema are asked of the connection once per transaction rather than per lookup. openTree runs this for every tree of the backend on every open, and getSchema() is a round trip of its own on postgres - pgjdbc answers it with a select current_schema(). A transaction holds one connection for its whole life, so one answer serves all of it. catalogTables() likewise takes the two names its caller has already asked for; a clear and a listTrees() were each asking twice over.

Your schema-as-a-pattern suggestion is in the same change, by the second of the two routes you offered: TABLE_CAT and TABLE_SCHEM are compared row by row, in isExistsTable and in the leftovers scan both. Escaping would have left the comparison resting on the driver's getSearchStringEscape; a name the driver does not give rules nothing out, oracle naming no catalog at all.

Two of the three new tests are green on e49fce21fb

Confirmed, by putting the previous shape back and running the suite: with unenrolFromCatalog ahead of the drop and no commit of its own, testADeletedTreeStaysOutOfTheCatalogWhenItsTransactionFails still passes and only the new case fails. Your reading of why was exact - the tree is opened read-write just before, so the drop branch is always taken and its commit carried the pending delete on all four engines.

testADeletedTreeStaysOutOfTheCatalogWhenItsTableIsAlreadyGone is added for the branch the reordering actually changed, built the way you suggested: the table is dropped behind the backend, then the tree is deleted and the write fails.

A leftover of a removed base DN is reported by nothing

Confirmed, and kept - with the text fixed rather than the behaviour. The stamp names the tree and never the backend it belonged to, so a table of a base DN this backend no longer serves is indistinguishable from a table of a backend sharing the database, and reporting it would be round 2's over-broad count again, one line further down. What was wrong was the advertising: the ours message no longer offers the case it cannot reach, and says what can reach it - a tree of a base DN this backend still serves, an attribute index taken out while it was disabled. The limitation itself is now stated in the javadoc of reportClearOutcome, and in the PR description.

After an upgrade the report calls the backend's own live tables unattributable

Half of this one. The unattributed line names the other reading in the same sentence - "or be leftovers of a version stamping no table at all" - and the line you point at as the right remedy is printed on that same clear, so the operator is not told only the alarming half. What was genuinely wrong is the order: the list came first and the explanation after it. The upgrade line is printed first now. Suppressing the list would have taken away the useful part of it, the names.

leftovers == null suppresses the "dropped nothing" line

Fixed. The line is decided by what the clear itself did, so a database which would not say what is standing silences the two lines above it and not this one - that silence is the whole of #888. It says in that case that what else is standing could not be read.

A failed readback is reported as a confident "no stamp"

Fixed, as a third bucket: unreadable, reported in a line of its own. stampedTree no longer swallows the SQLException, and the failed read is rolled back before the next table is asked about - postgres refuses every further statement of the transaction until it is (25P02), so one failure used to paint the whole remainder of the list. An engine with no comment readback of its own (dialectOf answering null) puts every table into that bucket rather than calling them all unattributable: nothing was asked, and "carries no stamp" would be a claim.

The tests bypass the normalisation, and assertReportsNothingOf is vacuous

Both fixed. The cases ask leftoverTables through catalogOf/schemaOf, the way a clear does. assertReportsNothingOf now also asks the neighbour the same question and requires it to report the table as its own, so the silence next door is a decision and not a scan that reached nothing - which needed the neighbours of both cases to serve a base DN of their own, your createBackendCfg nitpick.

An assertion that cannot fail

Replaced with the invariant it was reaching for: the catalog is last in the order catalogTables() answers in, which is the order the clear drops in.

The rest

• readStoredComment's javadoc is corrected. One thing there reads other than as written, though: CachedConnection.close() rolls back before the connection goes back to the cache (CachedConnection.java:155), so the reads a clear makes on the pooled connection leave no transaction behind. Only the text was stale.
• catalogOf/schemaOf's javadoc no longer says "a count too wide, not a wrong one"; it says what an unnarrowed answer costs where it decides a drop or a create, which is that it is exactly the question this code asked before there was anything to narrow it by.
• The catalog created by round 2 stays unstamped until the storage is opened again on this build - the stamp is one per open of the storage, by design, and an open is all it takes.
• dropTableIfExists's silent catch is left silent, with a comment: what it fails to drop is dropped by the dropStaleTrees() of the next run of the class, and a failure there must not replace the failure of the case it cleans up after. The pair of trees testTheSharedCompressedSchemaTreesAreNamedButNeverCleared creates is now dropped by hand, though - no clear ever removes it, so it was standing for the rest of the class.
• The ours advice no longer mentions re-adding a base DN.

Two more, from a pass of my own over the same code: a listing row naming no table was reported as a table called "null" (String.valueOf of a null), and the count of trees which had lost their table can only be non-zero in that line if the catalog table went while the clear was running - a catalog that names anything names itself last, so it is dropped and dropped is one. Each such tree is logged by the loop either way; the count is kept for that one race and the comment says so.

Tests

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

[maximthomas]

praise: round 3 is genuinely addressed, and two of the corrections are right

Re-traced, not taken on the summary's word. The isExistsTable narrowing does reach :1500, :1595,
:1620 and :1660; stampedTree propagates SQLException; unreadable is a real third bucket; the
null TABLE_NAME no longer prints as "null"; catalogTables() builds a LinkedHashMap so the
catalog-is-last assertion is sound. The two corrections in the comment both hold —
CachedConnection.close() does roll back before the connection returns to the cache, and the
missing-count race is described accurately.

---

issue (blocking): the narrowing is stricter than the resolution rule of the statements it guards

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1466-1471 vs :1500, :1595, :922, :1660

schemaOf(con) is, on pgjdbc 42.7.12, literally select current_schema() — the first existing
schema on search_path (shipped default "$user", public). The statements it guards are unqualified:

con.prepareStatement("create table "+getTableName(treeName)+" ("+getTableDialect()+")")   // :1500
con.prepareStatement("drop table " + tableName)                                           // :922

PostgreSQL resolves an unqualified reference across the whole search_path; an unqualified
CREATE lands in current_schema() alone. Round 3 asked isExistsTable(con, null, null, t) and
listed every schema, so the two agreed.

Reachable path: install as role opendj with no schema opendj → current_schema()=public, all ~25
tables and the catalog created in public. Later someone runs CREATE SCHEMA opendj AUTHORIZATION opendj (the standard remedy since PG15 took CREATE off public) → current_schema()=opendj.
Tables are still in public and ordinary DML still reaches them, but:

if (tableName.equalsIgnoreCase(rs.getString("TABLE_NAME")) && isInScope(rs, catalog, schema))  // :878

rejects TABLE_SCHEM='public' against asked 'opendj'. Consequences:

1. catalogTables() returns an empty map → removeStorageFiles() drops nothing, and
   reportClearOutcome stays silent because leftoverTables is narrowed the same way, so
   missing/ours/unattributed are all zero. This is the #888 symptom the PR exists to fix.
2. The next openTree creates a second, empty opendj_<hash> in schema opendj, which from that
   commit shadows the populated public one for every later unqualified reference. The backend comes
   up empty and the real data is orphaned.

SQL Server has the same shape (getSchema() = SELECT SCHEMA_NAME() = the user's DEFAULT_SCHEMA;
unqualified reference resolves DEFAULT_SCHEMA then dbo), but needs a deliberate ALTER USER.
MySQL and Oracle cannot diverge — Connector/J returns the database field, and ojdbc's getSchema()
is SYS_CONTEXT('USERENV','CURRENT_SCHEMA'), which equals USER because OpenDJ issues no
ALTER SESSION.

Nothing pins the schema across restarts: there is no currentSchema, setSchema or search_path
anywhere in the file, and connectProperties (:336-349) carries only login/connect/read timeouts.

Fix: either accept any schema on the connection's resolution path, or schema-qualify the DDL and DML
with current_schema() so the question and the statement always name the same table. Qualifying is
the smaller change and also disambiguates select k,v from <catalogTable> (:2047).

---

issue (blocking): the hand-drop removes the class-wide backend's live tables, not this case's

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

for (final TreeName shared : JDBCStorage.SHARED_COMPRESSED_SCHEMA_TREES) {
    dropTableIfExists(storage.getTableName(shared));
}

The rationale in the PR comment — "the pair this case creates is now dropped by hand … it was standing
for the rest of the class" — is inverted twice.

createBackendCfg(getBackendId()+"_schema") (:132-137) overrides only the backend id;
getDBDirectory() is still getJdbcUrl(), and getTableName is opendj_<sha224(treeName)> with no
backend id in it. So these are byte-identical to the tables the class-wide backend — opened in
PluggableBackendImplTestCase's @BeforeClass, finalized only in @AfterClass — is still using.
PersistentCompressedSchema.load() created and populated them at setUp; the case's own
openTree(shared, true) found them there. It creates nothing.

After the drop, still mid-class:

• testModifyEntry (PluggableBackendImplTestCase.java:734) is the only user of jpegphoto, so its
  encode is a cache miss → store() → txn.put on a dropped table → StorageRuntimeException →
  ERR_COMPSCHEMA_CANNOT_STORE_EX, failing replaceEntry.
• Any reopen without a clear (:1088, :1141, :1250, :1335) re-creates the pair empty via
  openTree(create=true), so id2entry rows written earlier decode against unknown AD/OC tokens.

testExportLDIFAndImportLDIF (:1005-1021) repairs it — but only if it runs after, and it is not
guaranteed to. sequential = true (PluggableBackendImplTestCase.java:92) is TestNG 6.9.8
singleThreaded: thread affinity, no ordering. Collection order is a
ClassHelper.getAvailableMethods hash-map walk. Reproduced on the project's own testng-6.9.8 with a
synthetic two-level hierarchy: the drop landed at index 7 of 54.

@BeforeClass dropStaleTrees (TestCase.java:93-110) already drops every opendj_% table before
super.setUp(), so the next run is clean without this. The hand-drop only affects the current run,
which is where it does harm.

Fix: delete the loop. The round-3 comment one line above it — "the shared pair is left where it lies,
exactly as the backend leaves it" — is still there and now contradicts it. If cleanup is genuinely
wanted, @AfterClass.

---

todo (blocking): one test, so the item above is visible

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java:1805, :1833, :1843

The only test-side change reaching the narrowing is an argument swap:

- storage.leftoverTables(con, con.getCatalog(), con.getSchema())
+ storage.leftoverTables(con, JDBCStorage.catalogOf(con), JDBCStorage.schemaOf(con))

All four suites run one container, one database, one schema, so those return exactly what the old
calls returned and isInScope can only ever answer true. isSameScope's null/empty/
equalsIgnoreCase logic could be inverted and every suite would stay green.

The one new @Test, testADeletedTreeStaysOutOfTheCatalogWhenItsTableIsAlreadyGone, is green on
87d3bb5f4c — the r3→r4 hunk in deleteTree adds only comment lines, and r3 already had
drop-then-unenrol. It pins round 3's fix, not round 4's.

Asked for: a PostgreSQL case that creates a second schema, puts the backend's table there, sets
search_path so it is reachable but is not current_schema(), and asserts the clear still finds it.
Fails on this head, passes on r3.

Not asked for: assertions on report wording — that is its own maintenance cost. Noting only that the
unreadable bucket is asserted on its empty path alone (:1810), nothing makes stampedTree throw
so the new per-table con.rollback() is unexercised, and the class captures no log output, so every
reportClearOutcome change is unfalsifiable by the 62/62 run.

---

issue (non-blocking): leftovers == null still silences the report, when the clear dropped something

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

if (dropped==0 && (missing>0 || ours>0 || unattributed>0 || unreadable>0 || leftovers==null)) {

When leftovers==null the three counts are forced to 0 at :997-999, so the condition collapses to
dropped==0. A clear that dropped ≥1 table on a database whose getTables failed emits nothing — no
report, no note that the accounting was skipped (only logger.trace at :1107). The comment at
:992-994, "a database which would not say what is standing silences the two lines above it and not
this one", holds only when dropped==0.

Fix: hoist the leftovers==null case out of the dropped==0 guard into a line of its own.

---

suggestion (non-blocking): a failed scope lookup is latched, and logged nowhere

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

if (!tableScopeKnown) {
    tableCatalog=catalogOf(con);
    tableSchema=schemaOf(con);
    tableScopeKnown=true;          // set even when both came from the catch
}

catalogOf/schemaOf (:1180-1197) each catch (Exception e) { return null; } with no log line. A
null pair reverts every later lookup of that transaction to isExistsTable(con, null, null, …) — the
unnarrowed question this commit removes — and that decides the create table skip at :1500 and the
drop table at :1660. Fail-open, and undiagnosable.

Fix: log inside both catches, and leave tableScopeKnown false on a null answer so the next lookup
re-asks. The same fail-open exists independently at removeStorageFiles, :904-905.

[vharseko]

Round 3 is answered in f51a1c14, and 7498571e on top of it is a pass of my own over the same code. Two of the things it found are worth reading as review points rather than tidying, so they are first.

The row of an enrolment was committed by nothing on the one open that needs it

The ordering argument of the round before this one held that a row is written before its table and carried into durability by the create table behind it — on postgres and sql server by that statement's commit, on mysql and oracle by the implicit commit its DDL begins with. That is true of every open which creates a table. It is not true of the open which fills the catalog of a backend upgraded from a version keeping none: there every table is already standing, no create table runs, and nothing of openTree() committed anything. A transaction failing anywhere after those rows took every one of them back, and the backend was left exactly where it started — tables named by nothing, and the next --clearBackend dropping nothing. The state #888 is filed about, reachable now by a route the fix itself opened.

So the row carries a commit of its own, the way deleteTree's unenrolment already does, and the invariant no longer rests on what an engine does with DDL.

…and that commit is the caller's, which is the reason not to issue it for a row already there

The other half of the same point. enrolInCatalog was writing a row per tree on every open — about 25 for a stock suffix — each of them the row that was already in the table, and committing them would now carry whatever else the caller had pending: RootContainer.open() opens every entry container in one write(), and DefaultIndex.afterOpen writes a trust flag through it.

What the catalog holds is therefore read once, when this storage first opens it, and a tree it already records at the table this version records it at is not written again. An open of a backend whose catalog is complete — every open after the first — now writes nothing to the catalog and commits nothing of its caller's, where the previous round wrote and would have committed 25 rows. The enrolment costs a write only where it has something to say.

Recording and not presence is what decides it, deliberately: a row naming a table this version would never create is rewritten rather than trusted, since a removal drops the table the row records, and trusting such a row would leave the real table standing, named by nothing and dropped by no clear ever after.

The case opening the shared compressed schema pair was dropping tables that are not its own

My own note in round 3 — "the pair this case creates is now dropped by hand, no clear ever removes it" — was wrong about who creates it. PluggableBackendImplTestCase.setUp() opens a backend for the whole class in @BeforeClass, and PersistentCompressedSchema makes those two tables there; what the case does is open two trees that are already standing. Dropping them in its finally took them out from under a backend still holding them, and any later case storing an attribute description it had not yet cached would have failed on a table that is not there. The suites pass either way, which is what makes it worth removing rather than leaving.

The rest

• A catalog row with no key at all is skipped and reported, like one whose key is not the name of a tree — it was thrown over instead, out of listTrees() and out of the clear.
• The drop loop of a clear rolls back every failure of its own and not the SQLException alone: the lookup deciding each drop answers with a StorageRuntimeException, and a drop left pending by one of those was going back only because the pooled connection rolls back as it is handed in.
• stampedTree's dialect==null guard is gone — its one caller answers that question before the loop that calls it, and the guard answered it the other way round ("no stamp" where the caller says "nothing was asked").
• The catalogTables(Connection) overload went with its last caller; the two tests using it ask the way a clear asks. listTrees()'s javadoc says what it had left unsaid, that the catalog is itself among the trees it names.
• What the narrowing of round 3 costs an installation whose tables are not in the schema its connection works in is now stated where the narrowing is, and in "Not fixed by this": the wider question of the version before it found such a table, this one does not, and an open recreates it empty. Narrow — the backend creates and reads its tables unqualified everywhere else — but it is a behaviour change and was written down nowhere.

Tests

Two cases, in jdbc/TestCase and so in all four engine suites:

case	asserts
testAReopenedTreeStaysInTheCatalogWhenItsTransactionFails	a write() throwing after an openTree of a tree whose table is already standing leaves the row where it is
testARowRecordingAnotherTableIsEnrolledAgain	a row recording a table this version would not create is written again, where one recording the right table is left alone

Each was run against its own half reverted: the first fails on mysql, sql server and oracle without the commit (green on postgres either way, which asks for the cursor index of every tree on every open and commits that), the second fails without the comparison — and in both runs nothing else did.

testCommentFailureLeavesTransactionIntact keeps its meaning by construction rather than by luck: both of its trees are enrolled by the storage that creates them, so the openTree under test records nothing and commits nothing of the write left pending for it.

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

Re-requesting review. Merge order is unchanged: this should still land after #881.

[vharseko]

Merged with master — no review point of round 4 is answered by it

master took #880 (read-only transactions, #874) and #883 (validated borrow and replay accounting,
#879) while this branch stood, and the two conflicts they left were not textual only. What the merge
decided, so it can be read rather than trusted:

• The enrolment writes, so it stands behind the read-only check. openTree(createOnDemand) calls
  checkReadOnly() before enrolInCatalog(), and deleteTree() before both the drop and the
  unenrolment. A read-only storage may open an existing tree and read it, and may not name one in the
  catalog.
• removeStorageFiles() borrows through getValidatedConnection(). JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 names the removal
  explicitly as a path that issues its statements far from the borrow, so a connection dropped inside
  the alive window would surface out of the clear instead of being compensated.
• Every commit the catalog issues of its own raises partlyCommitted. This is the one that had to
  be added rather than merged: JDBC backend validates the pooled connection on every borrow, costing a database round trip per operation #879 takes an attempt out of the conflict replay the moment part of its
  work is committed, and this branch commits on three paths of its own. openCatalogTable() now goes
  through commitStatement(sql, true) like every other DDL of a write transaction, and the enrolment
  and the unenrolment raise the flag in front of their con.commit(). Left as they were, write()
  could replay an attempt whose catalog row is already committed.
• isExistsTable(TreeName) keeps the scoped form of this branch; master's copy is the unscoped one
  this PR replaces.

mvn -pl opendj-server-legacy test-compile is green; the engine suites have not been re-run since the
merge, and CI is re-requested below.

To be explicit about what this is not: the blocking point of round 4 — schemaOf(con) narrowing
harder than PostgreSQL resolves an unqualified reference, and the search_path path you traced
through it — is untouched by the merge and is not answered here. This comment is about the
integration alone.

[maximthomas]

praise: The catalog is the right shape for #888. Its name follows from the configuration, so a
process that has opened nothing can still name its trees — which is exactly what the offline
import-ldif --clearBackend needed and what tree2table could never give. The rest that stands out:

• The write order is argued, not assumed: a row before its table, an unenrolment after the drop,
  with the asymmetry justified by which half-done state the removal can survive.
• The clear now says what it did not drop, and keeps unattributable apart from unreadable — a
  stamp the database would not give up is not reported as an absent one.
• Every table lookup narrowed to catalog+schema, with the _-as-wildcard hole closed by comparing
  names by equality rather than trusting the metadata pattern (JDBCStorage.java:889-903).
• catalogTables() skips and logs a malformed key instead of throwing out of every clear.
• The merge with master was written out point by point rather than trusted, and the round-4 blocker
  is declared unanswered rather than quietly dropped.
• Eleven cases against four real engines, with no mock of the code under test.

---

issue (blocking): The enrolment's own commit makes the first open of an upgraded backend
unreplayable — the invariant openTree's own comment states.

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

partlyCommitted = true;
con.commit();

RootContainer.open() is one storage.write() over ~25 openTree(createOnDemand=true) calls. On a
backend upgraded in place the catalog does not exist yet, so the first openTree creates it and then
commits one row per tree; replayReason() returns null for a partly-committed attempt and write()
throws at attempt 1. A deadlock or 40001 at tree N now fails backend start-up where master replayed
it. openTree's comment at :1879-1886 says this must not happen; enrolInCatalog is the first act
it names.

Suggested fix: issue the enrolment on the separate stampSession connection #866 already uses
(:1771), as commentTable does. The row commits on its own connection, the caller's attempt stays
replayable, and the flag is never raised.

---

issue (blocking): The drop-order guarantee is pinned by an assertion no database state can fail.

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

final List<TreeName> order = new ArrayList<>(recorded.keySet());
assertEquals(order.get(order.size() - 1), storage.getCatalogTree(), ...);

catalogTables() ends unconditionally with

trees.remove(catalogTree);
trees.put(catalogTree, catalogTable);

so the catalog is the last key always. The assertion pins that one builder line. The guarantee lives
in removeStorageFiles()'s loop (JDBCStorage.java:950) — sort the keys there, copy into a
HashSet, or drop the catalog first, and the case stays green while the round-1 "stale catalog row
outliving its table" defect returns.

Suggested fix: assert against removeStorageFiles, not the builder — capture the order the loop
actually drops in, or clear with one table already gone and assert the catalog row outlives every
tree it names.

---

issue (blocking): The four-engine evidence predates the merge.

The suite table in the PR body was produced before f96be02, and the merge added checkReadOnly
ordering, the validated borrow and three partlyCommitted raises by hand. Re-run PgSqlTestCase,
MySqlTestCase, MsSqlTestCase, OracleTestCase and update the table before merging.

---

suggestion (non-blocking): isExistsIndex still asks the whole server, while every table lookup
was narrowed.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2062
(callers :1903, :1911, :1920)

con.getMetaData().getIndexInfo(null, null, tableName, false, true)

getTableName is "opendj_" + SHA-224(treeName) — no catalog, schema or backend id in the input, so
two databases of one server hold identical table and index names. Connector/J 8+
(databaseTerm=CATALOG, nullDatabaseMeansCurrent=false) binds no TABLE_SCHEMA predicate for a
null catalog, so the neighbouring database's k_ index answers, create index is skipped forever,
and every cursor batch where k>? order by k is a full scan plus filesort. Verified on Connector/J
9.2.0 bytecode; moot on MSSQL, where k is varbinary(max) and carries no index anyway.

Pass the caller's (catalog, schema) — it already has them from the transaction's one round trip.
The line is untouched context, but the PR text says "Every lookup goes through them".

---

suggestion (non-blocking): Say that a read-write open now requires CREATE, and name the catalog
when it fails.

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

Failing hard is the right policy — the catalog is functional state, not a diagnostic like #866's
stamp. But on an upgraded install master's openTree issues no DDL at all, and this one always
issues create table opendj_<hash>; a DML-only account turns that into ERR_OPEN_ENV_FAIL with a
raw SQL error. Give the failure a message that says the backend could not create its tree catalog,
and add the new CREATE requirement to the "upgraded in place" paragraph.

[vharseko]

Round 5 answered, and the round-4 blocker with it

Your first blocker was not hypothetical for a moment longer than it took CI to run: the six failures
of JDBCStorageRetryTest on every ubuntu job of the last run are that same invariant, stated by
master's own suite. testOpeningAnExistingTreeLeavesTheAttemptReplayable and three others died on the
40001 they were supposed to replay; the remaining two counted statements the enrolment had added.

issue (blocking) — the enrolment's own commit. Fixed the way you suggested, one step further out:
the catalog is read and written on a connection of its own, opened when it is first read or created
and closed with the transaction. partlyCommitted is raised by commitStatement() alone again, so
RootContainer.open() is exactly as replayable as it is on master, upgraded backend or not. The
create of the catalog table went to that connection too - left on the caller's it would have raised
the flag once per installation, which is the same regression made rarer - and it runs under a lock
that tolerates a table another process created while it was being made.

Reading there rather than on the caller's connection is the part I did not take from your comment, and
the reason is worth stating: a select of the caller's transaction holds a lock on the catalog table
for the whole of that transaction, while the rows it decides are written from the other connection of
the same thread. A clear of this backend queueing between the two would then wait for the caller while
the caller waited for it - two sessions with no cycle the server can see, one of them blocked inside
the JVM. The cost is one physical connect per read-write open of a storage.

testFillingTheCatalogOfAnUpgradedBackendLeavesTheAttemptReplayable pins it in the same suite: every
table there, no catalog, and the write that creates and fills it is asserted to be replayed and to
have issued nothing on the caller's connection. The fixtures of that class now model a database of
named tables and hand out the connection the catalog uses; no assertion of any existing case was
touched, which is what made them worth keeping green.

issue (blocking) — the drop order pinned by an assertion nothing can fail. Right, and the map is
built with the catalog put last by hand, so it held of any loop at all. removeStorageFiles() drops
through a dropTable(con, tableName) of its own now, and testAClearDropsTheCatalogAfterEveryTreeItNames
records the order the loop actually drops in and asserts the catalog is last. Sorting the keys or
copying them into a HashSet fails it.

issue (blocking) — the four-engine evidence predates the merge. It does, and it is three rounds
old by now besides. Rather than paste a number from a run of an older head: every ubuntu job of this
repository runs those four suites (-P precommit, failsafe includes **/*TestCase.java, and the last
run reported Skipped: 0), so the evidence that matters is the CI of this head, and the table in the
description says so and points at it. Locally I ran what needs no database - JDBCStorageRetryTest,
CachedConnectionTestCase, StampConnectionTestCase, 136/136 - and the aggregate javadoc gate of
#899, which had never actually run on this branch: the build was failing at failsafe before it got
that far.

suggestion — isExistsIndex asks the whole server. Narrowed like every other lookup: it takes the
catalog of the scope and reads the schema of each row back through it. You were right that the PR text
claimed more than the code did.

suggestion — say that a read-write open now requires CREATE. The failure names the table and
says it holds the catalog, and the "upgraded in place" paragraph names the new requirement.

---

The round-4 blocker: schemaOf(con) against the resolution rule

Answered, and by the first of your two options rather than the second. Qualifying the statements with
current_schema() would have made the question and the statement agree by making both of them wrong
for the installation you traced: the tables stay in public, and a backend that qualifies everything
with opendj creates a second, empty set in front of them.

So the scope is now the database and the path an unqualified name resolves in:
current_schemas(true) on postgres, the default schema and then dbo on sql server, the current
schema on oracle, and the database itself on mysql - asked of the connection rather than assumed, so
that databaseTerm=SCHEMA narrows by the schema where the default narrows by the catalog. The
database half stays narrow, which is what it was narrowed for; the schema half never narrows to one
name. Listings are asked with no schema pattern at all and read back through the path, which also
retires the _-as-wildcard hole on that side.

PgSqlTestCase.testAClearFindsATableOfAnotherSchemaOfTheSearchPath is the case you asked for: the
tables are created in public, a schema is put ahead of them (through currentSchema= on a
connection string of its own, since the pools are keyed by it and an ALTER ROLE would leave every
pooled connection resolving as before), the fixture asserts current_schema() really is the new one,
and then the clear has to find and drop the table in public. It fails on the previous head.

The two non-blocking items of that round are in as well: the leftovers == null case has a line of
its own instead of being swallowed by the dropped == 0 guard, and a connection that will not say
where it works is logged and asked again rather than latched - once per transaction, not once per
tree.

---

Merged with #881, which changed more than the text

Two conflicts in JDBCStorage, neither only textual, and one case of #881 that the catalog was
failing. It is all written out in the merge commit and in the description; the short of it:

• isExistsTable(TreeName) keeps this branch's scope in the readable transaction [#873] Give each backend its own compressed schema trees #881 moved it to,
  and asks about the non-enrolling readTableName - a probe of a tree this backend does not own must
  leave no trace of it anywhere.
• The two names of [#873] Give each backend its own compressed schema trees #881 are followed wherever this branch names a table: the owner takes the enrolling
  one, the asker takes the other.
• testProbingATreeDoesNotPutItUpForRemoval gave both storages the same backend id. That was two
  backends only while ownership was a map in the process; a catalog is named after the backend id and
  outlives the open that filled it, so the second storage was being shown - correctly - the tree its
  own earlier open had enrolled. It is a backend of its own now, which is what the case says it is.
• testTheSharedCompressedSchemaTreesAreNamedButNeverCleared drops the legacy pair when it is done.
  Round 4 was right to rule that out when the pair belonged to the class-wide backend; since [#873] Give each backend its own compressed schema trees #881 no
  backend of the class makes it, so the tables are this case's own - and left standing they fail
  testCompressedSchemaTableIsQualifiedByBackendId whenever TestNG runs it second.
• isOwnTree() knows the pair [#873] Give each backend its own compressed schema trees #881 names after the backend id, so a clear no longer reports two
  tables carrying this backend's id in their stamp as tables nothing can be said about.

A last pass over the new connection tightened four things worth naming: it is established the way a
pooled connection is (the bounds of CachedConnection.ConnectDialect, READ_COMMITTED, and the read
bound of the login lifted only where this code set one - a bound of the connection string is the
administrator's); unenrolFromCatalog() caught SQLException alone while deleteRow() reports a
failed statement as a StorageRuntimeException, so its rollback was never reached; the table a
catalog row records is checked for the shape of a name and not only for its namespace, since it
reaches a drop table by concatenation; and the postgres probe of the search path runs behind a
savepoint, so a query an engine turns out not to have cannot poison the caller's transaction.

[vharseko]

The CI of this head came back, and it is worth reading twice

The last round answered your third blocker by pointing at this PR's own checks rather than at a number
from an older head. Run 33630195947
on 58c6876 is that run, and it says two things.

What it settles. All five ubuntu jobs got through the four engine suites against real databases:
MsSqlTestCase, MySqlTestCase and OracleTestCase 72 of 72 each, PgSqlTestCase 72 of its 73, with
Skipped: 0 everywhere — so no container was quietly missing — plus JDBCStorageRetryTest 67,
CachedConnectionTestCase 64, StampConnectionTestCase 5 and EncryptedTestCase 35. Of 32152 cases in
the module there was exactly one failure. The catalog, the ordering of the enrolment and the drop, the
scoped lookups and the merge with #881 are all inside that green, on four engines, on five JDKs.

What it caught. The one failure is not a database saying no. It is the harness:

RuntimeException: The @Test annotation for class PgSqlTestCase must include sequential=true
  to ensure that tests for a single class are run together
    at TestListener.enforceTestClassTypeAndAnnotations(TestListener.java:641)
    at TestListener.onTestStart(TestListener.java:356)

TestListener resolves that annotation from the class declaring the case, not from the one running
it, and it checks each declaring class once. Until this branch every case of the four engine suites was
declared in the abstract TestCase, whose nearest class-level annotation is the
@Test(groups = ..., sequential = true) of PluggableBackendImplTestCase; PgSqlTestCase declared
nothing of its own, so the bare @Test it has carried since it was written was never the one consulted.
testAClearFindsATableOfAnotherSchemaOfTheSearchPath — the postgres case you asked for in round 4 — is
the first case declared in PgSqlTestCase itself, and with it that bare annotation becomes the one that
answers: sequential defaults to false, and the case dies in onTestStart before its first statement.
OracleTestCase has carried sequential = true ever since it grew a case of its own
(test_issue_496_2); d759363 gives PgSqlTestCase the same one word, with the reason written down
beside it.

Nothing else moves with it. The annotation names no groups on this branch either, and the precommit
profile selects by the failsafe includes (**/*TestCase.java) and never by group, so no case changes
suite, order or fork. MySqlTestCase, MsSqlTestCase and EncryptedTestCase carry the same bare
@Test and are green for exactly as long as they declare no case of their own; they are left alone
rather than changed on speculation.

What is still unproven, and I would rather say it than let the table imply otherwise. The
search-path case has never run — not on this head and not on any other, since it failed ahead of its
body on every job of that round. Everything the run supports is in the description's suite table, split
so that PgSql's 73rd is not counted among the passes; the run now on d759363 is the one that either
proves the case or gives you a real postgres failure to read. The product diff of this round is empty:
one annotation and the comment explaining it, in a test class.

The five items of round 5 and the round-4 blocker stand answered as of the previous comment; this round
adds nothing to them and takes nothing back.

[vharseko]

@maximthomas #877 (#882) has landed and this branch is merged with it, f305785. One file conflicts, JDBCStorage.java, and every marked conflict is the same shape: a statement #882 gave a class to is a statement this branch rewrote. The two decide different things — how long a statement may take, and which table it names or how it is issued — so each site takes both. The description has the detail; the three marked ones are mechanical once put that way:

• removeStorageFiles() keeps the catalog-driven loop; the BULK class of the drop it replaces is given once inside dropTable().
• The readable transaction keeps its delegation to the scoped lookup; the bounded(con, OPERATION, …) you put on the body moves with the body, hard-coded rather than taken from the transaction — the rule JDBC backend: no statement is given a query timeout, and the read bound of an established connection is gone too #877 states for the catalog lookups of openTree().
• isExistsIndex() takes the scope narrowing of this branch and the operation bound of [#877] Bound a statement of the JDBC backend by the class of the work it belongs to #882.

The two the merge did not mark are the ones worth reading.

readCatalogRows() did not compile

It read from a live ResultSet through the one-argument executeResultSet(), which #882 removed — that removal being what puts the row transfer inside the bound. No textual conflict in either branch; a compile error in the merge. The rows are read inside the handler now, exactly as #882 converted every other such site, and take the same operation class those took.

createCatalogTable() would have been bounded as a client operation

Its create table goes through execute(statement). Before #882 that carried no bound at all; after it, the one-argument form means OPERATION. So the merge silently puts a create table under the bound of an entry read — nothing conflicted, nothing failed, and it is precisely the failure #877 argues against for DDL.

Master has no bare execute() left: every site there names its class, and every create table of this backend is BULK through commitStatement(sql, true). This one is BULK for the same reason.

Called out rather than folded in, because it is the merge choosing for a statement neither branch conflicted over. If you would rather it stayed at the default and were filed instead, say so and I will revert that line.

State

mvn -pl opendj-server-legacy test-compile is green. JDBCStatementBoundTestCase 37/37, JDBCStorageRetryTest 66/66, CachedConnectionTestCase 64/64, StampConnectionTestCase 5/5, BulkCursorTest 12/12, PersistentCompressedSchemaTest 8/8 — 193/193. The four engine suites have not been re-run since this merge; CI is running them now.

The points of your last round are answered in d759363, which is behind this merge and unchanged by it. Re-requesting review.

[maximthomas]

praise: The round-4 blocker is fixed properly, not papered over. Replacing the current_schema()
narrowing with a real search-path TableScope makes the question match the resolution rule of the
statements it guards, which is the right shape. Moving the catalog onto its own connection is also
the right call: an enrolment the caller's transaction cannot roll back is what keeps a committed
drop from outliving the row that named it.

The regression test does its job — testAClearFindsATableOfAnotherSchemaOfTheSearchPath goes red on
f51a1c14 (at the listTrees() assertion, before the clear it is named for) and green here.

The TestNG diagnosis in opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/PgSqlTestCase.java
is correct, and I checked it because two of my own reviewers said it was backwards.
BaseTestMethod has m_methodClass = com.getDeclaringClass() and getRealClass() returns it, so
enforcement really does resolve @Test from the declaring class. Leaving MySqlTestCase,
MsSqlTestCase and EncryptedTestCase alone is right — they declare no case of their own, so their
bare @Test is never consulted. Saying plainly in the PR comment that the case had never actually
run is worth more than the green table would have been.

Also right: the #881 re-justification of the hand-drop in
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/TestCase.java. Round 4's
objection genuinely dissolved when each backend got its own compressed-schema pair — the drop is
correct now and should stay.

---

issue (blocking): The catalog connection ignores the configured connect timeout.

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

newCatalogConnection() bounds its login with the literal DEFAULT_CONNECT_TIMEOUT_SECONDS (30) and
never reads org.openidentityplatform.opendj.jdbc.connect.timeout, which the pool reads on every
borrow (CachedConnection.java:636) and applies as min(property, pool.timeout)
(CachedConnection.java:719-729). With property=0 and pool.timeout=0 the pool sets no connect
bound at all (:907).

// JDBCStorage.java:569 — always 30, whatever the operator configured
dialect.bound(connectionString, properties, CachedConnection.DEFAULT_CONNECT_TIMEOUT_SECONDS)

An operator who raised the property to 120 because the login takes ~45 s — the only reason anyone
touches it — gets a pooled connect at 60 s that succeeds and a catalog connect that dies at 30 s.
SQLState 08001 is not class 40, so write() does not replay: enrolInCatalog:2354 →
openCatalog:2384 → :2435 → StorageRuntimeException → ERR_OPEN_ENV_FAIL. The backend stops
opening on an install that started fine before this round.

The javadoc at :560-562 calls the 30 s deliberate, but it argues for having a bound, not for
ignoring the configured one — and the commit is titled "Establish the catalog connection the way a
pooled one is established". Read the property the pool reads, and honour 0 as "no bound".

---

issue (blocking): The catalog connection has no deadline of any kind; a contended catalog table
hangs startup silently.

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

Three deadlines are missing at once:

• executeSessionStatement(con, dialect.lockTimeoutSql) has exactly one call site — :527, in
  newStampConnection. The catalog connection gets no session lock timeout.
• :584 restores setNetworkTimeout(..., 0) for the life of the connection.
• No catalog statement sets a query timeout (the only setQueryTimeout in the file is :1008,
  updateTableStatistics).

So the select :2426, create table :2458, upsert :2359 and delete :2506 all wait forever.
PostgreSQL and SQL Server have no server-default lock timeout. Run an offline
import-ldif --clearBackend on the same database while a server starts: its removeStorageFiles
drops the catalog table holding an exclusive lock, and the starting server blocks on the enrolment
inside RootContainer.open's single write, under synchronized (catalogLock). Start never returns,
nothing is thrown, nothing is logged.

// JDBCStorage.java:527 — the stamp connection, four lines above, does this
executeSessionStatement(con, dialect.lockTimeoutSql);
// newCatalogConnection() issues no session statement at all

The javadoc's "waits for its lock rather than dying on a read bound" is a fair argument for a
bounded wait; unbounded on a startup path is a different thing. Either give it the same session
lock timeout, or bound the four statements.

Counter I ruled out: the caller's pooled connection is equally unbounded, so the shape pre-exists —
but the catalog table is a new cross-process contention point, and the clear takes it on a
connection of its own.

---

issue (blocking): The new PostgreSQL case covers the lookup half of the blocker; the
shadowing-create half its own javadoc names is asserted by nothing.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/PgSqlTestCase.java:94-149

The fixture is right and the clear-drops-nothing half is covered. The javadoc at :86-87 names a
second harm — "the next open would create a second, empty set of tables in the schema ahead ... they
would shadow the populated ones" — and that is a different call site: openTree's
if (!isExistsTable(treeName)) create table, not the lookup feeding listTrees/removeStorageFiles.
The case never calls openTree on the ahead-resolving storage and asserts nothing about
opendj_ahead. Re-narrow only that guard and this case stays green while the populated table in
public is orphaned — the more destructive of the two halves, pinned by nothing.

Four lines, before the clear, while the populated table still stands:

storage.write(new WriteOperation() {
    @Override
    public void run(WriteableTransaction txn) throws Exception {
        txn.openTree(tree, true); // must find the public one, not create a second
    }
});
assertFalse(isExistsTableInSchema(AHEAD_ON_THE_PATH, tableName),
    "the open created a second, empty table in the schema ahead, shadowing the populated one");

---

issue (non-blocking): A read-write open now needs a third physical connection, and unlike the
second one its failure kills the open.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2266 and :2317

Within one openTree(createOnDemand=true) the catalog connection opens first and the stamp second;
both are per-transaction fields closed only in write()'s finally (:1741-1745), while the caller's
pooled connection is held throughout. Sizing the pool to the database's connection limit is ordinary
tuning, so the case that matters is a server that gets its pooled connection and no more. Before this
round the extra connection was the stamp — commentTable:827-839 caught the failure, warned, and the
open completed unstamped. Now the catalog connect throws first and becomes ERR_OPEN_ENV_FAIL.

Worse, the retry is asymmetric: the pooled borrow retries a limit refusal up to pool.timeout
(CachedConnection.java:646-676, isWorthRetrying); newCatalogConnection makes one DriverManager
attempt with no retry and no backoff, so it loses a race the pool beside it wins.

Pool it, or give it the pool's retry. Not the stamp's swallow — a catalog silently not written is
#888 again.

---

issue (non-blocking): A catalog row whose table name fails the new validator is dropped from the
read, so that tree's table survives every clear.

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

if (!isOwnTableName(tableName)) { logger.warn(...); continue; }

readCatalogRows is the shared body of catalogTables() (feeding removeStorageFiles and
listTrees) and readEnrolledTrees(). A skipped row is a tree the clear cannot see: neither the
opendj_<hash> table nor the row is dropped, and the clear's outcome report says nothing about it —
the #888 symptom, reached through this PR's own validator. And since listTrees() runs per
dbtest/backendstat invocation, the warn repeats on unrelated tool runs.

Nothing in this build can write such a row (toTableName always emits opendj_<hex>), which is why
this is non-blocking. Keep the skip — do not build drop table from an untrusted name — but count
those rows into the clear's outcome report alongside ours/unattributed/missing.

---

todo (non-blocking): Nothing in any test reaches the catalog connection's establishment or any of
its error paths.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:558-599, :608-664

A grep of the jdbc test package for newCatalogConnection|catalogSession|CatalogSession|catalogLock
returns nothing. The only non-container test that reaches newCatalogConnection is
JDBCStorageRetryTest via the jdbc:opendj-retry-stub: URL, and ConnectDialect.of returns null
for any unknown prefix — so readBoundSet is false and neither dialect.bound(...) at :569 nor the
setNetworkTimeout(executor, 0) restore at :584 ever executes.

Commit 58c6876 exists to bound that connect, and not one of its lines runs under test. That is why
the two blocking issues above survived five green CI runs.
testFillingTheCatalogOfAnUpgradedBackendLeavesTheAttemptReplayable comes closest but verifies
nothing on the catalog connection:

assertEquals(attempts.get(), 2, ...);
verify(statements, never()).executeUpdate();  // the CALLER's connection
// no verify(catalogCon).commit() — a regression that stopped enrolling entirely stays green

---

nitpick (non-blocking): The comment this PR added to the shared-schema case is now false and
argues for deleting a load-bearing drop.

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

// :1927 — written by 7498571, no longer true
// the pair holds the compressed schema of the backend this class opens in setUp(),
// which created these two tables
...
// :1955 — the finally, eight lines later, says the opposite and is the correct one
// since #881 each backend keeps its definitions in a pair of its own ...
// and the openTree above is what created these two tables

The second is right post-#881, which makes the hand-drop at :1959-1966 load-bearing —
testCompressedSchemaTableIsQualifiedByBackendId:369 asserts the shared table is absent and TestNG
may run it after this case. A reader who believes the first comment deletes the drop and breaks :369
non-deterministically. Delete or rewrite the stale one.

[vharseko]

@maximthomas 66579d3. Four of your six are fixed, one is a merge you reviewed one commit behind — with one instance of it that was real and is fixed — and the third connection is the one thing I am leaving for a decision. A code review of the same head turned up six more, which are in this commit too, and four things it found that I am naming rather than folding in.

The connect timeout (blocking) — fixed as asked

newCatalogConnection() reads the property the pool reads, through one method both go through now (CachedConnection.getConnectTimeoutSeconds()), and 0 is honoured as no bound of the connect. The javadoc that called 30 s deliberate is gone.

One thing your item did not have to say and I would rather write down than gloss: at a property of 0 the pool is still bounded by the deadline of the borrow, and this connect has no borrow to take one from. So a deployment that turns the bound off and meets a database which accepts the socket and never finishes the login parks inside the lock openCatalog() holds. That is in the javadoc now, stated as the difference it is rather than as parity.

"No deadline of any kind" (blocking) — mostly a merge you reviewed one behind

That round read 58c6876; f305785 brought #882 in. On this head the select of the catalog, its upsert and its delete go through the transaction of CatalogSession, which carries StatementBound.OPERATION — 120 s by default, with the socket read timeout of #882 behind it. Your start-up scenario therefore ends at that bound with the property named, rather than parking with nothing in the log.

What is left unbounded is the create table of the catalog, at BULK — the class #877 gives every DDL of this backend, create index and drop table included. I have left it there rather than making the catalog's DDL the one exception: say the word if you would rather it were OPERATION, or filed.

The session lock timeout is still deliberately absent, for the reason the javadoc gives (a catalog row is the state a clear reads; it queues for its lock rather than dying on a read bound), and the read bound of the login is still lifted once the login is through (#872), with the backstop of #882 arming per statement over it.

One instance of your point was real. TableScope.schemaPathOf()'s select unnest(current_schemas(true)) carried no bound at all — the one statement of this class outside the scheme of #877 — and a server that never answers it held the open of a tree with nothing able to end the wait. It takes OPERATION now, like the lookups the scope it builds narrows. The savepoint and the getSchema() fallback beside it answer for a query an engine refuses, not for one it never answers; you were right that they are not a deadline.

The postgres case (blocking) — fixed

The case opens the tree on the ahead-resolving storage before the clear and asserts that no second table appeared in opendj_ahead and that the populated one in public is untouched. Asked of information_schema with the schema and the name bound rather than through getTables(), whose schema is a pattern — opendj_ahead would match opendjXahead as readily, _ being a single-character wildcard there.

The third connection (non-blocking) — open, and I want your call

Not done, and a code review of this head raised it independently, so it is the one item of yours I am carrying forward rather than answering. Pooling it is what the javadoc argues against — the caller of openTree() is holding a pooled connection already, and a pool that cannot open a second one waits for the very thread that is waiting — so what it wants is the pool's retry, not its deque: one DriverManager attempt loses to a database at its connection limit or still recovering (53300, 57P03 and the equivalents) where the borrow beside it waits it out for up to the pool timeout. Not the stamp's swallow, agreed: a catalog silently not written is #888 again.

I will give it the retry unless you would rather it were pooled outright.

The validator skip (non-blocking) — fixed

readCatalogRows() describes every row it passes over to its caller, and the clear reports them as a line of their own: what such a row records is outside the opendj names the leftover scan walks, and the row is dropped by nothing either, so no other line could name them. Not a term of the "dropped nothing" line — a catalog whose table is there always names itself, so the two cannot co-occur — and that is written down beside it. Covered by testAClearAccountsForACatalogRowItCannotActOn on all four engines, which writes such a row by hand.

The todo — the catalog connection is under test now

CatalogConnectionTestCase, 5 cases, no database: the bound of the connect at a configured value, at the default and at 0, the set-up of the connection, and the close of one whose set-up failed. Your diagnosis of why nothing reached it was exactly right — ConnectDialect.of answers null for the retry stub's prefix, so dialect.bound(...) never ran. The probe here is answered a postgresql url pgjdbc cannot parse (a non-numeric port), so DriverManager walks past it to the probe while the prefix is still read as postgres. The two cases about the bound go red on 58c6876: connectTimeout … expected [120] but found [30].

JDBCStorageRetryTest now verifies the create, the row and the commit on the catalog connection. You were right that assertEquals(attempts, 2) and verify(statements, never()) held just as well of a storage that enrolled nothing at all.

The nitpick — fixed

The stale comment is gone; the finally was the correct one and the hand-drop stays.

What a code review of this head added

• reset() of both sessions, enrolInCatalog(), readEnrolledTrees() and createCatalogTable() caught SQLException alone, where unenrolFromCatalog() had always taken both. An unchecked failure skipped the rollback and left the connection every remaining tree of that open writes on in 25P02 — and in createCatalogTable it skipped the tolerance of a table another session had just created.
• A failed catalog connect goes through CachedConnection.reported(): the connection string of a backend carries a password, and this failure is logged in full inside ERR_OPEN_ENV_FAIL.
• The backend id is escaped into the name of the catalog tree, as PersistentCompressedSchema escapes it into its own prefix. TreeName.valueOf splits on the last slash, so an id carrying one named a tree that does not survive being read back — and a clear could not recognize the stamp of its own catalog table. An ordinary id is unchanged, so no table is renamed.
• listTrees() borrows a validated connection: answering from the catalog put it on the list getValidatedConnection() documents.
• readEnrolledTrees() asks through the non-enrolling name of [#873] Give each backend its own compressed schema trees #881, and two statements about tree2table this branch had made stale are corrected.

Four things it found that I am naming rather than folding in

• A clear that dropped only its catalog table prints no explanation: the line is gated on dropped==0, and a catalog table that is there while its rows are not makes dropped one. The condition wants to be "nothing but the catalog".
• unenrolFromCatalog() takes the tree out of the memo before it deletes the row. A concurrent openTree of that tree in between finds it absent, writes and commits its row, and the delete removes what was just written — a table nothing names, which is the state this change exists to prevent.
• leftoverTables() can name one table twice where two schemas of the search path hold that name, the single stamp read resolving whichever the path reaches first.
• The connect retry above.

The first three are small and I will take them in the next round together with the retry, unless you would rather they were filed.

Tests

suite	where	result
CatalogConnectionTestCase, JDBCStorageRetryTest, StampConnectionTestCase, CachedConnectionTestCase, JDBCStatementBoundTestCase	locally, -P precommit verify	178/178, Skipped: 0
the two cases about the bound of the catalog connect	locally, against 58c6876	red
PgSqlTestCase	locally, against a container	76/76, Skipped: 0 — the search-path case among them, which had never run until this round
MySqlTestCase	locally, against a container	75/75, Skipped: 0
MsSqlTestCase, OracleTestCase	CI on this head	running