yzeng1618 commented on PR #12731: URL: https://github.com/apache/seatunnel/pull/12731#issuecomment-6136720889
> Thanks for the quick follow-up, @yzeng1618! I re-reviewed the latest head (`b200e54600`) from scratch, including the full diff and the call chain, and there are no blocking issues. My previous review (on `fb5f0f9bb0`) already concluded no blockers; this round only needed to verify the new commit. > > # What Problem Does This PR Solve? > * **User pain point:** PostgreSQL CDC split sampling reads the whole split column. pgjdbc ignores `setFetchSize` while auto-commit is on, so the enumerator JVM can materialize the full column before the first `next()`. > * **Fix approach:** Switch auto-commit off around the sampling query so the existing fetch size becomes a real server-side cursor, and restore the previous value in `finally`. > * **One-sentence summary:** Makes the existing fetch size actually effective so sampling memory stays bounded by the sample, not the table. > > # 1. Code Change Review > ## 1.1 Core Logic Analysis > Only `PostgresUtils.skipReadAndSortSampleData` (`PostgresUtils.java:160-232`) changes. > > ``` > AbstractJdbcSourceChunkSplitter.splitTableIntoChunks (sampling branch) > -> PostgresChunkSplitter.sampleDataFromColumn > -> PostgresUtils.skipReadAndSortSampleData [changed] > -> setAutoCommit(false) (previous value remembered) :182 > -> forward-only stmt, fetchSize=1024, executeQuery :184-190 > -> keep every inverseSamplingRate-th row :192-204 > -> finally: close rs, close stmt, restore auto-commit :206-224 > -> Arrays.sort -> chunk boundaries > ``` > > **New in this commit:** the restore at `:219-223` is now wrapped in `try/catch (SQLException)` with a `log.warn`, so a failure to restore the connection state no longer replaces the original sampling exception. This fully resolves my previous Issue 1. The ordering is right too: result set and statement are closed before the auto-commit restore. > > **Key findings** > > 1. Normal path: every PostgreSQL CDC snapshot split that goes through sampling reaches this method, so both the problem and the fix are on the main path. > 2. The root cause is correct (auto-commit was the only missing pgjdbc cursor precondition) and the fix is precise. > 3. The query text and sampling/sort logic are unchanged, so split boundaries are identical to before. > 4. `setAutoCommit(false)` stays outside the `try`, so the restore only runs when the flip succeeded, which is the desired behavior. > > ## 1.2 Compatibility Impact > **Fully compatible.** No option, default, public API, split-boundary format, or serialized state changes; no `incompatible-changes.md` entry needed. > > ## 1.3 Performance / Side-Effect Analysis > Memory is bounded by one 1024-row batch plus the sample; round trips increase slightly, which is negligible against a full scan. The cursor keeps a read transaction open during the scan, which is inherent to cursor reads and acceptable for a snapshot phase. All work happens on the enumerator's calling thread, so there is no cross-thread interference. > > ## 1.4 Error Handling and Logging > The interrupt check still goes through `finally`. The new `log.warn` carries the exception and enough context. Nothing sensitive is logged. > > **Issue 1 (carryover from my previous review, non-blocking, Low): idle transaction if the prior auto-commit was `false`** > > * **Location:** `PostgresUtils.java:179-182, 219`; related `initStatement` at `:442-445` > * **Problem description:** `initStatement` (used at `:351`) sets auto-commit `false` on the shared `JdbcConnection` and never restores it. If it ran earlier on the same connection, `previousAutoCommit` is `false`, so the final `setAutoCommit(false)` is a no-op and the sampling `SELECT`'s transaction stays open until the next commit on that connection. When the previous value is `true`, restoring `true` commits implicitly, so that case is fine. > * **Potential risk:** An idle-in-transaction session pinning the xmin horizon. Low probability and bounded. > * **Best improvement:** In the `finally`, if `!previousAutoCommit`, call `connection.commit()` (or `rollback()`, as the work is read-only) before restoring, or add a comment stating that the splitter connection never goes through `initStatement`. Please feel free to leave this for a follow-up. > * **Severity:** Low > * **Raised by another reviewer:** No > > # 2. Code Quality Assessment > ## 2.1 Coding Standards > Follows project style; the comments explain the why (pgjdbc cursor requirement, preserving the original exception). No new public methods or fields. > > ## 2.2 Test Coverage and Test Stability > No test was added, and I agree a plain unit test cannot observe pgjdbc cursor behavior without a real PostgreSQL. The existing Postgres CDC E2E suites cover the sampling path end to end. A Testcontainers check that `getAutoCommit()` equals its original value after sampling (including the exception path) would be a nice follow-up, not a blocker. No UT/E2E test code changed, so there is no stability rating to give. > > ## 2.3 Documentation Updates > Not required; `docs/en` and `docs/zh` are unaffected. > > # 3. Architectural Soundness > ## 3.1 Elegance of the Solution > **Precise fix.** > > ## 3.2 Maintainability > Clear and local. > > ## 3.3 Extensibility > Now consistent with how the MySQL, Oracle and DB2 CDC variants handle fetching. > > ## 3.4 Historical-Version Compatibility > Fully compatible; running jobs and savepoints are unaffected because sampling only runs at split-generation time. > > # 4. Issue Summary > # Issue Location Severity > 1 Transaction left open if prior auto-commit was `false` (carryover, non-blocking) `PostgresUtils.java:179-182, 219` Low > # 5. Merge Recommendation > ### Conclusion: Ready to merge > 1. **Blockers — must be fixed:** None. > 2. **Recommended fixes — non-blocking:** Issue 1 (Low) is hardening around shared connection state and does not affect the correctness of the fix. > > **CI:** On this head the apache-side `Build`, `labeler` and `Notify test workflow` checks all succeed; `Build` points to the fork run (`yzeng1618/seatunnel-dev`, run 38246912686), which completed successfully. The branch is 7 commits behind `dev` (compare status: diverged) but GitHub reports it as mergeable; this is metadata only and does not need action. The remaining gate is a required maintainer approval. No other reviewers have commented on this PR, so there is no prior merge intent to reconcile. > > Thanks again for the fix and for addressing the earlier suggestion so promptly! Thanks for pointing this out. I checked the connection lifecycle and the call paths again. `AbstractJdbcSourceChunkSplitter.generateSplits()` opens a dedicated `JdbcConnection` for split generation and closes it via try-with-resources. `PostgresUtils.initStatement()` is called by the snapshot reader through `readTableSplitDataStatement()`, using a separate connection, so it does not run earlier on the splitter connection. If the splitter connection already has auto-commit disabled for another reason, it will still be closed when `generateSplits()` finishes. I would avoid calling `commit()` or `rollback()` inside this sampling utility, since it does not own a transaction that may have been started by its caller. The method restores the original auto-commit setting, while the connection owner controls its lifecycle. Given this call path, I suggest leaving transaction handling unchanged in this PR. Thanks for the careful review! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
