-
Notifications
You must be signed in to change notification settings - Fork 858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: add read(b,o,l) to BlobInputStream #2376
Conversation
pgjdbc/src/main/java/org/postgresql/largeobject/BlobInputStream.java
Outdated
Show resolved
Hide resolved
Hi, And also some strange Case:
in such case lo.read(b, off, len); should be executed with offest = 0, |
Unfortunately we have exposed this API so we really can't change it. I will test your other question |
if (len > 0 ) { | ||
bytesCopied += lo.read(b, off, len); | ||
absolutePosition += bytesCopied; | ||
if ( bytesCopied == 0 && (buffer == null || bufferPosition >= buffer.length) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, how could the second half of this condition ever be false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, pretty sure that should have been buffer != null thx, for the second set of eyes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think bytesCopied == 0 is the only thing to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure one of the tests failed, let me check again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testGetBinaryStreamWithBoundaries fails without the second test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the test actually right?
I am not seeing how to get here with data remaining in the buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I more or less copied it from the read() function. You may be right that it will never be exercised.
} | ||
} | ||
} catch (SQLException ex ) { | ||
throw new IOException(ex.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call toString on the causing exception?
This reverts commit 8825a38.
fixes #2374