[Fix][Connector-V2] Handle JDBC transaction deadlock retries safely by yzeng1618 · Pull Request #12769 · apache/seatunnel

yzeng1618 · 2026-10-14T12:19:17Z

Purpose of this pull request

I am glad to contribute to Apache SeaTunnel.

Concurrent JDBC writers can acquire row locks in different orders and receive database transaction conflicts such as PostgreSQL 40P01. Sorting records within one flush reduces the chance of a lock-order cycle, but it cannot handle conflicts spanning multiple flushes or custom SQL paths. Retrying without rolling back the whole transaction can also replay only part of the transaction.

Update in this push: the branch is rebased onto the latest dev. The NULL-key buffering change that previously duplicated #12803 has been dropped — this PR now builds directly on top of #12803's implementation, and the duplicated NULL-key tests were removed (dev already carries that coverage). One test compile signature was adapted accordingly.

Does this PR introduce any user-facing change?

Detects PostgreSQL 40P01, standard 40001, and strictly matched Oracle ORA-00060 transaction conflicts.
Retries only independent manual-commit flush transactions when the executor can retain logical records and clear the driver's partially submitted batch.
Propagates conflicts from checkpoint and XA transaction scopes so the upper-level recovery mechanism can replay the complete transaction.
Keeps the previous generic retry behavior for auto-commit connections.
Adds opt-in upsert_batch_key_ordered ordering for generated primary-key upsert batches; the default remains false for compatibility.
Prevents failed child JDBC batches from being executed again during close.
Adds unit coverage and a PostgreSQL two-connection deadlock integration test.

How was this patch tested?

Unit tests:
- JdbcOutputFormatReconnectSafetyTest: deadlock is rolled back and retried after a driver-batch reset; auto-commit conflicts keep the legacy generic retry (no rollback, two executeBatch calls); conflicts inside a checkpoint transaction propagate instead of replaying only the current flush; exhausted retries never replay the buffered batch during close (Buffered and BufferReduced executors).
- BufferReducedBatchStatementExecutorTest: keys replay in a deterministic order under upsert_batch_key_ordered; the comparator is transitive across mixed field types; resetForTransactionConflictRetry() retains the logical buffer and clears only the driver batch.
Integration test: JdbcPostgresIT#testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions produces a real PostgreSQL 40P01 with two coordinated connections (CyclicBarrier) and asserts the retried flush lands both rows.
Integration tests through the production wiring: JdbcPostgresIT#testDeadlockRetryWithRealUpsertExecutor builds both writers via JdbcOutputFormatBuilder with the real upsert executor (BufferReducedBatchStatementExecutor), forces a PostgreSQL 40P01 between opposite-order concurrent flushes, and asserts via the pg_stat_database.deadlocks counter that the conflict was detected and every buffered row was replayed. JdbcPostgresIT#testUpsertBatchKeyOrderedMixedUpsertAndDelete runs a mixed upsert/delete batch under upsert_batch_key_ordered=true and uses AFTER UPDATE/DELETE triggers to verify the option wiring and the primary-key execution order end to end.
docs/en and docs/zh connectors/sink/Jdbc.md updated for max_retries and upsert_batch_key_ordered.
Formatting validated with ./mvnw spotless:apply; CI runs the full build and JDBC integration lanes.

Check list

If any new Jar binary package adding in your PR, please add License Notice according
New License Guide
If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.
If you are contributing the connector code, please check that the following files are updated:
1. Update plugin-mapping.properties and add new connector information in it
2. Update the pom file of seatunnel-dist
3. Add ci label in label-scope-conf
4. Add e2e testcase in seatunnel-e2e
5. Update connector plugin_config

DanielLeens

Thanks for working on this, @yzeng1618! Deadlock handling in the JDBC sink is a real pain point and the transaction-boundary reasoning in this PR (retry only a flush that owns its own commit, propagate otherwise) is the right instinct. I went through the full diff and traced the flush, close and executor paths. I found one issue that I think needs to be fixed before merging, plus a few smaller items, all listed below. Happy to re-review once you push an update! This is a static review; I did not execute the PR. There are no other reviews or comments on this PR yet, so nothing to merge in from other reviewers.

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01, SQLSTATE 40001, Oracle ORA-00060). The old flush retry replayed executeBatch without rolling back, so the replay hit an aborted transaction or re-sent a partially submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(), and when the executor can retain its logical records and the connection owns its own per-flush commit (commitOnFlush), roll back, clear the driver batch and replay; otherwise propagate the exception. Also add an opt-in upsert_batch_key_ordered primary-key sort for the generated upsert path, and clear child batches on close after a failed flush.
One-sentence summary: make deadlock retries replay a whole flush transaction instead of a half-submitted batch, and optionally reduce deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

Runtime path:

JdbcSinkWriter(commitOnFlush = !checkpointEnabled)  [JdbcSinkWriter.java:130]
  -> JdbcOutputFormat.writeRecordWithAutoFlush -> flush()                      [JdbcOutputFormat.java:~190-295]
      for i in 0..max_retries:
        attemptFlush() -> BufferReduced/Buffered.executeBatch()  -> driver batch -> 40P01 / 40001 / ORA-00060
        catch SQLException e:
          recordFlushException(e)            // format.flushFailed = true, flushException set
          isRetryableTransactionConflict -> canRetryTransactionConflict()   [:223, :461]
              commitOnFlush && executor.supportsTransactionConflictRetry() && !autoCommit
              false -> throw FLUSH_DATA_FAILED (checkpoint / XA scope: upper layer replays the whole txn)
          rollbackAfterTransactionConflict()  [:238, :473]  -> connection.rollback(); executor.resetForTransactionConflictRetry()
          if (i >= max_retries) throw                       [:273]
          sleepBeforeFlushRetry(1000 * i); next iteration replays the retained buffer
close() -> flushBufferedRecords (skipped when format.flushFailed) -> closeStatements() -> executor.closeStatements()  [:351-355, :381-384]

Key findings:

The normal path of a batch job (no checkpoint, so commitOnFlush = true) reaches the new logic; a streaming job with checkpointing intentionally does not retry (the exception propagates and the checkpoint restore replays the transaction). The doc change for max_retries states this accurately.
Before, a conflict with max_retries > 0 was retried without rollback; on PostgreSQL the transaction is already aborted, so that retry could not succeed. Failing fast in the checkpoint scope is therefore not a functional regression.
The rollback + reset step runs for every classified conflict, including the attempt where retries are already exhausted (see Issue 1).
The sort: compareKeys is lexicographic over the key fields, orders by class name first when types differ, and puts nulls last, so it is a valid total order; it is applied to a copy of the reduced buffer, so the per-key reduction semantics (one entry per key) are unchanged and deletes/upserts on distinct keys commute.

1.2 Compatibility Impact

Fully compatible with one intentional behavior change: new option upsert_batch_key_ordered defaults to false; JdbcBatchStatementExecutor gets two default methods (existing implementors keep compiling); the 4-argument BufferReducedBatchStatementExecutor constructor is preserved. The behavior change is that a classified transaction conflict now fails fast when it cannot be replayed safely (checkpoint scope or an executor without retry support) instead of entering the generic retry loop; this is documented in docs/en and docs/zh.

1.3 Performance / Side-Effect Analysis

The opt-in sort copies the buffer into an ArrayList and sorts it once per flush (O(n log n) over batch_size, default 1000), which is negligible. No extra memory beyond one reference list. The first retry runs with sleepBeforeFlushRetry(1000 * 0), i.e. no backoff (Issue 3).

1.4 Error Handling and Logging

Conflict classification handles the BatchUpdateException chain through findSqlExceptions. The new error messages carry context. See Issue 1 for the one path where the error handling leaves the executor in a state that contradicts the failure.

Issue 1: Conflict rollback resets the executor's `flushFailed` latch even when no retry follows, so `close()` can replay the failed flush

