Skip to content

[#915] Bound the wait of a write transaction for a row lock another session holds - #1010

Open
vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/915-write-lock-bound
Open

vharseko wants to merge 1 commit into
OpenIdentityPlatform:masterfrom
vharseko:issues/915-write-lock-bound

Conversation

@vharseko

@vharseko vharseko commented Sep 9, 2026

Copy link
Copy Markdown
Member

Fixes #915, and with it #903 on the three engines that have a setting for this wait.

Rebased onto master, which now carries #936, #904, #999 and #1003, with round 1 answered in the same commit: the diff is the change alone, in one commit. armLockBound() is #936's withDdlLockBound() split into an arm and a release, so that a bound can be held around a whole transaction rather than around one statement; how the change sits on #904 is its own section below.

Problem

JDBCStorage.write() bounds its replays twice - by MAX_RETRIES and by a 10 s wall-clock window - and a clock bounds replays only while an attempt is shorter than it. What makes an attempt long is the lock wait ahead of the conflict, charged to the attempt that hit it, and nothing this backend sets ends that wait:

  • LOCK_TIMEOUT is -1 on sql server, lock_timeout is 0 on postgres, and oracle's row-lock enqueue is unlimited;
  • mysql bounds it itself, at innodb_lock_wait_timeout - 50 s, five times the window, which as far as the window is concerned is the same thing.

So an attempt alone outlasts the window and the operation gets no replay at all, however transient its conflict. That is #903, and the CI failure it was filed on: a sql server deadlock victim picked ~12 s into the first attempt of testConcurrentWritersInsertingDistinctKeys.

StatementBound.OPERATION does bound such a statement today, at 120 s - twelve times the window, and its failure is a cancel that isConflict() does not match, so it is not replayed either.

Change

org.openidentityplatform.opendj.jdbc.row.lock.timeout, in seconds, default 3. The session of a write attempt is told to give up on a row lock at that value, and is given its own value back before the connection goes back to the pool. 0 or negative leaves the wait exactly where it was; a value that is not a number keeps the default, and one past MAX_BOUND_SECONDS is clamped, through the same boundSeconds() every bound of this backend reads.

The default is a third of the window on purpose: an attempt that ends at this bound spends that much of it, so the replays a conflicted write gets are the window divided by this value - three at both defaults. A bound at or past the window leaves exactly one attempt, which is the shape #903 describes; the suite asserts that invariant rather than leaving the two constants to drift apart.

Per engine, in the unit its own setting takes:

Engine read back first set around the transaction put back afterwards
PostgreSQL nothing a savepoint, set local lock_timeout = <ms>, and the savepoint let go of nothing - a set local goes with the transaction
MySQL select @@session.innodb_lock_wait_timeout set session innodb_lock_wait_timeout = <s> the value read back
SQL Server select @@lock_timeout set lock_timeout <ms> the value read back
Oracle nothing nothing

MySQL gets innodb_lock_wait_timeout and never lock_wait_timeout. The first is the row lock a write waits for; the second is the metadata lock a DDL waits for, which is what #936 sets and which bounds nothing of this. Two settings, two defaults - the issue names Dialect.lockTimeoutSql as if it already carried this statement for all four engines, and on this engine it carries the other one.

Oracle is left alone because it has nothing to set. ddl_lock_timeout is the DDL lock, distributed_lock_timeout the distributed transaction, and the only place the wait of plain DML can be named is select ... for update wait n, which this backend does not issue. An attempt there stays bounded by StatementBound.OPERATION alone, whose cancel a session blocked in that enqueue does not act on - what ends such a wait is the socket read timeout, which takes the connection with it. On oracle what answers #903 is therefore not this property but the grant of #904 (see below), and the property's javadoc says so rather than implying four engines are fixed.

The bound is scoped to the transaction, never left on the pooled session. That is the placement #936 rejected for the DDL and rejects here for the same reason: postgres and sql server bound every lock wait of a session, and a read borrowing the connection next replays nothing at all, deliberately (ExportJob and VerifyJob are not idempotent), so it would see the 55P03 or the error 1222 as a hard failure. Armed after the connection is borrowed and taken off in the finally that closes the stamp and catalog sessions - after the rollback of a failed attempt, before the release hands the connection on, and not at all on a connection the database dropped, which carries nothing to anybody and which the pool has already been told to distrust. A DDL of the same attempt ends that transaction by committing, and commitStatement() takes the bound off there rather than leaving it on: on postgres the set local goes with that commit by itself, and the two engines whose setting is the session's are given the same treatment. What follows such a DDL is a write that has committed part of its work and is out of the replay whatever it waits for, and a bound on a wait nothing can replay only fails a write at 3 s where it used to wait for the blocker and go through.

A lock timeout is replayable only where this backend bounded that wait. 55P03 and error 1222 are no conflict: the engine rolled nothing back and the blocker is still holding the lock. What makes them replayable is the rollback write() issues before it asks, and what makes them worth replaying is that the wait they cost fits inside the window. Where nothing bounded that wait - the property at 0, oracle, a session that would not take the setting - they stay exactly as unreplayable as they were: a bound an operator set for themselves is not a licence for this loop to take that wait again. ArmedLockBound.bounded is that answer, and replayReason() reads it rather than re-deriving it from the property. Such a failure is deliberately no class of #904's Conflict: its verdict stays NONE, so it is replayed on the window alone and never granted past it.

A mysql lock wait timeout arrives in class 40 and stays the conflict it has been since #867; it is now reported at 3 s instead of 50 s, so it is replayed within the window rather than never.

The readback is paid once per pooled connection. This bound is armed around every write of the server where the DDL bound is armed around an open, so the value a session carries is remembered on the CachedConnection: only this backend writes that setting, every write puts it back, and a connection whose restore failed is kept out of the pool - so a remembered value cannot outlive the session that answered it. The DDL bound still reads every time, because it runs inside a write that may be carrying this bound on the very same setting (sql server has one LOCK_TIMEOUT for both). What a write costs on top of what it cost before is two round trips on mysql and sql server, three on postgres - the savepoint, the setting, and the savepoint let go of again, since a subtransaction left open would span every write and put its own xid on every row that write touches - and none on oracle.

A DDL inside a write keeps its own property. sql server has one LOCK_TIMEOUT for every lock wait of a session, and every DDL of this backend but the off-write catalog drop is issued from inside a write - which armed this bound one statement earlier. Deciding "the session already gives up sooner" against what the readback answers would decide it against a bound of ours: the DDL would answer alreadyTighter to its own 5 s, run at the 3 s of the row bound, and give up with the bare error 1222 rather than the failure #936 renames - DDL_LOCK_TIMEOUT_PROPERTY governing no DDL of a write at all, the defaults included, and an operator raising it for an index build blocked by long readers still getting 3 s. What that decision reads is therefore the value the row bound displaced, remembered on the CachedConnection - and only where one setting carries both waits (Dialect.oneSettingForBothBounds(), true of sql server and postgres). On mysql they are two variables with two defaults, and reading one for the other would leave a metadata lock waiting a year because a deployment had tightened the row one. The live readback stays what the release puts back: what a session has to be given back is what it carried a statement ago, bound of ours or not.

Deciding it that way is only half of it, since a decision does not put anything on the session. Where the deployment's own value is the tighter one - a LOCK_TIMEOUT between the two bounds, or a DDL bound raised for an index build over a deployment that bounds its sessions - "the session keeps what it has" would leave the DDL at neither figure, the row bound of the write being tighter than both. That value goes back on for the length of the DDL and the row bound is put back behind it; a failure under it is handed through exactly as it arrived, since what ended that wait was the deployment's setting and naming DDL_LOCK_TIMEOUT_PROPERTY for it would send an operator to raise a value that governs nothing there. Where the DDL bound is turned off altogether the row bound comes off in front of the DDL instead (commitStatement()), so that 0 leaves the wait as unbounded as it was before either bound existed - which is the whole of what that value asks for. mysql and oracle are untouched by both: two variables there, and none here.

One mechanism, two bounds. LockBound is the enum of the waits this backend bounds - DDL and ROW - carrying the property, the default and what each engine is told; armLockBound()/releaseLockBound() are #936's helper split in two, and withDdlLockBound() is now three lines over them. The warn-once latches are per bound rather than per storage - the one #1003 reports an unrecognised driver through among them: reported through one latch, the first open that could not take its bound would silence every write of that backend, and a backend opening on a mariadb, percona or aurora driver would say nothing about the row locks of the writes behind it.

What it costs a deployment

A write blocked behind a long writer of another session now fails once the window is spent, where it used to wait as long as it took and then succeed. That is the trade this bound is, and it is the reason the property exists: an operation held for the length of somebody else's transaction cannot be told from one that is never coming back, and the caller of a write is an LDAP client with a timeout of its own. A deployment that would rather wait sets the property to 0 and keeps the old behaviour - #903 along with it. Behind a DDL of its own a write is out of this bound as well as out of the replay, and waits for its row lock exactly as it did before this change.

What it does not change

  • the failure a caller finally sees, where the replays run out: it is the engine's own 55P03 / 1222 / class 40, as before. The DDL rewrite that names the property is [#885] Bound the wait of a JDBC DDL for a lock another session holds #936's and stays on the DDL path - naming a property in a failure whose transaction did minutes of work before it queued would claim more than the elapsed time can prove;
  • read(), which arms no bound and replays nothing;
  • the importer, which holds a connection of its own and arms nothing on it - clearTree()'s delete from, the one StatementBound.BULK statement taking row locks, is reached in src/main from that road alone. Issued inside a write it runs under this bound like every other statement of that transaction, since the bound belongs to the transaction rather than to a statement class;
  • MAX_RETRIES, RETRY_WINDOW_NANOS and the retry delay, all unchanged. RETRY_WINDOW_NANOS is package-private now, so the suite can assert the one invariant tying the two constants together.

Relationship to #904

#904 is the other answer to #903, and it landed first: it grants the first replay of a prompt conflict because "no clock can bound a wait that nothing else bounds". This PR sits on its machinery rather than beside it and leaves all of it in place - the Conflict classes, the one walk of conflictVerdict(), replayableWithin() and grantedPastTheWindow().

The two fit together like this. A wait this backend bounded ends in a failure that is no conflict - 55P03, error 1222 - so its verdict is Conflict.NONE: replayReason() replays it on the strength of ArmedLockBound.bounded, and replayableWithin() then holds it to the window with no grant, since the grant exists for the waits nothing bounds and this one is not. A mysql lock wait timeout stays AFTER_LOCK_WAIT, refused the grant as before, and now arrives at 3 s rather than 50 s, so the window governs it. What the grant is left with is exactly what this bound does not reach - oracle, the property at 0, a session that would not take the setting, and a driver dialectOf() does not recognise, under which nothing is armed - where the wait is as unbounded as it was when #904 was written, and one replay past the window is what remains of #903's answer there.

#904's comments described #915 as a future change - "#915 adds one" class, "#915 removes the trade", "#915 retires this method" - and they are brought up to date here rather than left describing a plan that did not happen: the row lock wait is no Conflict class, the trade of UNKNOWN_ENGINE stays because the bound arms nothing under an unrecognised driver, and the grant is kept rather than retired, for the engines and settings above. Nothing of #904's behaviour changes: its cases in JDBCStorageRetryTest pass as they are, the test-side replayReason() helper handing the widened signature the bound its cases were written under.

Testing

JDBCRowLockBoundTestCase - 36 cases, no database: what each engine is told and in what order, the property and every misconfiguration of it, a session already tighter (mysql at 1, sql server at 0 against its -1), a session that will not say what it carries, a setting refused, a restore that fails and the connection kept out of the pool, postgres in auto-commit and the savepoint in front of the setting, the readback paid once per pooled connection and every time off it, and the replay verdict for a lock timeout with and without a bound of ours - including one reported by commit(), one on an attempt that had committed part of its work, and one read from the release of the connection.

Four of them are round 2: a DDL armed inside a write on sql server, over a connection answering the readback with what the last setting left on it - the order of the statements that takes, the DDL's own property in the middle of them, and the rename of a 1222 raised under it - and one case per warn-once latch, each asserting that the bound which was not armed has said nothing.

Six are round 3: the same sql server fixture at a deployment LOCK_TIMEOUT of 4 s and at 10 s against a DDL bound raised to 30, where what goes on the session for the DDL is the deployment's own value and what comes back behind it is the row bound of the write; a 1222 under that value handed through without this property's name on it; a mysql CachedConnection answering each readback its own value, so that the arm reading innodb_lock_wait_timeout for a metadata lock is visible at all; and the savepoint of a postgres arm let go of where the setting went on, taken back to where it did not.

JDBCStorageRetryTest - 110 cases, eight of them driving write() end to end through the stub driver: a 55P03 replayed with the bound armed once per attempt, the same failure not replayed with the property at 0, and - round 2 - the row bound put back where a DDL of the attempt commits, read off the order of the statements that attempt issued, once and only once. Round 3 adds five: a DDL told to wait (the property at 0) meeting the row bound taken off in front of it, on postgres and on sql server; a DDL with a bound of its own left exactly where it was; a create index that gave up with 55P03 inside a write replayed on the copy of the bound the attempt ran under; and the bound not put back at all on a connection the database dropped. Its existing cases, #904's among them, pass the bound they were written under (ArmedLockBound.none) through the test's own replayReason() helper, which composes the verdict the way write() does.

JDBCDdlLockBoundTestCase - 51 cases, #1003's among them: the drop of a tree runs inside a write that arms this bound first, so the set local lock_timeout = 3000 of the attempt precedes the search path and the DDL bound of that case; a driver this backend knows no engine for reports the row lock bound of a write on a latch of its own, rather than being silenced by the open that reported the DDL one; and - round 3 - the case pinning a setting that went through now asserts the savepoint in front of it was let go of as well.

TestCase - the container suites of all four engines get testAWriteBlockedByARowLockIsReplayedInsideTheWindow: another session takes an exclusive lock on the row a write wants and lets it go after twice the bound. The write must give up on its first attempt and go through on its replay, both inside the window - and on oracle, which has no setting for that wait, it must go through on the first attempt instead, which is the one place the difference this bound makes can be read. assertBoundedWhileRowsAreLocked - the harness of the two statement-bound cases of #877 - turns this property off while it runs, since a bound tighter than the one under test is what would end their wait.

testTheDdlGivesUpOnALockAnotherSessionHolds runs at a DDL bound of 7 s rather than 2: sql server has one LOCK_TIMEOUT for both waits, so a DDL bound tighter than the row one passes that case whether this backend armed the DDL bound or merely inherited the row bound of the write around it - and inheriting it is what round 2 fixes.

mvn -o -pl opendj-server-legacy -Pprecommit \
    -Dit.test='JDBCRowLockBoundTestCase,JDBCDdlLockBoundTestCase,JDBCStorageRetryTest,JDBCStatementBoundTestCase' \
    -Dfailsafe.failIfNoSpecifiedTests=false verify

The javadoc goal is deliberately not skipped in that command. attach-javadocs runs on every leg of the build workflow, for every module, under the root pom's doclint=all,-missing and failOnWarnings=true - and at an earlier head a @link still naming the five-parameter replayReason() this branch widened is what took build-maven (macos-latest, 11) and (windows-latest, 26) down. Those two legs only failed first: -P precommit is set on Linux alone, so they reach this module's package phase in ~2.5 min while the ubuntu legs are still in integration tests, which would have failed on the same line.

At this head, with the javadoc goal on: the four suites above, 242 run, 0 failures - and the postgres and mysql container suites, measured at the head of round 2 and untouched by the two rounds since, the case above giving up at the bound and replaying on both:

PgSqlTestCase   82 run, 0 failures
MySqlTestCase   81 run, 0 failures

sql server and oracle are left to CI at this head, as they were at the one before the rebase - where all four suites ran green, oracle waiting the holder out on its first attempt.

Ten mutants of round 3's answer, each measured at this head and each caught by the case written for it:

  • the row bound left on in front of a DDL told to wait -> 2 failures, the order of the statements on postgres and on sql server; the same lift made unconditional -> the DDL that has a bound of its own;
  • the deployment's own value decided against but never put on -> 2 failures, at the defaults and at a raised DDL bound; the rename left on bound!=null -> the 1222 of a wait that value ended arriving with this property's name on it;
  • the mysql arm of carriedByTheDeployment() dropped -> the metadata lock left at a year because the row lock had been tightened;
  • the transaction not recording that it carries no bound -> the value put back twice; the replay decided on the transaction's copy -> a DDL lock wait thrown at the first attempt;
  • the restore issued on a dropped connection -> the stranded-bound warning it makes; the savepoint left open, and the savepoint released on the road that still has to roll back to it -> one failure each, in opposite directions.

And the seven of round 2, each measured at that head:

  • the DDL bound inside a write decided against the live readback (the value the row bound displaced ignored) -> 2 failures: the order of the statements, and the 1222 of that DDL arriving without the property that ended it;
  • the readback memoised for the DDL bound as well as for the row one -> 1 failure, the second select @@lock_timeout of that same case;
  • the row bound left on where the DDL of the attempt commits -> 1 failure, the end-to-end order of JDBCStorageRetryTest;
  • one warn latch for both bounds, and one warn moment for both bounds -> 1 failure each, in opposite directions: the row bound silenced by an open, and the DDL bound silenced by a write;
  • the unrecognised driver reported through one latch for both bounds -> 1 failure, the row bound of a write left unsaid by the open in front of it;
  • and the four of round 1, unchanged: mysql told the metadata lock instead of the row lock -> 3 failures; replayReason ignoring bounded -> 3; write() arming nothing -> 2; the release never putting the value back -> 12.

@vharseko
vharseko requested a review from maximthomas September 9, 2026 17:51
@vharseko vharseko added bug jdbc concurrency Thread-safety / race-condition bugs tests Test suites: fixing, enabling, un-disabling labels Sep 9, 2026
@vharseko

Copy link
Copy Markdown
Member Author

The two red legs were maven-javadoc-plugin:attach-javadocs on opendj-server-legacy, not the change under review:

JDBCStorage.java:3901: error: reference not found
 * {@link #replayReason(Throwable, String, boolean, boolean, boolean)}.

replayReason() grew a Dialect and an ArmedLockBound in this branch, and the fully qualified @link in write()'s javadoc still named the five-parameter signature - the one that resolves on master. The line itself is unchanged, so it sits in the diff as context, which is how it got past me.

Fixed in 5b1db9a by reducing it to the bare {@link #replayReason} the other three references in this file already use: there is no overload to disambiguate, and it does not rot the next time the signature moves.

Two things worth recording, because the failure reads as platform-specific and is not:

  • attach-javadocs is bound in the root pom for every module and runs on every leg, under doclint=all,-missing and failOnWarnings=true. macOS and Windows only failed first: -P precommit is set on Linux alone, so they reach this module's package phase in ~2.5 min while the ubuntu legs are still in integration tests. The ubuntu legs of that run were queued, not green, and would have failed on the same line. (Set Aggregate Javadoc Goal is the Linux/java-11 step, so if anything ubuntu runs more javadoc than the legs that caught this, not less.)
  • -Dmaven.javadoc.skip=true in the Testing command of the description is what hid it locally. Dropped from the description, together with a stale 24 cases for JDBCRowLockBoundTestCase - the suite reports 26.

Verified locally under JDK 11 against the same plugin configuration:

run result
the broken link put back, mvn -o -pl opendj-server-legacy package BUILD FAILURE - JDBCStorage.java:3901: error: reference not found, the CI error verbatim
as pushed, same command BUILD SUCCESS, no javadoc error, ...-javadoc.jar built
as pushed, -Pprecommit -Dit.test=JDBCRowLockBoundTestCase ... verify, javadoc not skipped Tests run: 26, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS

The first row is there on purpose: without it a green run only proves the build passed, not that it ran the reference check at all.

@vharseko
vharseko requested review from maximthomas and removed request for maximthomas September 10, 2026 07:07
@vharseko
vharseko force-pushed the issues/915-write-lock-bound branch from 5b1db9a to 9b167eb Compare September 11, 2026 05:37
@vharseko

Copy link
Copy Markdown
Member Author

Rebased onto master, which now carries both #936 and #904: one commit, 9b167eb, and the diff is the change alone - the "sits on top of #936" preamble of the description is gone with it.

What the rebase had to decide is how this bound sits on #904, which landed first. Nothing of #904 is changed or removed. A wait this backend bounded ends in a failure that is no conflict - 55P03, error 1222 - so its verdict is Conflict.NONE: replayReason() replays it on ArmedLockBound.bounded, and replayableWithin() then holds it to the window with no grant. grantedPastTheWindow() is kept for exactly what this bound does not reach - oracle, the property at 0, a session that would not take the setting, a driver dialectOf() does not recognise - where the wait is as unbounded as it was when #904 was written. The four comments of #904 that described #915 as a future change ("#915 adds one" class, "#915 removes the trade", "#915 retires this method") are brought up to date with what actually happened, and the description has a rewritten "Relationship to #904" section saying the same. MAX_RETRY_WINDOW_NANOS of this branch is #904's RETRY_WINDOW_NANOS, package-private for the same one invariant.

Also carried over: the RuntimeException catch #936's last round put into withDdlLockBound() (9cc5a0e), kept across the split into armLockBound()/releaseLockBound().

At this head, JDK 11, javadoc goal on:

run result
JDBCRowLockBoundTestCase 26, JDBCDdlLockBoundTestCase 39, JDBCStorageRetryTest 101, JDBCStatementBoundTestCase 44 210 run, 0 failures, BUILD SUCCESS
PgSqlTestCase 82, MySqlTestCase 81 0 failures; the new case 4.1 s / 4.3 s, giving up at the bound and replaying

sql server and oracle are left to CI at this head; both were green at the one before the rebase.

@vharseko
vharseko requested review from maximthomas and removed request for maximthomas September 11, 2026 05:38

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

praise: The bound is where the wait is, and it is taken off where it must be.

  • releaseLockBound runs in the finally that closes the stamp and catalog sessions, after the rollback (JDBCStorage.write, :4096), so no pooled connection carries it into a read
  • replayReason reads ArmedLockBound.bounded (:4235) rather than the property, so a wait an operator bounded for themselves is not replayed
  • mysql is told innodb_lock_wait_timeout (:1598-1600), not the metadata lock #936 sets

issue (blocking): On sql server the DDL bound inside a write yields to the backend's own ROW bound: DDL_LOCK_TIMEOUT_PROPERTY governs no DDL issued from write(), and #936's rename is lost there.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:1665-1667, :1686-1690, :2569, :2410, :4235

write() arms ROW with set lock_timeout 3000. Every DDL of the backend except the off-write catalog drop (:3219, getValidatedConnection()) reaches commitStatement(sql, true) inside that write → withDdlLockBoundarmLockBound(DDL); displacedValue reads the session live for the DDL kind and gets the 3000 the ROW arm just set; MICROSOFT.ddlLockBoundSql(5, 3000)previous>=0 && previous<=millis — answers null, so the DDL runs alreadyTighter with bound==null. It waits the ROW value, not the DDL property (the defaults, 5 >= 3, included; an operator raising the property to 60 s for an index build blocked by long readers still gets 3 s), and on 1222 withDdlLockBound throws it bare — armed.bound==null ? e : gaveUpOnTheLock(...) — which write() then replays as "a wait for a row lock this backend bounded" until the window and throws naming neither property. At BASE the same DDL read -1, set 5000, restored -1, and its 1222 was renamed. The guard's own comments (:1661-1664 "left exactly as the deployment set it") and the description ("the DDL rewrite that names the property is #936's and stays on the DDL path") assume a deployment-set value; the 3000 is this backend's, issued one statement earlier. Postgres is unaffected (its DDL set local overrides unconditionally), mysql has two settings. TestCase.testTheDdlGivesUpOnALockAnotherSessionHolds asserts the rename on sql server and passes only because it sets DDL=2 < 3.

// armLockBound: decide "already tighter" against what the session carried before this backend
// touched it; keep the live readback as what the release puts back
final Long previous=(query==null) ? null : displacedValue(con, dialect, kind, query);
if (query!=null && previous==null) {
    return ArmedLockBound.none(kind);
}
final Long carriedByTheDeployment=
    (kind!=LockBound.ROW && con instanceof CachedConnection
        && ((CachedConnection) con).rowLockBoundDisplaces()!=null)
    ? ((CachedConnection) con).rowLockBoundDisplaces() : previous;
final String bound=kind.boundSql(dialect, seconds, carriedByTheDeployment);
if (bound==null) {
    return ArmedLockBound.alreadyTighter(kind, seconds);
}
final String restore=(previous==null) ? null : kind.restoreSql(dialect, previous);

Pin: a JDBCDdlLockBoundTestCase case on Dialect.MICROSOFT arming ROW then DDL on one CachedConnection (session answering -1, then 3000) at DDL=5, ROW=3, asserting set lock_timeout 5000 is issued and set lock_timeout 3000 put back, and that a 1222 under it is renamed; and testTheDdlGivesUpOnALockAnotherSessionHolds run at the default DDL bound on sql server.


question (blocking): After an in-write DDL commits on mysql and sql server, is the rest of that write meant to run bounded at 3 s and out of the replay?

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4037, :4096, :4900-4902, :4225, :1543-1546

The ROW bound is armed before the operation and released only in the attempt's finally. commitStatement(sql, ddl=true) executes the DDL, sets partlyCommitted=true and commits. On postgres that commit discards the set local, and the dialect comment states the design: "the rest of that write runs unbounded ... out of the replay ... which is why the bound is not armed again behind it". On mysql (set session innodb_lock_wait_timeout) and sql server (set lock_timeout) the setting is the session's and survives that commit; nothing takes it off before the finally. A write that creates a tree (the openTree create-on-demand callers — an online index add writing its state after creating its trees) and then meets a row another session holds for more than 3 s fails at 3 s with the bare 1205/1222: replayReason reads partlyCommitted first, so the bounded-wait arm never sees it. At BASE that write waited 50 s (mysql) / indefinitely (sql server) and went through; the description's cost statement ("fails once the window is spent") describes the pre-DDL road only. If this is intended, the postgres comment and the "scoped to the transaction" paragraph are what need changing; if not, the fix is below.

// WriteableTransactionTransactionImpl: the bound of the attempt, handed in by write() after arming
ArmedLockBound rowLock=ArmedLockBound.none(LockBound.ROW);

private void commitStatement(String sql, boolean ddl) throws SQLException {
    ...
    if (ddl) {
        withDdlLockBound(con, dialectOf(con), issue);
        // the commit above ended the transaction this bound was scoped to; on the engines whose
        // setting is the session's it is still on - take it off here, the way postgres's set local
        // came off with the commit. The finally of write() then has nothing left to release.
        releaseLockBound(con, dialectOf(con), rowLock);
        rowLock=ArmedLockBound.none(LockBound.ROW);
    } else {
        issue.run();
    }
}
// write(): rowLock=armLockBound(con, dialect, LockBound.ROW); txn.rowLock=rowLock;
//          ... finally { ... releaseLockBound(con, dialect, txn.rowLock); }

Pin: a JDBCRowLockBoundTestCase write on a MYSQL or MICROSOFT recording connection issuing a DDL and then a statement, asserting the restore statement precedes the post-DDL statement.


suggestion (non-blocking): The "DDL reads the session every time, even pooled" arm of displacedValue is pinned by no case — a mutant memoising the DDL kind too survives 166/166.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2569, opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCRowLockBoundTestCase.java:309-320, opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java:683

Measured at the head: if (!(con instanceof CachedConnection)) in place of if (kind!=LockBound.ROW || !(con instanceof CachedConnection)) — JDBCRowLockBoundTestCase 26, JDBCDdlLockBoundTestCase 39, JDBCStorageRetryTest 101, all green. No case arms DDL after ROW, or DDL twice, on one CachedConnection: :258/:310 arm ROW only; :683 arms DDL once on an empty memo; the pooled ROW-then-DDL cases (:440/:462) are postgres, which has no readback query; the container sql server DDL case asserts nothing about what the DDL restores. Under the mutant a DDL inside a sql server write reads the borrow-time -1, restores -1 mid-attempt and leaves the rest of the attempt unbounded, with nothing on the wire to show it — the invariant the description rests the shared LOCK_TIMEOUT on.

// JDBCRowLockBoundTestCase, MICROSOFT, one CachedConnection answering -1 then 3000
storage.armLockBound(con, Dialect.MICROSOFT, LockBound.ROW);            // reads -1, sets 3000, memo=-1
storage.withDdlLockBound(con, Dialect.MICROSOFT, theDdl());             // must read again
assertEquals(issued.stream().filter("select @@lock_timeout"::equals).count(), 2L,
    "the DDL bound read the memo instead of the session");
assertThat(issued).containsSubsequence("set lock_timeout 3000", "select @@lock_timeout", "set lock_timeout 3000");

Pin: the same fixture is what the blocking issue above needs; written once, it pins both (with the fix, the second select is still issued and the DDL's own set lock_timeout 5000 sits between).


issue (non-blocking): A lock timeout raised on the catalog session is replayed as "a wait for a row lock this backend bounded" although nothing of ours is armed on that session.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4231-4236, :2831, :2125, :5042-5059

lockNotAvailable walks EVERY_LINK. The catalog connection is DriverManager.getConnection with no setting of ours (armLockBound has two call sites, both the pooled connection), so under a deployment-wide lock_timeout (postgres ALTER ROLE ... SET lock_timeout) enrolInCatalog's upsert can raise a 55P03 that is wrapped, reaches replayReason with rowLock.bounded true and is replayed under that reason; the warn line at :4142 names a bound that was not on that wait — the description's own rule ("a bound an operator set for themselves is not a licence for this loop to take that wait again") broken for that session. Benign: the catalog session is per attempt, the rollback was issued, the replay is window-bounded and Conflict.NONE. The stamp session is different — lockTimeoutSql (:1878) is ours, so a 55P03 there is a bounded wait.

// either decide the arm on the failure's origin (a link raised by the pooled connection's statement),
// or say so where the reason is read:
// "read off every link of the chain: a lock timeout of the catalog session, which carries no bound
//  of ours, is replayed under this reason too - once per attempt, inside the window, never granted"

issue (non-blocking): The new replay arm widens the RootContainer.open() replay exposure that #1002 closes.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4235, opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/RootContainer.java:195-229

#1002 is not an ancestor of this base. On postgres and sql server every write arms ROW, so rowLock.bounded is true; a DDL of open() that gave up at the DDL bound (55P03/1222, renamed, matched over EVERY_LINK) is now replayed when no earlier DDL of the attempt committed (partlyCommitted stays false — the execute threw before :4901). Base DN 1's trees exist and its container is registered in memory; base DN 2's create table waits on a lock another session holds and gives up → replay → registerEntryContainer(baseDN 1) again → ERR_ENTRY_CONTAINER_ALREADY_REGISTERED masks the lock timeout, attempt-1 indexes left registered. At BASE that failure was Conflict.NONE and surfaced as the renamed 55P03. The 40P01/40001 roads had the same exposure at BASE and #1002 removes it for all of them — a rebase over #1002 moots this; it matters only if this lands first.

// nothing in this PR: land #1002 first, or rebase this over it; if this lands first, name the
// widened road in the description

suggestion (non-blocking): The per-bound warn latches are pinned by nothing.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2711-2716, :2750-2758

lockBoundNotSetWarned.get(kind) and lockBoundLeftBehindWarned.get(kind) gate only a logger.warn; keepOutOfThePool and the returned ArmedLockBound(bounded=false) are computed outside the latch, and neither bound suite captures the logger. A mutant sharing one AtomicBoolean/AtomicLong across DDL and ROW — the "one latch would silence every write" shape the description argues against — survives every case. BASE's single latch was unpinned too; this PR doubles it and presents the split as a design point.

// a log-capturing case: arm DDL on a session that refuses the setting, then ROW on another;
// assert both warn lines were said, once each - the observable is what was said, per kind

nitpick (non-blocking): The description's "every StatementBound.BULK statement — clearTree()'s delete from among them — take row locks and are deliberately left waiting" is not true of a BULK DML issued inside write().

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4908-4909, :5347

The ROW bound is a session setting (mysql, sql server) or a transaction-wide set local (postgres) armed before writeOperation.run; nothing in execute(statement, BULK) or the non-DDL arm of commitStatement takes it off, so a delete from through txn.clearTree inside a write waits 3 s. No production caller today — git grep '\.clearTree(' over src/main at the head finds only the importer road (:6733, its own connection, unarmed) and the Importer implementations — so only the description's sentence is off; the code comment at :4908-4909 ("this bound" = the DDL bound) is right.


note (non-blocking): Not checked.

  • standing: the container suites (PgSqlTestCase, MySqlTestCase, MsSqlTestCase, OracleTestCase) need engines this reviewer's box does not run; the sql server and oracle roads of the two blocking items were traced, not executed — the five build-maven (ubuntu-latest, *) cells are green at 9b167eb (run 34566666576), which is the only run of them at this head.

@vharseko
vharseko force-pushed the issues/915-write-lock-bound branch from 9b167eb to 42e27db Compare September 21, 2026 12:22
@vharseko

Copy link
Copy Markdown
Member Author

Round 1 is answered at 42e27db633, rebased onto master - which now carries #999, #1003 and #1016 on top of #936 and #904. Both blocking items are fixed, the two suggestions are pinned, and the description is corrected where it was wrong. What the rebase itself had to decide is at the end.

1. The DDL bound inside a write yields to the ROW bound on sql server (blocking)

Confirmed, and it reads wider than the report: it fires at the defaults. armLockBound(DDL) reads the session live, sees the 3000 the ROW arm set one statement earlier, and MICROSOFT.ddlLockBoundSql(5, 3000) answers null on previous>=0 && previous<=millis - so at DDL 5 / ROW 3, with nothing configured at all, DDL_LOCK_TIMEOUT_PROPERTY governs no DDL issued from write() and the 1222 of such a DDL arrives bare, the rename of #936 lost with it.

The decision now reads what the deployment set rather than what the session carries, through the value the ROW arm displaced and remembered on the CachedConnection:

final Long previous=(query==null) ? null : displacedValue(con, dialect, kind, query);
...
final String bound=kind.boundSql(dialect, seconds, carriedByTheDeployment(con, dialect, kind, previous));

One correction to the snippet in the review: the arm cannot be keyed on kind!=LockBound.ROW alone. On mysql the two bounds are two variables - lock_wait_timeout for the metadata lock, innodb_lock_wait_timeout for the row lock - so the remembered value describes neither the other's session nor its default, and a deployment that had tightened innodb_lock_wait_timeout to 1 s would have its DDL answer "already tighter" and wait a year for a metadata lock. It is keyed on Dialect.oneSettingForBothBounds() instead: true of sql server (LOCK_TIMEOUT) and of postgres (lock_timeout, where it changes nothing - that dialect reads no previous and issues its set local unconditionally), false of mysql and oracle. The live readback stays what the release puts back, exactly as the review asks.

Pinned by JDBCRowLockBoundTestCase.testTheDdlBoundInsideAWriteIsDecidedAgainstWhatTheDeploymentSet, over a sql server connection answering the readback with whatever the last set lock_timeout left on it - the fixture the "DDL reads the session every time" arm needed too, so one case pins both:

select @@lock_timeout, set lock_timeout 3000,      the row bound of the write
select @@lock_timeout, set lock_timeout 5000,      the DDL inside it, at its own property
the ddl,
set lock_timeout 3000,                             what the session carried a statement ago
set lock_timeout -1                                and what the deployment set

and by testADdlInsideAWriteThatGivesUpNamesItsOwnProperty for the rename. TestCase.testTheDdlGivesUpOnALockAnotherSessionHolds now runs at 7 s rather than 2, since a DDL bound tighter than the row one passes it either way.

2. Is the rest of the write meant to run bounded at 3 s and out of the replay? (blocking)

No - that was not intended, and the postgres comment describes what all four engines should do. A DDL of the attempt commits, which ends the transaction this bound was armed around; commitStatement() now takes it off there, from a finally so that a DDL which failed after the engine committed before it (mysql, oracle) is treated the same:

if (ddl) {
    try {
        withDdlLockBound(con, dialectOf(con), issue);
    }finally {
        releaseTheRowLockBound();
    }
}else {
    issue.run();
}

The bound lives on the transaction now (txn.rowLock), so the finally of write() gives back whatever is still on and never puts a value back twice; the copy write() keeps is what the attempt ran under, which is what the replay is decided on. On postgres the set local still goes with that commit by itself and the release is a no-op there.

Pinned end to end in JDBCStorageRetryTest.testTheRowLockBoundComesOffWhereADdlOfTheAttemptCommits, over the stub driver on mysql: the restore has to precede the statement issued behind the DDL, which is the only place the order shows.

3. The "DDL reads the session every time, even pooled" arm is pinned by no case

Pinned by the case above: under the mutant the second select @@lock_timeout is not issued and the list does not match. Measured - the mutant is red now, and both the mutant and the fixture are in the description's list.

4. A lock timeout raised on the catalog session

Confirmed: enrolInCatalog writes on a connection of its own, nothing of ours is armed on it, and lockNotAvailable walks every link - so under a deployment-wide lock_timeout its 55P03 is replayed under this reason. Left as it is, with the reason said out loud at replayReason() rather than narrowed by the failure's origin: the catalog session is made and closed inside the attempt, its statement was rolled back with the rest, and such a replay is bounded by the window like any other - a wait long enough to outlast the window is not replayed at all. Deciding it on the origin would mean asking which connection raised a link, which the chain does not carry.

5. The RootContainer.open() replay exposure

Moot after the rebase, as the review says - with one correction to the attribution: the closeSilently(unregisterEntryContainer(baseDN)) loop in front of the opens came in with #999 (1af0a1247d), not with #1002, which is the test-only pin over it. 1af0a1247d was not an ancestor of the old head and is one of this one.

6. The per-bound warn latches are pinned by nothing

Pinned, by the latches themselves rather than by a captured log: there is no log-capturing fixture in this module, and what the mutant changes is observable directly - testTheWarningLatchesAreOnePerBound arms the DDL bound on a session that refuses the setting and asserts the ROW latch is still down before arming ROW, and testTheLatchOfABoundLeftBehindIsOnePerBoundToo does the same from the other side, over a restore that fails. Both directions are needed: a shared latch is invisible to whichever side is asserted first.

The rebase made this a third latch as well - see below.

7. The StatementBound.BULK sentence

Corrected in the description: inside a write such a statement runs under this bound like every other statement of that transaction, and clearTree()'s delete from is reached in src/main from the importer road alone, which holds a connection of its own and arms nothing on it.

What the rebase had to decide

#1003 landed a third way for the DDL bound to degrade - an engine dialectOf() does not recognise, reported once per storage through ddlLockBoundEngineUnknownWarned. Merged into the split of this PR, that report is now per bound like the other two, for the reason the split exists: a backend opening its trees on a mariadb, percona or aurora driver would otherwise have said the only line there is to say before the first write of that backend ever ran, and the property an operator is sent to is the property of the wait that was left unbounded. #1003's four cases keep their assertions, reading lockBoundEngineUnknownWarned.get(DDL); one case is added for the ROW side, and a mutant reporting both through one latch is red.

Also carried over unchanged: #1003's createCatalogTable() arming the DDL bound on the catalog's own connection - the one connection reaching armLockBound that was never in the pool - and the wording of the "left behind" warning that says so.

Runs at this head

run result
JDBCRowLockBoundTestCase 30, JDBCDdlLockBoundTestCase 51, JDBCStorageRetryTest 105, JDBCStatementBoundTestCase 45, javadoc goal on 231 run, 0 failures, BUILD SUCCESS
PgSqlTestCase 82, MySqlTestCase 81 0 failures

sql server and oracle are left to CI, as before - and the sql server road is the one both blocking items live on, so the container leg there is what confirms them against a real engine rather than against a mock.

Seven mutants measured at this head, each red: the DDL bound decided against the live readback (2 failures), the readback memoised for the DDL bound too (1), the row bound left on behind a committing DDL (1), one latch for both bounds and one moment for both bounds (1 each, in opposite directions), and the unrecognised driver reported through one latch (1). The four of round 1 are unchanged.

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

praise: Round 1's two Majors are answered where they arise, and the replay is decided on the right copy of the bound.

  • carriedByTheDeployment() (JDBCStorage.java:2674-2678) decides "already tighter" for the DDL of a write against the value the row bound displaced, remembered on the CachedConnection, and only where one setting carries both waits (Dialect.oneSettingForBothBounds()).
  • commitStatement() (JDBCStorage.java:5079-5104) takes the row bound off in the finally of the DDL that ends the transaction, on the two engines whose setting is the session's — round 1's [1].
  • write() (JDBCStorage.java:4189-4196) keeps the attempt's own copy of ArmedLockBound for replayReason() and the transaction's for the release, so a DDL that took the bound off does not take the replay with it; ArmedLockBound.bounded (:4402) leaves the property at 0, oracle and a session that would not take the setting exactly as unreplayable as they were.

issue (blocking): DDL_LOCK_TIMEOUT_PROPERTY at 0 inside a write on postgres or sql server does not "wait as before": the DDL runs under the row bound of the attempt, gives up at 3 s with a bare 1222 / 55P03, and is replayed as a row-lock wait.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2547-2550, :5079-5104, :1571-1573, :1716-1719

armLockBound() returns ArmedLockBound.none(kind) at seconds<=0 before any readback or carriedByTheDeployment(), and commitStatement() calls withDdlLockBound() with the attempt's row bound still on — releaseTheRowLockBound() is the finally after it. On the two oneSettingForBothBounds() engines that row bound is the very setting the DDL bound uses: set lock_timeout 3000 on the sql server session, set local lock_timeout = 3000 in the postgres transaction (the dialect comment at :1560 says it bounds "any lock of the transaction"). So a create table / create index of openTree(create=true) blocked by another session gives up at 3 s; withDdlLockBound() (:2460) renames nothing since armed.bound==null, partlyCommitted stays false on both engines, and replayReason() (:4402) replays it as "a wait for a row lock this backend bounded" — re-issuing the DDL and re-waiting 3 s — until the window is spent. The caller sees the bare vendor error. This breaks boundSeconds()'s "0 leaves what it bounds as unbounded as it was before that bound existed", JDBCDdlLockBoundTestCase:137, and the description's "a DDL inside a write keeps its own property … the defaults included"; the only way out is ROW_LOCK_TIMEOUT_PROPERTY=0 as well, which turns the row bound off for every write. mysql (two variables) and oracle (nothing armed) are unaffected. Not run: needs a postgres/mssql container with a blocker on the relation.

// WriteableTransactionTransactionImpl.commitStatement(), the ddl branch
if (ddl) {
	try {
		liftTheRowLockBoundForAnUnboundedDdl();
		withDdlLockBound(con, dialectOf(con), issue);
	}finally {
		releaseTheRowLockBound();
	}
}

/**
 * A DDL whose property is 0 waits as this backend waited before the bound existed - which, on an
 * engine with one setting for both waits, the row bound of this attempt is currently cutting to 3 s.
 */
private void liftTheRowLockBoundForAnUnboundedDdl() throws SQLException {
	final Dialect dialect=dialectOf(con);
	if (LockBound.DDL.seconds()>0 || dialect==null || !dialect.oneSettingForBothBounds()) {
		return;
	}
	if (dialect.boundLivesInTheTransaction()) {
		// the set local of the row bound has no value to give back: reset it for the rest of the transaction
		executeSessionStatement(con, "set local lock_timeout to default");
	}
	releaseTheRowLockBound(); // sql server: the deployment's LOCK_TIMEOUT back ahead of the DDL
}

Pin: a JDBCRowLockBoundTestCase case with DDL at 0 and ROW at 3 on the liveLockTimeout() sql server fixture, the DDL issued from a write — InOrder set lock_timeout 3000, set lock_timeout -1, the DDL; and the postgres twin asserting set local lock_timeout to default ahead of the DDL.


issue (blocking): On sql server a deployment LOCK_TIMEOUT strictly between the row and the DDL bound makes the in-write DDL "already tighter" against the remembered value while the session carries the row bound — the DDL runs at 3 s and its 1222 is handed through bare.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2571-2578, :2674-2678, :1694-1697, :2460

armLockBound() asks kind.boundSql(dialect, seconds, carriedByTheDeployment(...)) and returns alreadyTighter on null. For the DDL of a write previous is the live readback — the row arm's 3000 — and carriedByTheDeployment() hands back the memo, the deployment's value. With lockTimeout=4000 (the mssql-jdbc URL property, or any value in (ROW*1000, DDL*1000]) at the 3 s / 5 s defaults — or DDL raised to 30 s for an index build with a deployment at 10000, the description's own scenario — MICROSOFT.ddlLockBoundSql(5, 4000) answers null (0<=4000<=5000), nothing is issued, and the create index runs at the session's live 3000: not the deployment's value, not DDL_LOCK_TIMEOUT_PROPERTY. alreadyTighter's javadoc (:2519) assumes "the session keeps what it has"; what it has is this backend's row bound. A 1222 after 3 s is thrown bare (armed.bound==null), releaseTheRowLockBound() puts 4000 back, and replayReason() replays it as a row-lock wait each attempt until the window is spent. Round 1's [2] on a narrower arm; JDBCRowLockBoundTestCase:429 and :459 pin the deployment at "-1" only. postgres never reaches this arm (no readback, the DDL always sets its own set local); mysql reads two variables.

// armLockBound(), in place of the alreadyTighter return
final Long deployment=carriedByTheDeployment(con, dialect, kind, previous);
String bound=kind.boundSql(dialect, seconds, deployment);
if (bound==null) {
	if (deployment==null || deployment.equals(previous)) {
		return ArmedLockBound.alreadyTighter(kind, seconds);
	}
	// the deployment gives up sooner than this bound would, but what the session carries now is a
	// bound of ours on the same setting: put the deployment's value on for the work, the live one back after
	bound=kind.restoreSql(dialect, deployment);
}

Pin: the liveLockTimeout() fixture at "4000" (and at "10000" with DDL_LOCK_TIMEOUT_PROPERTY=30), ROW then a DDL of the same write — InOrder set lock_timeout 3000, set lock_timeout 4000, the DDL, set lock_timeout 3000, set lock_timeout 4000; at the head the middle three are absent.


suggestion (non-blocking): The mysql arm of carriedByTheDeployment()!dialect.oneSettingForBothBounds() returning the live readback, never the row memo — is pinned by no case.

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

The mutant dropping that clause (or flipping MYSQL.oneSettingForBothBounds() to true; :2674 is its only call site) diverges only when a DDL is armed on a mysql CachedConnection carrying a row memo and exactly one of memo / live is ≤ the DDL bound. The only ROW-then-DDL cases on one connection are Dialect.MICROSOFT (JDBCRowLockBoundTestCase:428, :457); the mysql cases answer both readbacks with 31536000 (JDBCDdlLockBoundTestCase:684, :706) or run the memo-1 arm on a plain mock (:872). Every fixture answers one value to every readback query (statementsOf, engine(cls, carries)), so memo == live on every road. Not measured; the arm table is exhaustive.

// JDBCRowLockBoundTestCase, Dialect.MYSQL on a CachedConnection: innodb readback "1", lock_wait_timeout readback "31536000"
// (a fixture answering each query its own value), ROW armed then a DDL of the same write
assertThat(issued).contains("set session lock_wait_timeout=5");   // under the mutant the memo 1 <= 5 answers alreadyTighter: absent

suggestion (non-blocking): "Nothing is put back twice" (releaseTheRowLockBound() javadoc) is pinned by no case.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCStorageRetryTest.java:1189-1196, JDBCStorage.java:5117-5119

testTheRowLockBoundComesOffWhereADdlOfTheAttemptCommits reads issued.indexOf("set session innodb_lock_wait_timeout=50") and asserts order only. Delete rowLock=ArmedLockBound.none(LockBound.ROW); at :5119 and write()'s finally (:4257) restores a second time; both succeed on the mock, indexOf finds the first, every order assertion holds. The postgres cases have no restore statement to double; JDBCRowLockBoundTestCase:428-449 drives withDdlLockBound directly, never commitStatement. Not run.

assertThat(Collections.frequency(issued, "set session innodb_lock_wait_timeout=50")).isEqualTo(1);

suggestion (non-blocking): How many attempts a DDL lock wait inside write() costs on postgres / sql server is pinned by no case, and testARewrittenSqlServerLockWaitIsMadeNoMoreReplayable's javadoc says the opposite of what the head does.

opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/JDBCDdlLockBoundTestCase.java:474-492, JDBCStorage.java:4370-4375, :4402

replayReason()'s javadoc documents as deliberate that a create index which gave up at DDL_LOCK_TIMEOUT_PROPERTY inside a write is replayed through :4402 on the attempt's copy of the bound. The case pins conflictVerdict(..).conflict==Conflict.NONE alone, and its javadoc ("left as unreplayable as it was … a DDL made to look like one would be replayed into the same wait") is false at the head. Mutant: rowLock=txn.rowLock; beside write()'s inner finally — a DDL lock timeout then reaches :4402 after releaseTheRowLockBound() set the transaction's copy to none: thrown at attempt 1. JDBCStorageRetryTest:1113-1129 (55P03 from a DML, copies identical) and :1087-1098 (40P01, decided at :4395) pass; JDBCDdlLockBoundTestCase:730-750 drives a DDL 55P03 through write() and asserts only the message — attempts occurs 0 times in that suite. Not run.

// JDBCStorageRetryTest, postgres: openTree(create) inside write() whose create index throws 55P03 while the ROW bound is armed
assertThat(attempts.get()).isEqualTo(2);

Or: rewrite the javadoc at :474-476 to what the head does — replayed on the window, never granted past it.


suggestion (non-blocking): write()'s finally restores the row bound on a connection the database dropped, and the failure of that restore is logged as a stranded bound.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4257, :4214, :2896-2903

releaseLockBound(con, dialect, txn.rowLock) runs after dropped=isConnectionFailure(e,con) with no guard: on mysql / sql server a write that armed normally issues its set … on the dead connection, fails, keepOutOfThePool() (already condemned) and logs "the lock bound of a write transaction could not be taken off a connection … may have been left carrying" once per 10 s per bound. A database restart under write load reads as a stream of stranded-bound warnings for connections that are simply dead. Cosmetic — the connection is not handed on.

if (!dropped) { // a dropped connection carries nothing to anybody: the pool distrusts it already
	releaseLockBound(con, dialect, txn.rowLock);
}

suggestion (non-blocking): On postgres every write attempt's row arm leaves a subtransaction open: the savepoint ahead of the set local is never released.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:2588, :2708-2718

savepointBeforeTheBound() takes con.setSavepoint() whenever the bound lives in the transaction, and the success road returns with it open — the file's only releaseSavepoint (:4013) is the search_path probe's. The subtransaction then spans the whole attempt: two xids and a pg_subtrans entry per write, the subxid as xmin on every tuple it touches, and a pg_subtrans lookup for readers of its in-progress rows. One per transaction stays under the 64-subxid cap, so no SLRU pathology; the description prices this as "a savepoint" round trip. Either con.releaseSavepoint(beforeTheBound) after the set local succeeds (SET LOCAL survives a RELEASE; one more round trip on the hot path) or a sentence in the javadoc naming the subtransaction.


nitpick (non-blocking): ROW_LOCK_TIMEOUT_PROPERTY's javadoc says the readback "is paid once per borrow, not once per write" — a borrow is one per attempt.

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

write() calls getConnection() inside the attempt loop (:4172), so "per borrow" is at least once per write, the opposite of the contrast drawn. The memo lives for the CachedConnection's life (rowLockBoundDisplaces, written once at :2637-2641, never cleared); displacedValue()'s own javadoc (:2621) and the description say "once per pooled connection".


nitpick (non-blocking): The reason line "a wait for a row lock this backend bounded" names a row lock for the two other waits that reach it.

opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:4403, :4370-4375, :4397-4401

A DDL of the attempt that gave up at DDL_LOCK_TIMEOUT_PROPERTY and a catalog-session 55P03 are both replayed under this line, and both are documented as deliberate; "a lock wait this backend bounded" would be true of all three.

…a row lock another session holds

The replays of write() are bounded by a wall clock, and a clock bounds them only while an
attempt is shorter than it. What makes an attempt long is the lock wait ahead of the conflict,
charged to the attempt that hit it: sql server, postgres and oracle wait for a row lock with no
bound of their own, and mysql bounds it at 50 s, five times the window. So an attempt outlasted
the window and the operation was left with no replay at all (OpenIdentityPlatform#903).

org.openidentityplatform.opendj.jdbc.row.lock.timeout, 3 s by default, is set on the session of
a write attempt and taken off again before the connection goes back to the pool - a set local on
postgres, innodb_lock_wait_timeout on mysql (never the lock_wait_timeout of a metadata lock),
LOCK_TIMEOUT on sql server, and nothing on oracle, which has no session setting for that wait.
The failure an engine ends such a wait with is replayed, and only where this backend bounded the
wait: 55P03 and error 1222 are no conflict, and a wait nothing bounds is what no clock can
govern.

The mechanism is OpenIdentityPlatform#885's, split into an arm and a release so a bound can be held around a whole
transaction rather than around one statement, with LockBound naming the two waits this backend
bounds and what each engine is told about them.
@vharseko
vharseko force-pushed the issues/915-write-lock-bound branch from 42e27db to a9212d5 Compare September 21, 2026 17:07
@vharseko

Copy link
Copy Markdown
Member Author

Round 3 is answered at a9212d523b. Both blocking items are fixed where they arise, the five suggestions and the two nitpicks are taken, and one correction to the second snippet is at the end of item 2.

1. DDL_LOCK_TIMEOUT_PROPERTY at 0 inside a write does not wait as before (blocking)

Confirmed, exactly as traced. armLockBound() returns at seconds<=0 before anything is read or decided, and commitStatement() had the release in the finally behind withDdlLockBound() - so on the two engines with one setting for both waits the DDL ran under the row bound of the write: set lock_timeout 3000 on the sql server session, set local lock_timeout = 3000 on the postgres transaction, which bounds every lock wait of it. The create table / create index of an open then gave up at 3 s, arrived bare - nothing of ours armed around it, so there is no bound to name it by - and replayReason() replayed it until the window was spent. The one way out was turning the row bound off as well, which is not what an operator asking for the old DDL behaviour asked for.

The bound now comes off in front of such a DDL, from the placement the review names - the transaction is the only thing that knows a set local is on, postgres having no readback and no memo:

if (ddl) {
    try {
        liftTheRowLockBoundForAnUnboundedDdl();
        withDdlLockBound(con, dialectOf(con), issue);
    }finally {
        releaseTheRowLockBound();
    }
}

What each engine is told is a dialect method rather than a literal in the transaction, like every other statement of these bounds: POSTGRES.rowLockLiftSql() is set local lock_timeout to default - a set local has no displaced value to give back, and the default is the value the transaction would have carried had this backend set nothing - and on sql server the release puts the deployment's own LOCK_TIMEOUT back ahead of the statement. Only where this backend armed something: a session already tighter, a setting it refused, mysql and oracle leave the DDL exactly where it was. It throws rather than reporting, unlike the release - a set local that failed has aborted the transaction anyway, and the failure saying why is worth more than the 25P02 of the DDL behind it.

Pinned end to end in JDBCStorageRetryTest, on both engines: testADdlToldToWaitDoesNotWaitAtTheRowBoundOnPostgres and ...OnSqlServer read the order off the statements the attempt issued, and testABoundedDdlLeavesTheRowBoundOfTheWriteWhereItIs holds the other side - a DDL with a bound of its own is left alone, so this is not a round trip per DDL of every open.

2. A deployment LOCK_TIMEOUT between the row and the DDL bound (blocking)

Confirmed. carriedByTheDeployment() decided against the right value and then nothing put that value on the session: alreadyTighter means "it keeps what it has", and what it had was the row bound of the write - 3 s, tighter than the deployment's 4 s and than the DDL's 5 s. The deployment's own figure now goes on for the length of the work, and the live readback - the row bound - is what the release puts back:

final String bound=(atOurFigure!=null) ? atOurFigure : theDeploymentsOwnValue(kind, dialect, deployment, previous);
if (bound==null) {
    return ArmedLockBound.alreadyTighter(kind, seconds);
}

theDeploymentsOwnValue() answers null where the two are the same value, which is every other road: the decision is then the whole answer, exactly as before.

One correction to the snippet. Setting bound alone would make withDdlLockBound() rename a failure under it - armed.bound!=null is what that test reads - and the rename says "gave up ... at the 5s of ddl.lock.timeout: raise that property". The wait it names was ended by the deployment's 4 s, and raising our property changes nothing while the deployment's own value is the tighter one. So the rename is decided on a new ArmedLockBound.ourFigure rather than on bound!=null: the value is put on and given back, and the failure is handed through exactly as it arrived. testADdlThatGaveUpAtTheDeploymentsValueIsNotNamedByThisProperty pins that, testADdlInsideAWriteRunsAtWhatTheDeploymentSetWhereThatIsTighter the order of the statements at the defaults over a liveLockTimeout() fixture at 4000, and testTheSameWhereTheDdlBoundWasRaisedForAnIndexBuild the description's own scenario - 30 s asked for, 10 s allowed.

3. The mysql arm of carriedByTheDeployment() (non-blocking)

Taken. Every fixture in these suites answered one value to every readback, which is why memo and live agreed on every road; answering() now answers each query its own value, and testTheDdlBoundOfAMysqlWriteIsDecidedAgainstItsOwnVariable arms ROW at an innodb_lock_wait_timeout of 1 s and then a DDL on the same CachedConnection, asserting set session lock_wait_timeout=5 is issued. The mutant - the clause dropped - is red.

4. "Nothing is put back twice" (non-blocking)

Taken: assertEquals(frequency(issued, "set session innodb_lock_wait_timeout=50"), 1) in testTheRowLockBoundComesOffWhereADdlOfTheAttemptCommits. The mutant deleting rowLock=ArmedLockBound.none(LockBound.ROW) is red where it was green.

5. The attempts a DDL lock wait costs, and the javadoc that said the opposite (non-blocking)

Both. JDBCStorageRetryTest.testADdlLockWaitInsideAWriteIsReplayedOnTheBoundTheAttemptRanUnder drives a create index that gives up with 55P03 inside a write and asserts attempts==2; the mutant deciding the replay on the transaction's copy (rowLock=txn.rowLock beside the inner finally) throws at attempt 1 and is red. And the javadoc at JDBCDdlLockBoundTestCase:474 now says what the head does: the rename leaves the failure no conflict of isConflict() - which is what the case asserts - and no conflict is not the same as never replayed, a lock wait of a bounded attempt being replayed on the window and never past it.

6. The restore on a connection the database dropped (non-blocking)

Taken, with the guard the review gives. distrustPool() has already told the pool to validate every connection of that generation, and a dropped one answers isValid() with a failure, so nothing is handed on - the only thing the restore added was a stranded-bound warning per dead connection. testTheRowLockBoundIsNotPutBackOnAConnectionTheDatabaseDropped pins it.

7. The savepoint (non-blocking)

Taken - released, rather than named in the javadoc. One correction to the framing: the savepoint is #885's, not this PR's, and master does not release it either; what this PR changes is that the bound it is taken for is now armed around every write rather than around an open, which is what makes the subtransaction worth the round trip it costs to let go of. releaseTheSavepoint() runs on the success road alone, so a setting that failed is still taken back to it. testPostgresLetsGoOfThatSavepointOnceTheSettingIsOn and testASettingThatFailedIsTakenBackToThatSavepoint pin the two directions, and JDBCDdlLockBoundTestCase.testATransactionWhoseSettingWentThroughIsNotTakenBack now asserts the release on the DDL road as well.

8. "once per borrow" (non-blocking)

Corrected to "once per pooled connection", which is what displacedValue() does and what the description said. The postgres line of the same paragraph now reads three round trips rather than one plus a savepoint, the release being the third.

9. The reason line (non-blocking)

a lock wait of an attempt this backend bounded. "A lock wait this backend bounded" would still be untrue of the catalog session, whose bound is the deployment's - what all three have in common is the attempt they were raised in, which is what carries the bound the replay is decided on.

Runs at this head

run result
JDBCRowLockBoundTestCase 36, JDBCDdlLockBoundTestCase 51, JDBCStorageRetryTest 110, JDBCStatementBoundTestCase 45 242 run, 0 failures
the javadoc goal on (package) BUILD SUCCESS

Ten mutants measured at this head, each red and each caught by the case written for it: the lift dropped (2 failures), the lift made unconditional, the deployment's figure never put on (2), the rename left on bound!=null, the mysql arm of carriedByTheDeployment() dropped, the transaction not recording that it carries no bound, the replay decided on the transaction's copy, the restore issued on a dropped connection, the savepoint left open, and the savepoint released on the road that still has to roll back to it.

The container suites are as they were - sql server and oracle to CI, and the sql server road is the one both blocking items live on.

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

Labels

bug concurrency Thread-safety / race-condition bugs jdbc tests Test suites: fixing, enabling, un-disabling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JDBC backend: nothing bounds an attempt of the transaction replay, so the retry window has to work around it

2 participants