Repository navigation
fix: flush LargeObject output stream before marking closed - #4248
Merged
Merged
Conversation
LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which in turn calls LargeObject.write(). write() calls checkClosed() and throws because closed is already true. Move closed=true to after the flush so the write can complete. Fixes pgjdbc#4247
sehrope
reviewed
Jun 24, 2026
LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which calls LargeObject.write() and fails its checkClosed() guard. Wrap the flush in try/finally so the object is always marked closed and lo_close is always issued, even if the flush throws. Fixes pgjdbc#4247
vlsi
reviewed
Jun 26, 2026
vlsi
left a comment
vlsi
left a comment
Member
There was a problem hiding this comment.
The root-cause fix is correct: flushing before closed is set is what #4247 needs, and the test reproduces the buffered-write path well.
One design point on the finally wrapper (suggested rewrite inline). Moving lo_close and commit into finally changes two behaviours worth a deliberate decision:
- A failure from
lo_closeorcommitnow replaces the original flush exception instead of surfacing it. A flush failure usually leaves the transaction aborted, solo_close/committend to throw too, and the caller seescurrent transaction is abortedrather than the real cause. - With
commitOnClose, the transaction is committed even when the flush failed, which can persist a half-written large object.
Releasing the descriptor unconditionally is the right call. The inline suggestion keeps that, preserves the original error via addSuppressed, and skips the commit when the flush failed.
Minor, not for this PR: closed is now set only after the flush, so two concurrent close() calls can both pass the if (closed) guard and run the body twice. LargeObject is not documented as thread-safe and the lock was removed in #3905, so the window simply widened.
The previous try/finally structure ran lo_close and commit unconditionally in the finally block. If os.flush() threw and then lo_close also threw (typical after a transaction abort), the finally exception replaced the original, so callers saw "current transaction is aborted" instead of the real flush cause. The commitOnClose path could also commit a half-written large object after a flush failure. Replace the try/finally with explicit error capture: flush errors are stored, lo_close always runs but its failure is attached via addSuppressed, and commit only fires when both flush and lo_close succeeded. closed is still set after flush so BlobOutputStream.flush() can call back into LargeObject.write() without hitting checkClosed(). Addresses review feedback from vlsi on PR pgjdbc#4248.
davecramer
marked this pull request as ready for review
June 26, 2026 20:50
Member
Author
|
@vlsi I have merged in your request |
davecramer
added a commit
to davecramer/pgjdbc
that referenced
this pull request
Jul 6, 2026
Covers all changes since REL42.7.11: search_path GUC_REPORT cache invalidation (PG 18+), flushCacheOnDdl, connectExecutor, classLoaderStrategy, reWriteBatchedInserts protocol-limit cap, large object 64-bit API, updatable ResultSet search_path fix, XA autoCommit fix, FIPS trust anchors, SSL key-format auto-detection, batching improvements, and numerous other bug fixes. docs: prepare 42.7.13 release notes after 42.7.12 security release 42.7.12 shipped as a security-only release for the silent channel-binding downgrade (CVE-2026-54291), so the feature and bug-fix notes that had been drafted for 42.7.12 now belong to 42.7.13. - bump pgjdbc.version to 42.7.13 - add a published 42.7.12 changelog entry covering CVE-2026-54291 - retitle the drafted maintenance notes from 42.7.12 to 42.7.13 (CHANGELOG.md and the website changelog) - record PR pgjdbc#4248 (LargeObject.close() flushes before marking closed, and lo_close no longer masks a flush failure) under 42.7.13 docs: fix PR links and add missing entries in 42.7.13 release notes Point each changelog entry at the PR that merged the change rather than at the issue or at the superseded pgjdbc#3400. Add two entries that were missing: the reWriteBatchedInserts parameterless-INSERT fix (pgjdbc#4207) and the GitHub release jar/signature fix (pgjdbc#3814). Drop the stray connectThreadFactory entry from CHANGELOG.md, which connectExecutor replaced before release. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: credit translators and attribute large-object commits to James Howe Move the BlobInputStream skip and reset commits to James Howe, who co-authored them, and add a short contributors note crediting the Italian, Simplified Chinese, and Russian-review translators. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: add the empty-timestamp fix (pgjdbc#4278) to the 42.7.13 release notes Reflect PR pgjdbc#4278, merged after the release-notes branch was opened: an empty timestamp/timestamptz/date value now yields a clear SQLException instead of ArrayIndexOutOfBoundsException. Co-Authored-By: Claude Opus 4.8 <[email protected]>
davecramer
added a commit
that referenced
this pull request
Jul 6, 2026
* docs: add 42.7.12 release changelog Covers all changes since REL42.7.11: search_path GUC_REPORT cache invalidation (PG 18+), flushCacheOnDdl, connectExecutor, classLoaderStrategy, reWriteBatchedInserts protocol-limit cap, large object 64-bit API, updatable ResultSet search_path fix, XA autoCommit fix, FIPS trust anchors, SSL key-format auto-detection, batching improvements, and numerous other bug fixes. docs: prepare 42.7.13 release notes after 42.7.12 security release 42.7.12 shipped as a security-only release for the silent channel-binding downgrade (CVE-2026-54291), so the feature and bug-fix notes that had been drafted for 42.7.12 now belong to 42.7.13. - bump pgjdbc.version to 42.7.13 - add a published 42.7.12 changelog entry covering CVE-2026-54291 - retitle the drafted maintenance notes from 42.7.12 to 42.7.13 (CHANGELOG.md and the website changelog) - record PR #4248 (LargeObject.close() flushes before marking closed, and lo_close no longer masks a flush failure) under 42.7.13 docs: fix PR links and add missing entries in 42.7.13 release notes Point each changelog entry at the PR that merged the change rather than at the issue or at the superseded #3400. Add two entries that were missing: the reWriteBatchedInserts parameterless-INSERT fix (#4207) and the GitHub release jar/signature fix (#3814). Drop the stray connectThreadFactory entry from CHANGELOG.md, which connectExecutor replaced before release. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: credit translators and attribute large-object commits to James Howe Move the BlobInputStream skip and reset commits to James Howe, who co-authored them, and add a short contributors note crediting the Italian, Simplified Chinese, and Russian-review translators. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: add the empty-timestamp fix (#4278) to the 42.7.13 release notes Reflect PR #4278, merged after the release-notes branch was opened: an empty timestamp/timestamptz/date value now yields a clear SQLException instead of ArrayIndexOutOfBoundsException. Co-Authored-By: Claude Opus 4.8 <[email protected]> * docs: diarize all 42.7.13 changes in CHANGELOG.md The root CHANGELOG.md [42.7.13] section listed only 20 entries while about 50 user-facing changes have landed since the 42.7.12 baseline. Bring it in line with the reviewed website changelog: add the missing feat/fix/refactor entries and correct PR references. Newly diarized items include getSchemas() on a hot standby (#4274), getColumnPrivileges()/CHAR_OCTET_LENGTH (#4231), FIPS PKIX trust anchors (#4193), SSL key-format auto-detection (#3946), GSS response timeout, autosave savepoints for SET LOCAL (#4203), BatchResultHandler on a closed connection (#4187), SQL_TSI_FRAC_SECOND (#4229), the batching/describe fixes, NumberParser/PGInterval validation, connectThreadFactory, OID constants, and getPrimaryKeys derivation. Docs, i18n, build, ci, test, and dependency-bump commits are omitted, consistent with the file's existing "Notable changes" scope. * docs: date the 42.7.13 website changelog to its release date Rename the release notes to 2026-07-06 to reflect the release date, and add the remaining PR links (flushCacheOnDdl #4067, connectExecutor #4165, reWriteBatchedInserts cap #4207, OID constants #4220, 64-bit LO #4204, and the refactor entries #4160/#4166/#4215). * docs: correct the 42.7.13 commits-by-author counts Reconcile the "Commits by author" section with git history: - Vladimir Sitnikov: 100 -> 104 (stale count carried from the 42.7.12 draft) - Dave Cramer: 4 -> 5, add the missing "maintainer vs committer" (#4234) entry - add Michael Baumgartner (1 commit), previously omitted James Howe remains credited for the two co-authored large-object commits. * remove connectThread as it was replaced by connectExecutor --------- Co-authored-by: Vladimir Sitnikov <[email protected]>
vlsi
pushed a commit
to vlsi/pgjdbc
that referenced
this pull request
Jul 8, 2026
* fix: flush LargeObject output stream before marking closed LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which in turn calls LargeObject.write(). write() calls checkClosed() and throws because closed is already true. Move closed=true to after the flush so the write can complete. Fixes pgjdbc#4247 * fix: flush LargeObject output stream before marking closed LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which calls LargeObject.write() and fails its checkClosed() guard. Wrap the flush in try/finally so the object is always marked closed and lo_close is always issued, even if the flush throws. Fixes pgjdbc#4247 * fix: prevent lo_close from masking flush errors in LargeObject.close() The previous try/finally structure ran lo_close and commit unconditionally in the finally block. If os.flush() threw and then lo_close also threw (typical after a transaction abort), the finally exception replaced the original, so callers saw "current transaction is aborted" instead of the real flush cause. The commitOnClose path could also commit a half-written large object after a flush failure. Replace the try/finally with explicit error capture: flush errors are stored, lo_close always runs but its failure is attached via addSuppressed, and commit only fires when both flush and lo_close succeeded. closed is still set after flush so BlobOutputStream.flush() can call back into LargeObject.write() without hitting checkClosed(). Addresses review feedback from vlsi on PR pgjdbc#4248.
vlsi
added a commit
to vlsi/pgjdbc
that referenced
this pull request
Jul 8, 2026
* docs: add 42.7.12 release changelog Covers all changes since REL42.7.11: search_path GUC_REPORT cache invalidation (PG 18+), flushCacheOnDdl, connectExecutor, classLoaderStrategy, reWriteBatchedInserts protocol-limit cap, large object 64-bit API, updatable ResultSet search_path fix, XA autoCommit fix, FIPS trust anchors, SSL key-format auto-detection, batching improvements, and numerous other bug fixes. docs: prepare 42.7.13 release notes after 42.7.12 security release 42.7.12 shipped as a security-only release for the silent channel-binding downgrade (CVE-2026-54291), so the feature and bug-fix notes that had been drafted for 42.7.12 now belong to 42.7.13. - bump pgjdbc.version to 42.7.13 - add a published 42.7.12 changelog entry covering CVE-2026-54291 - retitle the drafted maintenance notes from 42.7.12 to 42.7.13 (CHANGELOG.md and the website changelog) - record PR pgjdbc#4248 (LargeObject.close() flushes before marking closed, and lo_close no longer masks a flush failure) under 42.7.13 docs: fix PR links and add missing entries in 42.7.13 release notes Point each changelog entry at the PR that merged the change rather than at the issue or at the superseded pgjdbc#3400. Add two entries that were missing: the reWriteBatchedInserts parameterless-INSERT fix (pgjdbc#4207) and the GitHub release jar/signature fix (pgjdbc#3814). Drop the stray connectThreadFactory entry from CHANGELOG.md, which connectExecutor replaced before release. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: credit translators and attribute large-object commits to James Howe Move the BlobInputStream skip and reset commits to James Howe, who co-authored them, and add a short contributors note crediting the Italian, Simplified Chinese, and Russian-review translators. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: add the empty-timestamp fix (pgjdbc#4278) to the 42.7.13 release notes Reflect PR pgjdbc#4278, merged after the release-notes branch was opened: an empty timestamp/timestamptz/date value now yields a clear SQLException instead of ArrayIndexOutOfBoundsException. Co-Authored-By: Claude Opus 4.8 <[email protected]> * docs: diarize all 42.7.13 changes in CHANGELOG.md The root CHANGELOG.md [42.7.13] section listed only 20 entries while about 50 user-facing changes have landed since the 42.7.12 baseline. Bring it in line with the reviewed website changelog: add the missing feat/fix/refactor entries and correct PR references. Newly diarized items include getSchemas() on a hot standby (pgjdbc#4274), getColumnPrivileges()/CHAR_OCTET_LENGTH (pgjdbc#4231), FIPS PKIX trust anchors (pgjdbc#4193), SSL key-format auto-detection (pgjdbc#3946), GSS response timeout, autosave savepoints for SET LOCAL (pgjdbc#4203), BatchResultHandler on a closed connection (pgjdbc#4187), SQL_TSI_FRAC_SECOND (pgjdbc#4229), the batching/describe fixes, NumberParser/PGInterval validation, connectThreadFactory, OID constants, and getPrimaryKeys derivation. Docs, i18n, build, ci, test, and dependency-bump commits are omitted, consistent with the file's existing "Notable changes" scope. * docs: date the 42.7.13 website changelog to its release date Rename the release notes to 2026-07-06 to reflect the release date, and add the remaining PR links (flushCacheOnDdl pgjdbc#4067, connectExecutor pgjdbc#4165, reWriteBatchedInserts cap pgjdbc#4207, OID constants pgjdbc#4220, 64-bit LO pgjdbc#4204, and the refactor entries pgjdbc#4160/pgjdbc#4166/pgjdbc#4215). * docs: correct the 42.7.13 commits-by-author counts Reconcile the "Commits by author" section with git history: - Vladimir Sitnikov: 100 -> 104 (stale count carried from the 42.7.12 draft) - Dave Cramer: 4 -> 5, add the missing "maintainer vs committer" (pgjdbc#4234) entry - add Michael Baumgartner (1 commit), previously omitted James Howe remains credited for the two co-authored large-object commits. * remove connectThread as it was replaced by connectExecutor --------- Co-authored-by: Vladimir Sitnikov <[email protected]>
vlsi
pushed a commit
to vlsi/pgjdbc
that referenced
this pull request
Jul 8, 2026
* fix: flush LargeObject output stream before marking closed LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which in turn calls LargeObject.write(). write() calls checkClosed() and throws because closed is already true. Move closed=true to after the flush so the write can complete. Fixes pgjdbc#4247 * fix: flush LargeObject output stream before marking closed LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which calls LargeObject.write() and fails its checkClosed() guard. Wrap the flush in try/finally so the object is always marked closed and lo_close is always issued, even if the flush throws. Fixes pgjdbc#4247 * fix: prevent lo_close from masking flush errors in LargeObject.close() The previous try/finally structure ran lo_close and commit unconditionally in the finally block. If os.flush() threw and then lo_close also threw (typical after a transaction abort), the finally exception replaced the original, so callers saw "current transaction is aborted" instead of the real flush cause. The commitOnClose path could also commit a half-written large object after a flush failure. Replace the try/finally with explicit error capture: flush errors are stored, lo_close always runs but its failure is attached via addSuppressed, and commit only fires when both flush and lo_close succeeded. closed is still set after flush so BlobOutputStream.flush() can call back into LargeObject.write() without hitting checkClosed(). Addresses review feedback from vlsi on PR pgjdbc#4248.
vlsi
added a commit
to vlsi/pgjdbc
that referenced
this pull request
Jul 8, 2026
* docs: add 42.7.12 release changelog Covers all changes since REL42.7.11: search_path GUC_REPORT cache invalidation (PG 18+), flushCacheOnDdl, connectExecutor, classLoaderStrategy, reWriteBatchedInserts protocol-limit cap, large object 64-bit API, updatable ResultSet search_path fix, XA autoCommit fix, FIPS trust anchors, SSL key-format auto-detection, batching improvements, and numerous other bug fixes. docs: prepare 42.7.13 release notes after 42.7.12 security release 42.7.12 shipped as a security-only release for the silent channel-binding downgrade (CVE-2026-54291), so the feature and bug-fix notes that had been drafted for 42.7.12 now belong to 42.7.13. - bump pgjdbc.version to 42.7.13 - add a published 42.7.12 changelog entry covering CVE-2026-54291 - retitle the drafted maintenance notes from 42.7.12 to 42.7.13 (CHANGELOG.md and the website changelog) - record PR pgjdbc#4248 (LargeObject.close() flushes before marking closed, and lo_close no longer masks a flush failure) under 42.7.13 docs: fix PR links and add missing entries in 42.7.13 release notes Point each changelog entry at the PR that merged the change rather than at the issue or at the superseded pgjdbc#3400. Add two entries that were missing: the reWriteBatchedInserts parameterless-INSERT fix (pgjdbc#4207) and the GitHub release jar/signature fix (pgjdbc#3814). Drop the stray connectThreadFactory entry from CHANGELOG.md, which connectExecutor replaced before release. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: credit translators and attribute large-object commits to James Howe Move the BlobInputStream skip and reset commits to James Howe, who co-authored them, and add a short contributors note crediting the Italian, Simplified Chinese, and Russian-review translators. Co-Authored-By: Claude Opus 4.8 <[email protected]> docs: add the empty-timestamp fix (pgjdbc#4278) to the 42.7.13 release notes Reflect PR pgjdbc#4278, merged after the release-notes branch was opened: an empty timestamp/timestamptz/date value now yields a clear SQLException instead of ArrayIndexOutOfBoundsException. Co-Authored-By: Claude Opus 4.8 <[email protected]> * docs: diarize all 42.7.13 changes in CHANGELOG.md The root CHANGELOG.md [42.7.13] section listed only 20 entries while about 50 user-facing changes have landed since the 42.7.12 baseline. Bring it in line with the reviewed website changelog: add the missing feat/fix/refactor entries and correct PR references. Newly diarized items include getSchemas() on a hot standby (pgjdbc#4274), getColumnPrivileges()/CHAR_OCTET_LENGTH (pgjdbc#4231), FIPS PKIX trust anchors (pgjdbc#4193), SSL key-format auto-detection (pgjdbc#3946), GSS response timeout, autosave savepoints for SET LOCAL (pgjdbc#4203), BatchResultHandler on a closed connection (pgjdbc#4187), SQL_TSI_FRAC_SECOND (pgjdbc#4229), the batching/describe fixes, NumberParser/PGInterval validation, connectThreadFactory, OID constants, and getPrimaryKeys derivation. Docs, i18n, build, ci, test, and dependency-bump commits are omitted, consistent with the file's existing "Notable changes" scope. * docs: date the 42.7.13 website changelog to its release date Rename the release notes to 2026-07-06 to reflect the release date, and add the remaining PR links (flushCacheOnDdl pgjdbc#4067, connectExecutor pgjdbc#4165, reWriteBatchedInserts cap pgjdbc#4207, OID constants pgjdbc#4220, 64-bit LO pgjdbc#4204, and the refactor entries pgjdbc#4160/pgjdbc#4166/pgjdbc#4215). * docs: correct the 42.7.13 commits-by-author counts Reconcile the "Commits by author" section with git history: - Vladimir Sitnikov: 100 -> 104 (stale count carried from the 42.7.12 draft) - Dave Cramer: 4 -> 5, add the missing "maintainer vs committer" (pgjdbc#4234) entry - add Michael Baumgartner (1 commit), previously omitted James Howe remains credited for the two co-authored large-object commits. * remove connectThread as it was replaced by connectExecutor --------- Co-authored-by: Vladimir Sitnikov <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LargeObject.close() set closed=true before calling BlobOutputStream.flush(), which in turn calls LargeObject.write(). write() calls checkClosed() and throws because closed is already true.
Move closed=true to after the flush so the write can complete.
Fixes #4247