Location: JdbcOutputFormat.java:238 and :273; BufferReducedBatchStatementExecutor.java:223-227; BufferedBatchStatementExecutor.java:82-85; BufferReducedBatchStatementExecutor.java:194-208
Problem description: flush() calls rollbackAfterTransactionConflict() (which calls resetForTransactionConflictRetry() and sets the executor's flushFailed = false) before it checks i >= max_retries at :273. With the default max_retries = 0 in a batch job, or after the last retry, the conflict is rethrown, but the executor now believes its last flush did not fail. The writer then closes: JdbcOutputFormat.close() skips its own flush because the format-level flushFailed is true, but closeStatements() (:355) reaches BufferReduced/Buffered.closeStatements(), whose guard is !buffer.isEmpty() && !flushFailed. That guard is now true, so the executor runs executeBatch() on the retained buffer in a new transaction, on a connection we just rolled back, after the sink already reported failure.
Potential risk: The existing contract (see the comment in BufferExecutorTest.testCacheAlwaysExistWhenInsertFailed: "closeStatements should not flush a batch that already failed") is that a failed flush is never silently re-executed on close. On PostgreSQL/MySQL the uncommitted replay is discarded when the connection closes, so the cost is extra DB work and possibly another lock wait/deadlock during close. My inference (not reproduced) from the Oracle JDBC driver behavior is that Connection.close() with auto-commit off commits the pending transaction, which would make the replayed flush durable even though the task failed; that matters because ORA-00060 is explicitly in scope for this PR.
Best improvement: Option A (smallest): in flush(), only call rollbackAfterTransactionConflict() when another attempt will actually follow (i < maxRetries); when retries are exhausted, still rollback() the connection but do not call resetForTransactionConflictRetry(). Option B: keep the reset where it is, but make resetForTransactionConflictRetry() clear only the child driver batches, and clear flushFailed at the start of the next executeBatch() attempt (it is already set to false on success at BufferReduced:128/Buffered:62), so the latch stays set whenever the flush loop gives up. Please also add a test: conflict with max_retries = 0, then close(), and assert the executor's executeBatch is not called a second time.
Severity: High (blocker: it defeats an existing failure latch on the default-config failure path)
Raised by another reviewer: No

Issue 2: The new `else if (flushFailed)` branches in both `closeStatements()` methods are not covered by any test

Location: BufferReducedBatchStatementExecutor.java:198-203, BufferedBatchStatementExecutor.java:92-96
Problem description: The PR description lists "Prevents failed child JDBC batches from being executed again during close" as a behavior, but the tests never assert that clearBatch() is called on the children when closeStatements() runs after a failed executeBatch(). The existing RecordingExecutor/BufferExecutorTest doubles have a no-op clearBatch, so deleting both branches leaves every test green.
Potential risk: The safeguard can silently regress. This is also the area where Issue 1 lives.
Best improvement: Add a case that fails executeBatch(), calls closeStatements(), and asserts the child clearBatch() call count and that no second executeBatch() happens.
Severity: Medium
Raised by another reviewer: No

Issue 3: First conflict retry has no backoff, and there is no jitter

Location: JdbcOutputFormat.java:291 (sleepBeforeFlushRetry((long) sleepMs * i))
Problem description: On the first retry i = 0, so the sleep is 0 ms. The deadlock victim re-runs immediately while the surviving writer may still hold locks, and two writers that retry in lockstep can deadlock again. The sleep is also deterministic across writers.
Potential risk: Extra conflicts and burned retries under heavy contention, which is exactly the workload this PR targets.
Best improvement: For conflict retries, sleep a small randomized interval (for example sleepMs * (i + 1) plus jitter) so competing writers desynchronize.
Severity: Low (non-blocking)
Raised by another reviewer: No

Issue 4: The integration test exercises a hand-written executor, not the production executors or the new option

Location: JdbcPostgresIT.java testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions and DeadlockExecutor
Problem description: The IT proves the JdbcOutputFormat rollback-and-retry loop against a real PostgreSQL 40P01 (nicely done: the barrier makes the deadlock deterministic), but its DeadlockExecutor.resetForTransactionConflictRetry() is a no-op and it never goes through BufferReducedBatchStatementExecutor / BufferedBatchStatementExecutor or the connector config. upsert_batch_key_ordered and the factory/config wiring (JdbcSinkFactory, JdbcSinkConfig) have no test beyond the executor unit test, and the sort test does not cover deletes interleaved with upserts, null keys, byte[] keys or composite keys.
Potential risk: A wiring mistake (option not reaching the executor, or the reset not clearing the real driver batch) would not be caught.
Best improvement: Drive the same two-connection deadlock through a real upsert executor (or a small E2E job with max_retries set), and add one executor test with mixed upsert/delete rows plus a null and a composite key under keyOrdered = true.
Severity: Medium (non-blocking)
Raised by another reviewer: No

Issue 5: New private core methods have no explanatory comments

Location: JdbcOutputFormat.java:461 (canRetryTransactionConflict), :473 (rollbackAfterTransactionConflict), :482 (isRetryableTransactionConflict)
Problem description: These carry the transaction-boundary rules (why commitOnFlush and !autoCommit are required, why ORA-00060 is matched by vendor code plus message, why 40001 is treated as a conflict). Only a single inline comment exists.
Best improvement: Add short Javadoc stating the retry preconditions and the SQLSTATE/vendor-code table.
Severity: Low (non-blocking)
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

License headers and formatting are fine; public Javadoc on the new interface methods and compareKeys is good. See Issue 5 for the private methods.

2.2 Test Coverage and Test Stability

Unit tests (JdbcOutputFormatReconnectSafetyTest: retry-after-rollback asserts exactly one rollback() and two executeBatch calls; checkpoint-scope case asserts no rollback and one attempt; BufferReducedBatchStatementExecutorTest: key order, retained buffer after reset) would fail if the production logic were removed, which is what I want from them. Coverage gaps are Issue 2 and Issue 4, plus the exhausted-retries/close case from Issue 1.

Test-stability rating: Stable. The unit tests are mock-based with no sleeps (NoSleepJdbcOutputFormat). The IT uses a CyclicBarrier with 30 s/60 s timeouts and relies on PostgreSQL's default deadlock_timeout of 1 s, which leaves wide margin; it creates and drops its own table.

2.3 Documentation Updates

docs/en and docs/zh connectors/sink/Jdbc.md are both updated and consistent (new option row, option section, max_retries wording).

3. Architectural Soundness

3.1 Elegance of the Solution

A precise fix with a sound design: the executor owns the logical records, the output format owns the transaction boundary. Once Issue 1 is addressed the lifecycle is coherent.

3.2 Maintainability

The retry preconditions are spread across JdbcOutputFormat and two executors; a short Javadoc (Issue 5) would help future readers.

3.3 Extensibility

The default supportsTransactionConflictRetry() returning false is a good opt-in contract for other executors.

3.4 Historical-Version Compatibility

Compatible with existing jobs and configs; no serialization or protocol change.

4. Issue Summary

#	Issue	Location	Severity
1	Conflict rollback resets the `flushFailed` latch even when no retry follows; `close()` can replay the failed flush	`JdbcOutputFormat.java:238,273`; `BufferReducedBatchStatementExecutor.java:223-227`; `BufferedBatchStatementExecutor.java:82-85`	High (blocker)
2	New `closeStatements()` failed-flush branches are untested	`BufferReducedBatchStatementExecutor.java:198-203`; `BufferedBatchStatementExecutor.java:92-96`	Medium
4	IT does not exercise the real executors or `upsert_batch_key_ordered` wiring	`JdbcPostgresIT.java`	Medium
3	No backoff/jitter on the first conflict retry	`JdbcOutputFormat.java:291`	Low
5	Private core methods lack comments	`JdbcOutputFormat.java:461,473,482`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- Issue 1 (High): do not reset the executor's failure latch unless a retry actually follows, and add a close-after-exhausted-retries test.
Recommended fixes - non-blocking
- Issues 2 and 4 (Medium): add tests that fail when the failed-flush clearing or the executor wiring regresses. Issues 3 and 5 (Low): backoff with jitter, and Javadoc on the new private methods.

Overall assessment: the direction and the transaction-scope split are right and the unit tests have real failing power; the one thing I would not merge without is the close-path latch. A better alternative for Issue 1 is Option B (clear flushFailed at the start of the next attempt rather than at rollback time), which keeps the latch correct for every exit path of the retry loop.

CI: the apache-side Build check is green on this head (0a5e9c98e1). The branch is 15 commits behind dev and 1 ahead (compare status: diverged) with no merge conflicts; GitHub shows the merge state as blocked, which here means a committer approval is still required (my account is comment-only). Please sync the latest dev when you push the fix for Issue 1 so the next CI run is on a current base.

ryanmeowy

Read the current head (968c33fd9) against the earlier review. The replay path looks correct now:

resetForTransactionConflictRetry() only drops the driver's partial batch; it leaves the logical buffer and the flushFailed latch alone (BufferedBatchStatementExecutor.java:83-85, BufferReducedBatchStatementExecutor.java:224-227). So closeStatements() still takes the else if (flushFailed) branch (:92-96 / :199-204) and cannot re-submit the failed batch.
!transactionConflict at JdbcOutputFormat.java:278 keeps the executor — and the buffer it still holds — in place across a conflict retry, so the next attempt replays the same rows; without it a fresh executor would silently drop the pending buffer. The flushFailed latch is the separate piece that stops close() from replaying once the retries are exhausted.
testExhaustedDeadlockRetryDoesNotReplayBufferedBatchOnClose, testExhaustedDeadlockRetryDoesNotReplayReducedBatchOnClose and testTransactionConflictResetRetainsLogicalBuffer pin exactly that path. The new IT does produce a real PostgreSQL 40P01 and asserts the retried flush lands both rows, though it still runs a test-local executor, so it covers the format-level retry rather than the sink wiring the earlier review asked about.

Two things I'd still look at:

Sorting the merged entries (BufferReducedBatchStatementExecutor.java:98-104) is the right layer — the per-key reduce still decides the last write. One cost worth a sentence in the option description: the loop flushes the pending child executor on every change-flag transition (:107-118), and a key-sorted order interleaves upserts and deletes, so a batch that mixes them can now issue one child executeBatch() per transition instead of at most two — more round trips inside the same flush. If that ever shows up, grouping by change flag first and sorting keys inside each group caps it at two; the price is that the strictest lock order is lost when two transactions disagree on whether a key is an upsert or a delete, so the full key sort is the safer default. I'd just document the trade-off.
The retry is gated on commitOnFlush && !connection.getAutoCommit() && supportsTransactionConflictRetry() (JdbcOutputFormat.java:466-476), and the last one is true only for the two buffered upsert executors (interface default is false, JdbcBatchStatementExecutor.java:47-49). So a plain insert sink now fails fast on 40P01/40001 where the old loop retried the same batch, and a checkpointed job propagates the conflict instead. Both look right to me, but the max_retries paragraph in the docs only describes the checkpoint case — one clause about the "upsert + manual commit" scope would save the next reader the same trace.

compareFields falling back to the class name (:168-171) is what makes the order deterministic when a malformed row mixes numeric and textual values, so testCompareKeysIsTransitiveAcrossDifferentTypes covers the right thing.

DanielLeens

Thanks for the quick update, @yzeng1618, and thanks @ryanmeowy for the extra pass. I re-reviewed the full diff of the current head (968c33fd9) against dev, not only the delta. The latch problem from my last round is fixed. I also owe you an apology: I missed one compatibility item last time, and it is a carryover from the previous version rather than something new in this commit (details in Issue 1). Sorry for that, that is on me.

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01, SQLSTATE 40001, Oracle ORA-00060). The old retry replayed executeBatch without rolling back, so on a manual-commit connection it re-ran inside an aborted transaction or re-sent a partially submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(); when the executor can retain its logical records and the flush owns its own commit, roll back, clear the driver batch and replay; otherwise propagate. Add an opt-in upsert_batch_key_ordered primary-key sort for the generated upsert path.
One-sentence summary: replay a whole flush transaction after a deadlock instead of a half-submitted batch, and optionally lower deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review (ef1aa6718 -> 968c33fd9, second commit only; the first commit is byte-identical to what I reviewed):

before: resetForTransactionConflictRetry() { child.clearBatch(); flushFailed = false; }   // latch cleared at rollback time
after : resetForTransactionConflictRetry() { child.clearBatch(); }                         // latch kept
        executeBatch() { flushFailed = false; ... }                                        // latch cleared only when a new attempt starts
        BufferReducedBatchStatementExecutor.java:93, BufferedBatchStatementExecutor.java:56

Runtime path of the default max_retries = 0 batch-job failure, which was my blocker:

JdbcOutputFormat.flush()                         [JdbcOutputFormat.java:199]
  attemptFlush() -> executor.executeBatch() -> SQLException 40P01
     executor: flushFailed = true                [BufferReduced:131, Buffered:65]
  canRetryTransactionConflict() == true          [:223, :466]
  rollbackAfterTransactionConflict()             [:238, :482]  connection.rollback(); child.clearBatch()  (latch untouched)
  i >= maxRetries -> throw FLUSH_DATA_FAILED
close() -> flushBufferedRecords() skipped (format.flushFailed)
        -> closeStatements() -> executor.closeStatements()
              !buffer.isEmpty() && !flushFailed == false  -> takes `else if (flushFailed)` -> clearBatch only

I traced this and confirmed the close path no longer re-executes the failed flush, and that on a real retry the next executeBatch() re-adds the retained buffer to a cleared driver batch after the rollback, so the replay is whole-transaction. This closes my previous Issue 1 (blocker) and Issue 5 (the new Javadoc on the three private methods is good). The new tests testExhaustedDeadlockRetryDoesNotReplayBufferedBatchOnClose / ...ReducedBatchOnClose assert executeBatch is called exactly once across flush plus close, so they would fail if the latch were cleared at rollback time again. I also agree with @ryanmeowy's reading of the replay path.

1.2 Compatibility Impact

Partially incompatible (see Issue 1). New option upsert_batch_key_ordered defaults to false; the two new JdbcBatchStatementExecutor default methods keep existing implementors compiling; the 4-argument BufferReducedBatchStatementExecutor constructor is kept.

1.3 Performance / Side-Effect Analysis

The opt-in sort is one ArrayList copy plus an O(n log n) sort per flush over batch_size, so it is negligible. One cost @ryanmeowy already pointed out, which I agree is worth a sentence in the option description: a key-sorted order interleaves upserts and deletes, and the loop executes the pending child batch on every change-flag transition (BufferReducedBatchStatementExecutor.java:107-118), so a mixed batch can issue more child executeBatch() round trips than before.

1.4 Error Handling and Logging

Conflict classification and the new error messages carry enough context. The first conflict retry still sleeps 0 ms (Issue 3).

Issue 1 (carryover from my previous review): the new fail-fast rule also changes the default `auto_commit = true` path, which used to retry deadlocks

Location: JdbcOutputFormat.java:219-236 and :466-477 (canRetryTransactionConflict); JdbcSinkOptions.AUTO_COMMIT default true; JdbcSinkWriter.java:130 (commitOnFlush = !checkpointEnabled)
Problem description: canRetryTransactionConflict() returns true only when commitOnFlush is set, the executor is one of the two buffered upsert executors, and connection.getAutoCommit() is false. Every other combination now throws FLUSH_DATA_FAILED on the first 40P01/40001/ORA-00060, even when max_retries > 0. On dev, the same conflict fell into the generic retry loop and replayed the retained buffer. With the default auto_commit = true each statement is committed by the driver, there is no transaction spanning flushes, and the buffered upsert executors are idempotent, so that old retry did work for users who set max_retries > 0 (for example a PostgreSQL upsert sink, or a MySQL upsert sink where a deadlock is reported as SQLSTATE 40001). I wrote last time that failing fast "is not a functional regression", and that was too broad: it is true for a manual-commit connection, but I did not account for the default auto-commit connection. The docs only describe the checkpoint-transaction case, and @ryanmeowy noted the plain insert sink as well.
Potential risk: A job that survived occasional deadlocks through max_retries now fails on the first one and, in streaming mode, restarts from the last checkpoint. That changes the behavior of an existing configuration without any migration note.
Best improvement: Option A (smallest, keeps legacy semantics): fail fast only where replay is provably unsafe, that is on a manual-commit connection whose transaction spans flushes (!commitOnFlush, XA) or an executor that cannot replay on a manual-commit connection; when the connection is in auto-commit mode keep the previous generic retry. Option B: keep the current behavior but document it precisely (preconditions: auto_commit=false, no checkpoint, generated upsert path; everything else fails fast) in docs/en and docs/zh max_retries, and add an entry to docs/en/introduction/concepts/incompatible-changes.md with a migration note. I lean to Option A because it needs no migration, but I would also be fine with B if you prefer the stricter contract, as long as it is written down.
Severity: High (compatibility; blocks merge until A or B is chosen)
Raised by another reviewer: Partially (@ryanmeowy raised the plain insert sink and the docs gap, not the auto-commit default)

Issue 2: the failed-flush branches of `closeStatements()` are still only partly pinned

Location: BufferReducedBatchStatementExecutor.java:199-204, BufferedBatchStatementExecutor.java:92-96, JdbcOutputFormatReconnectSafetyTest.java (assertExhaustedDeadlockRetryDoesNotReplayOnClose)
Problem description: The new tests do catch the latch regression (the times(1) on executeBatch is the real guard). The clearBatch assertion is atLeastOnce(), and resetForTransactionConflictRetry() already calls clearBatch() once before close, so deleting the else if (flushFailed) branches would still leave the tests green.
Best improvement: assert the exact clearBatch() count (reset plus close) for the exhausted-retry case, or add one case where executeBatch() fails with a non-conflict error and closeStatements() must clear the child batch.
Severity: Low
Raised by another reviewer: No

Issue 3: first conflict retry has no backoff or jitter (carryover, unchanged)

Location: JdbcOutputFormat.java:291
Problem description: i = 0 on the first retry, so the sleep is 0 ms and competing writers retry in lockstep.
Best improvement: a small randomized delay for conflict retries.
Severity: Low
Raised by another reviewer: No

Issue 4: the integration test still uses a test-local executor (carryover, unchanged)

Location: JdbcPostgresIT.java testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions
Problem description: It produces a real 40P01 and proves the format-level rollback/replay, but it never goes through BufferReducedBatchStatementExecutor / BufferedBatchStatementExecutor or upsert_batch_key_ordered wiring (JdbcSinkFactory, JdbcSinkConfig, JdbcOutputFormatBuilder). Also, this IT has not run on the current head yet, because the fork's unit-test lane failed first and the IT lanes were skipped (see CI).
Best improvement: drive the same two-connection deadlock through a real upsert executor, and add an executor case with mixed upserts and deletes plus a null and a composite key under keyOrdered = true.
Severity: Medium (non-blocking)
Raised by another reviewer: Yes (@ryanmeowy, same observation)

2. Code Quality Assessment

2.1 Coding Standards

ASF headers, formatting and the Javadoc on the new interface methods, compareKeys and the three private methods are fine.

2.2 Test Coverage and Test Stability

Stability rating: Stable. The unit tests are mock based with no sleeps (NoSleepJdbcOutputFormat); the IT uses a CyclicBarrier with generous timeouts and creates and drops its own table. Coverage gaps are Issue 2 and Issue 4, plus a test for whichever way Issue 1 is resolved (for example: auto_commit = true, conflict, max_retries = 1 -> expect a retry under Option A, or an explicit fail-fast assertion under Option B).

2.3 Documentation Updates

docs/en and docs/zh connectors/sink/Jdbc.md are updated and consistent. The max_retries paragraph needs the preconditions from Issue 1 in both languages, and the option paragraph for upsert_batch_key_ordered could mention the extra round trips from 1.3.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix for the manual-commit case: the executor owns the logical records, the output format owns the transaction boundary.

3.2 Maintainability

Good now that the private methods are documented.

3.3 Extensibility

supportsTransactionConflictRetry() defaulting to false is a safe opt-in contract.

3.4 Historical-Version Compatibility

No serialization, state or protocol change. The behavior change in Issue 1 is the only compatibility concern.

4. Issue Summary

#	Issue	Location	Severity
1	Fail-fast also removes the previous deadlock retry for `auto_commit=true` and non-replayable executors (carryover)	`JdbcOutputFormat.java:219-236,466-477`	High
4	IT does not exercise the real executors or option wiring	`JdbcPostgresIT.java`	Medium
2	`closeStatements()` failed-flush branches only loosely asserted	`BufferReducedBatchStatementExecutor.java:199-204`, `BufferedBatchStatementExecutor.java:92-96`	Low
3	No backoff/jitter on first conflict retry	`JdbcOutputFormat.java:291`	Low

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must be fixed
- Issue 1: either keep the legacy retry for the auto-commit path (Option A) or document the narrower contract and add an incompatible-changes entry (Option B).
Recommended fixes - non-blocking
- Issue 4 (Medium), Issues 2 and 3 (Low), and the round-trip sentence in 1.3.

CI on this head (968c33fd9): the fork's Build run finished red, but the only failing test is SavePointTest.testSavePointButJobGoingToFail in seatunnel-engine-server (ConditionTimeout), an engine cancel/savepoint test this PR does not touch. The JDBC unit tests (JdbcOutputFormatReconnectSafetyTest 8/8, BufferReducedBatchStatementExecutorTest 4/4) passed in that run, but because unit-test failed first, the JDBC integration lanes were skipped, so JdbcPostgresIT has not run on this head. Please retry only the failed job (for example gh run rerun <run-id> --failed, or "Re-run failed jobs" in the Actions UI) rather than the whole workflow. The branch is up to date with dev (0 behind) and has no merge conflicts; GitHub shows blocked because a committer approval is still required.

Overall assessment: the latch fix is exactly what I hoped for, and the transaction-scope split is the right design. The remaining decision is how to treat the auto-commit default, and I am happy to help review either option quickly.

DanielLeens

Thanks for the fast turnaround, @yzeng1618! I re-reviewed the full diff of the current head (0a9f92ab6) against dev, not only the delta since my last round (968c33fd9). The compatibility blocker I raised (Issue 1 last time) is addressed in a way that matches what I called Option A, and the Javadoc/test nits were picked up too. Thank you. What is left is a merge conflict plus one duplicated change that is the likely cause of it. This is a static review; I did not execute the PR.

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01, SQLSTATE 40001, Oracle ORA-00060). The old retry replayed executeBatch without rolling back, so on a manual-commit connection it re-ran inside an aborted transaction or re-sent a partially submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(); when the flush owns its own commit and the executor can retain its logical records, roll back, clear the driver batch and replay; otherwise propagate. Add an opt-in upsert_batch_key_ordered primary-key sort for the generated upsert path.
One-sentence summary: replay a whole flush transaction after a deadlock instead of a half-submitted batch, and optionally lower deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review (4 new commits; two are formatting-only):

before: canRetryTransactionConflict() false  -> always throw FLUSH_DATA_FAILED   (also for auto_commit=true)
after : canRetry == false && !isAutoCommitConnection() -> throw
        canRetry == false &&  isAutoCommitConnection() -> fall through to the legacy generic retry   [JdbcOutputFormat.java:219-236, 489-493]
        retry sleep for conflicts: sleepMs*(i+1) + random(0..sleepMs)                                [JdbcOutputFormat.java:296-302]

Runtime path for the three connection shapes:

flush() -> attemptFlush() -> executor.executeBatch() -> SQLException 40P01
  (a) auto_commit=true            : canRetry=false, autoCommit=true  -> transactionConflict=false -> legacy loop
                                    (shouldRefreshExecutor / updateExecutor as on dev), same as before this PR
  (b) auto_commit=false, flush-owned commit, buffered upsert executor
                                  : canRetry=true -> rollback() + child.clearBatch() -> retain buffer -> replay whole flush
  (c) checkpoint / XA / non-replayable executor on manual commit
                                  : throw FLUSH_DATA_FAILED -> upper layer (checkpoint recovery) replays the full transaction

