Re: Stream Copy for 8.1 - 8.3dev

On Wed, 27 Jun 2007, Kalle Hallivuori wrote:

> Stream Copy extension for 8.1 - 8.3dev drivers is available with examples at
>
> http://kato.iki.fi/sw/db/postgresql/jdbc/copy/
>

Some thoughts after giving this a read (but not a test).

1) Your CopyObjects example pays no attention to encoding or escaping.
Calling getBytes() will give you bytes in the JVM's character set which
can easily be different than the client_encoding which the driver will
always set to UTF-8 (for 7.3 and up). You also have to think about what
happens when your data contains a null escape, delimiter, or newline
itself.

2) I'm not sure it's helpful to have the copy methods throw both an IO and
SQL exception. Why not just wrap any IOExceptions inside a SQLException?

3) What is the purpose of the reuse_buffer parameter? What is the use
case for someone wanting a "fresh" buffer every time?

4) buildCopyQuery doesn't handle quoting/escaping of identifiers. By
providing something like this you're now responsible for all the "hard"
stuff. I would leave it out unless you're prepared to do a lot more
thinking about it. It also doesn't handle all the possible copy options.
Perhaps you should split this into core copy functionality and a helper
class that builds upon it and can provide other useful things (escaping /
conversion of java objects to pg datatypes).

5) The coding in QueryExecutorImpl is unsafe because once you get
CopyInResponse you loop firing away data without listening for any return
data. Consider a table which had a trigger on it which issued a notice
for each row it received. If you don't read from the server the server
will block and then you'll block sending it data. See the comments in
core/v3/QueryExecutorImpl near MAX_BUFFERED_QUERIES for more details.

6) You mix warnings and errors together. They should be kept separate.

7) When you get CopyResponses and the user hasn't provided the appropriate
stream you bail leaving the protocol in an unknown state. You should
issue a CopyFail and wait for ReadyForQuery so the whole connection isn't
lost.

Kris Jurka