JDBC backend: the DDL of openTree waits for a lock with no bound, and an established connection has no read bound

[vharseko]

Describe the bug

What is left waiting on the database with no bound once #876 (the connect), #882 (the statements) and #886 (the catalog lookup) are in. The first of these is the one that can still hang the open of a backend outright; the second is what the rest of them need.

Both live in CachedConnection, in code #876 introduces, which is why no branch off master has reached them yet.

1. The DDL of openTree waits for a lock with no bound

openTree(createOnDemand) issues create table and create index, and deleteTree() / removeStorageFiles() issue drop table. #882 puts all of them in its bulk class:

// opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java
BULK("org.openidentityplatform.opendj.jdbc.bulk.timeout", 0);   // unbounded by default

A class that is legitimately long — it also holds select count(*) and the delete from that empties a tree before an import — so bounded() returns before setting anything at all: no cancel, no socket read timeout behind it. Which leaves the one part of this backend that takes locks as the one part with no bound of any kind, and the engines wait a long time for a lock by default:

Engine	Lock the DDL takes	Default wait
MySQL	metadata lock	lock_wait_timeout = 31536000 s (a year)
SQL Server	Sch-M	LOCK_TIMEOUT = -1 (forever)
PostgreSQL	table lock	lock_timeout = 0 (forever)
Oracle	DDL lock	ddl_lock_timeout = 0 — gives up at once

So the open of a backend can wait forever on three engines out of four, behind an unrelated transaction of another session on the same database, and neither #876 nor #882 covers it: the connect is through and the statement class is unbounded.

It is not a rare path either. On PostgreSQL the index statement is create index if not exists, issued on every openTree(createOnDemand) with no check in front of it — about 25 DDL statements per backend open for a stock suffix. CREATE INDEX takes a SHARE lock on the table, so a backend opening behind a long transaction does not only wait: it queues in front of every writer of that table for as long as it waits.

#866 makes exactly this argument for the comment statements it adds, and bounds their lock wait per dialect. The DDL that has been in openTree() all along was never given the same treatment.

A query timeout is not the tool here, and that is the point of a lock timeout: it tells a statement that is working from a statement that is queued, which is why the bulk class can stay unbounded and still not hang.

Where it goes. A lock timeout is a session setting and a pooled connection cannot carry one — CachedConnection.close() only rolls back, so it would leak to the next borrower. Once per physical connect, in CachedConnection, with one uniform value is the clean place, and it needs the value to sit below the query timeout of #882 so that the lock timeout is what fires first. Per dialect: SET lock_timeout (PostgreSQL, ms), SET SESSION lock_wait_timeout and innodb_lock_wait_timeout (MySQL, s — metadata and row locks are two settings), SET LOCK_TIMEOUT (SQL Server, ms), ALTER SESSION SET ddl_lock_timeout (Oracle, s). PostgreSQL undoes a plain SET when the transaction that ran it is rolled back, so it needs a commit behind it — #866 documents that trap.

There is a narrower variant that does not wait for #876: bound the wait around the DDL statements only, on the caller's connection, putting the setting back afterwards. SET LOCAL lock_timeout is transaction-scoped on PostgreSQL and undoes itself, so nothing leaks there; the other three need an explicit reset, remembering that DDL is an implicit commit on MySQL and Oracle.

Opt-in or not is a real question, since each engine starts from a different default — Oracle gives up at once today, MySQL waits a year, the other two forever — so a uniform value changes behaviour in both directions.

2. The read bound of an established connection has no setting of its own

#876 bounds the login of a new connection with two driver properties per dialect, the second of which — socketTimeout on MySQL and SQL Server, oracle.jdbc.ReadTimeout on Oracle — is a socket read timeout for the whole life of the connection. It is lifted as soon as the login is through (CachedConnection.relaxReadBound()), because leaving it in place would fail every statement slower than it.

That is the right default, but it leaves a deployment no way to ask for one, and #882 only arms a socket read timeout for the duration of a bounded statement. Outside that window a connection whose peer has gone quiet still waits indefinitely: the rows a cursor fetches from a ResultSet after executeQuery() has returned, commit() and rollback(), the catalog reads of section 3, and every statement of the bulk class.

Wanted: org.openidentityplatform.opendj.jdbc.read.timeout, applied by relaxReadBound() instead of lifting the bound to 0. A read bound the connection string sets itself must keep precedence, as it does today.

Its default has to be 0 — that is a consequence, not caution. A standing socket read timeout has to exceed the longest silence the server may legitimately produce, which is the longest statement, which is the bulk class, which is unbounded by default. There is no non-zero value that does not contradict bulk.timeout=0.

Which leaves the deployment that does set it in the same bind: read.timeout=600 would cut every bulk statement at 600 s while bulk.timeout=0 promises the opposite. The way out is symmetrical to the backstop of #882, in the other direction: lift the socket bound for the duration of a bulk statement and put it back afterwards. The bulk class says outright that this may legitimately take long, so it earns the wider window, and the read timeout keeps covering everything else.

The composition with #882 needs nothing further: since 3da33eab9a its backstop only ever tightens, so a read.timeout stricter than the backstop is left in force while a statement runs, and a looser one is put back after it.

3. A consequence: the catalog reads and the commit

DatabaseMetaData.getTables() in isExistsTable() and getIndexInfo() in isExistsIndex() take no setQueryTimeout, and neither does Connection.commit() — the commit of an import is not a quick one. None of them goes through the bounded path of #882.

This is no longer work of its own: #886 made the catalog lookup a single indexed row, so "slow" is not a state it reaches any more, and what remains — a socket that has gone quiet — is exactly what the read timeout of section 2 covers. It closes with that one.

Impact

Section 1 is the open of a backend, and dsconfig create-backend-index on a running server, hanging behind an unrelated transaction of another session — the symptom of #872 and #877 through a door neither of them covers, and on PostgreSQL blocking the writers of that table while it waits. Section 2 is every other read on a connection whose peer stopped answering after the login went through.

Expected behavior

• the lock wait of the DDL is bounded, per dialect, below the query timeout of [#877] Bound every statement of the JDBC backend by the class of its call site #882, so a statement that is queued is told from a statement that is working;
• a read timeout for an established connection is configurable, defaulting to today's unbounded behaviour, with the bulk class running with it lifted;
• with those two in, no path of this backend waits on the database without an end.

Environment

master (5.2.x), all four JDBC dialects (PostgreSQL, MySQL, Oracle, MS SQL Server). Both sections depend on #876 being merged, except for the narrower variant of section 1 described above.