Key findings:

The auto-commit default path keeps its previous max_retries behavior, so the carryover compatibility issue is closed. testAutoCommitDeadlockKeepsGenericRetryBehavior asserts no rollback() and two executeBatch calls, which would fail if the fail-fast rule leaked back into auto-commit.
The else if (flushFailed) branch in closeStatements() is now pinned by times(2) on clearBatch() (reset plus close), so removing the branch turns the test red. That closes my previous Issue 2.
Conflict retries now back off with jitter, which closes my previous Issue 3.
The docs for max_retries and upsert_batch_key_ordered (en and zh) now describe the auto-commit rule, the manual-commit rule and the extra child round trips. That covers the points @ryanmeowy and I raised.

1.2 Compatibility Impact

Fully compatible. The new option defaults to false; the two new JdbcBatchStatementExecutor default methods keep existing implementors compiling; auto-commit users keep the old retry; manual-commit users who previously got an unsafe partial replay now either get a correct whole-flush replay or a propagated error that the checkpoint mechanism handles. I found no serialization, state or protocol change.

1.3 Performance / Side-Effect Analysis

The opt-in sort is one ArrayList copy plus an O(n log n) sort over at most batch_size entries per flush, so it is negligible. The extra child executeBatch() round trips for mixed upsert/delete batches are now documented. The conflict jitter adds at most sleepMs of delay per retry and only when a conflict was detected.

1.4 Error Handling and Logging

Conflict classification and the error messages carry enough context.

Issue 1: the PR cannot be merged as is (merge conflict), and the cause looks like a duplicated change

Location: BufferReducedBatchStatementExecutor.java (bufferKey(...), Triple buffer, near :45-110) and its tests (testRowsWithNullKeyAreNotReduced, testDeleteWithNullKeyKeepsOrderWithUpserts, testRowsWithSameKeyAreReduced); GitHub reports mergeStateStatus = DIRTY, 37 commits behind dev
Problem description: The latest compatibility commit copies the NULL-key handling (bufferKey, Pair -> Triple) that already landed on dev via #12803. That is outside the scope of this PR (deadlock retry), and because dev has the same lines edited differently, the branch now conflicts in four files.
Potential risk: A hand-merged copy of the same logic can silently diverge from dev, and any conflict resolution that keeps the older Pair form would drop #12803's behavior.
Best improvement: Rebase or merge the latest dev, resolve by taking dev's version of the NULL-key code and re-applying only this PR's deadlock/ordering changes on top (the key-ordered sort should sort on getMiddle() of the Triple, as it does now). Then drop the duplicated NULL-key tests from this PR, since #12803 already carries its own coverage. Please re-check git diff origin/dev afterwards shows no NULL-key lines.
Severity: High (blocks merge until the conflict is resolved)
Raised by another reviewer: No

Issue 2: the integration test still uses a test-local executor (carryover)

Location: JdbcPostgresIT.java, testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions
Problem description: It produces a real 40P01 and proves the format-level rollback and replay, but it does not go through BufferReducedBatchStatementExecutor / BufferedBatchStatementExecutor or the upsert_batch_key_ordered wiring in JdbcSinkFactory, JdbcSinkConfig and JdbcOutputFormatBuilder. It also has not run on this head yet: the fork's Build run was still in progress when I looked.
Best improvement: drive the same two-connection deadlock through a real upsert executor, and add one case with mixed upserts and deletes under keyOrdered = true.
Severity: Medium (non-blocking)
Raised by another reviewer: Yes (@ryanmeowy, same observation)

2. Code Quality Assessment

2.1 Coding Standards

ASF headers, formatting and Javadoc on the new interface methods and private helpers are fine. The last two commits are formatting-only (the large whole-file diff in e86c1fe is normalized in the final head; line endings in the final tree are LF, same as dev).

2.2 Test Coverage and Test Stability

Stability rating: Stable. Unit tests are mock based with no sleeps (NoSleepJdbcOutputFormat); the IT uses a CyclicBarrier with generous timeouts and creates and drops its own table. Remaining gap is Issue 2.

2.3 Documentation Updates

docs/en and docs/zh connectors/sink/Jdbc.md are updated consistently for max_retries and upsert_batch_key_ordered.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix: the executor owns the logical records, the output format owns the transaction boundary, and the auto-commit path is left on its legacy semantics.

3.2 Maintainability

Good. The retry decision is now split across canRetryTransactionConflict() and isAutoCommitConnection(); a short comment at the call site explaining why the auto-commit case falls through to the legacy loop would help the next reader.

3.3 Extensibility

supportsTransactionConflictRetry() defaulting to false is a safe opt-in contract for other executors.

3.4 Historical-Version Compatibility

No behavior change for existing configurations (see 1.2).

4. Issue Summary

#	Issue	Location	Severity
1	Merge conflict with `dev`; NULL-key change from #12803 duplicated in this PR	`BufferReducedBatchStatementExecutor.java`, `BufferReducedBatchStatementExecutorTest.java`	High
2	IT does not exercise the real executors or option wiring (carryover)	`JdbcPostgresIT.java`	Medium

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers - must fix
- Issue 1: sync with latest dev, resolve conflicts by keeping dev's NULL-key implementation, and remove the duplicated NULL-key changes and tests from this PR.
Recommended fixes - non-blocking
- Issue 2 (drive the IT through a real executor).
CI: the fork Build run on this head was still in progress when I checked, so there is no conclusive result yet. Please also re-check CI after the sync; if a job fails on an unrelated test, retry only the failed job.

Overall assessment: the design is sound and the previous review points are all addressed. Once the conflict is resolved and the duplicated change is removed, I expect this to be close to ready. Thanks again for the careful iterations!

DanielLeens · 2026-10-20T11:08:21Z

Quick status update so this thread does not sit on a stale CI fact. In my last review I noted that the fork Build for head 0a9f92ab66 was still in progress; it has now completed and is passing.

The blocker from that review is unchanged, though: GitHub reports this branch as having merge conflicts with dev (mergeable_state: dirty, 45 commits behind). Please sync with the latest dev, resolve the conflicts by keeping dev's NULL-key implementation, and drop the duplicated NULL-key changes and tests from this PR, as described in Issue 1 of my review. After that, please let the required checks rerun on the new head and I will take another pass. The green Build result does not clear the merge-state blocker.

DanielLeens

Thanks for the quick rebase, @yzeng1618, and for picking up my earlier points! I re-reviewed the full diff of the current head (a7bd94a7f) against dev, not only the delta since my last round (0a9f92ab6). The branch is now 3 commits ahead and 0 behind dev, so the merge conflict from last time is gone. Static review only; nothing was executed locally.

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01/40001, Oracle ORA-00060). The old retry replayed executeBatch without rolling back, so on a manual-commit connection it re-ran inside an aborted transaction or re-sent a half-submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(); when the flush owns its commit boundary and the executor can keep its logical records, roll back, clear only the driver batch, back off with jitter and replay; otherwise propagate. Add an opt-in upsert_batch_key_ordered primary-key sort for the generated upsert path.
One-sentence summary: replay a whole flush transaction after a deadlock instead of a half-submitted batch, and optionally lower deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

What changed since my last review: the branch was rebased on current dev. The duplicated NULL-key handling I flagged is gone; BufferReducedBatchStatementExecutor.bufferKey(...) is now exactly dev's version (identity key for rows with a NULL key field) and the sort is applied on top of it using the Triple middle element. The PR's own test diff contains no NULL-key tests anymore. That closes my previous Issue 1.

Runtime path for the three connection shapes (re-verified in JdbcOutputFormat.flush(), lines ~215-300, and canRetryTransactionConflict() / isAutoCommitConnection() / rollbackAfterTransactionConflict()):

flush() -> attemptFlush() -> executor.executeBatch() -> SQLException 40P01 / 40001 / ORA-00060
  (a) auto_commit=true             : canRetry=false, autoCommit=true -> legacy generic retry loop (same as dev)
  (b) auto_commit=false, commitOnFlush=true (no checkpoint), executor.supportsTransactionConflictRetry()
                                   : connection.rollback() + executor.resetForTransactionConflictRetry()
                                     -> logical buffer retained -> sleep (i+1)*1s + rand(0..1s) -> replay the whole flush
  (c) manual commit but checkpoint/XA-owned transaction, or executor without support
                                   : throw FLUSH_DATA_FAILED -> checkpoint recovery replays the full transaction

Key findings:

Buffer retention is correct: BufferReducedBatchStatementExecutor.executeBatch() and BufferedBatchStatementExecutor.executeBatch() only clear their logical buffer after the child executeBatch() succeeds, so a replay after rollback re-sends every record. resetForTransactionConflictRetry() clears only the child driver batches, as the Javadoc on JdbcBatchStatementExecutor requires.
A mixed upsert/delete batch that fails halfway (for example the upsert child committed its batch in the same open transaction, then the delete child deadlocks) is rolled back as a whole and replayed in full, so there is no partial-state replay.
closeStatements() no longer re-executes a failed batch (flushFailed branch calls clearBatch() on the children), and executeBatch() resets flushFailed at entry so a successful replay is not treated as failed.
compareKeys is a deterministic, transitive ordering (type name first, then Comparable, byte arrays unsigned, NULL last). List.sort is stable, and rows with NULL keys keep a stable relative order. Sorting is only on the opt-in path.
isRetryableTransactionConflict matches 40P01, 40001 and Oracle ORA-00060; 40001 also covers MySQL deadlock errors. Lock-wait timeouts are intentionally not matched.

1.2 Compatibility Impact

Fully compatible. upsert_batch_key_ordered defaults to false and is registered in JdbcSinkFactory, JdbcSinkOptions and JdbcSinkConfig; the two new JdbcBatchStatementExecutor methods are default, so third-party executors keep compiling; the old 4-argument BufferReducedBatchStatementExecutor constructor is preserved. Auto-commit users keep the legacy retry. One behavioral note, not an issue: on manual-commit connections where the flush cannot own the transaction (checkpoint enabled, or an executor that does not opt in), a detected conflict now fails fast instead of retrying inside an already aborted transaction. On PostgreSQL that earlier retry would only have produced 25P02 anyway, so I do not consider it a regression. No serialization, checkpoint-state or protocol change.

1.3 Performance / Side-Effect Analysis

The opt-in sort is one ArrayList copy plus an O(n log n) sort over at most batch_size entries per flush, which is negligible. Sorting can interleave upserts and deletes by key, producing more child executeBatch() round trips for mixed batches; this is documented in Jdbc.md. Conflict backoff adds at most sleepMs of jitter per retry and only after a detected conflict, and the sleep is bounded by max_retries.

