Обсуждение: Implementing setBinaryStream(int, InputStream, long)
Hi all, I have implemented the JDBC 4 function setBinaryStream(int, InputStream, long) in the PreparedStatement. It is a bit of a hack since I added long to the ParameterList interface and StreamWrapper. I suppose that was a bit of an overkill. PostgreSQL doesn't support field values larger than 1GB according to http://www.postgresql.org/about/ so I suppose a check if the length given is larger than Integer.MAX_VALUE and throw an exception is reasonable. What exception makes sense? The Integer.MAX_VALUE is chosen based on the underlying protocol using length values in int32. Is there any sense in choosing a smaller value for this? With the appropriate check, the implementation is trivial: preparedParameters.setBytea( parameterIndex, value, (int) length ); The V3 protocol does not allow piecewise transmission of data, so implementing setBinaryStream( int, InputStream ) makes no sense. Notice the absense of the length field. I can submit a patch once I know to deal with values that don't fit in an int. -- Johann Oskarsson http://www.2ndquadrant.com/ |[] PostgreSQL Development, 24x7 Support, Training and Services --+-- | Blog: http://my.opera.com/myrkraverk/blog/
First of all thanks for the effort, and secondly welcome to the ambiguity of JDBC. Looking at QueryExecutorImpl.java PSQLException is thrown for errors in executing the protocol. Integer.MAX_VALUE seems like a very large number but if the protocol supports it we should use it. The user on the other hand may want to reconsider. Thanks, Dave Cramer dave.cramer(at)credativ(dot)ca http://www.credativ.ca On Tue, May 29, 2012 at 9:45 AM, Johann 'Myrkraverk' Oskarsson <johann@2ndquadrant.com> wrote: > Hi all, > > I have implemented the JDBC 4 function setBinaryStream(int, InputStream, > long) in the PreparedStatement. It is a bit of a hack since I added > long to the ParameterList interface and StreamWrapper. I suppose that > was a bit of an overkill. > > PostgreSQL doesn't support field values larger than 1GB according to > http://www.postgresql.org/about/ so I suppose a check if the length > given is larger than Integer.MAX_VALUE and throw an exception is > reasonable. What exception makes sense? The Integer.MAX_VALUE is > chosen based on the underlying protocol using length values in int32. > > Is there any sense in choosing a smaller value for this? > > With the appropriate check, the implementation is trivial: > > preparedParameters.setBytea( parameterIndex, value, (int) length ); > > The V3 protocol does not allow piecewise transmission of data, so > implementing setBinaryStream( int, InputStream ) makes no sense. Notice > the absense of the length field. > > I can submit a patch once I know to deal with values that don't fit in > an int. > > > -- > Johann Oskarsson http://www.2ndquadrant.com/ |[] > PostgreSQL Development, 24x7 Support, Training and Services --+-- > | > Blog: http://my.opera.com/myrkraverk/blog/ > > -- > Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-jdbc
Dave Cramer <pg@fastcrypt.com> writes:
> First of all thanks for the effort, and secondly welcome to the
> ambiguity of JDBC.
>
> Looking at QueryExecutorImpl.java PSQLException is thrown for errors
> in executing the protocol.
> Integer.MAX_VALUE seems like a very large number but if the protocol
> supports it we should use it. The user on the other hand may want to
> reconsider.
Based on the following protocol description of bind messages
Next, the following pair of fields appear for each parameter:
Int32
The length of the parameter value, in bytes (this count does not
include itself). Can be zero. As a special case, -1 indicates a
NULL parameter value. No value bytes follow in the NULL case.
Byte(n)
The value of the parameter, in the format indicated by the
associated format code. n is the above length.
I chose Integer.MAX_VALUE. Though by this time the message is already
wrapped in a header with total length given in Int32 so the context does
not allow Integer.MAX_VALUE parameters.
Is there a reason to check for negtive long values too?
This is how the function looks with the attached patch (edited to fit in
80 columns).
public void setBinaryStream(int parameterIndex,
InputStream value, long length) throws SQLException
{
if (length > Integer.MAX_VALUE)
throw new PSQLException(GT.tr
("Object is too large to send over the protocol."),
PSQLState.NUMERIC_CONSTANT_OUT_OF_RANGE);
preparedParameters.setBytea(parameterIndex, value, (int)length);
}
I am not sure if the message is clear enough, nor whether the SQL state
constant is appropriate. Comments?
--
Johann Oskarsson http://www.2ndquadrant.com/ |[]
PostgreSQL Development, 24x7 Support, Training and Services --+--
|
Blog: http://my.opera.com/myrkraverk/blog/
Вложения
"Johann 'Myrkraverk' Oskarsson" <johann@2ndquadrant.com> writes:
> This is how the function looks with the attached patch (edited to fit
> in 80 columns).
>
> public void setBinaryStream(int parameterIndex,
> InputStream value, long length) throws SQLException
> {
> if (length > Integer.MAX_VALUE)
> throw new PSQLException(GT.tr
> ("Object is too large to send over the protocol."),
> PSQLState.NUMERIC_CONSTANT_OUT_OF_RANGE);
>
> preparedParameters.setBytea(parameterIndex, value, (int)length);
> }
>
> I am not sure if the message is clear enough, nor whether the SQL state
> constant is appropriate. Comments?
Is there any chance this can be reviewed/commented on soon? It is the
natural way (IMO) to upload files to the server.
File somefile = new File( "filename" );
preparedStatement.setBinaryStream( 1,
new FileInputStream( somefile ),
somefile.length() );
Since File.length() is a long value. And this is how I'm using it in a
tool of mine.
--
Johann Oskarsson http://www.2ndquadrant.com/ |[]
PostgreSQL Development, 24x7 Support, Training and Services --+--
|
Blog: http://my.opera.com/myrkraverk/blog/
Johann,
I've committed the patch with a small formatting difference.
if (test)
do something
In my experience ends up creating hard to find bugs when someone does
if (test)
log
do something
my preference is
if (test)
{
do something
}
Also it would be nice if a test case came with this patch.
Dave Cramer
dave.cramer(at)credativ(dot)ca
http://www.credativ.ca
On Sat, Jun 2, 2012 at 11:14 AM, Johann 'Myrkraverk' Oskarsson
<johann@2ndquadrant.com> wrote:
> "Johann 'Myrkraverk' Oskarsson" <johann@2ndquadrant.com> writes:
>
>> This is how the function looks with the attached patch (edited to fit
>> in 80 columns).
>>
>> public void setBinaryStream(int parameterIndex,
>> InputStream value, long length) throws SQLException
>> {
>> if (length > Integer.MAX_VALUE)
>> throw new PSQLException(GT.tr
>> ("Object is too large to send over the protocol."),
>> PSQLState.NUMERIC_CONSTANT_OUT_OF_RANGE);
>>
>> preparedParameters.setBytea(parameterIndex, value, (int)length);
>> }
>>
>> I am not sure if the message is clear enough, nor whether the SQL state
>> constant is appropriate. Comments?
>
> Is there any chance this can be reviewed/commented on soon? It is the
> natural way (IMO) to upload files to the server.
>
> File somefile = new File( "filename" );
> preparedStatement.setBinaryStream( 1,
> new FileInputStream( somefile ),
> somefile.length() );
>
> Since File.length() is a long value. And this is how I'm using it in a
> tool of mine.
>
>
> --
> Johann Oskarsson http://www.2ndquadrant.com/ |[]
> PostgreSQL Development, 24x7 Support, Training and Services --+--
> |
> Blog: http://my.opera.com/myrkraverk/blog/
>
> --
> Sent via pgsql-jdbc mailing list (pgsql-jdbc@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-jdbc
Dave Cramer <pg@fastcrypt.com> writes:
> Johann,
>
> I've committed the patch with a small formatting difference.
Thank you.
> Also it would be nice if a test case came with this patch.
> Dave Cramer
I'll do that soon, maybe today.
>>> if (length > Integer.MAX_VALUE)
Here I'm a bit concerned the test should be
if ( length > Integer.MAX_VALUE || length < 0 )
since "small enough" negative long values will become positive integers
during the cast.
The question is "how much do we hold the user's hands?" Consider an
infinite stream such as /dev/random with a length of
(long)Integer.MIN_VALUE - 1L.
Here I'm not at all concerned with negative values passed to setBytea()
- handling that is setBytea()'s problem.
Of course we can also test for length < Integer.MIN_VALUE but does it
ever make sense to upload negative length streams?
I can throw in a patch for that when I deliver a unit test, if wanted.
--
Johann Oskarsson http://www.2ndquadrant.com/ |[]
PostgreSQL Development, 24x7 Support, Training and Services --+--
|
Blog: http://my.opera.com/myrkraverk/blog/
Dave Cramer <pg@fastcrypt.com> writes: > Also it would be nice if a test case came with this patch. I've written a testsuite for this patch. It tests the three cases of length < 0, length > Integer.MAX_VALUE and that it actually works. The testcase that tests if the feature is working properly uses the prepared statement SELECT ?::TEXT and compares the returned hex value. This is not a good idea because it depends on the bytea -> text conversion which has changed in recent Postgres versions, right? What is a good way to isolate the test code from version specific behaviour? In this case, whether bytea to text is _escape_ or _hex_. Now that we're using git, is there any point in CVS identification strings in new files? Also, I only state Copyright 2012 for new files, and not 20xx-2012, right? When tests are catching SQL exceptions and comparing the error code, do we want to hard code them, or refer to PSQLState constants? Personally, I do not believe in using the same constants in test and production code. -- Johann Oskarsson http://www.2ndquadrant.com/ |[] PostgreSQL Development, 24x7 Support, Training and Services --+-- | Blog: http://my.opera.com/myrkraverk/blog/