Re: PATCH: Batch/pipelining support for libpq

On 18 November 2016 at 14:04, Tsunakawa, Takayuki
<tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> Hello, Craig,
>
> I'm sorry to be late to review your patch. I've just been able to read the HTML doc first. Can I get the latest .patch file for reading and running the code?

The latest is what's attached upthread and what's in the git repo also
referenced upthread.

I haven't been able to update in response to more recent review due to
other development commitments. At this point I doubt I'll be able to
get update it again in time for v10, so anyone who wants to adopt it
is welcome.

> Here are some comments and questions. I tried to avoid the same point as other reviewers, but there may be an overlap.
>
>
> (1)
> The example
> UPDATE mytable SET x = x + 1;
> should be
> UPDATE mytable SET x = x + 1 WHERE id = 42;

Good catch.

> (2)
> "The server usually begins executing the batch before all commands in the batch are queued and the end of batch command is sent."
>
> Does this mean that the app developer cannot control or predict how many TCP transmissions a batch is sent with?

That's not what that sentence means since the TCP layer is much lower
level, but what you say is also true.

All the docs are saying there is that there's no explicit control over
when we start sending the batch to the server. How that translates to
individual TCP packets, etc, is not its problem.

> For example, if I want to insert 10 rows into a table in bulk, can I send those 10 rows (and the end of batch command) efficiently in one TCP transmission, or are they split by libpq into multiple TCP transmissions?

libpq neither knows nor cares about individual TCP packets. It sends
things to the kernel and lets the kernel deal with that.

That said, you can use socket options TCP_CORK and TCP_NODELAY to get
some control over how and when data is sent. If you know you're about
to send more, you might cork the socket to give the kernel a hint that
it should expect more data to send.

> (3)
> "To avoid deadlocks on large batches the client should be structured around a nonblocking I/O loop using a function like select, poll, epoll, WaitForMultipleObjectEx, etc."
>
> Can't we use some (new) platform-independent API instead of using poll() or WaitForMultipleObject()? e.g. some thin wrapper around pqWait(). It seems a bit burdonsome to have to use an OS-specific API to just wait for libpq. Apart from that, it does not seem possible to wait for the socket in 64-bit apps on Windows, because SOCKET is 64-bit while PQsocket() returns int.

IMO this problem is out of scope for this patch. A wait abstraction
might be nice, but next thing we know we'll be reinventing APR or
NSPR, I think that's a totally different problem.

Not being able to get a win32 SOCKET from libpq seems like a bit of an
oversight, and it'd definitely be good to address that to make async
mode more usable on Windows. There's some other ugliness in PQsocket
already per the comments there. I think it should be a separate patch,
though.

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