1.4 Error Handling and Logging

Conflict classification and error messages carry enough context, and a rollback failure is wrapped in a JdbcConnectorException with the cause.

Issue 1: the integration test still uses a test-local executor (carryover)

Location: JdbcPostgresIT.java, testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions and its DeadlockExecutor
Problem description: It produces a real 40P01 and proves the format-level rollback and replay, but it never goes through BufferReducedBatchStatementExecutor / BufferedBatchStatementExecutor, or through the upsert_batch_key_ordered wiring in JdbcSinkFactory -> JdbcSinkConfig -> JdbcOutputFormatBuilder. The unit tests cover those pieces with mocks, but no test exercises the real SQL path end to end.
Potential risk: a regression in the option wiring or in the real upsert executors' batch reset would not be caught against a real database.
Best improvement: drive the same two-connection deadlock through a real upsert executor, and add one case with mixed upserts and deletes under upsert_batch_key_ordered = true verifying final table contents.
Severity: Medium (non-blocking)
Raised by another reviewer: Yes (@ryanmeowy made the same observation earlier; I raised it before as well)

2. Code Quality Assessment

2.1 Coding Standards

ASF headers, formatting and Javadoc on the new interface methods and helpers are fine. The new private helpers explain why checkpoint/XA transactions must propagate the conflict.

2.2 Test Coverage and Test Stability

Rating: Stable. Unit tests in JdbcOutputFormatReconnectSafetyTest cover rollback plus batch reset, the unchanged auto-commit behavior, no single-flush replay inside a checkpoint transaction, and no replay on close after exhausted retries; BufferReducedBatchStatementExecutorTest covers key ordering and buffer retention. They use a no-sleep subclass. The IT relies on a CyclicBarrier with 30/60 second timeouts and creates and drops its own table, so I see no sleep, fixed-port or shared-state hazard. The remaining gap is Issue 1.

2.3 Documentation Updates

docs/en and docs/zh connectors/sink/Jdbc.md are updated consistently for max_retries and upsert_batch_key_ordered.

3. Architectural Soundness

3.1 Elegance

Precise fix: the executor owns the logical records, the output format owns the transaction boundary, and the auto-commit path keeps its legacy semantics.

3.2 Maintainability

Good. The retry decision is split across canRetryTransactionConflict() and isAutoCommitConnection(); a short comment at the call site explaining why the auto-commit case falls through to the legacy loop would help the next reader (optional).

3.3 Extensibility

supportsTransactionConflictRetry() defaulting to false is a safe opt-in contract for other executors.

3.4 Historical-Version Compatibility

No behavior change for existing configurations (see 1.2).

4. Issue Summary

Number	Issue	Location	Severity
1	IT does not exercise the real executors or option wiring (carryover)	`JdbcPostgresIT.java`	Medium

5. Merge Recommendation

Conclusion: Ready to merge

Blockers (must fix): none from the source side. The earlier merge conflict and the duplicated NULL-key change are resolved.
Recommended fixes (non-blocking): Issue 1 (drive the IT through a real upsert executor).
CI: on this head the apache-side Build is still pending and points to the fork run, which was in progress when I looked, so there is no conclusive result yet. Please let it finish; if a job fails on something unrelated to this change, retry only that failed job.

Overall assessment: the design is sound, all my previous blockers are addressed, and the conflict is resolved. Merge should wait for the Build result and the usual maintainer approval; this COMMENT is not an approval and does not claim that CI or the tests passed. Thank you for the careful iterations!

yzeng1618 · 2026-10-25T12:27:02Z

Hi @DanielLeens @ryanmeowy, I pushed 1ce3d2e to address the remaining Issue 1 (IT exercises only a test-local executor):

JdbcPostgresIT#testDeadlockRetryWithRealUpsertExecutor builds both writers through the production wiring (JdbcOutputFormatBuilder -> BufferReducedBatchStatementExecutor, manual commit with commitOnFlush). Two connections flush the same 40 pre-seeded keys in opposite primary-key order from a shared barrier, so PostgreSQL reports a real 40P01 for one flush; the test asserts the conflict via the pg_stat_database.deadlocks counter (the scenario retries up to 3 times in the rare case the batches do not overlap) and that every row was rewritten by the replay, so a lost-buffer regression in the real executors' batch reset would fail.
JdbcPostgresIT#testUpsertBatchKeyOrderedMixedUpsertAndDelete runs a mixed upsert/delete batch under upsert_batch_key_ordered=true and uses AFTER UPDATE/DELETE triggers to record the execution order, asserting the option reaches the real executor and entries land in primary-key order (U1, D2, U3, D4, U5) with the expected final table contents.

I also added the short comment at the auto-commit fall-through in JdbcOutputFormat.flush() suggested in section 3.2. spotless:apply is clean; CI is running on the new head.

yzeng1618 · 2026-10-25T17:25:46Z

CI status update: the updated-modules-integration-test-part-4 lane failed on my two new IT tests from 1ce3d2e. Root cause: I passed the PostgreSQL database name as the sink config database, so the generated upsert SQL addressed "<database>"."<table>" while the test DDL created the tables in the default search_path schema — every flush failed with relation does not exist. The unrelated-looking numeric_special_value job failure in the same log is expected behavior (that test asserts the job fails by default).

Fixed in ae3e188: tables are now created schema-qualified under public, and the sink config database is derived from the same qualified name, so the generated SQL and the DDL always address the same schema. I also raised the deadlock test's batch to 200 rows so the two opposite-order writers overlap more reliably. CI is running on the new head.

yzeng1618 · 2026-10-25T22:53:31Z

CI is green on the fixed head ae3e188: the fork Build run passed (5h23m), and in updated-modules-integration-test-part-4 both new integration tests ran and passed — testDeadlockRetryWithRealUpsertExecutor (observed a real PostgreSQL 40P01 and verified the rolled-back flush replayed every buffered row) and testUpsertBatchKeyOrderedMixedUpsertAndDelete (trigger-recorded order U1, D2, U3, D4, U5 under upsert_batch_key_ordered=true). JdbcPostgresIT: 36/36 passed. Ready for re-review whenever you have time.

DanielLeens

Thank you @yzeng1618 for the quick and thorough follow-up, and thank you @ryanmeowy for the earlier careful read. I confirm the replies in your comments: the integration tests now go through the production wiring (JdbcOutputFormatBuilder -> BufferReducedBatchStatementExecutor), the short comment on the auto-commit fall-through in JdbcOutputFormat.flush() is in, the relation does not exist problem from the schema/database mix-up is fixed in ae3e18842e, and the fork Build is green. I re-reviewed the complete diff of the current head ae3e18842e5c81ad853a5cbd74fb389dbe412669 (13 files, +938/-10) against dev, not only the three new commits (a7bd94a7f -> 1ce3d2e2a -> ae3e18842, which touch only a code comment and JdbcPostgresIT). Static review only; nothing was executed locally. Unfortunately, while checking whether the new test can really fail on the regression it is meant to catch, I found one gap that I should have asked about more precisely last round (see Issue 1).

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01/40001, Oracle ORA-00060). The old retry replayed executeBatch without rollback, so on a manual-commit connection it ran inside an aborted transaction or re-sent a half-submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(); when the flush owns its commit boundary and the executor can keep its logical records, roll back, clear only the driver batch, back off with jitter and replay the whole flush; otherwise propagate. Add an opt-in upsert_batch_key_ordered primary-key sort.
Summary: replay a whole flush transaction after a deadlock instead of a half-submitted batch, and optionally lower deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

Production code is unchanged since my previous review apart from the new comment at JdbcOutputFormat.java near line 223; my analysis from the last round still holds:

flush() -> attemptFlush() -> executor.executeBatch() -> SQLException 40P01 / 40001 / ORA-00060
  (a) auto_commit=true            : legacy generic retry loop (same as dev)
  (b) manual commit, commitOnFlush, executor.supportsTransactionConflictRetry()
                                  : rollback + resetForTransactionConflictRetry() -> logical buffer kept
                                    -> jittered backoff -> replay the whole flush
  (c) checkpoint/XA transaction or executor without opt-in
                                  : throw FLUSH_DATA_FAILED -> checkpoint recovery replays the transaction

Normal-path reach: shape (a) is the default for most sinks and is unchanged. Shape (b) is reached only by the two buffered upsert executors with manual commit and no checkpoint-owned transaction.
The logical buffers are cleared only after the child executeBatch() succeeds, resetForTransactionConflictRetry() clears only the driver batches, and closeStatements() does not re-execute a failed batch (flushFailed). The NULL-key behavior is exactly dev's, and the sort is applied on top of it.
Sort order is deterministic and transitive; it is only used when upsert_batch_key_ordered=true.

1.2 Compatibility Impact

Fully compatible. The option defaults to false and is registered in JdbcSinkFactory, JdbcSinkOptions and JdbcSinkConfig; the two new executor-interface methods are default; the old 4-argument executor constructor is kept; no serialization, checkpoint state or protocol change. On manual-commit connections where the flush cannot own the transaction, a conflict now fails fast instead of retrying inside an aborted transaction, which is documented in Jdbc.md (EN and ZH).

1.3 Performance / Side-Effect Analysis

The sort is one list copy plus O(n log n) over at most batch_size entries per flush, only when the option is on. Mixed upsert/delete batches can issue more child batches; this is documented. Backoff happens only after a detected conflict and is bounded by max_retries.

1.4 Error Handling and Logging

Rollback failures are wrapped with the cause; conflicts are classified narrowly (lock-wait timeouts intentionally not retried). No new issue in production code.

Issue 1: the new real-executor deadlock IT cannot fail on the regression it describes

Location: JdbcPostgresIT.testDeadlockRetryWithRealUpsertExecutor and runConcurrentUpsertDeadlockAttempt (the final assertions after both flush() calls).
Problem description: the test asserts that every row holds value 1 or 2, plus that the pg_stat_database.deadlocks counter went up. If the deadlock victim lost its buffered rows instead of replaying them (the exact regression the comment says it guards against), the winner's flush would still have written all 200 rows, so every row is still 1 or 2 and the counter still increased: the test stays green. A correct replay also yields a uniform table (the victim overwrites the winner), so the current check cannot distinguish the two outcomes.
Potential risk: the headline guarantee of this PR (rolled-back flush replays every record) is only protected by the mock-based unit tests; a regression in the real executors' batch reset would pass this IT.
Best improvement: record every successful write, for example with an AFTER INSERT OR UPDATE row trigger that appends to an audit table (the second IT already uses this technique), and assert exactly 2 * rowCount audit entries after a deadlock was observed (winner N plus victim replay N); a lost buffer would give N. Alternatively assert that all rows share a single value and that it equals the value written by the writer whose flush was retried (for example detected through a counting wrapper on the JdbcConnectionProvider or a rollback counter).
Severity: Medium
Raised by another reviewer: Related to @ryanmeowy's earlier note that the first IT did not cover the sink wiring; this is the remaining depth gap.

