Skip to content
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

Improved support for BLOBs #219

Merged
merged 7 commits into from Dec 26, 2014
Merged

Improved support for BLOBs #219

merged 7 commits into from Dec 26, 2014

Conversation

@alexismeneses
Copy link
Contributor

@alexismeneses alexismeneses commented Nov 23, 2014

Added the support for very large objects (4TB) when backend is at least 9.3 using lo_xxx64 functions (see release 9.3 notes).

Added support for various JDBC4 methods related to Blobs:

  • setBlob with input stream in PreparedStatement
  • getBinaryStream with position and offset in Blob (also helps a lot handling very large objects)

BTW, changed junit dependency to 4.11 to get assertArrayEquals

@alexismeneses
Copy link
Contributor Author

@alexismeneses alexismeneses commented Nov 25, 2014

I've just found out that since the origin, junit tests using blobs were not deleting them at the end! I've just added 227bddd in this PR to fix it.

Those who often run the tests may want to reclaim some wasted blob space in the database used for testing purposes...

@ringerc
Copy link
Member

@ringerc ringerc commented Dec 1, 2014

This looks pretty sane at a quick read.

My only concern is with the fastpath API changes. The PgJDBC ffastpath support is public API that corresponds to PostgreSQL server backend functionality clients may use directly. I don't think it's appropriate to just change the return type. Why is that necessary?

BTW, I really appreciate that you're adding appropriate tests with these patches.

@alexismeneses
Copy link
Contributor Author

@alexismeneses alexismeneses commented Dec 1, 2014

Right. I did not understand why fastpath() tries to do a conversion instead of leaving getInteger() do the job. I didn't took care enough of it being a public API before doing the change.

I propose to do the following

  • Revert the 3-arg Fastpath.fastpath() method returning Object and mark it as deprecated
  • Keep all internal calls on the 2-arg returning byte[].
  • PgJDBC clients should be encouraged by javadoc to use Fastpath.getInteger(), Fastpath.getLong() or any Fastpath.getXXX to get converted results.

Anyway the javadoc was wrong because the third arg was still mentionned.

@ringerc
Copy link
Member

@ringerc ringerc commented Dec 1, 2014

On 12/02/2014 04:57 AM, Alexis Meneses wrote:

Right. I did not understand why fastpath() tries to do a conversion
instead of leaving getInteger() do the job. I didn't took care enough of
it being a public API before doing the change.

I propose to do the following

  • Revert the 3-arg |Fastpath.fastpath()| method returning |Object| and
    mark it as deprecated
  • Keep all internal calls on the 2-arg returning |byte[]|.
  • PgJDBC clients should be encouraged by javadoc to use
    |Fastpath.getInteger()|, |Fastpath.getLong()| or any
    |Fastpath.getXXX| to get converted results.

That sounds reasonable.

I doubt we have many real-world users of the fastpath API, but I'd still
rather not go breaking things.

The fact that this change didn't break any regression tests shows that
we don't have any coverage there, either.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

@alexismeneses
Copy link
Contributor Author

@alexismeneses alexismeneses commented Dec 4, 2014

Ok. Done as proposed.

davecramer added a commit that referenced this pull request Dec 26, 2014
Improved support for BLOBs
@alexismeneses thanks!
@davecramer davecramer merged commit 9d81ecd into pgjdbc:master Dec 26, 2014
1 check passed
1 check passed
@davecramer
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants