Re: FYI: porting Copy API to 8.x

Hi!

I'm porting the Copy API to 8.x series of pgjdbc. You may expect a
patch at the commit list this week.

I've copied Chris Jurka's patch for the 7.x series from here:
http://archives.postgresql.org/pgsql-jdbc/2003-12/msg00186.php

I'm just modifying that old patch to fit current implementation.
Suggestions for enhancements or alternative approaches are welcome so
long as they are sufficiently trivial to implement :)

Should this patched patch get accepted for inclusion it might make it
into official distribution for 8.4. Otherwise (and meanwhile) people
will have to keep building their own drivers in order to be able to
use COPY TO/FROM STDIN.

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/

Kalle,

The reason that the patch has never been officially accepted is
because it creates two protocol paths.

To do this correctly you need to add the copy functionality into the
current protocol handler as opposed to having a separate protocol
handler for copy.

Dave
On 30-May-07, at 8:29 AM, Kalle Hallivuori wrote:

> Hi!
>
> I'm porting the Copy API to 8.x series of pgjdbc. You may expect a
> patch at the commit list this week.
>
> I've copied Chris Jurka's patch for the 7.x series from here:
> http://archives.postgresql.org/pgsql-jdbc/2003-12/msg00186.php
>
> I'm just modifying that old patch to fit current implementation.
> Suggestions for enhancements or alternative approaches are welcome so
> long as they are sufficiently trivial to implement :)
>
> Should this patched patch get accepted for inclusion it might make it
> into official distribution for 8.4. Otherwise (and meanwhile) people
> will have to keep building their own drivers in order to be able to
> use COPY TO/FROM STDIN.
>
> --
> Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

Hi Dave!

2007/5/30, Dave Cramer <pg(at)fastcrypt(dot)com>:
> The reason that the patch has never been officially accepted is
> because it creates two protocol paths.
>
> To do this correctly you need to add the copy functionality into the
> current protocol handler as opposed to having a separate protocol
> handler for copy.

Thanks, now I understand why it's not there. I can take a look at
alternative approaches tomorrow, and only go with the ugly one if I
don't come up with an (moderately easily implementable) acceptable
design.

Is it more of an open design problem that many people have looked
closely into, or rather just a matter of someone getting to it? Ie. is
it likely there is no clean way to add these special cases inside the
protocol handler?

As for redesign, which would be the least abhorred way of providing
the functionality for use:

1. Separate Copy API available via a getCopyAPI() method in PGConnection API?
2. Separate Copy API available with some cleaner approach, which?
3. Special Copy methods in PGConnection API?
4. Special cases to batch processing (FROM STDIN) and resultset
collecting (TO STDOUT)?
5. Special cases to some other part of the standard JDBC API, which?

Cheers,

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/

Hi Kalle,

On 30-May-07, at 3:45 PM, Kalle Hallivuori wrote:

> Hi Dave!
>
> 2007/5/30, Dave Cramer <pg(at)fastcrypt(dot)com>:
>> The reason that the patch has never been officially accepted is
>> because it creates two protocol paths.
>>
>> To do this correctly you need to add the copy functionality into the
>> current protocol handler as opposed to having a separate protocol
>> handler for copy.
>
> Thanks, now I understand why it's not there. I can take a look at
> alternative approaches tomorrow, and only go with the ugly one if I
> don't come up with an (moderately easily implementable) acceptable
> design.
>
> Is it more of an open design problem that many people have looked
> closely into, or rather just a matter of someone getting to it? Ie. is
> it likely there is no clean way to add these special cases inside the
> protocol handler?
>
> As for redesign, which would be the least abhorred way of providing
> the functionality for use:
>
> 1. Separate Copy API available via a getCopyAPI() method in
> PGConnection API?
> 2. Separate Copy API available with some cleaner approach, which?
> 3. Special Copy methods in PGConnection API?
> 4. Special cases to batch processing (FROM STDIN) and resultset
> collecting (TO STDOUT)?
> 5. Special cases to some other part of the standard JDBC API, which?
>
I think you have to make it an extension somehow. I'd leave
getCopyAPI in place, I think the harder part is getting the protocol
to work,
> Cheers,
>
> --
> Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

Hi again.

2007/5/31, Dave Cramer <pg(at)fastcrypt(dot)com>:
> I think you have to make it an extension somehow. I'd leave
> getCopyAPI in place, I think the harder part is getting the protocol
> to work,

I found these hooks for protocol level implementation at end of
org.postgresql.core.v3.QueryExecutorImpl.processResults():