Issue 2: stability of the deadlock detection through `pg_stat_database`

Location: queryDeadlockCount() and runConcurrentUpsertDeadlockAttempt (return queryDeadlockCount() > deadlocksBefore); image postgis/postgis (untagged) at JdbcPostgresIT.java:93.
Problem description: cumulative statistics in PostgreSQL are published asynchronously (in PostgreSQL 15+ a backend flushes pending counters at most about every second, and up to 10 seconds when it becomes idle; older versions go through the stats collector with a similar delay). The sink connections stay open after flush(), and the counter is read immediately afterwards, so a real 40P01 can be missed. When it is missed, the loop retries up to three times and then fails with "should report a deadlock", so the test can fail even though the product code behaved correctly. The untagged image also means the server version can change between runs.
Potential risk: an intermittent red in connector-jdbc-e2e-part-3 / updated-modules-integration-test-part-4 that blocks unrelated PRs.
Best improvement: detect the conflict at the source instead of the statistics views: wrap the connection provider (or the executor's connection) in a test-local counter that records rollback() calls made by the transaction-conflict path, or poll the counter with a bounded Awaitility wait (for example up to 15 seconds) before concluding no deadlock happened. Pinning the image tag would also make the behavior reproducible.
Severity: Medium (stability)
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

ASF headers, formatting and Javadoc on the new interface methods and helpers are fine; the added comment on the auto-commit fall-through explains the reason well.

2.2 Test Coverage and Test Stability

Unit tests (JdbcOutputFormatReconnectSafetyTest, BufferReducedBatchStatementExecutorTest) cover rollback plus batch reset, unchanged auto-commit behavior, no replay inside a checkpoint transaction, no replay on close after exhausted retries, and key ordering. testUpsertBatchKeyOrderedMixedUpsertAndDelete is a good test: triggers record the execution order U1, D2, U3, D4, U5 and the final table contents, so it would fail if the option stopped reaching the executor. Its trigger and function cleanup is in finally.

Rating: unit tests Stable; testDeadlockRetryWithRealUpsertExecutor Medium risk for the two reasons above (it is also timing based: it relies on two 200-row flushes overlapping, mitigated by the three-attempt loop and the CyclicBarrier).

CI: the apache-side Build check is green for this head and points to the fork run, which concluded successfully (updated-modules-integration-test-part-4 ran both new ITs and passed according to your update). A green run shows the test can pass; it does not show that it would fail on a lost buffer.

2.3 Documentation Updates

docs/en and docs/zh Jdbc.md document upsert_batch_key_ordered and the max_retries scope for auto-commit, manual-commit and checkpoint transactions. Both points from @ryanmeowy's earlier review (the extra child batches for mixed batches, and the max_retries scope) are covered by the current text.

3. Architectural Soundness

3.1 Elegance

The executor owns the logical records, the output format owns the transaction boundary, and auto-commit keeps legacy semantics.

3.2 Maintainability

Good; the retry decision helpers are small and commented.

3.3 Extensibility

supportsTransactionConflictRetry() defaulting to false is a safe opt-in contract.

3.4 Historical-Version Compatibility

No behavior change for existing configurations.

4. Issue Summary

Number	Issue	Location	Severity
1	Real-executor deadlock IT cannot detect a lost buffer	`JdbcPostgresIT.runConcurrentUpsertDeadlockAttempt`	Medium
2	Deadlock detection via `pg_stat_database` is asynchronous; untagged image	`JdbcPostgresIT.queryDeadlockCount`	Medium

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers (must fix): Issue 1 and Issue 2. Both are in the test only, and both can be solved together by counting writes with a trigger (or counting rollbacks in a wrapper) instead of reading the statistics view. The production change itself looks correct to me.
Recommended fixes: pin the PostgreSQL image tag used by the part-3 IT if the maintainers agree.
Overall: the implementation is sound and CI is green; I only want the headline guarantee to be protected by a test that fails when the guarantee breaks. The branch is 5 commits behind dev with no conflict, and CI already passes, so no sync is needed.
Alternatives: none needed.

This COMMENT is a technical assessment, not an approval. Thank you again for the very responsive iterations!

yzeng1618 · 2026-10-26T06:11:20Z

Thanks @DanielLeens — both fair points, fixed in 0b6c405:

Issue 1 (weak final assertions): an AFTER INSERT OR UPDATE trigger now records every committed row write into an audit table (created after the seed INSERT, so it captures exactly the sink writes). After both flushes the test asserts exactly rowCount entries per writer value (200 × value 1 + 200 × value 2). If the victim's replay lost its buffered rows, only the winner's 200 entries would exist and the assertion fails — the IT now actually fails on the regression it describes.
Issue 2 (async pg_stat_database): queryDeadlockCount() is gone. The test wraps each JdbcConnectionProvider in a small counting wrapper that returns a proxy Connection and counts rollback() calls. On these connections only the transaction-conflict retry path calls rollback, so rollbacks >= 1 is a deterministic deadlock signal with no stats-publishing delay; when no conflict happened (batches did not overlap) the attempt returns false and the retry loop runs again.

On the recommended image pin: PG_IMAGE = "postgis/postgis" is a pre-existing constant shared by every test in JdbcPostgresIT (including catalog, geometry and DuckDB tests that predate this PR), so pinning it here would change unrelated tests' environment. I'd suggest handling it as a separate small PR if the maintainers agree — happy to prepare it.

CI is running on the new head.

…real executors Drive the two-connection PostgreSQL deadlock through the production upsert wiring (JdbcOutputFormatBuilder -> BufferReducedBatchStatementExecutor) and verify via pg_stat_database that a real 40P01 triggers the rollback and whole-flush replay without losing buffered rows. Add a mixed upsert/delete IT under upsert_batch_key_ordered=true: AFTER UPDATE/DELETE triggers record the execution order, proving the option is wired from JdbcSinkFactory config to the real executor and that entries land in primary-key order. Also document why an auto-commit connection falls through to the legacy generic retry after a transaction conflict.

The new IT helpers passed the PostgreSQL database name as the sink config database, so the generated upsert and delete SQL addressed "<database>"."<table>" while the test DDL created the tables in the default search_path schema. The flush then failed with 'relation does not exist' on every attempt. Create the tables schema-qualified under public and derive the sink config database from the same qualified name, so the generated SQL and the DDL always address the same schema.

…istically The final assertions could not fail on the regression they describe: if the deadlock victim lost its buffered rows, the winner's flush still wrote every row, so the value check passed either way, and pg_stat_database counters are published asynchronously, so the deadlock signal could be missed entirely. Replace both signals: wrap the connection provider so every rollback() call made by the transaction-conflict retry path is counted (proving a real 40P01 without depending on the stats views), and record every committed row write with an AFTER INSERT OR UPDATE trigger, asserting exactly rowCount entries per writer value. A replay that lost the buffer would leave only one writer's entries.

DanielLeens

Thank you @yzeng1618 for the careful and quick follow-up. I confirm the replies in your latest comment: the audit-table trigger now records every committed sink write, and queryDeadlockCount() was replaced by a rollback-counting connection provider. I re-reviewed the complete diff of the current head 2405a0d947996984513e2d49e6f84ccaf40d1241 (13 files, +1030/-11 against dev, 7 commits), not only the newest commits. The branch has been rebased on the latest dev (0 behind, mergeable, no conflict) and now sits on top of the dev version of BufferReducedBatchStatementExecutor that keeps the net effect per key (#12808). Static review only; nothing was executed locally. The apache-side Build check and the fork Build run for this exact head were still queued/in progress when I wrote this, so CI is not yet a fact for this head.

What Problem Does This PR Solve?

User pain point: concurrent JDBC sink writers can take row locks in different orders and hit transaction conflicts (PostgreSQL 40P01/40001, Oracle ORA-00060). The old generic retry replayed executeBatch without a rollback, so on a manual-commit connection it ran inside an aborted transaction or re-sent a half-submitted batch.
Fix approach: classify conflicts in JdbcOutputFormat.flush(); when the flush owns its commit boundary and the executor can keep its logical records, roll back, clear only the driver batch, back off with jitter and replay the whole flush; otherwise propagate. Add an opt-in upsert_batch_key_ordered primary-key sort.
Summary: replay a whole flush transaction after a deadlock instead of a half-submitted batch, and optionally lower deadlock probability by sorting keys.

1. Code Change Review

1.1 Core Logic Analysis

Production code is functionally the same as the version I reviewed last round (the only differences are the rebase onto dev and formatting). The runtime path, re-verified against this head:

flush() -> attemptFlush() -> executor.executeBatch() -> SQLException 40P01 / 40001 / ORA-00060
  (a) auto_commit=true            : legacy generic retry loop (same as dev), no rollback
  (b) manual commit, commitOnFlush, executor.supportsTransactionConflictRetry()
                                  : connection.rollback() + resetForTransactionConflictRetry()
                                    (driver batch cleared, logical buffer + existedBeforeBatch kept)
                                    -> jittered backoff -> replay the whole flush
  (c) checkpoint/XA transaction or executor without opt-in
                                  : throw FLUSH_DATA_FAILED -> checkpoint recovery replays the transaction

Normal-path reach: shape (a) is the default for auto-commit sinks and is unchanged. Shape (b) is reached only by the two buffered upsert executors on manual-commit connections with no checkpoint-owned transaction.
BufferReducedBatchStatementExecutor.executeBatch() resets flushFailed at the start of each attempt, the logical buffer is cleared only after a successful child executeBatch(), and closeStatements() clears (never executes) a failed partial batch. existedBeforeBatch survives the replay, so an INSERT-then-UPDATE keeps being written as an INSERT.
The key sort works on the key row of the Triple, keeps dev's NULL-key identity handling intact, and is applied only when upsert_batch_key_ordered=true.

1.2 Compatibility Impact

Fully compatible. The option defaults to false and is registered in JdbcSinkOptions, JdbcSinkConfig and JdbcSinkFactory; the two new executor-interface methods are default; the old 4-argument executor constructor is kept; no serialization, checkpoint state or protocol change. On manual-commit connections where the flush cannot own the transaction, a conflict now fails fast instead of retrying inside an aborted transaction, and this is documented in Jdbc.md (EN and ZH).

1.3 Performance / Side-Effect Analysis

The sort is one list copy plus O(n log n) over at most batch_size entries per flush, only when the option is on. Backoff happens only after a detected conflict and is bounded by max_retries (nextLong(sleepMs) is safe because sleepMs is the constant 1000). Mixed upsert/delete batches can issue more child batches; this is documented.

1.4 Error Handling and Logging

Rollback and transaction-state failures are wrapped with the cause; conflicts are classified narrowly (lock-wait timeouts are intentionally not retried). Both issues from my previous review are resolved:

Previous Issue 1 (final assertions could not detect a lost buffer): resolved. testDeadlockRetryWithRealUpsertExecutor now asserts exactly 200 audit entries per writer value (the winner's 200 plus the victim's replayed 200), so a replay that dropped its buffered rows fails the assertion.
Previous Issue 2 (asynchronous pg_stat_database counter): resolved. RollbackCountingProvider counts Connection.rollback(), and the only production caller on this path is rollbackAfterTransactionConflict(), so a non-zero count is a deterministic deadlock signal; an attempt without overlap returns false and the bounded three-attempt loop runs again.

No new production issue found.

2. Code Quality Assessment

2.1 Coding Standards

ASF headers, formatting and Javadoc on the new interface methods and helpers are fine; the comment on the auto-commit fall-through explains why legacy retry is safe there.

2.2 Test Coverage and Test Stability

Unit tests (JdbcOutputFormatReconnectSafetyTest, BufferReducedBatchStatementExecutorTest) cover rollback plus batch reset, unchanged auto-commit behavior, no replay inside a checkpoint transaction, no replay on close after exhausted retries, and key ordering. testUpsertBatchKeyOrderedMixedUpsertAndDelete records the execution order (U1, D2, U3, D4, U5) through triggers. Test setup and cleanup are in finally blocks and use schema-qualified table names.

Rating: Stable for unit tests. The real-executor deadlock IT is "Risk present" only in the sense that it still relies on two 200-row flushes overlapping (mitigated by the CyclicBarrier and the three-attempt loop); I do not consider that a blocker. On the postgis/postgis image pin: I agree with your reasoning that it is a pre-existing constant shared by unrelated tests, so a separate small PR is the right place for it.

2.3 Documentation Updates

docs/en and docs/zh Jdbc.md document upsert_batch_key_ordered and the max_retries scope for auto-commit, manual-commit and checkpoint transactions.

3. Architectural Soundness

3.1 Elegance

The executor owns the logical records, the output format owns the transaction boundary, and auto-commit keeps legacy semantics.

3.2 Maintainability

The retry decision helpers are small and commented.

3.3 Extensibility

supportsTransactionConflictRetry() defaulting to false is a safe opt-in contract for other executors.

3.4 Historical-Version Compatibility

No behavior change for existing configurations.

4. Issue Summary

Number	Issue	Location	Severity
-	No open code or test issue on this head	-	-

5. Merge Recommendation

Conclusion: Ready to merge

Blockers (must fix): none from the code side. The only open item is CI: the Build check for 2405a0d947 was still queued/in progress at review time, so please let it finish (and rerun an unrelated flaky lane if one shows up) before merging.
Recommended non-blocking fixes: pin the PostgreSQL image tag in a separate follow-up PR if the maintainers agree.
Overall assessment: the implementation is sound, backward compatible and now protected by tests that can actually fail on a lost buffer or a missing rollback. Thank you again for the very responsive iterations.
Better alternative: none needed.

This COMMENT is a technical assessment, not an approval; I will switch to an approval once the CI on this head is green.

DanielLeens

Quick follow-up on my previous review of this PR: the CI fact I cited there has changed. At that time the Build for this head was queued/in progress, and the only open item from my side was waiting for it to finish. The Build check on head 2405a0d94799 is now completed and green.

In my previous round I found no source-side blocker for the JDBC deadlock retry handling, and the head is unchanged since then, so that technical conclusion stands. With CI now passing, I am updating my review to Approve. The usual write-capable maintainer approval and merge gates still apply on top of this.

Thanks again for the work on this!

Resolve the JdbcOutputFormat conflict by reusing the catch-block sqlExceptions variable dev already declares and keeping the transaction-conflict guard on shouldRefreshExecutor: a conflict retry must keep the executor because its buffer still holds the records the next attempt replays after the rollback. The transaction state manager dev added is inert on the commitOnFlush retry path (it only records work for checkpoint-owned transactions), so the whole-flush replay and the poison-track fail-fast compose without behavior overlap.

DanielLeens

Thanks for the rebase and the stronger real-executor coverage. I re-read the full current diff and traced its interaction with the recently merged JDBC transaction-state protection. The retry design remains sound, but this head has a concrete test-contract conflict: all six failed CI lanes report the same existing reconnect regression. My previous green-CI conclusion was for an older head and does not apply here.

What Problem Does This PR Solve?

User pain point: Concurrent JDBC writers can acquire row locks in opposite orders. A deadlock or serialization abort needs a rollback and replay of the complete retained flush, rather than retrying a partially submitted driver batch.

Fix approach: Add opt-in primary-key ordering for generated upsert batches and a capability-gated retry path for transaction conflicts when the flush owns the manual-commit transaction.

One-sentence summary and example: Two writers updating keys 1 and 2 in opposite order can retry the rolled-back flush; upsert_batch_key_ordered=true gives both generated upsert batches a deterministic key order to reduce conflicts.

1. Code Change Review

1.1 Core Logic Analysis

JdbcSinkFactory accepts the option; JdbcSinkConfig passes it through JdbcOutputFormatBuilder only to the key-reducing executor. JdbcSinkWriter enables commitOnFlush only when checkpoints are disabled. The output format recognizes SQLState 40P01/40001 and Oracle vendor code 60 with ORA-00060, verifies transaction ownership and replay capability, rolls back, clears driver batches while retaining logical records, and retries on the same executor with bounded jitter. Successful execution clears the logical buffer; successful commit clears the flush failure latch.

Before:

// Generic retry could refresh an executor and lose its retained logical batch.
if (shouldRefreshExecutor(sqlExceptions)) { updateExecutor(true); }

After:

// Only a flush-owned manual-commit transaction may use full replay.
rollbackAfterTransactionConflict(); // rollback + reset driver batches, retain records
// Keep the same executor for the next attempt; clear records only on success.

Key Findings

Checkpoint-owned and XA transactions do not enter local full-flush replay.
Plain inserts and custom-query statements also use BufferedBatchStatementExecutor and can qualify for replay; eligibility is not limited to generated upserts. Key sorting is limited to the generated primary-key path.
Default executor capability is false; unsupported executors fail safely for manual-commit conflicts.
BufferReducedBatchStatementExecutor orders retained key rows and flushes the appropriate child on INSERT/DELETE operation transitions, preserving the global key order.
Failed executor close clears driver batches rather than resubmitting them.
Existing shared transaction-loss state still rejects commits/retries after earlier uncommitted work is lost.

Complete runtime path

JdbcSinkFactory -> JdbcSinkWriter (checkpointEnabled determines commitOnFlush)
 -> JdbcOutputFormatBuilder -> buffered or key-reducing executor
 -> writeRecord/flush -> executeBatch -> recognized conflict
 -> ownership/capability check -> rollback connection -> reset driver batches
 -> jitter/backoff -> replay retained logical rows -> commit flush
Checkpoint/XA-owned conflict -> fail -> engine checkpoint recovery, no local full replay

The replay path preserves records after executeBatch failure and never replaces its executor before replay. Resetting child JDBC batches prevents duplicate pending driver commands. The transaction manager records only successfully flushed uncommitted work; lost earlier work remains poisoned and cannot be committed. A commit failure is wrapped as JdbcConnectorException outside the SQLException retry path, avoiding replay after an uncertain commit outcome. Key comparison handles nulls, unsigned byte arrays and different Java types deterministically; same-type non-Comparable keys retain stable ties. Ordering reduces conflicts, but does not promise to eliminate every database deadlock or match database collation.

1.2 Compatibility Impact

Partially incompatible. The key-ordering option defaults to false, existing constructor overloads remain, and executor capabilities are default methods. Checkpoint/split formats and connector registration are unchanged. Recognized manual-commit conflicts on checkpoint-owned or unsupported executors now fail fast instead of taking the generic local retry; documentation describes this boundary. The legacy auto-commit generic retry remains; this PR does not establish a new exactly-once guarantee for arbitrary custom SQL or partially committed auto-commit batches.

1.3 Performance / Side-Effect Analysis

Opt-in key ordering copies and sorts the batch, adding O(n log n) comparison cost and O(n) references. Alternating upsert/delete operations can increase child executeBatch calls. Conflict retries add roughly 1-2 seconds initially and increasing bounded jitter per configured attempt, holding the writer flush path while waiting; ordinary successful flushes do not sleep.

1.4 Error Handling and Logging

SQL exception chains are inspected rather than relying only on the top exception. Rollback/reset failures fail the job; interrupted backoff restores the interrupt flag. Flush/commit failure latches prevent later writes from hiding an exhausted failure. No connection credential logging is added.

Issue 1: Reconcile the existing reconnect regression with the new transaction boundary

Location: JdbcOutputFormatReconnectTest.java:375-391; JdbcOutputFormat.java:256-272
Problem description: The existing test uses a manual-commit connection, the three-argument format constructor (commitOnFlush=false), and a TrackingJdbcBatchExecutor without replay capability, then expects SQLState 40001 to be retried. The new guard deliberately rejects that combination. Current-head CI reports this exact failure in all four unit-test lanes and both edge-agent lanes. The test came from upstream #13054 (794a025), already contained in this head; it was not reconciled when the branch incorporated that change.
Potential risk: The merge cannot pass required CI, and the test currently documents a contract opposite to the intended checkpoint-owned fail-fast behavior. This is a concrete source/test integration blocker, not infrastructure noise.
Best improvement: Retain the regression but update its checkpoint-owned scenario to assert fail-fast and one attempt; retain/add a separate flush-owned, replay-capable executor scenario asserting rollback, retained records and successful retry. Keep the existing shared-pending-work loss tests. Do not weaken the production ownership guard or delete the coverage to obtain green CI.
Severity: Medium; blocking.
Raised by another reviewer: No

Issue 2: Make deadlock IT cleanup independent of individual close failures

Location: JdbcPostgresIT.java: testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions:427-431 and runConcurrentUpsertDeadlockAttempt finally blocks
Problem description: The new concurrent tests call the two output closes sequentially before executor shutdown and table cleanup. If the first close throws, the remaining cleanup is skipped. The real-upsert test also depends on opposite-order batches overlapping within three attempts; its start barrier does not force a first-lock interleaving.
Potential risk: Exceptional/timeout runs can leave workers or database resources alive and obscure the original assertion; scheduling can also prevent the real-builder test from observing a conflict. No current CI failure has been attributed to these risks. The earlier review noted scheduling risk without a formal issue; this explicitly records that carryover.
Best improvement: Use nested finally/independent resource cleanup so both outputs and the executor are always closed, preserving the original failure. Keep bounded futures and the real-builder assertions; consider a test-only lock-interleaving hook if overlap flakiness is observed.
Severity: Medium; non-blocking.
Raised by another reviewer: No

2. Code Quality Assessment

2.1 Coding Standards

The changes stay inside JDBC and preserve its option/factory registration. Capability and reset contracts explain retained-record ownership; key comparator comments explain transitivity. The existing generic-retry comment that an empty transaction is safe should be read together with the new stricter ownership guard, and can be clarified alongside Issue 1.

2.2 Test Coverage and Test Stability

Test stability: Risk present. Issue 2 records bounded-scheduling and exceptional-cleanup risks. Unit tests cover rollback/retry counts, auto-commit preservation, checkpoint fail-fast, retry exhaustion without close replay, retained buffer replay, numeric ordering and mixed-type comparator transitivity. PostgreSQL IT includes a deterministic two-lock test-local executor and a real JdbcOutputFormatBuilder/generated-upsert test with 200 rows, opposite ordering, bounded futures and rollback counting. Trigger audit counts assert every writer committed all 200 rows; a separate mixed UPDATE/DELETE test pins actual database operation order. These improve the earlier final-row-only and asynchronous-statistics checks. Issue 1 is a deterministic failing existing test, separate from the stability recommendation. No local tests or database were run.

Local analysis: fetched the head, created independent branch seatunnel-review-12769, read the complete 13-file diff and traced affected callers with git show/git grep. No local compilation, unit tests, integration tests, E2E, containers, or job execution was performed, as requested. These are source-analysis conclusions; execution evidence is the current-head CI below.

2.3 Documentation Updates

Both English and Chinese JDBC docs explain the default-off option, mixed-operation batching cost, flush-owned manual-commit replay, unchanged auto-commit behavior, checkpoint recovery and XA limits. Those additions address ryanmeowy's scope/performance questions.

3. Architectural Soundness

3.1 Elegance of the Solution

Precise fix. Transaction ownership is decided by the writer, retry capability by the executor, and rollback/retry orchestration by the output format. This avoids retrying only one fragment of a checkpoint-owned transaction and retains the existing shared-transaction safety manager.

3.2 Maintainability

Keep production ownership checks and tests aligned when incorporating upstream JDBC changes. Capability defaults make new executors explicitly opt into replay; the real-builder IT protects against testing only a custom executor.

3.3 Extensibility

Other executors can opt in only when they retain the complete logical batch and can clear driver state without losing those records. Broader SQLState or vendor support needs equally explicit transaction semantics.

3.4 Historical-Version Compatibility

No job-state migration is introduced. Existing checkpoint-owned jobs may fail sooner on recognized transaction conflicts and recover from their last checkpoint, as documented. This review makes no rolling-upgrade or live production acceptance claim.

4. Issue Summary

Number	Issue	Location	Severity
1	Reconcile the existing reconnect regression with the new transaction boundary	`JdbcOutputFormatReconnectTest.java:375-391; JdbcOutputFormat.java:256-272`	Medium
2	Make deadlock IT cleanup independent of individual close failures	`JdbcPostgresIT.java: testDeadlockIsRolledBackAndRetriedForIndependentFlushTransactions:427-431 and runConcurrentUpsertDeadlockAttempt finally blocks`	Medium

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Blockers — must be fixed
- Issue 1: Reconcile the existing reconnect regression with the new transaction boundary Fix the concrete reconnect-test contract mismatch and obtain green current-head CI. Effective maintainer approval remains required; Issue 2 is non-blocking.
Recommended fixes — non-blocking
- Issue 2: Make deadlock IT cleanup independent of individual close failures

Overall assessment: The previous replay/data-completeness concerns are addressed, but integrating the upstream transaction protection left a test expecting the old boundary. This head must not inherit the earlier approval.

Existing-review reconciliation: +1 to ryanmeowy's replay-buffer verification and their scope/cost observations; the current docs cover both, so they are not duplicated as issues. The author's schema-qualified real-builder setup, committed-row audit and rollback-call signal resolve the earlier IT coverage weaknesses. The prior Daniel APPROVED event belongs to 2405a0d, not this head, and READ permission does not provide effective maintainer approval. No independent maintainer APPROVED/CHANGES_REQUESTED event was present in the refreshed history. Author replies have been incorporated into this single full review.

Head fabb7e1af2678cfb99a18e3c5feab704c0b39caf. Live base dev at 3ac2d1cec6aa7b3741c4dacdd23020f778e23c02; comparison ahead, ahead 8 / behind 0; merge state BLOCKED. Behind/diverged alone is recorded metadata, not a request to synchronize upstream. Checks: Signal pull request update = SUCCESS; Build = FAILURE

Fork run 41022655376 fails its JDK 11/17 Windows/Linux unit-test lanes and both edge-agent lanes on JdbcOutputFormatReconnectTest.testFlushShouldRetryRolledBackBatchWhenNothingIsPending:387, with "JDBC transaction conflict cannot be retried inside the current transaction boundary." The edge-agent lanes fail during connector-jdbc test preparation, not an edge-agent runtime assertion. Upstream #13054 is already included; fetched dev retains the old test and does not contain a cure for this PR-specific interaction. A simple sync or retry is therefore not the recommended fix. No CI run was triggered by this review.

Daniel has READ repository permission. This COMMENT records the technical conclusion; it is not an effective maintainer approval or a dismissal of any historical requested-changes entry. A write-capable maintainer owns the final repository approval gate.

github-actions Bot added document e2e jdbc connectors-v2 labels Oct 14, 2026

DanielLeens reviewed Oct 15, 2026

View reviewed changes

yzeng1618 force-pushed the codex/jdbc-deadlock-fix branch from 1b4d14b to 968c33f Compare October 16, 2026 02:45

yzeng1618 requested a review from DanielLeens October 16, 2026 02:51

ryanmeowy reviewed Oct 16, 2026

View reviewed changes

DanielLeens reviewed Oct 16, 2026

View reviewed changes

DanielLeens reviewed Oct 19, 2026

View reviewed changes

zhangshenghang mentioned this pull request Oct 19, 2026

[Docs] Fix Kafka, Hive, Jdbc and OLAP connector doc inaccuracies verified against source code #12911

Merged

4 tasks done

davidzollo added risk: 4-high and removed risk: 4-high labels Oct 20, 2026

davidzollo mentioned this pull request Oct 20, 2026

[Feature][CI] Add impact triage labels for pull requests #12706

Open

4 tasks

nzw921rx mentioned this pull request Oct 22, 2026

[Roadmap] Apache SeaTunnel 3.1.0 Roadmap #12949

Open

yzeng1618 force-pushed the codex/jdbc-deadlock-fix branch from 0a9f92a to a7bd94a Compare October 25, 2026 08:30

DanielLeens reviewed Oct 25, 2026

View reviewed changes

yzeng1618 force-pushed the codex/jdbc-deadlock-fix branch from dd0cf3a to ae3e188 Compare October 25, 2026 17:25

DanielLeens reviewed Oct 26, 2026

View reviewed changes

zengyi added 7 commits October 26, 2026 14:19

[Fix][Connector-V2] Handle JDBC transaction deadlock retries safely

6b958fa

[Fix][Connector-V2] Preserve failed JDBC flush state after deadlock

1e2a475

[Fix][Connector-V2] Preserve JDBC deadlock retry compatibility

60fdc7d

[Chore][Connector-V2] Normalize JDBC executor formatting after rebase

2405a0d

yzeng1618 force-pushed the codex/jdbc-deadlock-fix branch from 0b6c405 to 2405a0d Compare October 26, 2026 06:51

DanielLeens reviewed Oct 26, 2026

View reviewed changes

DanielLeens approved these changes Oct 27, 2026

View reviewed changes

github-actions Bot added the reviewed label Oct 27, 2026

JacobZheng0927 mentioned this pull request Oct 28, 2026

[Fix][Connector-V2] Initialize JDBC delete statements only when needed #13035

Open

3 tasks done

DanielLeens reviewed Oct 31, 2026

View reviewed changes

Conversation

yzeng1618 commented Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

Issue 1: Conflict rollback resets the executor's flushFailed latch even when no retry follows, so close() can replay the failed flush

Issue 2: The new else if (flushFailed) branches in both closeStatements() methods are not covered by any test

Issue 3: First conflict retry has no backoff, and there is no jitter

Issue 4: The integration test exercises a hand-written executor, not the production executors or the new option

Issue 5: New private core methods have no explanatory comments

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

ryanmeowy left a comment

Choose a reason for hiding this comment

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

Issue 1 (carryover from my previous review): the new fail-fast rule also changes the default auto_commit = true path, which used to retry deadlocks

Issue 2: the failed-flush branches of closeStatements() are still only partly pinned

Issue 3: first conflict retry has no backoff or jitter (carryover, unchanged)

Issue 4: the integration test still uses a test-local executor (carryover, unchanged)

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

3.4 Historical-Version Compatibility

4. Issue Summary

5. Merge Recommendation

Conclusion: Ready to merge after fixes

Uh oh!

DanielLeens left a comment

Choose a reason for hiding this comment

What Problem Does This PR Solve?

1. Code Change Review

1.1 Core Logic Analysis

1.2 Compatibility Impact

1.3 Performance / Side-Effect Analysis

1.4 Error Handling and Logging

Issue 1: the PR cannot be merged as is (merge conflict), and the cause looks like a duplicated change

Issue 2: the integration test still uses a test-local executor (carryover)

2. Code Quality Assessment

2.1 Coding Standards

2.2 Test Coverage and Test Stability

2.3 Documentation Updates

3. Architectural Soundness

3.1 Elegance of the Solution

3.2 Maintainability

3.3 Extensibility

yzeng1618 commented Oct 14, 2026 •

edited

Loading

Issue 1: Conflict rollback resets the executor's `flushFailed` latch even when no retry follows, so `close()` can replay the failed flush

Issue 2: The new `else if (flushFailed)` branches in both `closeStatements()` methods are not covered by any test

Issue 1 (carryover from my previous review): the new fail-fast rule also changes the default `auto_commit = true` path, which used to retry deadlocks

Issue 2: the failed-flush branches of `closeStatements()` are still only partly pinned

Issue 2: stability of the deadlock detection through `pg_stat_database`