case 'G': // CopyInResponse
case 'H': // CopyOutResponse
case 'c': // CopyDone
case 'd': // CopyData
{
// COPY FROM STDIN / COPY TO STDOUT, neither of
which are currently
// supported.
...
handler.handleError(new PSQLException(GT.tr("The
driver currently does not support COPY operations."),
PSQLState.NOT_IMPLEMENTED));
}
break;

AFAIK (counting out breaking current design) the only way to implement
this at protocol level without touching core.v3 would be to clone the
whole package with every class inherited from core.v3, and copy
contents of QueryExecutorImpl there. I'd rather not.

Problem is that class has its stuff (pgStream) declared as private, so
I can't just inherit a class to override that single method.
Furthermore, I'd end up duplicating the protocol support implemented
in original version of that method. Thus, I'll edit it in place. Even
if it ends up in a rejected patch at least the patch gets it right.

The protocol spec is clear enough by itself.

I'll still have to look how to best pass the CopyData rows between BE
and application. They could probably be encapsulated as tuples, but
I'll have to read a bit more to find out the best way.

I won't handle field types (text/binary in the protocol level) because
I'm looking forward to a solution where I could pass whole or possibly
even multiple rows as raw(ish) byte[] at once. If this becomes a
barrier for acceptance I can reconsider.

Thanks for the clear source. It is what makes enhancements such as
this possible on short notice.

So no commits this week, but there's still hoping I'll get something out.

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/

Hi again.

I have an implementation of COPY subprotocol support that is
implemented inside org.postgresql.core.v3.QueryExecutorImpl and used
via a clean API available from PGConnection.getCopyAPI(). I'm trying
to test it today and commit the patch next week.

To ensure inclusion in the official source, I tried to follow the way
Fastpath was implemented.

In this implementation a COPY SQL statement is executed normally,
after which the response from server initiating COPY subprotocol is
handled in the usual processResults(). After that, specific methods
must be used to exchange copydata with the server and return to normal
protocol. All responses from server during that time are handled in a
separate private processCopyResults() method.

Alternatively processCopyResults() could be merged into
processResults() or separate everything COPY-related from there,
keeping it as it was. Opinions?

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/

Kalle Hallivuori wrote:
> To ensure inclusion in the official source, I tried to follow the way
> Fastpath was implemented.

I can't comment on whether that approach is good or not, but the fast
path has been pretty much obsoleted since the introduction of
server-side prepared statements, in 7.4 IIRC. That piece of code is not
necessarily a good example to follow.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Hello again.

Thanks to Chris Jurka for notifying me that there's a limit on size of
postings on this list.

Attached is a patch against postgresql-jdbc-8.2-505.src containing
Copy support inside v3/QueryExecutorImpl usable through a separate API
together with minimal unit tests and documentation.

I'd like to hear whether I should still fix something before making a
version for the 8.3 branch (there are plenty of possible approaches
for both client-backend and API, after all). Also I wonder if it would
be possible to include this in the official driver?

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/

On Mon, 11 Jun 2007, Kalle Hallivuori wrote:

> Attached is a patch against postgresql-jdbc-8.2-505.src containing
> Copy support inside v3/QueryExecutorImpl usable through a separate API
> together with minimal unit tests and documentation.
>

This patch doesn't apply cleanly because there's a mix of unix and windows
EOL characters and it seems to have some other application problems.

The API is not thread safe. If two threads are using the same connection
the synchronization in QueryExecutorImpl prevents them from stepping on
each others toes and writing garbage to the backend. So the call to
QueryExecutor must be an atomic operation and implies that the
QueryExecutorImpl will be the controller and demand/push data from some
kind of copy client instead of the other way around.

Also I'm not especially fond of the row based API that you've come up
with. It seems like you should either go to an element or stream based
API. Who has a premade row? They've either got single fields or a bulk
stream. Consider someone with a CSV file on their client machine that
they want to copy to the server, they'd have to parse it to break it into
rows which can be tricky with embedded newlines. They'd like an easy way
to just dump it to the server and let it deal with it.

Kris Jurka

Hi Kris!

Happy to receive feedback.

2007/6/13, Kris Jurka <books(at)ejurka(dot)com>:
> This patch doesn't apply cleanly because there's a mix of unix and windows
> EOL characters and it seems to have some other application problems.

Sorry for that. I'll pay closer attention to the format in future.

> The API is not thread safe. If two threads are using the same connection
> the synchronization in QueryExecutorImpl prevents them from stepping on
> each others toes and writing garbage to the backend. So the call to
> QueryExecutor must be an atomic operation and implies that the
> QueryExecutorImpl will be the controller and demand/push data from some
> kind of copy client instead of the other way around.

Yes. (I thought that was alright since I spotted a statement of no
support for synchronization somewhere, but that turned out to be at
lower level, pgStream.)

I'll look into current synchronization and hopefully I'll be able (as
in have the time) to do that.

> Also I'm not especially fond of the row based API that you've come up
> with. It seems like you should either go to an element or stream based
> API. Who has a premade row? ...

It doesn't matter what kinds of chunks you feed it (I think that was
ingenious of the COPY specification writers). With a CSV file you can
read it as a whole into a single byte[] and write that at once. Or you
can have static size byte buffer and pass it repeatedly to write().

In my application I now succesfully collect a bunch of field values as
separate byte[]'s mixed with references to static instances of
delimiter byte[]'s into a large, static size byte[][], write when it
fills up, repeat until out of data. Cut time spent in import by half.
(The other half of import time is spent post-processing the data :))

However, if that isn't intuitive it has to be explained clearly in all
documentation or a more intuitive API offered.

I think you're right on all accounts. I'll try to find some time to
make it synchronized and self-evident. Shouldn't be too much of an
effort now.

--
Kalle Hallivuori +358-41-5053073 http://korpiq.iki.fi/