Horton

Thread

Commits

  1. Add libpq pipeline mode support to pgbench

  2. Implement pipeline mode in libpq

  1. PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-05-23T09:19:09Z

    Hi all

Following on from the foreign table batch inserts thread[1], here's a patch
to add support for pipelining queries into asynchronous batches in libpq.

Attached, and also available at
https://github.com/2ndQuadrant/postgres/tree/dev/libpq-async-batch (subject
to rebasing and force pushes).

It's cleaned up over the draft I posted on that thread and has error
recovery implemented. I've written and included the SGML docs for it. The
test program is now pretty comprehensive, more so than for anything else in
libpq anyway. I'll submit it to the next CF as a 9.7/10.0 candidate.

I'm measuring 300x (not %) performance improvements doing batches on
servers over the Internet, so this seems pretty worthwhile. It turned out
to be way less invasive than I expected too.

(I intentionally didn't add any way for clients to annotate each work-item
in a batch with their own private data. I think that'd be really useful and
would make implementing clients easier, but should be a separate patch).

This should be very useful for optimising FDWs, Postgres-XC, etc.


[1]
http://www.postgresql.org/message-id/CAMsr+YFgDUiJ37DEfPRk8WDBuZ58psdAYJd8iNFSaGxtw=wU3g@mail.gmail.com

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

  2. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2016-05-23T15:50:02Z

    Hi,

On 2016-05-23 17:19:09 +0800, Craig Ringer wrote:
> Hi all
> Following on from the foreign table batch inserts thread[1], here's a patch
> to add support for pipelining queries into asynchronous batches in libpq.

Yay!


> I'm measuring 300x (not %) performance improvements doing batches on
> servers over the Internet, so this seems pretty worthwhile. It turned out
> to be way less invasive than I expected too.

yay^2.


> (I intentionally didn't add any way for clients to annotate each work-item
> in a batch with their own private data. I think that'd be really useful and
> would make implementing clients easier, but should be a separate patch).
> 
> This should be very useful for optimising FDWs, Postgres-XC, etc.

And optimizing normal clients.

Not easy, but I'd be very curious how much psql's performance improves
when replaying a .sql style dump, and replaying from a !tty fd, after
hacking it up to use the batch API.

- Andres

  3. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2016-05-23T16:00:44Z

    On Mon, May 23, 2016 at 8:50 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2016-05-23 17:19:09 +0800, Craig Ringer wrote:
>> Following on from the foreign table batch inserts thread[1], here's a patch
>> to add support for pipelining queries into asynchronous batches in libpq.
>
> Yay!
>> I'm measuring 300x (not %) performance improvements doing batches on
>> servers over the Internet, so this seems pretty worthwhile. It turned out
>> to be way less invasive than I expected too.
>
> yay^2.

I'll follow this mood. Yeha.

>> (I intentionally didn't add any way for clients to annotate each work-item
>> in a batch with their own private data. I think that'd be really useful and
>> would make implementing clients easier, but should be a separate patch).
>>
>> This should be very useful for optimising FDWs, Postgres-XC, etc.
>
> And optimizing normal clients.
>
> Not easy, but I'd be very curious how much psql's performance improves
> when replaying a .sql style dump, and replaying from a !tty fd, after
> hacking it up to use the batch API.

Did you consider the use of simple_list.c instead of introducing a new
mimic as PGcommandQueueEntry? It would be cool avoiding adding new
list emulations on frontends.
-- 
Michael

  4. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-05-24T01:20:43Z

    On 24 May 2016 at 00:00, Michael Paquier <michael.paquier@gmail.com> wrote:

> On Mon, May 23, 2016 at 8:50 AM, Andres Freund <andres@anarazel.de>
> wrote:
>
>> This should be very useful for optimising FDWs, Postgres-XC, etc.
> >
> > And optimizing normal clients.
> >
> > Not easy, but I'd be very curious how much psql's performance improves
> > when replaying a .sql style dump, and replaying from a !tty fd, after
> > hacking it up to use the batch API.
>

I didn't, but agree it'd be interesting. So would pg_restore, for that
matter, though its use of COPY for the bulk of its work means it wouldn't
make tons of difference.

I think it'd be safe to enable it automatically in psql's
--single-transaction mode. It's also safe to send anything after an
explicit BEGIN and until the next COMMIT as a batch from libpq, and since
it parses the SQL enough to detect statement boundaries already that
shouldn't be too hard to handle.

However: psql is synchronous, using the PQexec family of blocking calls.
It's all fairly well contained in SendQuery and PSQLexec, but making it use
batching still require restructuring those to use the asynchronous
nonblocking API and append the query to a pending-list, plus the addition
of a select() loop to handle results and dispatch more work. MainLoop()
isn't structured around a select or poll, it loops over gets. So while it'd
be interesting to see what difference batching made the changes to make
psql use it would be a bit more invasive. Far from a rewrite, but to avoid
lots of code duplication it'd have to change everything to use nonblocking
mode and a select loop, which is a big change for such a core tool.

This is a bit of a side-project and I've got to get back to "real work" so
I don't expect to do a proper patch for psql any time soon. I'd rather not
try to build too much on this until it's seen some review and I know the
API won't need a drastic rewrite anyway. I'll see if I can do a hacked-up
version one evening to see what it does for performance though.

Did you consider the use of simple_list.c instead of introducing a new
> mimic as PGcommandQueueEntry? It would be cool avoiding adding new
> list emulations on frontends.
>

Nope. I didn't realise it was there; I've done very little on the C client
and library side so far. So thanks, I'll update it accordingly.

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

  5. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-05-24T02:01:43Z

    On 24 May 2016 at 00:00, Michael Paquier <michael.paquier@gmail.com> wrote:

> On Mon, May 23, 2016 at 8:50 AM, Andres Freund <andres@anarazel.de> wrote:
>


> > yay^2.
>
> I'll follow this mood. Yeha.



BTW, I've publushed the HTML-ified SGML docs to
http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview.


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

  6. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-05-24T04:02:15Z

    On 24 May 2016 at 00:00, Michael Paquier <michael.paquier@gmail.com> wrote:


>
> Did you consider the use of simple_list.c instead of introducing a new
> mimic as PGcommandQueueEntry? It would be cool avoiding adding new
> list emulations on frontends.
>

I'd have to extend simple_list to add a generic object version, like


struct my_list_elem
{
    PG_SIMPLE_LIST_ATTRS;
    mytype mycol;
    myothertype myothercol;
}

Objections?

I could add a void* version that's a simple clone of the string version,
but having to malloc both a list cell and its contents separately is
annoying.

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

  7. Re: PATCH: Batch/pipelining support for libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-24T04:28:40Z

    Craig Ringer <craig@2ndquadrant.com> writes:
> On 24 May 2016 at 00:00, Michael Paquier <michael.paquier@gmail.com> wrote:
>> Did you consider the use of simple_list.c instead of introducing a new
>> mimic as PGcommandQueueEntry? It would be cool avoiding adding new
>> list emulations on frontends.

> I'd have to extend simple_list to add a generic object version, like

> struct my_list_elem
> {
>     PG_SIMPLE_LIST_ATTRS;
>     mytype mycol;
>     myothertype myothercol;
> }

> Objections?

That doesn't look exactly "generic".

> I could add a void* version that's a simple clone of the string version,
> but having to malloc both a list cell and its contents separately is
> annoying.

I'd be okay with a void* version, but I'm not sure about this.

			regards, tom lane

  8. Re: PATCH: Batch/pipelining support for libpq

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-05-24T13:55:08Z

    On 5/23/16 4:19 AM, Craig Ringer wrote:
> +    Batching less useful when information from one operation is required by the

SB "Batching is less useful".
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461

  9. Re: PATCH: Batch/pipelining support for libpq

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2016-05-27T02:43:43Z

    From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Craig Ringer
I'll follow this mood. Yeha.


BTW, I've publushed the HTML-ified SGML docs to http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview.


Sorry for my late reply.  Fantastic performance improvement, nice documentation, and amazing rapid development!  I think I’ll join the review & testing in 2016/9 CF.

Regards
Takayuki Tsunakawa

  10. Re: PATCH: Batch/pipelining support for libpq

    Dmitry Igrishin <dmitigr@gmail.com> — 2016-06-03T11:51:54Z

    2016-05-24 5:01 GMT+03:00 Craig Ringer <craig@2ndquadrant.com>:
>
> On 24 May 2016 at 00:00, Michael Paquier <michael.paquier@gmail.com> wrote:
>>
>> On Mon, May 23, 2016 at 8:50 AM, Andres Freund <andres@anarazel.de> wrote:
>
>
>>
>> > yay^2.
>>
>> I'll follow this mood. Yeha.
>
>
>
> BTW, I've publushed the HTML-ified SGML docs to
> http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview.
Typo detected: "Returns 1 if the batch curently being received" -- "curently".

-- 
// Dmitry.

  11. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2016-08-10T06:44:49Z

    On Fri, Jun 3, 2016 at 8:51 PM, Dmitry Igrishin <dmitigr@gmail.com> wrote:
>> BTW, I've publushed the HTML-ified SGML docs to
>> http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview.
> Typo detected: "Returns 1 if the batch curently being received" -- "curently".

I am looking a bit more seriously at this patch and assigned myself as
a reviewer.

testlibpqbatch.c:1239:73: warning: format specifies type 'long' but
the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
        printf("batch insert elapsed:      %ld.%06lds\n",
elapsed_time.tv_sec, elapsed_time.tv_usec);
macos complains here. You may want to replace %06lds by just %06d.

(lldb) bt
* thread #1: tid = 0x0000, 0x0000000108bcdd56
libpq.5.dylib`closePGconn(conn=0x00007fd642d002d0) + 438 at
fe-connect.c:3010, stop reason = signal SIGSTOP
  * frame #0: 0x0000000108bcdd56
libpq.5.dylib`closePGconn(conn=0x00007fd642d002d0) + 438 at
fe-connect.c:3010
    frame #1: 0x0000000108bc9db0
libpq.5.dylib`PQfinish(conn=0x00007fd642d002d0) + 32 at
fe-connect.c:3072
    frame #2: 0x0000000108bc9ede
libpq.5.dylib`PQping(conninfo="dbname=postgres port=5432 host='/tmp'
connect_timeout=5") + 46 at fe-connect.c:539
    frame #3: 0x0000000108bb5210
pg_ctl`test_postmaster_connection(pm_pid=78218, do_checkpoint='\0') +
976 at pg_ctl.c:681
    frame #4: 0x0000000108bb388e pg_ctl`do_start + 302 at pg_ctl.c:915
    frame #5: 0x0000000108bb29b4 pg_ctl`main(argc=5,
argv=0x00007fff5704e5c0) + 2836 at pg_ctl.c:2416
    frame #6: 0x00007fff8b8b65ad libdyld.dylib`start + 1
(lldb) down 1
frame #0: 0x0000000108bcdd56
libpq.5.dylib`closePGconn(conn=0x00007fd642d002d0) + 438 at
fe-connect.c:3010
   3007        queue = conn->cmd_queue_recycle;
   3008        {
   3009            PGcommandQueueEntry *prev = queue;
-> 3010            queue = queue->next;
   3011            free(prev);
   3012        }
   3013        conn->cmd_queue_recycle = NULL;
This patch generates a core dump, use for example pg_ctl start -w and
you'll bump into the trace above. There is something wrong with the
queue handling.

Do you have plans for a more generic structure for the command queue list?

+   <application>libpq</application> supports queueing up mulitiple queries into
s/mulitiple/multiple/.

+  <para>
+   An example of batch use may be found in the source distribution in
+   <filename>src/test/examples/libpqbatch.c</filename>.
+  </para>
You mean testlibpqbatch.c here.

+   <para>
+    Batching less useful when information from one operation is required by the
+    client before it knows enough to send the next operation. The client must
"Batching *is* less useful".

src/test/examples/.gitignore needs a new entry for the new test binary.

+           fprintf(stderr, "internal error, COPY in batch mode");
+           abort();
I don't think that's a good idea. defaultNoticeProcessor can be
overridden to allow applications to have error messages sent
elsewhere. Error messages should also use libpq_gettext, and perhaps
be stored in conn->errorMessage as we do so for OOMs happening on
client-side and reporting them back even if they are not expected
(those are blocked PQsendQueryStart in your patch).

src/test/examples is a good idea to show people what this new API can
do, but this is never getting compiled. It could as well be possible
to include tests in src/test/modules/, in the same shape as what
postgres_fdw is doing by connecting to itself and link it to libpq. As
this patch complicates quote a lot fe-exec.c, I think that this would
be worth it. Thoughts?
-- 
Michael

  12. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-08-23T00:27:01Z

    On 10 August 2016 at 14:44, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Fri, Jun 3, 2016 at 8:51 PM, Dmitry Igrishin <dmitigr@gmail.com> wrote:
> >> BTW, I've publushed the HTML-ified SGML docs to
> >> http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a
> preview.
> > Typo detected: "Returns 1 if the batch curently being received" --
> "curently".
>
> I am looking a bit more seriously at this patch and assigned myself as
> a reviewer.
>

Much appreciated.


> testlibpqbatch.c:1239:73: warning: format specifies type 'long' but
> the argument has type '__darwin_suseconds_t' (aka 'int') [-Wformat]
>         printf("batch insert elapsed:      %ld.%06lds\n",
> elapsed_time.tv_sec, elapsed_time.tv_usec);
> macos complains here. You may want to replace %06lds by just %06d.
>

Yeah, or cast to a type known to be big enough. Will amend.


> This patch generates a core dump, use for example pg_ctl start -w and
> you'll bump into the trace above. There is something wrong with the
> queue handling.
>

Huh. I didn't see that here (Fedora 23). I'll look more closely.


> Do you have plans for a more generic structure for the command queue list?
>

No plans, no. This was a weekend experiment that turned into a useful patch
and I'm having to scrape up time for it amongst much more important things
like logical failover / sequence decoding and various other replication
work.

Thanks for the docs review too, will amend.


> +           fprintf(stderr, "internal error, COPY in batch mode");
> +           abort();
> I don't think that's a good idea. defaultNoticeProcessor can be
> overridden to allow applications to have error messages sent
> elsewhere. Error messages should also use libpq_gettext, and perhaps
> be stored in conn->errorMessage as we do so for OOMs happening on
> client-side and reporting them back even if they are not expected
> (those are blocked PQsendQueryStart in your patch).
>
> src/test/examples is a good idea to show people what this new API can
> do, but this is never getting compiled. It could as well be possible
> to include tests in src/test/modules/, in the same shape as what
> postgres_fdw is doing by connecting to itself and link it to libpq. As
> this patch complicates quote a lot fe-exec.c, I think that this would
> be worth it. Thoughts?


I didn't think it added much complexity to fe-exec.c personally. A lot of
the appeal is that it has very minor impact on anything that isn't using it.

I think it makes sense to (ab)use the recovery module tests for this,
invoking the test program from there.

Ideally I'd like to teach pgsql and pg_restore how to use async mode, but
that's a whole separate patch.

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

  13. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-08-23T06:56:44Z

    On 23 August 2016 at 08:27, Craig Ringer <craig@2ndquadrant.com> wrote:

> On 10 August 2016 at 14:44, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>
>>

> I am looking a bit more seriously at this patch and assigned myself as
>> a reviewer.
>>
>
> Much appreciated.
>
>
>>

> macos complains here. You may want to replace %06lds by just %06d.
>>
>
> Yeah, or cast to a type known to be big enough. Will amend.
>

I used an upcast to (long), because on Linux it's a long. I don't see the
point of messing about adding a configure test for something this trivial.


> This patch generates a core dump, use for example pg_ctl start -w and
>> you'll bump into the trace above. There is something wrong with the
>> queue handling.
>>
>
> Huh. I didn't see that here (Fedora 23). I'll look more closely.
>
>
>> Do you have plans for a more generic structure for the command queue list?
>>
>
> No plans, no. This was a weekend experiment that turned into a useful
> patch and I'm having to scrape up time for it amongst much more important
> things like logical failover / sequence decoding and various other
> replication work.
>
> Thanks for the docs review too, will amend.
>
>
>> +           fprintf(stderr, "internal error, COPY in batch mode");
>> +           abort();
>> I don't think that's a good idea.
>
>
My thinking there was that it's a "shouldn't happen" case. It's a problem
in library logic. I'd use an Assert() here in the backend.

I could printfPQExpBuffer(...) an error and return failure instead if you
think that's more appropriate. I'm not sure how the app would handle it
correctly, but OTOH it's generally better for libraries not to call
abort(). So I'll do that, but since it's an internal error that's not meant
to happen I'll skip the gettext calls.


> Error messages should also use libpq_gettext, and perhaps
>> be stored in conn->errorMessage as we do so for OOMs happening on
>> client-side and reporting them back even if they are not expected
>> (those are blocked PQsendQueryStart in your patch).
>>
>
I didn't get that last part, re PQsendQueryStart.


> src/test/examples is a good idea to show people what this new API can
>> do, but this is never getting compiled. It could as well be possible
>> to include tests in src/test/modules/, in the same shape as what
>> postgres_fdw is doing by connecting to itself and link it to libpq. As
>> this patch complicates quote a lot fe-exec.c, I think that this would
>> be worth it. Thoughts?
>
>
I think it makes sense to use the TAP framework. Added
src/test/modules/test_libpq/ with a test for async mode. Others can be
added/migrated based on that. I thought it made more sense for the tests to
live there than in src/interfaces//libpq/ since they need test client
programs and shouldn't pollute the library directory.

I've made the docs changes too. Thanks.

I fixed the list handling error. I'm amazed it appears to run fine, and
without complaint from valgrind, here, since it was an accidentally
_deleted_ line.

Re lists, I looked at simple_list.c and it's exceedingly primitive. Using
it would mean more malloc()ing since we'll have a list cell and then a
struct pointed to it, and won't recycle members, but... whatever. It's not
going to matter a great deal. The reason I did it with an embedded list
originally was because that's how it's done for PGnotify, but that's not
exactly new code

The bigger problem is that simple_list also uses pg_malloc, which won't set
conn->errorMessage, it'll just fprintf() and exit(1). I'm not convinced
it's appropriate to use that for libpq.

For now I've left list handling unchanged. If it's to move to a generic
list, it should probably be one that knows how to use
pg_malloc_extended(size, MCXT_ALLOC_NO_OOM) and emit its own
libpq-error-handling-aware error. I'm not sure whether that list should use
cell heads embedded in the structures it manages or pointing to them,
either.

Updated patch attached.

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

  14. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2016-09-06T08:10:08Z

    	Craig Ringer wrote:

> Updated patch attached.

Please find attached a couple fixes for typos I've came across in
the doc part.

Also it appears that PQqueriesInBatch() doesn't work as documented.
It says:
 "Returns the number of queries still in the queue for this batch"
but in fact it's implemented as a boolean:

+/* PQqueriesInBatch
+ *   Return true if there are queries currently pending in batch mode
+ */+int
+PQqueriesInBatch(PGconn *conn)
+{
+	if (!PQisInBatchMode(conn))
+		return false;
+
+	return conn->cmd_queue_head != NULL;
+}

However, is this function really needed? It doesn't seem essential to
the API. You don't call it in the test program either.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  15. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-09-06T11:01:50Z

    On 6 September 2016 at 16:10, Daniel Verite <daniel@manitou-mail.org> wrote:
>         Craig Ringer wrote:
>
>> Updated patch attached.
>
> Please find attached a couple fixes for typos I've came across in
> the doc part.

Thanks, will apply and post a rebased patch soon, or if someone picks
this up in the mean time they can apply your diff on top of the patch.

> Also it appears that PQqueriesInBatch() doesn't work as documented.
> It says:
>  "Returns the number of queries still in the queue for this batch"
> but in fact it's implemented as a boolean:

Whoops. Will fix.

I think the function is useful and necessary. There's no reason not to
expose that, but also it's good for when your query dispatch isn't as
tightly coupled to your query handling as in the example, so your app
might need to keep processing until it sees the end of queued results.


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

  16. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2016-10-03T02:10:55Z

    On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 6 September 2016 at 16:10, Daniel Verite <daniel@manitou-mail.org> wrote:
>>         Craig Ringer wrote:
>>
>>> Updated patch attached.
>>
>> Please find attached a couple fixes for typos I've came across in
>> the doc part.
>
> Thanks, will apply and post a rebased patch soon, or if someone picks
> this up in the mean time they can apply your diff on top of the patch.

Could you send an updated patch then? At the same time I am noticing
that git --check is complaining... This patch has tests and a
well-documented feature, so I'll take a look at it soon at the top of
my list. Moved to next CF for now.
-- 
Michael

  17. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-10-03T03:48:51Z

    On 3 October 2016 at 10:10, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> On 6 September 2016 at 16:10, Daniel Verite <daniel@manitou-mail.org> wrote:
>>>         Craig Ringer wrote:
>>>
>>>> Updated patch attached.
>>>
>>> Please find attached a couple fixes for typos I've came across in
>>> the doc part.
>>
>> Thanks, will apply and post a rebased patch soon, or if someone picks
>> this up in the mean time they can apply your diff on top of the patch.
>
> Could you send an updated patch then? At the same time I am noticing
> that git --check is complaining... This patch has tests and a
> well-documented feature, so I'll take a look at it soon at the top of
> my list. Moved to next CF for now.

Thanks.

I'd really like to teach psql in non-interactive mode to use it, but
(a) I'm concerned about possible subtle behaviour differences arising
if we do that and (b) I won't have the time. I think it's mostly of
interest to app authors and driver developers and that's what it's
aimed at. pg_restore could benefit a lot too.

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

  18. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2016-10-03T14:52:26Z

    	Craig Ringer wrote:

> I think it's mostly of interest to app authors and driver developers
> and that's what it's aimed at. pg_restore could benefit a lot too.

Wouldn't pgbench benefit from it?
It was mentioned some time ago [1], in relationship to the 
\into construct, how client-server latency was important enough to
justify the use of a "\;" separator between statements, to send them
as a group.

But with the libpq batch API, maybe this could be modernized
with meta-commands like this:
  \startbatch
  ...
  \endbatch
which would essentially call PQbeginBatchMode() and PQsendEndBatch().
Inside the batch section, collecting results would be interleaved
with sending queries.
Interdepencies between results and subsequent queries could
be handled or ignored, depending on how sophisticated we'd want
this.

This might also draw more users to the batch API, because
it would make it easier to check how exactly it affects
the performance of specific sequences of SQL statements to be
grouped in a batch.
For instance it would make sense for programmers to benchmark mock-ups
of their code with pgbench with/without batching, before embarking on
adapting it from blocking mode to asynchronous/non-blocking mode.


[1]
https://www.postgresql.org/message-id/alpine.DEB.2.20.1607140925400.1962%40sto

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  19. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2016-10-04T07:15:23Z

    On Mon, Oct 3, 2016 at 11:52 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
> Wouldn't pgbench benefit from it?
> It was mentioned some time ago [1], in relationship to the
> \into construct, how client-server latency was important enough to
> justify the use of a "\;" separator between statements, to send them
> as a group.
>
> But with the libpq batch API, maybe this could be modernized
> with meta-commands like this:
>   \startbatch
>   ...
>   \endbatch

Or just \batch [on|off], which sounds like a damn good idea to me for
some users willing to test some workloads before integrating it in an
application.
-- 
Michael

  20. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-10-04T08:38:48Z

    On 4 Oct. 2016 15:15, "Michael Paquier" <michael.paquier@gmail.com> wrote:
>
> On Mon, Oct 3, 2016 at 11:52 PM, Daniel Verite <daniel@manitou-mail.org>
wrote:
> > Wouldn't pgbench benefit from it?
> > It was mentioned some time ago [1], in relationship to the
> > \into construct, how client-server latency was important enough to
> > justify the use of a "\;" separator between statements, to send them
> > as a group.
> >
> > But with the libpq batch API, maybe this could be modernized
> > with meta-commands like this:
> >   \startbatch
> >   ...
> >   \endbatch
>
> Or just \batch [on|off], which sounds like a damn good idea to me for
> some users willing to test some workloads before integrating it in an
> application.

A batch jsnt necessarily terminated by a commit, so I'm more keen on
start/end batch. It's more in line with begin/commit. Batch is not only a
mode, you also have to delineate batches.

  21. Re: PATCH: Batch/pipelining support for libpq

    Gavin Flower <gavinflower@archidevsys.co.nz> — 2016-10-04T08:39:26Z

    On 04/10/16 20:15, Michael Paquier wrote:
> On Mon, Oct 3, 2016 at 11:52 PM, Daniel Verite <daniel@manitou-mail.org> wrote:
>> Wouldn't pgbench benefit from it?
>> It was mentioned some time ago [1], in relationship to the
>> \into construct, how client-server latency was important enough to
>> justify the use of a "\;" separator between statements, to send them
>> as a group.
>>
>> But with the libpq batch API, maybe this could be modernized
>> with meta-commands like this:
>>    \startbatch
>>    ...
>>    \endbatch
> Or just \batch [on|off], which sounds like a damn good idea to me for
> some users willing to test some workloads before integrating it in an
> application.

+1

'\batch' is a bit easier, to find, & to remember than '\startbatch'

  22. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2016-10-05T04:54:07Z

    On Mon, Oct 3, 2016 at 12:48 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 3 October 2016 at 10:10, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Tue, Sep 6, 2016 at 8:01 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>> On 6 September 2016 at 16:10, Daniel Verite <daniel@manitou-mail.org> wrote:
>>>>         Craig Ringer wrote:
>>>>
>>>>> Updated patch attached.
>>>>
>>>> Please find attached a couple fixes for typos I've came across in
>>>> the doc part.
>>>
>>> Thanks, will apply and post a rebased patch soon, or if someone picks
>>> this up in the mean time they can apply your diff on top of the patch.
>>
>> Could you send an updated patch then? At the same time I am noticing
>> that git --check is complaining... This patch has tests and a
>> well-documented feature, so I'll take a look at it soon at the top of
>> my list. Moved to next CF for now.
>
> Thanks.
>
> I'd really like to teach psql in non-interactive mode to use it, but
> (a) I'm concerned about possible subtle behaviour differences arising
> if we do that and (b) I won't have the time. I think it's mostly of
> interest to app authors and driver developers and that's what it's
> aimed at. pg_restore could benefit a lot too.

Looking at it now... The most interesting comments are first.

I wanted to congratulate you. I barely see a patch with this level of
details done within the first versions. Anybody can review the patch
just by looking at the code and especially the docs without looking at
the thread. There are even tests to show what this does, for the
client.

+PQisInBatchMode           172
+PQqueriesInBatch          173
+PQbeginBatchMode          174
+PQendBatchMode            175
+PQsendEndBatch            176
+PQgetNextQuery            177
+PQbatchIsAborted          178
This set of routines is a bit inconsistent. Why not just prefixing
them with PQbatch? Like that for example:
PQbatchStatus(): consists of disabled/inactive/none, active, error.
This covers both PQbatchIsAborted() and PQisInBatchMode().
PQbatchBegin()
PQbatchEnd()
PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to
add and process a sync message into the queue.
PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,
-1 on failure
PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on
failure (OOM)

+   <para>
+    Much like asynchronous query mode, there is no performance disadvantage to
+    using batching and pipelining. It somewhat increased client application
+    complexity and extra caution is required to prevent client/server network
+    deadlocks, but can offer considerable performance improvements.
+   </para>
I would reword that a bit "it increases client application complexity
and extra caution is required to prevent client/server deadlocks but
offers considerable performance improvements".

+    ("ping time") is high, and when many small operations are being
performed in
Nit: should use <quote> here. Still not quoting it would be fine.

+     After entering batch mode the application dispatches requests
+     using normal asynchronous <application>libpq</application> functions like
+     <function>PQsendQueryParams</function>,
<function>PQsendPrepare</function>,
+     etc. The asynchronous requests are followed by a <link
It may be better to list all the functions here, PQSendQuery

  *   query work remains or an error has occurred (e.g. out of
  *   memory).
  */
-
 PGresult *
 PQgetResult(PGconn *conn)
Some noise in the patch.

git diff --check complains:
                usage_exit(argv[0]);
warning: 1 line adds whitespace errors.

+++ b/src/interfaces/libpq/t/001_libpq_async.pl
@@ -0,0 +1,15 @@
+use strict;
+use warnings;
+
+use Config;
+use PostgresNode;
+use TestLib;
+use Test::More;
+
+my $node = get_new_node('main');
+$node->init;
+$node->start;
+
+my $port = $node->port;
+
+$node->stop('fast');
Er... This does nothing..

The changes in src/test/examples/ are not necessary anymore. You moved
all the tests to test_libpq (for the best actually).

+   while (queue != NULL)
+   {
+       PGcommandQueueEntry *prev = queue;
+       queue = queue->next;
+       free(prev);
+   }
This should free prev->query.

/* blah comment
* blah2 comment
*/
A lot of comment blocks are like that, those should be reformated.

Running directly make check in src/test/modules/test_libpq/ does not work:
# Postmaster PID for node "main" is 10225
# Running: testlibpqbatch dbname=postgres 10000 disallowed_in_batch
Command 'testlibpqbatch' not found in [...PATH list ...]
The problem is that testlibpqbatch is not getting compiled but I think
it should.

+ * testlibpqbatch.c
+ *     Test of batch execution funtionality
[...]
+               fprintf(stderr, "%s is not a recognised test name\n", argv[3]);
s/funtionality/functionality/
s/recognised/recognized/

+   if (!PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+                          dummy_params, NULL, NULL, 0))
+   {
+       fprintf(stderr, "dispatching first SELECT failed: %s\n",
PQerrorMessage(conn));
+       goto fail;
+   }
+
+   if (!PQsendEndBatch(conn))
+   {
+       fprintf(stderr, "Ending first batch failed: %s\n",
PQerrorMessage(conn));
+       goto fail;
+   }
+
+   if (!PQsendQueryParams(conn, "SELECT $1", 1, dummy_param_oids,
+                          dummy_params, NULL, NULL, 0))
+   {
+       fprintf(stderr, "dispatching second SELECT failed: %s\n",
PQerrorMessage(conn));
+       goto fail;
+   }
May be better to use a loop here and set in the queue a bunch of queries..

You could just remove the VERBOSE flag in the tests, having a test
more talkative is always better.

+    <para>
+     The batch API was introduced in PostgreSQL 9.6, but clients using it can
+     use batches on server versions 8.4 and newer. Batching works on any server
+     that supports the v3 extended query protocol.
+    </para>
Postgres 10, not 9.6.

It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an
error for the new routines.

+int
+PQgetNextQuery(PGconn *conn)
[...]
+       return false;
The new routines had better return explicitly 0 or 1, not a boolean
for consistency with the rest.

+      for processing. Returns 0 and has no effect if there are no query results
+      pending, batch mode is not enabled, or if the query currently processed
+      is incomplete or still has pending results. See <link
It would be useful for the user to make a difference between those
different statuses. As of your patch, PQgetNextQuery() returns false/0
in all those cases so it is a bit hard to know what's going on. Maybe
PQgetNextQuery() could be renamed to PQbatchGetNext, PQbatchQueueNext
to outline the fact that it is beginning the next query in the queue.
This helps also to understand that this can just be used with the
batch mode. Actually no, I am wrong. It is possible to guess each one
of those errors with respectively PQgetResult == NULL,
PQisInBatchMode() and PQqueriesInBatch(). Definitely it should be
mentioned in the docs that it is possible to make a difference between
all those states.

+   entry = PQmakePipelinedCommand(conn);
+   entry->queryclass = PGQUERY_SYNC;
+   entry->query = NULL;
PQmakePipelinedCommand() returns NULL, and boom.

+   bool        in_batch;       /* connection is in batch (pipelined) mode */
+   bool        batch_aborted;  /* current batch is aborted,
discarding until next Sync */
Having only one flag would be fine. batch_aborted is set of used only
when in_batch is used, so both have a strong link.

    /* OK, it's launched! */
-   conn->asyncStatus = PGASYNC_BUSY;
+   if (conn->in_batch)
+       PQappendPipelinedCommand(conn, pipeCmd);
+   else
+       conn->asyncStatus = PGASYNC_BUSY;
No, it is put in the queue.

+      <para>
+      Returns the number of queries still in the queue for this batch, not
+      including any query that's currently having results being processsed.
+      This is the number of times <function>PQgetNextQuery</function> has to be
+      called before the query queue is empty again.
+
+<synopsis>
+int PQqueriesInBatch(PGconn *conn);
+</synopsis>
This is not true. It does not return a count, just 0 or 1.

It may be a good idea to add a test for COPY and trigger a failure.

If I read the code correctly, it seems to me that it is possible to
enable the batch mode, and then to use PQexec(), PQsendPrepare will
just happily process queue the command. Shouldn't PQexec() be
prevented in batch mode? Similar remark for PQexecParams(),
PQexecPrepared() PQdescribePrepared and PQprepare(). In short
everything calling PQexecFinish().

I haven't run the tests directly on Windows wiht MSVC, but they won't
run as vcregress modulescheck lacks knowledge about modules using TAP.
I cannot blame you for that..
-- 
Michael

  23. Re: PATCH: Batch/pipelining support for libpq

    Shay Rojansky <roji@roji.org> — 2016-10-12T11:51:47Z

    Hi all. I thought I'd share some experience from Npgsql regarding
batching/pipelining - hope this isn't off-topic.

Npgsql has supported batching for quite a while, similar to what this patch
proposes - with a single Sync message is sent at the end.

It has recently come to my attention that this implementation is
problematic because it forces the batch to occur within a transaction; in
other words, there's no option for a non-transactional batch. This can be a
problem for several reasons: users may want to sent off a batch of inserts,
not caring whether one of them fails (e.g. because of a unique constraint
violation). In other words, in some scenarios it may be appropriate for
later batched statements to be executed when an earlier batched statement
raised an error. If Sync is only sent at the very end, this isn't possible.
Another example of a problem (which actually happened) is that transactions
acquire row-level locks, and so may trigger deadlocks if two different
batches update the same rows in reverse order. Both of these issues
wouldn't occur if the batch weren't implicitly batched.

My current plan is to modify the batch implementation based on whether
we're in an (explicit) transaction or not. If we're in a transaction, then
it makes perfect sense to send a single Sync at the end as is being
proposed here - any failure would cause the transaction to fail anyway, so
skipping all subsequent statements until the batch's end makes sense.
However, if we're not in an explicit transaction, I plan to insert a Sync
message after each individual Execute, making non-transactional batched
statements more or less identical in behavior to non-transactional
unbatched statements. Note that this mean that a batch can generate
multiple errors, not just one.

I'm sharing this since it may be relevant to the libpq batching
implementation as well, and also to get any feedback regarding how Npgsql
should act.

  24. Re: PATCH: Batch/pipelining support for libpq

    Jim Nasby <jim.nasby@bluetreble.com> — 2016-10-14T01:04:14Z

    On 10/4/16 11:54 PM, Michael Paquier wrote:
> +   <para>
> +    Much like asynchronous query mode, there is no performance disadvantage to
> +    using batching and pipelining. It somewhat increased client application
> +    complexity and extra caution is required to prevent client/server network
> +    deadlocks, but can offer considerable performance improvements.
> +   </para>
> I would reword that a bit "it increases client application complexity
> and extra caution is required to prevent client/server deadlocks but
> offers considerable performance improvements".

Unrelated, but another doc bug, on line 4647:

  +     The batch API was introduced in PostgreSQL 9.6, but clients 
using it can

That should read 10.0 (or just 10?)
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461

  25. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-10-14T05:02:07Z

    On 12 October 2016 at 19:51, Shay Rojansky <roji@roji.org> wrote:
> Hi all. I thought I'd share some experience from Npgsql regarding
> batching/pipelining - hope this isn't off-topic.

Not at all.

> Npgsql has supported batching for quite a while, similar to what this patch
> proposes - with a single Sync message is sent at the end.

PgJDBC does too. The benefits of it there are what prompted me to do
this, not only so libpq users can use it directly but so psqlODBC,
psycopg2, etc can benefit from it if they choose to expose it.

> It has recently come to my attention that this implementation is problematic
> because it forces the batch to occur within a transaction; in other words,
> there's no option for a non-transactional batch.

That's not strictly the case. If you explicitly BEGIN and COMMIT,
those operations are honoured within the batch.

What's missing is implicit transactions. Usually if you send a series
of messages they will each get their own implicit transaction. If you
batch them, the whole lot gets an implicit transaction. This is
because the PostgreSQL v3 protocol conflates transaction delineation
with protocol error recovery into a single Sync message type.

It's very similar to the behaviour of multi-statements, where:

psql -c "CREATE TABLE fred(x integer); DROP TABLE notexists;"

doesn't create "fred", but

psql -c "BEGIN; CREATE TABLE fred(x integer); COMMIT; BEGIN; DROP
TABLE notexists; COMMMIT;"

... does. And in fact it's for almost the same reason. They're sent as
a single SimpleQuery message by psql and split up client side, but the
effect is the same as two separate Query messages followed by a Sync.

It isn't simple to manage this client-side, because libpq doesn't know
whether a given command string may contain transaction delimiting
statements or not and can't reliably look for them without client-side
parsing of the SQL. So it can't dispatch its own BEGIN/COMMIT around
statements in a batch that it thinks might be intended to be
autocommit, and anyway that'd result in sending 3 queries for every 1
client query, which would suck.

If the mythical v4 protocol ever happens I'd want to split Sync into
two messages, one which is a protocol synchronisation message and
another that is a transaction delimiter. Or give it flags or whatever.

In the mean time:

> This can be a problem for
> several reasons: users may want to sent off a batch of inserts, not caring
> whether one of them fails (e.g. because of a unique constraint violation).
> In other words, in some scenarios it may be appropriate for later batched
> statements to be executed when an earlier batched statement raised an error.
> If Sync is only sent at the very end, this isn't possible.

Right, and that remains the case even with explicit transaction
delineation, because the first ERROR causes processing of all
subsequent messages to be skipped.

The design I have in libpq allows for this by allowing the client to
delimit batches without ending batch mode, concurrently consuming a
stream of multiple batches. Each endbatch is a Sync. So a client that
wants autocommit-like behavour can send a series of 1-query batches.

I think I'll need to document this a bit better since it's more subtle
than I properly explained.

> Another example
> of a problem (which actually happened) is that transactions acquire
> row-level locks, and so may trigger deadlocks if two different batches
> update the same rows in reverse order. Both of these issues wouldn't occur
> if the batch weren't implicitly batched.

Same solution as above.

> My current plan is to modify the batch implementation based on whether we're
> in an (explicit) transaction or not. If we're in a transaction, then it
> makes perfect sense to send a single Sync at the end as is being proposed
> here - any failure would cause the transaction to fail anyway, so skipping
> all subsequent statements until the batch's end makes sense. However, if
> we're not in an explicit transaction, I plan to insert a Sync message after
> each individual Execute, making non-transactional batched statements more or
> less identical in behavior to non-transactional unbatched statements. Note
> that this mean that a batch can generate multiple errors, not just one.

Yes, that's what I suggest, and basically what the libpq batch
interface does, though it expects the client to deal with the
transaction boundaries.

You will need to think hard about transaction boundaries as they
relate to multi-statements unless nPgSQL parses out each statement
from multi-statement strings like PgJDBC does. Otherwise a user can
sneak in:

somestatement; BEGIN; someotherstatement;

or

somestatement; CoMMiT; otherstatement;

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

  26. Re: PATCH: Batch/pipelining support for libpq

    Shay Rojansky <roji@roji.org> — 2016-10-14T10:09:44Z

    >
> > It has recently come to my attention that this implementation is
> problematic
> > because it forces the batch to occur within a transaction; in other
> words,
> > there's no option for a non-transactional batch.
>
> That's not strictly the case. If you explicitly BEGIN and COMMIT,
> those operations are honoured within the batch.
>

I wasn't precise in my formulation (although I think we understand each
other)... The problem I'm trying to address here is the fact that in the
"usual" batching implementation (i.e. where a single Sync message is sent
at the end of the batch), there's no support for batches which have no
transactions whatsoever (i.e. where each statement is auto-committed and
errors in earlier statements don't trigger skipping of later statements).

What's missing is implicit transactions. Usually if you send a series
> of messages they will each get their own implicit transaction. If you
> batch them, the whole lot gets an implicit transaction. This is
> because the PostgreSQL v3 protocol conflates transaction delineation
> with protocol error recovery into a single Sync message type.
>

That's right. I sent a message complaining about this conflation a while
ago:
https://www.postgresql.org/message-id/CADT4RqDdo9EcFbxwB_YO2H3BVZ0t-1qqZ%3D%2B%2BdVMnYaN6BpyUGQ%40mail.gmail.com.
There weren't any responses, although I'll add a note on the wiki on this
as a requested feature for the v4 protocol.

If the mythical v4 protocol ever happens I'd want to split Sync into
> two messages, one which is a protocol synchronisation message and
> another that is a transaction delimiter. Or give it flags or whatever.
>

Totally agree.


> In the mean time:
>
> > This can be a problem for
> > several reasons: users may want to sent off a batch of inserts, not
> caring
> > whether one of them fails (e.g. because of a unique constraint
> violation).
> > In other words, in some scenarios it may be appropriate for later batched
> > statements to be executed when an earlier batched statement raised an
> error.
> > If Sync is only sent at the very end, this isn't possible.
>
> Right, and that remains the case even with explicit transaction
> delineation, because the first ERROR causes processing of all
> subsequent messages to be skipped.
>
> The design I have in libpq allows for this by allowing the client to
> delimit batches without ending batch mode, concurrently consuming a
> stream of multiple batches. Each endbatch is a Sync. So a client that
> wants autocommit-like behavour can send a series of 1-query batches.


> I think I'll need to document this a bit better since it's more subtle
> than I properly explained.
>

Ah, I see. libpq's API is considerably more low-level than what Npgsql
needs to provide. If I understand correctly, you allow users to specify
exactly where to insert Sync messages (if at all), so that any number of
statements arbitrarily interleaved with Sync messages can be sent without
starting to read any results. If so, then the user indeed has everything
they need to control the exact transactional behavior they want (including
full auto-commit) without compromising on performance in any way (i.e. by
increasing roundtrips).

The only minor problem I can see is that PQsendEndBatch not only adds a
Sync message to the buffer, but also flushes it. This means that you may be
forcing users to needlessly flush the buffer just because they wanted to
insert a Sync. In other words, users can't send the following messages in a
single buffer/packet:
Prepare1/Bind1/Describe1/Execute1/Sync1/Prepare2/Bind2/Describe2/Execute2/Sync2
- they have to be split into different packets. Of course, this is a
relatively minor performance issue (especially when compared to the overall
performance benefits provided by batching), and providing an API
distinction between adding a Sync and flushing the buffer may
over-complicate the API. I just thought I'd mention it.


> Yes, that's what I suggest, and basically what the libpq batch
> interface does, though it expects the client to deal with the
> transaction boundaries.
>
> You will need to think hard about transaction boundaries as they
> relate to multi-statements unless nPgSQL parses out each statement
> from multi-statement strings like PgJDBC does. Otherwise a user can
> sneak in:
>
> somestatement; BEGIN; someotherstatement;
>
> or
>
> somestatement; CoMMiT; otherstatement;
>

That's a good point. I definitely don't want to depend on client-side
parsing of SQL in any way (in fact a planned feature is to allow using
Npgsql without any sort of client-side parsing/manipulation of SQL).
However, the fact that BEGIN/COMMIT can be sent in batches doesn't appear
too problematic to me.

When it's about to send a batch, Npgsql knows whether it's in an (explicit)
transaction or not (by examining the transaction indicator on the last
ReadyForQuery message it received). If it's not in an (explicit)
transaction, it automatically inserts a Sync message after every Execute.
If some statement happens to be a BEGIN, it will be executed as a normal
statement and so on. The only issue is that if an error occurs after a
sneaked-in BEGIN, all subsequent statements will fail, and all have the
Sync messages Npgsql inserted. The end result will be a series of errors
that will be raised up to the user, but this isn't fundamentally different
from the case of a simple auto-commit batch containing multiple failures
(because of unique constraint violation or whatever) - multiple errors is
something that will have to be handled in any case.

Thanks for all your comments. Npgsql's support of batches needs to be more
complicated than libpq's since it's a more high-level interface - whereas
libpq offloads some of the sending/processing complexity to the user,
Npgsql needs to take care of most of it internally (another good example is
deadlock avoidance...).

  27. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-10-14T12:53:34Z

    On 14 October 2016 at 18:09, Shay Rojansky <roji@roji.org> wrote:
>> > It has recently come to my attention that this implementation is
>> > problematic
>> > because it forces the batch to occur within a transaction; in other
>> > words,
>> > there's no option for a non-transactional batch.
>>
>> That's not strictly the case. If you explicitly BEGIN and COMMIT,
>> those operations are honoured within the batch.
>
>
> I wasn't precise in my formulation (although I think we understand each
> other)... The problem I'm trying to address here is the fact that in the
> "usual" batching implementation (i.e. where a single Sync message is sent at
> the end of the batch), there's no support for batches which have no
> transactions whatsoever (i.e. where each statement is auto-committed and
> errors in earlier statements don't trigger skipping of later statements).

Right, you can't use implicit transactions delimited by protocol
message boundaries when batching.

>> The design I have in libpq allows for this by allowing the client to
>> delimit batches without ending batch mode, concurrently consuming a
>> stream of multiple batches. Each endbatch is a Sync. So a client that
>> wants autocommit-like behavour can send a series of 1-query batches.
>
> Ah, I see. libpq's API is considerably more low-level than what Npgsql needs
> to provide. If I understand correctly, you allow users to specify exactly
> where to insert Sync messages (if at all)

They don't get total control, but they can cause a Sync to be emitted
after any given statement when in batch mode.

> so that any number of statements
> arbitrarily interleaved with Sync messages can be sent without starting to
> read any results.

Right.

> if so, then the user indeed has everything they need to
> control the exact transactional behavior they want (including full
> auto-commit) without compromising on performance in any way (i.e. by
> increasing roundtrips).

Right.

> The only minor problem I can see is that PQsendEndBatch not only adds a Sync
> message to the buffer, but also flushes it.

It only does a non-blocking flush.

> This means that you may be
> forcing users to needlessly flush the buffer just because they wanted to
> insert a Sync. In other words, users can't send the following messages in a
> single buffer/packet:
> Prepare1/Bind1/Describe1/Execute1/Sync1/Prepare2/Bind2/Describe2/Execute2/Sync2
> - they have to be split into different packets.

Oh, right. That's true, but I'm not really sure we care. I suspect
that the OS will tend to coalesce them anyway, since we're not
actually waiting until the socket sends the message. At least when the
socket isn't able to send as fast as input comes in. It might matter
for local performance in incredibly high throughput applications, but
I suspect there will be a great many other things that come first.

Anyway, the client can already control this with TCP_CORK.

>  Of course, this is a
> relatively minor performance issue (especially when compared to the overall
> performance benefits provided by batching), and providing an API distinction
> between adding a Sync and flushing the buffer may over-complicate the API. I
> just thought I'd mention it.

Unnecessary IMO. If we really want to add it later we'd probably do so
by setting a "no flush on endbatch" mode and adding an explicit flush
call. But I expect TCP_CORK will satisfy all realistic needs.

> That's a good point. I definitely don't want to depend on client-side
> parsing of SQL in any way (in fact a planned feature is to allow using
> Npgsql without any sort of client-side parsing/manipulation of SQL).
> However, the fact that BEGIN/COMMIT can be sent in batches doesn't appear
> too problematic to me.
>
> When it's about to send a batch, Npgsql knows whether it's in an (explicit)
> transaction or not (by examining the transaction indicator on the last
> ReadyForQuery message it received). If it's not in an (explicit)
> transaction, it automatically inserts a Sync message after every Execute. If
> some statement happens to be a BEGIN, it will be executed as a normal
> statement and so on. The only issue is that if an error occurs after a
> sneaked-in BEGIN, all subsequent statements will fail, and all have the Sync
> messages Npgsql inserted. The end result will be a series of errors that
> will be raised up to the user, but this isn't fundamentally different from
> the case of a simple auto-commit batch containing multiple failures (because
> of unique constraint violation or whatever) - multiple errors is something
> that will have to be handled in any case.

I'm a bit hesitant about how this will interact with multi-statements
containing embedded BEGIN or COMMIT, where a single protocol message
contains multiple ; delimited queries. But at least at this time of
night I can't give a concrete problematic example.

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

  28. Re: PATCH: Batch/pipelining support for libpq

    Shay Rojansky <roji@roji.org> — 2016-10-14T14:15:55Z

    >
> >  Of course, this is a
> > relatively minor performance issue (especially when compared to the
> overall
> > performance benefits provided by batching), and providing an API
> distinction
> > between adding a Sync and flushing the buffer may over-complicate the
> API. I
> > just thought I'd mention it.
>
> Unnecessary IMO. If we really want to add it later we'd probably do so
> by setting a "no flush on endbatch" mode and adding an explicit flush
> call. But I expect TCP_CORK will satisfy all realistic needs.
>

Unless I'm mistaken TCP_CORK is not necessarily going to work across all
platforms (e.g. Windows), although SO_LINGER (which is more standard) also
helps here.

> When it's about to send a batch, Npgsql knows whether it's in an
> (explicit)
> > transaction or not (by examining the transaction indicator on the last
> > ReadyForQuery message it received). If it's not in an (explicit)
> > transaction, it automatically inserts a Sync message after every
> Execute. If
> > some statement happens to be a BEGIN, it will be executed as a normal
> > statement and so on. The only issue is that if an error occurs after a
> > sneaked-in BEGIN, all subsequent statements will fail, and all have the
> Sync
> > messages Npgsql inserted. The end result will be a series of errors that
> > will be raised up to the user, but this isn't fundamentally different
> from
> > the case of a simple auto-commit batch containing multiple failures
> (because
> > of unique constraint violation or whatever) - multiple errors is
> something
> > that will have to be handled in any case.
>
> I'm a bit hesitant about how this will interact with multi-statements
> containing embedded BEGIN or COMMIT, where a single protocol message
> contains multiple ; delimited queries. But at least at this time of
> night I can't give a concrete problematic example.


Unless I'm mistaken, in the extended protocol you can't combine multiple ;
delimited queries in a single Parse - that's a feature supported only by
the Query message of the simple protocol. But then, if you're in the simple
protocol Sync doesn't play any role, does it? I mean, a Query message
behaves as though it's implicitly followed by a Sync message - an error in
a previous Query message doesn't cause later messages to be skipped...

Note that Npgsql only rarely uses the simple protocol for user messages.
Npgsql is a binary-only driver, and the simple protocol doesn't support
requesting binary results. So only the extended protocol is used, except
for some edge cases where it's possible to use the simple protocol for
efficiency - statements with no parameters and where the ExecuteNonQuery
API is used (i.e. the user won't access any results).

  29. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-10-14T14:28:21Z

    On 14 October 2016 at 22:15, Shay Rojansky <roji@roji.org> wrote:

> Unless I'm mistaken TCP_CORK is not necessarily going to work across all
> platforms (e.g. Windows), although SO_LINGER (which is more standard) also
> helps here.

Yeah, true.

You can also twiddle TCP_NODELAY on and off on other platforms.

Anyway, my point is that it's not likely to be crucial, especially
since even without socket options the host will often do packet
combining if the queue isn't empty.

> Unless I'm mistaken, in the extended protocol you can't combine multiple ;
> delimited queries in a single Parse - that's a feature supported only by the
> Query message of the simple protocol. But then, if you're in the simple
> protocol Sync doesn't play any role, does it? I mean, a Query message
> behaves as though it's implicitly followed by a Sync message - an error in a
> previous Query message doesn't cause later messages to be skipped.

Right, good point. So that concern isn't relevant.

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

  30. Re: PATCH: Batch/pipelining support for libpq

    Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> — 2016-11-18T06:04:31Z

    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?

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;


(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?  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?


(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.

[winsock2.h]
/*
 * The new type to be used in all
 * instances which refer to sockets.
 */
typedef UINT_PTR        SOCKET;

Regards
Takayuki Tsunakawa

  31. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-11-18T08:18:15Z

    On 18 November 2016 at 14:04, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.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

  32. Re: PATCH: Batch/pipelining support for libpq

    Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-22T07:14:23Z

    On Fri, Nov 18, 2016 at 7:18 PM, Craig Ringer <craig@2ndquadrant.com> wrote:

> 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.
>

Currently patch status is marked as "returned with feedback" in the 11-2016
commitfest. Anyone who wants to work on it can submit the updated patch
by taking care of all review comments and change the status of the patch
at any time.

Thanks for the patch.

Regards,
Hari Babu
Fujitsu Australia

  33. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2016-11-22T07:31:52Z

    On 22 November 2016 at 15:14, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
> On Fri, Nov 18, 2016 at 7:18 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>
>> 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.
>
>
> Currently patch status is marked as "returned with feedback" in the 11-2016
> commitfest. Anyone who wants to work on it can submit the updated patch
> by taking care of all review comments and change the status of the patch
> at any time.
>
> Thanks for the patch.

Thanks. Sorry I haven't had time to work on it. Priorities.

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

  34. Re: PATCH: Batch/pipelining support for libpq

    Iwata, Aya <iwata.aya@jp.fujitsu.com> — 2017-02-14T07:27:56Z

    Hi,



On 18 November 2016 at 08:18, Craig Ringer wrote:

>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.

I am interested in pipeline/batch support for ECPG, and found this thread.

I updated Craig's patch [1] to apply this one to HEAD. Moreover, I fixed an easy typo.



First, I'm changing PQqueriesInBatch() to work as documented.

After that, I plan to reflect contents of reviews in the patch.



On Mon, Oct 3, 2016 at 11:52 PM, Daniel Verite wrote:

> Wouldn't pgbench benefit from it?

> It was mentioned some time ago [1], in relationship to the

> \into construct, how client-server latency was important enough to

> justify the use of a "\;" separator between statements, to send them

> as a group.

>

> But with the libpq batch API, maybe this could be modernized

> with meta-commands like this:

>   \startbatch

>   ...

>   \endbatch

I'm planning to work on meta-commands to support batch mode after committing this patch successfully.





[1]https://github.com/2ndQuadrant/postgres/tree/dev/libpq-async-batch

Regards,

Aya Iwata

FUJITSU

  35. Re: PATCH: Batch/pipelining support for libpq

    Prabakaran, Vaishnavi <vaishnavip@fast.au.fujitsu.com> — 2017-02-17T05:17:03Z

    On 22 November 2016 at 18:32, Craig Ringer<craig@2ndquadrant.com> wrote:
> On 22 November 2016 at 15:14, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> >
> > On Fri, Nov 18, 2016 at 7:18 PM, Craig Ringer <craig@2ndquadrant.com>
> wrote:
> >>
> >> 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.
> >
> >
> > Currently patch status is marked as "returned with feedback" in the
> > 11-2016 commitfest. Anyone who wants to work on it can submit the
> > updated patch by taking care of all review comments and change the
> > status of the patch at any time.
> >
> > Thanks for the patch.
>
> Thanks. Sorry I haven't had time to work on it. Priorities.
Hi,
I am interested in this patch and addressed various below comments from reviewers. And, I have separated out code and test module into 2 patches. So, If needed, test patch can be enhanced more, meanwhile code patch can be committed.

>Renaming and refactoring new APIs
> +PQisInBatchMode           172
>+PQqueriesInBatch          173
>+PQbeginBatchMode          174
>+PQendBatchMode            175
>+PQsendEndBatch            176
>+PQgetNextQuery            177
>+PQbatchIsAborted          178
>This set of routines is a bit inconsistent. Why not just prefixing them with PQbatch? Like that for example:
> PQbatchStatus(): consists of disabled/inactive/none, active, error. This covers both PQbatchIsAborted() and PQisInBatchMode().
>PQbatchBegin()
>PQbatchEnd()
>PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to add and process a sync message into the queue.

Renamed and modified batch status APIs as below
PQisInBatchMode & PQbatchIsAborted ==> PQbatchStatus
PQqueriesInBatch ==> PQbatchQueueCount
PQbeginBatchMode ==> PQbatchBegin
PQendBatchMode ==> PQbatchEnd
PQsendEndBatch ==> PQbatchQueueSync
PQgetNextQuery ==> PQbatchQueueProcess

>PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,-1 on failure
>PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on failure (OOM)
I think it is still ok to keep the current behaviour like other ones present in the same file. E.g:"PQsendPrepare" "PQsendQueryGuts"

>PQqueriesInBatch() (Newname(NN):PQbatchQueueCount)doesn't work as documented.
>It says:
>"Returns the number of queries still in the queue for this batch"
>but in fact it's implemented as a Boolean.
Modified the logic to count number of entries in pending queue and return the count

>The changes in src/test/examples/ are not necessary anymore. You moved all the tests to test_libpq (for the best actually).
Removed these unnecessary changes from src/test/examples folder and corrected the path mentioned in comments section of testlibpqbatch.c

> +   while (queue != NULL)
>+  {
>       PGcommandQueueEntry *prev = queue;
>+       queue = queue->next;
>+       free(prev);
>+   }
>This should free prev->query.
Both prev->query and prev is freed. Also, this applies to "cmd_queue_recycle" too.

>Running directly make check in src/test/modules/test_libpq/ does not work
Modified "check" rule in makefile

>You could just remove the VERBOSE flag in the tests, having a test more talkative is always better.
Removed ifdef VERBOSE checks.

>But with the libpq batch API, maybe this could be modernized
>with meta-commands like this:
>\startbatch
>...
>\endbatch
I think it is a separate patch candidate.

> It is possible to guess each one of those errors(occurred in PQbatchQueueProcess API) with respectively
> PQgetResult == NULL, PQisInBatchMode() and PQqueriesInBatch().
> Definitely it should be mentioned in the docs that it is possible to make a difference between all those states.
Updated documentation section of PQbatchQueueProcess() with these details.

> +   entry = PQmakePipelinedCommand(conn);
>+   entry->queryclass = PGQUERY_SYNC;
>+   entry->query = NULL;
>PQmakePipelinedCommand() returns NULL, and boom.
Corrected to return false if PQmakePipelinedCommand() returns NULL.

> +   bool        in_batch;       /* connection is in batch (pipelined) mode */
>+   bool        batch_aborted;  /* current batch is aborted, discarding until next Sync */
>Having only one flag would be fine. batch_aborted is set of used only
>when in_batch is used, so both have a strong link
Yes, agree that tracking the batch status via one flag is more clean
So, Added new enum typedef enum
{
        PQBATCH_MODE_OFF,
        PQBATCH_MODE_ON,
        PQBATCH_MODE_ABORTED
} PQBatchStatus;
and " PQBatchStatus batch_status"  member of pg_conn is used to track the status of batch mode.

>/* OK, it's launched! */
>-   conn->asyncStatus = PGASYNC_BUSY;
>+   if (conn->in_batch)
>+       PQappendPipelinedCommand(conn, pipeCmd);
>+   else
>+       conn->asyncStatus = PGASYNC_BUSY;
>No, it is put in the queue
Though it is put in queue, technically the command is sent to server and server might have started executing it.
So it is ok to use launched I think.

> It may be a good idea to add a test for COPY and trigger a failure.
Added new test - "copy_failure" to trigger failures during copy state.
Please note that COPY is allowed in batch mode, but only syncing batch/queuing any command while copy is in progress is blocked due to potential failure of entire batch.
So updated the documentation for more clarity in this case.

>If I read the code correctly, it seems to me that it is possible to enable the batch mode, and then to use PQexec(), PQsendPrepare will
>just happily process queue the command. Shouldn't PQexec() be prevented in batch mode? Similar remark for PQexecParams(),
>PQexecPrepared() PQdescribePrepared and PQprepare(). In short everything calling PQexecFinish().
Check to verify whether the connection is in batch mode or not is already present in PQexecStart().
And, all the functions calling PQexecFinish() will not be allowed in batch mode anyways. So, no new check is needed I think.

> It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an error for the new routines.
All the APIs which supports asynchronous command processing can be executed only if PG_PROTOCOL_MAJOR >= 3. So, adding it to new routines are not really needed.

> +     After entering batch mode the application dispatches requests
>+     using normal asynchronous <application>libpq</application> functions like
>+     <function>PQsendQueryParams</function>,
><function>PQsendPrepare</function>,
>+     etc.
>It may be better to list all the functions here, PQSendQuery
Documentation updated with list of functions - PQsendQueryParams,PQsendPrepare,
PQsendQueryPrepared,PQdescribePortal,PQdescribePrepared,
PQsendDescribePortal,PQsendDescribePrepared.

>1. Typos in document part - diff-typos.txt file contains the typos specified here
>2. git --check is complaining
>3. s/funtionality/functionality/ and s/recognised/recognized/ typos in testlibpqbatch.c file
>4. s/Batching less useful/Batching is less useful in libpq.sgml
>5.  +   <para>
>+    Much like asynchronous query mode, there is no performance disadvantage to
>+    using batching and pipelining. It somewhat increased client application
>+    complexity and extra caution is required to prevent client/server network
>+    deadlocks, but can offer considerable performance improvements.
>+   </para>
>I would reword that a bit "it increases client application complexity
>and extra caution is required to prevent client/server deadlocks but
>offers considerable performance improvements".
>6. +    ("ping time") is high, and when many small operations are being performed in
>Nit: should use <quote> here. Still not quoting it would be fine.
>7. Postgres 10, not 9.6.
Corrected.


Thanks & Regards,
Vaishnavi
Fujitsu Australia
Disclaimer

The information in this e-mail is confidential and may contain content that is subject to copyright and/or is commercial-in-confidence and is intended only for the use of the above named addressee. If you are not the intended recipient, you are hereby notified that dissemination, copying or use of the information is strictly prohibited. If you have received this e-mail in error, please telephone Fujitsu Australia Software Technology Pty Ltd on + 61 2 9452 9000 or by reply e-mail to the sender and delete the document and all copies thereof.


Whereas Fujitsu Australia Software Technology Pty Ltd would not knowingly transmit a virus within an email communication, it is the receiver’s responsibility to scan all communication and any files attached for computer viruses and other defects. Fujitsu Australia Software Technology Pty Ltd does not accept liability for any loss or damage (whether direct, indirect, consequential or economic) however caused, and whether by negligence or otherwise, which may result directly or indirectly from this communication or any files attached.


If you do not wish to receive commercial and/or marketing email messages from Fujitsu Australia Software Technology Pty Ltd, please email unsubscribe@fast.au.fujitsu.com

  36. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-02-21T07:51:34Z

    On Fri, Feb 17, 2017 at 2:17 PM, Prabakaran, Vaishnavi
<VaishnaviP@fast.au.fujitsu.com> wrote:
> On 22 November 2016 at 18:32, Craig Ringer<craig@2ndquadrant.com> wrote:
> I am interested in this patch and addressed various below comments from reviewers. And, I have separated out code and test module into 2 patches. So, If needed, test patch can be enhanced more, meanwhile code patch can be committed.

Cool. Nice to see a new version of this patch.
(You may want to your replies breath a bit, for example by adding an
extra newline to the paragraph you are answering to.)

>>Renaming and refactoring new APIs
>> +PQisInBatchMode           172
>>+PQqueriesInBatch          173
>>+PQbeginBatchMode          174
>>+PQendBatchMode            175
>>+PQsendEndBatch            176
>>+PQgetNextQuery            177
>>+PQbatchIsAborted          178
>>This set of routines is a bit inconsistent. Why not just prefixing them with PQbatch? Like that for example:
>> PQbatchStatus(): consists of disabled/inactive/none, active, error. This covers both PQbatchIsAborted() and PQisInBatchMode().
>>PQbatchBegin()
>>PQbatchEnd()
>>PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to add and process a sync message into the queue.
>
> Renamed and modified batch status APIs as below
> PQisInBatchMode & PQbatchIsAborted ==> PQbatchStatus
> PQqueriesInBatch ==> PQbatchQueueCount
> PQbeginBatchMode ==> PQbatchBegin
> PQendBatchMode ==> PQbatchEnd
> PQsendEndBatch ==> PQbatchQueueSync
> PQgetNextQuery ==> PQbatchQueueProcess

Thanks. This seems a way cleaner interface to me. Others may have a
different opinion so let's see if there are some.

>>PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,-1 on failure
>>PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on failure (OOM)
>
> I think it is still ok to keep the current behaviour like other ones present in the same file. E.g:"PQsendPrepare" "PQsendQueryGuts"
>
>>PQqueriesInBatch() (Newname(NN):PQbatchQueueCount)doesn't work as documented.
>>It says:
>>"Returns the number of queries still in the queue for this batch"
>>but in fact it's implemented as a Boolean.
>
> Modified the logic to count number of entries in pending queue and return the count
>
>>The changes in src/test/examples/ are not necessary anymore. You moved all the tests to test_libpq (for the best actually).
> Removed these unnecessary changes from src/test/examples folder and corrected the path mentioned in comments section of testlibpqbatch.c

>>But with the libpq batch API, maybe this could be modernized
>>with meta-commands like this:
>>\startbatch
>>...
>>\endbatch
>
> I think it is a separate patch candidate.

Definitely agreed on that. Let's not complicate things further more.

>> It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an error for the new routines.
>
> All the APIs which supports asynchronous command processing can be executed only if PG_PROTOCOL_MAJOR >= 3. So, adding it to new routines are not really needed.

OK. Let's not complicate the patch more than necessary.

After an extra lookup at the patch, here are some comments.

In the docs when listing any of the PQBATCH_MODE_* variables, you
should have a markup <literal> around them. It would be cleaner to
make a list of the potential values that can be returned by
PQbatchStatus() using <variablelist>.

+   while (queue != NULL)
+   {
+       PGcommandQueueEntry *prev = queue;
+
+       queue = queue->next;
+       if (prev->query)
+           free(prev->query);
+       free(prev);
+   }
+   conn->cmd_queue_recycle = NULL
This could be useful as a separate routine.

+/* PQmakePipelinedCommand
+ * Get a new command queue entry, allocating it if required. Doesn't add it to
+ * the tail of the queue yet, use PQappendPipelinedCommand once the command has
+ * been written for that. If a command fails once it's called this, it should
+ * use PQrecyclePipelinedCommand to put it on the freelist or release it.
+ *
This comment block is not project-like. Please use an empty line as
first line with only "/*". The same comment applies to a bunch of the
routines introduced by this patch.

Not sure there is much point in having PQstartProcessingNewQuery. It
only does two things in two places, so that's not worth the loss in
readability.

+   if (conn->batch_status != PQBATCH_MODE_OFF)
+   {
+       pipeCmd = PQmakePipelinedCommand(conn);
+
+       if (pipeCmd == NULL)
+           return 0;           /* error msg already set */
+
+       last_query = &pipeCmd->query;
+       queryclass = &pipeCmd->queryclass;
+   }
+   else
+   {
+       last_query = &conn->last_query;
+       queryclass = &conn->queryclass;
+   }
This setup should happen further down.

conn->in_batch is undefined, causing the patch to fail to compile. And
actually you should not need it.

-                       conn->asyncStatus = PGASYNC_READY;
+                       conn->asyncStatus = PGASYNC_READY_MORE;
                        return;
This should really not be changed, or it should be changed to a status
dedicated to batching only if batch mode is activated.

If you are planning for integrating this patch into Postres 10, please
be sure that this is registered in the last commit fest that will
begin next week:
https://commitfest.postgresql.org/13/
I'll try to book a couple of cycles to look at it if you are able to
register it into the CF and provide a new version.
-- 
Michael

  37. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-02-22T06:14:28Z

    On Tue, Feb 21, 2017 at 6:51 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Fri, Feb 17, 2017 at 2:17 PM, Prabakaran, Vaishnavi
> <VaishnaviP@fast.au.fujitsu.com> wrote:
> > On 22 November 2016 at 18:32, Craig Ringer<craig@2ndquadrant.com> wrote:
> > I am interested in this patch and addressed various below comments from
> reviewers. And, I have separated out code and test module into 2 patches.
> So, If needed, test patch can be enhanced more, meanwhile code patch can be
> committed.
>
> Cool. Nice to see a new version of this patch.
> (You may want to your replies breath a bit, for example by adding an
> extra newline to the paragraph you are answering to.)
>
> >>Renaming and refactoring new APIs
> >> +PQisInBatchMode           172
> >>+PQqueriesInBatch          173
> >>+PQbeginBatchMode          174
> >>+PQendBatchMode            175
> >>+PQsendEndBatch            176
> >>+PQgetNextQuery            177
> >>+PQbatchIsAborted          178
> >>This set of routines is a bit inconsistent. Why not just prefixing them
> with PQbatch? Like that for example:
> >> PQbatchStatus(): consists of disabled/inactive/none, active, error.
> This covers both PQbatchIsAborted() and PQisInBatchMode().
> >>PQbatchBegin()
> >>PQbatchEnd()
> >>PQbatchQueueSync() or PQbatchQueueFlush, same as PQsendEndBatch() to add
> and process a sync message into the queue.
> >
> > Renamed and modified batch status APIs as below
> > PQisInBatchMode & PQbatchIsAborted ==> PQbatchStatus
> > PQqueriesInBatch ==> PQbatchQueueCount
> > PQbeginBatchMode ==> PQbatchBegin
> > PQendBatchMode ==> PQbatchEnd
> > PQsendEndBatch ==> PQbatchQueueSync
> > PQgetNextQuery ==> PQbatchQueueProcess
>
> Thanks. This seems a way cleaner interface to me. Others may have a
> different opinion so let's see if there are some.


>
>>PQbatchQueueCount(): returns N>0 if there are N entries, 0 if empty,-1 on
> failure
> >>PQbatchQueueProcess(): returns 1 if process can begin, 0 if not, -1 on
> failure (OOM)
> >
> > I think it is still ok to keep the current behaviour like other ones
> present in the same file. E.g:"PQsendPrepare" "PQsendQueryGuts"
> >
> >>PQqueriesInBatch() (Newname(NN):PQbatchQueueCount)doesn't work as
> documented.
> >>It says:
> >>"Returns the number of queries still in the queue for this batch"
> >>but in fact it's implemented as a Boolean.
> >
> > Modified the logic to count number of entries in pending queue and
> return the count
> >
> >>The changes in src/test/examples/ are not necessary anymore. You moved
> all the tests to test_libpq (for the best actually).
> > Removed these unnecessary changes from src/test/examples folder and
> corrected the path mentioned in comments section of testlibpqbatch.c
>
> >>But with the libpq batch API, maybe this could be modernized
> >>with meta-commands like this:
> >>\startbatch
> >>...
> >>\endbatch
> >
> > I think it is a separate patch candidate.
>
> Definitely agreed on that. Let's not complicate things further more.
>
> >> It may be a good idea to check for PG_PROTOCOL_MAJOR < 3 and issue an
> error for the new routines.
> >
> > All the APIs which supports asynchronous command processing can be
> executed only if PG_PROTOCOL_MAJOR >= 3. So, adding it to new routines are
> not really needed.
>
> OK. Let's not complicate the patch more than necessary.
>
> After an extra lookup at the patch, here are some comments.
>

Thanks for reviewing the patch.



>
> In the docs when listing any of the PQBATCH_MODE_* variables, you
> should have a markup <literal> around them. It would be cleaner to
> make a list of the potential values that can be returned by
> PQbatchStatus() using <variablelist>.
>

Modified the format of PQbatchStatus() and other batch APIs too in
documentation along with addition of <literal> tags wherever required.



>
> +   while (queue != NULL)
> +   {
> +       PGcommandQueueEntry *prev = queue;
> +
> +       queue = queue->next;
> +       if (prev->query)
> +           free(prev->query);
> +       free(prev);
> +   }
> +   conn->cmd_queue_recycle = NULL
> This could be useful as a separate routine.
>

Ok, Moved this code to new function - PQfreeCommandQueue().



>
> +/* PQmakePipelinedCommand
> + * Get a new command queue entry, allocating it if required. Doesn't add
> it to
> + * the tail of the queue yet, use PQappendPipelinedCommand once the
> command has
> + * been written for that. If a command fails once it's called this, it
> should
> + * use PQrecyclePipelinedCommand to put it on the freelist or release it.
> + *
> This comment block is not project-like. Please use an empty line as
> first line with only "/*". The same comment applies to a bunch of the
> routines introduced by this patch.
>

Corrected this.



>
> Not sure there is much point in having PQstartProcessingNewQuery. It
> only does two things in two places, so that's not worth the loss in
> readability.
>

Yes, removed this function and placed those two things in line.



>
> +   if (conn->batch_status != PQBATCH_MODE_OFF)
> +   {
> +       pipeCmd = PQmakePipelinedCommand(conn);
> +
> +       if (pipeCmd == NULL)
> +           return 0;           /* error msg already set */
> +
> +       last_query = &pipeCmd->query;
> +       queryclass = &pipeCmd->queryclass;
> +   }
> +   else
> +   {
> +       last_query = &conn->last_query;
> +       queryclass = &conn->queryclass;
> +   }
> This setup should happen further down.
>


Moved it down post other validations in this function.




>
> conn->in_batch is undefined, causing the patch to fail to compile. And
> actually you should not need it.
>

That is an update missed in my previous patch, corrected in the new patch.



>
> -                       conn->asyncStatus = PGASYNC_READY;
> +                       conn->asyncStatus = PGASYNC_READY_MORE;
>                         return;
> This should really not be changed, or it should be changed to a status
> dedicated to batching only if batch mode is activated.
>

Since pqParseInput3() reads all the input data post "Row description"
message, yes, this change is not needed here.



>
> If you are planning for integrating this patch into Postres 10, please
> be sure that this is registered in the last commit fest that will
> begin next week:
> https://commitfest.postgresql.org/13/


Yes, I have created a new patch entry into the commitfest 2017-03 and
attached the latest patch with this e-mail.



>
> I'll try to book a couple of cycles to look at it if you are able to
> register it into the CF and provide a new version.
>


Thanks again.

Regards,
Vaishnavi,
Fujitsu Australia.

  38. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-02-23T02:37:17Z

    On 22 Feb. 2017 14:14, "Vaishnavi Prabakaran" <vaishnaviprabakaran@gmail.com>
wrote:

Thanks for reviewing the patch.
>

Thanks for picking it up! I've wanted to see this process for some time,
but just haven't had the bandwidth for it.

  39. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-07T16:52:35Z

    	Vaishnavi Prabakaran wrote:

> Yes, I have created a new patch entry into the commitfest 2017-03 and
> attached the latest patch with this e-mail.

Please find attached a companion patch implementing the batch API in
pgbench, exposed as \beginbatch and \endbatch meta-commands
(without documentation).

The idea for now is to make it easier to exercise the API and test
how batching performs. I guess I'll submit the patch separately in
a future CF, depending on when/if the libpq patch goes in.

While developing this, I noted a few things with 0001-v4:

1. lack of initialization for count in PQbatchQueueCount.
Trivial fix:

--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1874,7 +1874,7 @@ PQisBusy(PGconn *conn)
 int
 PQbatchQueueCount(PGconn *conn)
 {
-	int			count;
+	int			count = 0;
	PGcommandQueueEntry *entry;

2. misleading error message in PQexecStart. It gets called by a few other
functions than PQexec, such as PQprepare. As I understand it, the intent
here is to forbid the synchronous functions in batch mode, so this error
message should not single out PQexec.

@@ -1932,6 +2425,13 @@ PQexecStart(PGconn *conn)
	if (!conn)
		return false;
 
+	if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status !=
PQBATCH_MODE_OFF)
+	{
+		printfPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("cannot
PQexec in batch mode\n"));
+		return false;
+	}
+

3. In relation to #2, PQsendQuery() is not forbidden in batch mode
although I don't think it can work with it, as it's based on the old
protocol. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  40. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-03-08T01:56:40Z

    On 8 March 2017 at 00:52, Daniel Verite <daniel@manitou-mail.org> wrote:
>         Vaishnavi Prabakaran wrote:
>
>> Yes, I have created a new patch entry into the commitfest 2017-03 and
>> attached the latest patch with this e-mail.
>
> Please find attached a companion patch implementing the batch API in
> pgbench, exposed as \beginbatch and \endbatch meta-commands
> (without documentation).
>
> The idea for now is to make it easier to exercise the API and test
> how batching performs. I guess I'll submit the patch separately in
> a future CF, depending on when/if the libpq patch goes in.

That's great, thanks, and thanks also for the fixes. Any chance you
can attach your updated patch?

I looked at modifying psql to support batching when run
non-interactively, but it would've required major restructuring of its
control loop and I ran out of time. I didn't think of modifying
pgbench. Great to see.

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

  41. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-08T02:04:24Z

    On Wed, Mar 8, 2017 at 3:52 AM, Daniel Verite <daniel@manitou-mail.org>
wrote:

>         Vaishnavi Prabakaran wrote:
>
> > Yes, I have created a new patch entry into the commitfest 2017-03 and
> > attached the latest patch with this e-mail.
>
> Please find attached a companion patch implementing the batch API in
> pgbench, exposed as \beginbatch and \endbatch meta-commands
> (without documentation).
>
> The idea for now is to make it easier to exercise the API and test
> how batching performs. I guess I'll submit the patch separately in
> a future CF, depending on when/if the libpq patch goes in.
>
>

Thanks for the companion patch and here are some comments:

1. I see, below check is used to verify if the connection is not in batch
mode:
    if (PQbatchStatus(st->con) != PQBATCH_MODE_ON)

    But, it is better to use if (PQbatchStatus(st->con) ==
PQBATCH_MODE_OFF) for this verification. Reason is there is one more state
in batch mode - PQBATCH_MODE_ABORTED. So, if the batch mode is in aborted
status, this check will still assume that the connection is not in batch
mode.

In a same way, "if(PQbatchStatus(st->con) == PQBATCH_MODE_ON)" check is
better to be modified as "PQbatchStatus(st->con) != PQBATCH_MODE_OFF".


2.  @@ -2207,7 +2227,47 @@ doCustom(TState *thread, CState *st, StatsData
*agg)
+ if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
+ {
+ commandFailed(st, "already in batch mode");
+ break;
+ }

This is not required as below PQbatchBegin() internally does this
verification.

+ PQbatchBegin(st->con);


3. It is better to check the return value of PQbatchBegin() rather than
assuming success. E.g: PQbatchBegin() will return false if the connection
is in copy in/out/both states.



> While developing this, I noted a few things with 0001-v4:
>
> 1. lack of initialization for count in PQbatchQueueCount.
> Trivial fix:
>
> --- a/src/interfaces/libpq/fe-exec.c
> +++ b/src/interfaces/libpq/fe-exec.c
> @@ -1874,7 +1874,7 @@ PQisBusy(PGconn *conn)
>  int
>  PQbatchQueueCount(PGconn *conn)
>  {
> -       int                     count;
> +       int                     count = 0;
>         PGcommandQueueEntry *entry;
>
>
Thanks for your review and yes, Corrected.


> 2. misleading error message in PQexecStart. It gets called by a few other
> functions than PQexec, such as PQprepare. As I understand it, the intent
> here is to forbid the synchronous functions in batch mode, so this error
> message should not single out PQexec.
>
> @@ -1932,6 +2425,13 @@ PQexecStart(PGconn *conn)
>         if (!conn)
>                 return false;
>
> +       if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status !=
> PQBATCH_MODE_OFF)
> +       {
> +               printfPQExpBuffer(&conn->errorMessage,
> +                                                 libpq_gettext("cannot
> PQexec in batch mode\n"));
> +               return false;
> +       }
> +
>
>

Hmm, this error message goes with the flow of other error messages in the
same function. E.g: "PQexec not allowed during COPY BOTH" . But, anyways I
modified the
error message to be more generic.



> 3. In relation to #2, PQsendQuery() is not forbidden in batch mode
> although I don't think it can work with it, as it's based on the old
> protocol.
>
>
>
It is actually forbidden and you can see the error message "cannot
PQsendQuery in batch mode, use PQsendQueryParams" when you use
PQsendQuery().
Attached the updated patch.

Thanks & Regards,
Vaishnavi
Fujitsu Australia.

  42. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-09T21:43:46Z

    	Vaishnavi Prabakaran wrote:

>    if (PQbatchStatus(st->con) != PQBATCH_MODE_ON)
> 
>    But, it is better to use if (PQbatchStatus(st->con) ==
> PQBATCH_MODE_OFF) for this verification. Reason is there is one more state
> in batch mode - PQBATCH_MODE_ABORTED. So, if the batch mode is in aborted
> status, this check will still assume that the connection is not in batch
> mode.
> In a same way, "if(PQbatchStatus(st->con) == PQBATCH_MODE_ON)" check is
> better to be modified as "PQbatchStatus(st->con) != PQBATCH_MODE_OFF".

Agreed, these two tests need to be changed to account for the
PQBATCH_MODE_ABORTED case. Fixed in new patch.

> 2.  @@ -2207,7 +2227,47 @@ doCustom(TState *thread, CState *st, StatsData
> *agg)
> + if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
> + {
> + commandFailed(st, "already in batch mode");
> + break;
> + }
> 
> This is not required as below PQbatchBegin() internally does this
> verification.
> 
> + PQbatchBegin(st->con);

The point of this test is to specifically forbid a sequence like this
\beginbatch
...(no \endbatch)
\beginbatch
...
and according to the doc for PQbatchBegin, it looks like the return
code won't tell us:
   "Causes a connection to enter batch mode if it is currently idle or
   already in batch mode.
    int PQbatchBegin(PGconn *conn);
   Returns 1 for success"

My understanding is that "already in batch mode" is not an error.

   "Returns 0 and has no effect if the connection is not currently
   idle, i.e. it has a result ready, is waiting for more input from
   the server, etc. This function does not actually send anything to
   the server, it just changes the libpq connection state"

> 3. It is better to check the return value of PQbatchBegin() rather than
> assuming success. E.g: PQbatchBegin() will return false if the connection
> is in copy in/out/both states.

Given point #2 above, I think both tests are needed:
   if (PQbatchStatus(st->con) != PQBATCH_MODE_OFF)
    {
       commandFailed(st, "already in batch mode");
       break;
    }
    if (PQbatchBegin(st->con) == 0)
    {
       commandFailed(st, "failed to start a batch");
       break;
    }

> > 3. In relation to #2, PQsendQuery() is not forbidden in batch mode
> > although I don't think it can work with it, as it's based on the old
> > protocol.
> >
> It is actually forbidden and you can see the error message "cannot
> PQsendQuery in batch mode, use PQsendQueryParams" when you use
> PQsendQuery().

Sorry for blaming the innocent piece of code, looking closer
it's pgbench that does not display the message.
With the simple query protocol, sendCommand() does essentially:

  r = PQsendQuery(st->con, sql);

... other stuff...

  if (r == 0)
  {
    if (debug)
      fprintf(stderr, "client %d could not send %s\n",
	  st->id, command->argv[0]);
    st->ecnt++;
    return false;
  }

So only in debug mode does it inform the user that it failed, and
even then, it does not display PQerrorMessage(st->con).

In non-debug mode it's opaque to the user. If it always fail, it appears
to loop forever. The caller has this comment that is relevant to the problem:

	if (!sendCommand(st, command))
	  {
	    /*
	     * Failed. Stay in CSTATE_START_COMMAND state, to
	     * retry. ??? What the point or retrying? Should
	     * rather abort?
	     */
	    return;
	  }

I think it doesn't bother anyone up to now because the immediate
failure modes of PQsendQuery() are resource allocation or protocol
failures that tend to never happen.

Anyway currently \beginbatch avoids the problem by checking
whether querymode == QUERY_SIMPLE, to fail gracefully
instead of letting the endless loop happen.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  43. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-10T13:52:05Z

      Hi,

I notice that PQsetSingleRowMode() doesn't work when getting batch results.  

The function is documented as:
" int PQsetSingleRowMode(PGconn *conn);

  This function can only be called immediately after PQsendQuery or one
  of its sibling functions, before any other operation on the connection
  such as PQconsumeInput or PQgetResult"

But PQbatchQueueProcess() unconditionally clears conn->singleRowMode,
so whatever it was when sending the query gets lost, and besides
other queries might have been submitted in the meantime.

Also if trying to set that mode when fetching like this

     while (QbatchQueueProcess(conn)) {
       r = PQsetSingleRowMode(conn);
       if (r!=1) {
	  fprintf(stderr, "PQsetSingleRowMode() failed");
       }
       .. 

it might work the first time, but on the next iterations, conn->asyncStatus
might be PGASYNC_READY, which is a failure condition for
PQsetSingleRowMode(), so that won't do.

ISTM that the simplest fix would be that when in batch mode,
PQsetSingleRowMode() should register that the last submitted query
does request that mode, and when later QbatchQueueProcess dequeues
the batch entry for the corresponding query, this flag would be popped off
and set as the current mode.

Please find attached the suggested fix, against the v5 of the patch.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  44. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-13T00:54:02Z

    On Sat, Mar 11, 2017 at 12:52 AM, Daniel Verite <daniel@manitou-mail.org>
wrote:

>   Hi,
>
> I notice that PQsetSingleRowMode() doesn't work when getting batch results.
>
> The function is documented as:
> " int PQsetSingleRowMode(PGconn *conn);
>
>   This function can only be called immediately after PQsendQuery or one
>   of its sibling functions, before any other operation on the connection
>   such as PQconsumeInput or PQgetResult"
>
> But PQbatchQueueProcess() unconditionally clears conn->singleRowMode,
> so whatever it was when sending the query gets lost, and besides
> other queries might have been submitted in the meantime.
>
> Also if trying to set that mode when fetching like this
>
>      while (QbatchQueueProcess(conn)) {
>        r = PQsetSingleRowMode(conn);
>        if (r!=1) {
>           fprintf(stderr, "PQsetSingleRowMode() failed");
>        }
>        ..
>
> it might work the first time, but on the next iterations, conn->asyncStatus
> might be PGASYNC_READY, which is a failure condition for
> PQsetSingleRowMode(), so that won't do.
>


Thanks for investigating the problem, and could you kindly explain what
"next iteration" you mean here? Because I don't see any problem in
following sequence of calls - PQbatchQueueProcess(),PQsetSingleRowMode()
, PQgetResult(). Am I missing anything?
Please note that it is MUST to call PQgetResult immediately after
PQbatchQueueProcess() as documented.



>
> ISTM that the simplest fix would be that when in batch mode,
> PQsetSingleRowMode() should register that the last submitted query
> does request that mode, and when later QbatchQueueProcess dequeues
> the batch entry for the corresponding query, this flag would be popped off
> and set as the current mode.
>
> Please find attached the suggested fix, against the v5 of the patch.
>

Before going with this fix, I would like you to consider the option of
asking batch processing users(via documentation) to set single-row mode
before calling PQgetResult().
Either way we need to fix the documentation part, letting users know how
they can activate single-row mode while using batch processing.
Let me know your thoughts.

Best Regards,
Vaishnavi,
Fujitsu Australia.

  45. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-03-13T01:32:47Z

    On 13 March 2017 at 08:54, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:

> Before going with this fix, I would like you to consider the option of
> asking batch processing users(via documentation) to set single-row mode
> before calling PQgetResult().
> Either way we need to fix the documentation part, letting users know how
> they can activate single-row mode while using batch processing.
> Let me know your thoughts.

Thanks for looking into this, it's clear that I didn't cover the combo
of single-row-mode and batch mode adequately.


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

  46. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-13T17:19:39Z

    	Vaishnavi Prabakaran wrote:

> >      while (QbatchQueueProcess(conn)) {
> >        r = PQsetSingleRowMode(conn);
> >        if (r!=1) {
> >           fprintf(stderr, "PQsetSingleRowMode() failed");
> >        }
> >        ..

> Thanks for investigating the problem, and could you kindly explain what
> "next iteration" you mean here? Because I don't see any problem in
> following sequence of calls - PQbatchQueueProcess(),PQsetSingleRowMode()
> , PQgetResult()

I mean the next iteration of the above while statement. Referring
to the doc, that would be the "next batch entry":

  " To get the result of the first batch entry the client must call
   PQbatchQueueProcess. It must then call PQgetResult and handle the
   results until PQgetResult returns null (or would return null if
   called). The result from the next batch entry may then be retrieved
   using PQbatchQueueProcess and the cycle repeated"

Attached is a bare-bones testcase showing the problem.
As it is, it works, retrieving results for three "SELECT 1"
in the same batch.  Now in order to use the single-row
fetch mode, consider adding this:

    r = PQsetSingleRowMode(conn);
    if (r!=1) {
      fprintf(stderr, "PQsetSingleRowMode() failed for i=%d\n", i);
    }

When inserted after the call to PQbatchQueueProcess,
which is what I understand you're saying works for you,
it fails for me when starting to get the results of the 2nd query
and after. 


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  47. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-14T06:50:19Z

    On Tue, Mar 14, 2017 at 4:19 AM, Daniel Verite <daniel@manitou-mail.org>
wrote:

>
> I mean the next iteration of the above while statement. Referring
> to the doc, that would be the "next batch entry":
>
>   " To get the result of the first batch entry the client must call
>    PQbatchQueueProcess. It must then call PQgetResult and handle the
>    results until PQgetResult returns null (or would return null if
>    called). The result from the next batch entry may then be retrieved
>    using PQbatchQueueProcess and the cycle repeated"
>
> Attached is a bare-bones testcase showing the problem.
> As it is, it works, retrieving results for three "SELECT 1"
> in the same batch.  Now in order to use the single-row
> fetch mode, consider adding this:
>
>     r = PQsetSingleRowMode(conn);
>     if (r!=1) {
>       fprintf(stderr, "PQsetSingleRowMode() failed for i=%d\n", i);
>     }
>
> When inserted after the call to PQbatchQueueProcess,
> which is what I understand you're saying works for you,
> it fails for me when starting to get the results of the 2nd query
> and after.
>
>

Thanks for explaining the issue in detail and yes, I do see the issue using
your attached test file.

And I think the problem is with the "parseInput" call at the end of
PQbatchQueueProcess().
I don't see the need for "parseInput" to cover the scope of
PQbatchQueueProcess(), also anyways we do it in PQgetResult().
This function call changes the asyncstatus of connection to READY(following
command complete message from backend), so eventually PQsetSingleRowMode()
is failing. So, attached the alternative fix for this issue.
Please share me your thoughts.

I would also like to hear Craig's opinion on it before applying this fix to
the original patch, just to make sure am not missing anything here.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  48. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-16T02:42:33Z

    On Tue, Mar 14, 2017 at 5:50 PM, Vaishnavi Prabakaran <
vaishnaviprabakaran@gmail.com> wrote:

>
>
> On Tue, Mar 14, 2017 at 4:19 AM, Daniel Verite <daniel@manitou-mail.org>
> wrote:
>
>>
>> I mean the next iteration of the above while statement. Referring
>> to the doc, that would be the "next batch entry":
>>
>>   " To get the result of the first batch entry the client must call
>>    PQbatchQueueProcess. It must then call PQgetResult and handle the
>>    results until PQgetResult returns null (or would return null if
>>    called). The result from the next batch entry may then be retrieved
>>    using PQbatchQueueProcess and the cycle repeated"
>>
>> Attached is a bare-bones testcase showing the problem.
>> As it is, it works, retrieving results for three "SELECT 1"
>> in the same batch.  Now in order to use the single-row
>> fetch mode, consider adding this:
>>
>>     r = PQsetSingleRowMode(conn);
>>     if (r!=1) {
>>       fprintf(stderr, "PQsetSingleRowMode() failed for i=%d\n", i);
>>     }
>>
>> When inserted after the call to PQbatchQueueProcess,
>> which is what I understand you're saying works for you,
>> it fails for me when starting to get the results of the 2nd query
>> and after.
>>
>>
>
> Thanks for explaining the issue in detail and yes, I do see the issue
> using your attached test file.
>
> And I think the problem is with the "parseInput" call at the end of
> PQbatchQueueProcess().
> I don't see the need for "parseInput" to cover the scope of
> PQbatchQueueProcess(), also anyways we do it in PQgetResult().
> This function call changes the asyncstatus of connection to
> READY(following command complete message from backend), so eventually
> PQsetSingleRowMode() is failing. So, attached the alternative fix for this
> issue.
>
Please share me your thoughts.
>
> I would also like to hear Craig's opinion on it before applying this fix
> to the original patch, just to make sure am not missing anything here.
>
>
Attached the code patch applied with the fix explained above and moving the
CF status seeking review.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  49. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-16T13:37:03Z

    	Vaishnavi Prabakaran wrote:

> So, attached the alternative fix for this issue.
> Please share me your thoughts.

I assume you prefer the alternative fix because it's simpler.

> I would also like to hear Craig's opinion on it before applying this fix
> to the original patch, just to make sure am not missing anything here.

+1

The main question is whether the predicates enforced
by PQsetSingleRowMode() apply in batch mode in all cases
when it's legit to call that function. Two predicates
that may be problematic are:
	if (conn->asyncStatus != PGASYNC_BUSY)
		return 0;
and
	if (conn->result)
		return 0;

The general case with batch mode is that, from the doc:
"The client interleaves result processing with sending batch queries"
Note that I've not even tested that here, I've tested
batching a bunch of queries in a first step and getting the results
in a second step.
I am not confident that the above predicates will be true
in all cases. Also your alternative fix assumes that we add
a user-visible exception to PQsetSingleRowMode in batch mode,
whereby it must not be called as currently documented:
  "call PQsetSingleRowMode immediately after a successful call of 
   PQsendQuery (or a sibling function)"
My gut feeling is that it's not the right direction, I prefer making
the single-row a per-query attribute internally and keep
PQsetSingleRowMode's contract unchanged from the
user's perspective.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  50. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-20T01:32:15Z

    On Fri, Mar 17, 2017 at 12:37 AM, Daniel Verite <daniel@manitou-mail.org>
wrote:

>         Vaishnavi Prabakaran wrote:
>
> > So, attached the alternative fix for this issue.
> > Please share me your thoughts.
>
> I assume you prefer the alternative fix because it's simpler.
>

I would like add one more reason for this fix, I think "PQsetSingleRowMode"
should be called only when the result is ready to be processed and before
starting to consume result as it is documented currently as follows -
"To enter single-row mode, call PQsetSingleRowMode immediately after a
successful call of PQsendQuery (or a sibling function). This mode selection
is effective only for the currently executing query. Then call PQgetResult
repeatedly..."


I agree that first fix (you shared) will allow user to set single-row mode
after PQsendQuery, but it also allows user to set this mode at any time of
batch processing(not necessarily "immediately after PQsendQuery"), also
"mode selection is effective only for the currently executing query" will
be false. Please note that I don't see any problem with this deviation. I
like to outline that documentation here anyways needs an update/note.


 Before going further, I would like to mention that I have modified the
documentation of batch processing( in v6 code patch) as below:
"To enter single-row mode, call PQsetSingleRowMode immediately after a
successful call of PQbatchQueueProcess. This mode selection is effective
only for the currently executing query. For more information on the
use of PQsetSingleRowMode
, refer to Section 33.6, “Retrieving Query Results Row-By-Row”. "

Please let me know if you think this is not enough but wanted to update
section 33.6 also?



>
> > I would also like to hear Craig's opinion on it before applying this fix
> > to the original patch, just to make sure am not missing anything here.
>
> +1
>
> The main question is whether the predicates enforced
> by PQsetSingleRowMode() apply in batch mode in all cases
> when it's legit to call that function. Two predicates
> that may be problematic are:
>         if (conn->asyncStatus != PGASYNC_BUSY)
>                 return 0;
> and
>         if (conn->result)
>                 return 0;
>
> The general case with batch mode is that, from the doc:
> "The client interleaves result processing with sending batch queries"
>

While sending batch queries in middle of result processing, only the query
is appended to the list of queries maintained for batch processing and no
current connection attribute impacting result processing will be changed.
So, calling the PQsetSingleRowMode in-between result processing will fail
as it tries to set single-row mode for currently executing query for which
result processing is already started.


Note that I've not even tested that here,

I've tested
> batching a bunch of queries in a first step and getting the results
> in a second step.
> I am not confident that the above predicates will be true
> in all cases.

Also your alternative fix assumes that we add
> a user-visible exception to PQsetSingleRowMode in batch mode,
> whereby it must not be called as currently documented:
>   "call PQsetSingleRowMode immediately after a successful call of
>    PQsendQuery (or a sibling function)"

My gut feeling is that it's not the right direction, I prefer making
> the single-row a per-query attribute internally and keep
> PQsetSingleRowMode's contract unchanged from the
> user's perspective.
>
>
Am going to include the test which you shared in the test patch. Please let
me know if you want to cover anymore specific cases to gain confidence.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  51. Re: PATCH: Batch/pipelining support for libpq

    David Steele <david@pgmasters.net> — 2017-03-24T15:21:09Z

    Hi Vaishnavi,

On 3/19/17 9:32 PM, Vaishnavi Prabakaran wrote:
> On Fri, Mar 17, 2017 at 12:37 AM, Daniel Verite <daniel@manitou-mail.org
>
> Please let me know if you think this is not enough but wanted to update
> section 33.6 also?

Daniel, any input here?

>     > I would also like to hear Craig's opinion on it before applying this fix
>     > to the original patch, just to make sure am not missing anything here.

Craig?

>
> Am going to include the test which you shared in the test patch. Please
> let me know if you want to cover anymore specific cases to gain
> confidence.

I have marked this submission "Waiting for Author" since it appears a 
new patch is required based on input and adding a new test.

-- 
-David
david@pgmasters.net

  52. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-03-25T12:50:47Z

    On 24 March 2017 at 23:21, David Steele <david@pgmasters.net> wrote:
> Hi Vaishnavi,
>
> On 3/19/17 9:32 PM, Vaishnavi Prabakaran wrote:
>>
>> On Fri, Mar 17, 2017 at 12:37 AM, Daniel Verite <daniel@manitou-mail.org
>>
>> Please let me know if you think this is not enough but wanted to update
>> section 33.6 also?
>
>
> Daniel, any input here?
>
>>     > I would also like to hear Craig's opinion on it before applying this
>> fix
>>     > to the original patch, just to make sure am not missing anything
>> here.
>
>
> Craig?

I'm fairly confident that I overlooked single row mode entirely in the
original patch, though it's long enough ago that it's hard for me to
remember exactly.

I don't really have much of an opinion on the best handling of it.

I would expect to be setting single-row mode just before I requested
the *result* from the next pending query, since it relates to result
processing rather than query dispatch. But that's about all the
opinion I have here.

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

  53. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-27T07:26:12Z

    On Sat, Mar 25, 2017 at 9:50 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> I'm fairly confident that I overlooked single row mode entirely in the
> original patch, though it's long enough ago that it's hard for me to
> remember exactly.
>
> I don't really have much of an opinion on the best handling of it.
>
> I would expect to be setting single-row mode just before I requested
> the *result* from the next pending query, since it relates to result
> processing rather than query dispatch. But that's about all the
> opinion I have here.

Yeah, I think that it makes sense to allow users to switch to single
row mode before requesting a result in the queue. It seems to me that
this should also be effective only during the fetching of one single
result set. When the client moves on to the next item in the queue we
should make necessary again a call to PQsetSingleRowMode().

Regarding the patch, I have spotted the following things in the last version:
- src/test/modules/Makefile should include test_libpq.
- Regression tests from 0002 are failing:
ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
not ok 6 - testlibpqbatch copyfailure
-- 
Michael

  54. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-03-27T07:42:08Z

    On 27 March 2017 at 15:26, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Sat, Mar 25, 2017 at 9:50 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> I'm fairly confident that I overlooked single row mode entirely in the
>> original patch, though it's long enough ago that it's hard for me to
>> remember exactly.
>>
>> I don't really have much of an opinion on the best handling of it.
>>
>> I would expect to be setting single-row mode just before I requested
>> the *result* from the next pending query, since it relates to result
>> processing rather than query dispatch. But that's about all the
>> opinion I have here.
>
> Yeah, I think that it makes sense to allow users to switch to single
> row mode before requesting a result in the queue. It seems to me that
> this should also be effective only during the fetching of one single
> result set. When the client moves on to the next item in the queue we
> should make necessary again a call to PQsetSingleRowMode().
>
> Regarding the patch, I have spotted the following things in the last version:
> - src/test/modules/Makefile should include test_libpq.
> - Regression tests from 0002 are failing:
> ok 1 - testlibpqbatch disallowed_in_batch
> ok 2 - testlibpqbatch simple_batch
> ok 3 - testlibpqbatch multi_batch
> ok 4 - testlibpqbatch batch_abort
> ok 5 - testlibpqbatch timings
> not ok 6 - testlibpqbatch copyfailure

There are only a few more days left of this commit fest.

Things are sounding pretty ready here, with some final cleanups
pending. It'd be cool to get this into Pg 10 :)

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

  55. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-27T08:18:19Z

    On Mon, Mar 27, 2017 at 4:42 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 27 March 2017 at 15:26, Michael Paquier <michael.paquier@gmail.com> wrote:
>> On Sat, Mar 25, 2017 at 9:50 PM, Craig Ringer <craig@2ndquadrant.com> wrote:
>>> I'm fairly confident that I overlooked single row mode entirely in the
>>> original patch, though it's long enough ago that it's hard for me to
>>> remember exactly.
>>>
>>> I don't really have much of an opinion on the best handling of it.
>>>
>>> I would expect to be setting single-row mode just before I requested
>>> the *result* from the next pending query, since it relates to result
>>> processing rather than query dispatch. But that's about all the
>>> opinion I have here.
>>
>> Yeah, I think that it makes sense to allow users to switch to single
>> row mode before requesting a result in the queue. It seems to me that
>> this should also be effective only during the fetching of one single
>> result set. When the client moves on to the next item in the queue we
>> should make necessary again a call to PQsetSingleRowMode().
>>
>> Regarding the patch, I have spotted the following things in the last version:
>> - src/test/modules/Makefile should include test_libpq.
>> - Regression tests from 0002 are failing:
>> ok 1 - testlibpqbatch disallowed_in_batch
>> ok 2 - testlibpqbatch simple_batch
>> ok 3 - testlibpqbatch multi_batch
>> ok 4 - testlibpqbatch batch_abort
>> ok 5 - testlibpqbatch timings
>> not ok 6 - testlibpqbatch copyfailure
>
> There are only a few more days left of this commit fest.
>
> Things are sounding pretty ready here, with some final cleanups
> pending. It'd be cool to get this into Pg 10 :)

Yes, that's one of the items I'd like to help with by the end of the CF.
-- 
Michael

  56. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-27T13:28:51Z

    	Vaishnavi Prabakaran wrote:

> Please let me know if you think this is not enough but wanted to update
> section 33.6 also?

Yes, if the right place to call PQsetSingleRowMode() is immediately
after PQbatchQueueProcess(), I think we need to update
"33.6. Retrieving Query Results Row-By-Row"
with that information, otherwise what it says is just wrong
when in batch mode.

Also, in 33.5, I suggest to not use the "currently executing
query" as a synonym for the "query currently being processed"
to avoid any confusion between what the backend is executing
and a prior query whose results are being collected by the client
at the same moment.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  57. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-27T15:27:27Z

    	Vaishnavi Prabakaran wrote:

> Am going to include the test which you shared in the test patch. Please let
> me know if you want to cover anymore specific cases to gain confidence.

One bit of functionality that does not work in batch mode and is left
as is by the patch is the PQfn() fast path interface and the large object
functions that use it.

Currently, calling lo_* functions inside a batch will fail with a message
that depends on whether the internal lo_initialize() has been successfully
called before.

If it hasn't, PQerrorMessage() will be:
   "Synchronous command execution functions are not allowed in batch mode"
which is fine, but it comes by happenstance from lo_initialize()
calling PQexec() to fetch the function OIDs from pg_catalog.pg_proc.

OTOH, if lo_initialize() has succeeded before, a call to a large
object function will fail with a different message:
  "connection in wrong state"
which is emitted by PQfn() based on conn->asyncStatus != PGASYNC_IDLE

Having an unified error message would be more user friendly.

Concerning the doc, when saying in 33.5.2:
 "In batch mode only asynchronous operations are permitted"
the server-side functions are indeed ruled out, since PQfn() is
synchronous, but maybe we should be a bit more explicit
about that?

Chapter 34.3 (Large Objects / Client Interfaces) could also
mention the incompatibility with batch mode.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  58. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-28T04:57:28Z

    Thanks Craig and Michael for confirming that "PQsetSingleRowMode" should be
called right after "PQbatchQueueProcess".

Michael Paquier wrote:

> It seems to me that
>this should also be effective only during the fetching of one single
>result set. When the client moves on to the next item in the queue we
>should make necessary again a call to PQsetSingleRowMode().

Yes, the current behavior(with V6 code patch) is exactly the same as you
described above. PQsetSingleRowMode() should be called each time after
"PQbatchQueueProcess"
to set result processing to single-row mode.

>src/test/modules/Makefile should include test_libpq.

Yes, added test_libpq to the list in Makefile.

Daniel Verite wrote:

>> Please let me know if you think this is not enough but wanted to update
>> section 33.6 also?

>Yes, if the right place to call PQsetSingleRowMode() is immediately
>after PQbatchQueueProcess(), I think we need to update
>"33.6. Retrieving Query Results Row-By-Row"
>with that information, otherwise what it says is just wrong
>when in batch mode.

Yes, I have updated Chapter 33.6 by adding note for batch mode.

>Also, in 33.5, I suggest to not use the "currently executing
>query" as a synonym for the "query currently being processed"
>to avoid any confusion between what the backend is executing
>and a prior query whose results are being collected by the client
>at the same moment.

Yes correct, I modified the words to "query currently being processed" as
suggested.



> One bit of functionality that does not work in batch mode and is left
> as is by the patch is the PQfn() fast path interface and the large object
> functions that use it.
>
> Currently, calling lo_* functions inside a batch will fail with a message
> that depends on whether the internal lo_initialize() has been successfully
> called before.
>
> If it hasn't, PQerrorMessage() will be:
>    "Synchronous command execution functions are not allowed in batch mode"
> which is fine, but it comes by happenstance from lo_initialize()
> calling PQexec() to fetch the function OIDs from pg_catalog.pg_proc.
>
> OTOH, if lo_initialize() has succeeded before, a call to a large
> object function will fail with a different message:
>   "connection in wrong state"
> which is emitted by PQfn() based on conn->asyncStatus != PGASYNC_IDLE
>
> Having an unified error message would be more user friendly.
>
>
Thanks for finding out this and yes, added a new check in PQfn() to give
the same error message - "Synchronous command execution functions are not
allowed in batch mode" when called in batch mode.



> Concerning the doc, when saying in 33.5.2:
>  "In batch mode only asynchronous operations are permitted"
> the server-side functions are indeed ruled out, since PQfn() is
> synchronous, but maybe we should be a bit more explicit
> about that?


> Chapter 34.3 (Large Objects / Client Interfaces) could also
> mention the incompatibility with batch mode.
>
>
Updated 33.5.2 to be more clear about what functions are allowed and what
are not allowed. Updated Chapter 33.3(Large Objects/ Client Interfaces) to
let the user know about the incompatibility with batch mode .


Attached the latest patch and here is the RT run result:
ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
ok 6 - testlibpqbatch copyfailure
ok 7 - testlibpqbatch test_singlerowmode
ok
All tests successful.
Files=1, Tests=7,  5 wallclock secs ( 0.01 usr  0.00 sys +  1.79 cusr  0.35
csys =  2.15 CPU)
Result: PASS

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  59. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-28T08:10:15Z

    On Tue, Mar 28, 2017 at 1:57 PM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
> Thanks Craig and Michael for confirming that "PQsetSingleRowMode" should be
> called right after "PQbatchQueueProcess".
>
> Michael Paquier wrote:
>
>> It seems to me that
>>this should also be effective only during the fetching of one single
>>result set. When the client moves on to the next item in the queue we
>>should make necessary again a call to PQsetSingleRowMode().
>
> Yes, the current behavior(with V6 code patch) is exactly the same as you
> described above. PQsetSingleRowMode() should be called each time after
> "PQbatchQueueProcess" to set result processing to single-row mode.

Okay, that's fine for me then.

> Attached the latest patch and here is the RT run result:
> ok 1 - testlibpqbatch disallowed_in_batch
> ok 2 - testlibpqbatch simple_batch
> ok 3 - testlibpqbatch multi_batch
> ok 4 - testlibpqbatch batch_abort
> ok 5 - testlibpqbatch timings
> ok 6 - testlibpqbatch copyfailure
> ok 7 - testlibpqbatch test_singlerowmode
> ok
> All tests successful.
> Files=1, Tests=7,  5 wallclock secs ( 0.01 usr  0.00 sys +  1.79 cusr  0.35
> csys =  2.15 CPU)
> Result: PASS

ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
not ok 6 - testlibpqbatch copyfailure
Hm... Not here. I saw something like that a couple of days ago on my
macos laptop and that was related to a variable not initialized. From
001_libpq_async_main.log:
7-03-28 17:05:49.159 JST [31553] t/001_libpq_async.pl LOG:  execute
<unnamed>: copy batch_demo(id) to stdout;
2017-03-28 17:05:49.159 JST [31553] t/001_libpq_async.pl LOG:  execute
<unnamed>: copy batch_demo(itemno) FROM stdin;
2017-03-28 17:05:49.160 JST [31553] t/001_libpq_async.pl ERROR:
unexpected message type 0x50 during COPY from stdin
2017-03-28 17:05:49.160 JST [31553] t/001_libpq_async.pl CONTEXT:
COPY batch_demo, line 1

From regress_log_001_libpq_async :
ok 5 - testlibpqbatch timings
# Running: testlibpqbatch dbname=postgres 10000 copyfailure
dispatching SELECT query failed: cannot queue commands during COPY

COPYBUF: 5

Error status and message got from server due to COPY command failure
are : PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during
COPY from stdin
CONTEXT:  COPY batch_demo, line 1

So it seems to me that you are still missing something..

 src/test/modules/Makefile                        |    1 +
 src/test/modules/test_libpq/.gitignore           |    5 +
 src/test/modules/test_libpq/Makefile             |   25 +
 src/test/modules/test_libpq/README               |    1 +
 src/test/modules/test_libpq/t/001_libpq_async.pl |   26 +
 src/test/modules/test_libpq/testlibpqbatch.c     | 1661 ++++++++++++++++++++++
 6 files changed, 1719 insertions(+)
Could you as well update src/tools/msvc/vcregress.pl, aka the routine
modulescheck so as this new test is skipped. I am sure that nobody
will scream if this test is not run on Windows, but the buildfarm will
complain if the patch is let in its current state.
-- 
Michael

  60. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-28T13:28:52Z

    	Michael Paquier wrote:

> # Running: testlibpqbatch dbname=postgres 10000 copyfailure
> dispatching SELECT query failed: cannot queue commands during COPY
> 
> COPYBUF: 5
> 
> Error status and message got from server due to COPY command failure
> are : PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during
> COPY from stdin
> CONTEXT:  COPY batch_demo, line 1

Same result here.

BTW the doc says: 
  "In batch mode only asynchronous operations are permitted, and COPY is
   not recommended as it most likely will trigger failure in batch
processing"
Yet it seems that the test assumes that it should work nonetheless.
I don't quite understand what we expect of this test, given what's
documented. Or what am I missing?

While looking at the regress log, I noticed multiple spurious
test_singlerowmode tests among the others. Should be fixed with:

--- a/src/test/modules/test_libpq/testlibpqbatch.c
+++ b/src/test/modules/test_libpq/testlibpqbatch.c
@@ -1578,6 +1578,7 @@ main(int argc, char **argv)
			run_batch_abort = 0;
			run_timings = 0;
			run_copyfailure = 0;
+			run_singlerowmode = 0;
			if (strcmp(argv[3], "disallowed_in_batch") == 0)
				run_disallowed_in_batch = 1;
			else if (strcmp(argv[3], "simple_batch") == 0)

There's also the fact that this test doesn't care whether it fails 
(compare with all the "goto fail" of the other tests).

And it happens that PQsetSingleRowMode() fails after the call to
PQbatchQueueProcess() that instantiates a PGresult
of status PGRES_BATCH_END to indicate the end of the batch,

To me this shows how this way of setting the single row mode is not ideal.
In order to avoid the failure, the loop should know in advance 
what kind of result it's going to dequeue before calling PQgetResult(),
which doesn't feel right.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  61. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-29T03:40:17Z

    Michael Paquier wrote:

>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
>modulescheck so as this new test is skipped. I am sure that nobody
>will scream if this test is not run on Windows, but the buildfarm will
>complain if the patch is let in its current state.

Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
"subdircheck" will fail for this module(because it does not have
"sql"/"expected" folders in it) and hence it will be skipped.
But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
anyways will not be run in Windows and also because it uses linux specific
APIs(gettimeofday , timersub) .

On Wed, Mar 29, 2017 at 12:28 AM, Daniel Verite <daniel@manitou-mail.org>
wrote:

>         Michael Paquier wrote:
>
> > # Running: testlibpqbatch dbname=postgres 10000 copyfailure
> > dispatching SELECT query failed: cannot queue commands during COPY
> >
> > COPYBUF: 5
> >
> > Error status and message got from server due to COPY command failure
> > are : PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during
> > COPY from stdin
> > CONTEXT:  COPY batch_demo, line 1
>
> Same result here.
>
> BTW the doc says:
>   "In batch mode only asynchronous operations are permitted, and COPY is
>    not recommended as it most likely will trigger failure in batch
> processing"
> Yet it seems that the test assumes that it should work nonetheless.
> I don't quite understand what we expect of this test, given what's
> documented. Or what am I missing?
>


copyfailure test is basically tests the error scenarios arising in batch
mode on using copy command. And, test expects error(not that it should
work) and the test is made to pass if expected error message is received.
So, it is error case testing behaves like:
expects error -> receives error = test pass
expects error -> no error = test fail

There are 2 cases tested here:

1. Example for the case - Using COPY command in batch mode will trigger
failure. (Specified in documentation)
Failure can be seen only when processing the copy command's results. That
is, after dispatching COPY command, server goes into COPY state , but the
client dispatched another query in batch mode.

Below error messages belongs to this case :
Error status and message got from server due to COPY command failure are :
PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during COPY from
stdin
CONTEXT:  COPY batch_demo, line 1

message type 0x5a arrived from server while idle


2. When connection is is copy state, that is, while processing copy
command's result, Cannot queue any query in batch mode.

Below error message belongs to this case:
dispatching SELECT query failed: cannot queue commands during COPY

I added this test following Michael's review comment - "It may be a good
idea to add a test for COPY and trigger a failure."
Hope this clarifies the presence of error message in log file and why the
test is made to pass.



>
> While looking at the regress log, I noticed multiple spurious
> test_singlerowmode tests among the others. Should be fixed with:
>
> --- a/src/test/modules/test_libpq/testlibpqbatch.c
> +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> @@ -1578,6 +1578,7 @@ main(int argc, char **argv)
>                         run_batch_abort = 0;
>                         run_timings = 0;
>                         run_copyfailure = 0;
> +                       run_singlerowmode = 0;
>                         if (strcmp(argv[3], "disallowed_in_batch") == 0)
>                                 run_disallowed_in_batch = 1;
>                         else if (strcmp(argv[3], "simple_batch") == 0)
>


Thanks for finding it out and yes corrected.



> There's also the fact that this test doesn't care whether it fails
> (compare with all the "goto fail" of the other tests).
>


Modified the test to check for failure condition.


>
> And it happens that PQsetSingleRowMode() fails after the call to
> PQbatchQueueProcess() that instantiates a PGresult
> of status PGRES_BATCH_END to indicate the end of the batch,
>
> To me this shows how this way of setting the single row mode is not ideal.
> In order to avoid the failure, the loop should know in advance
> what kind of result it's going to dequeue before calling PQgetResult(),
> which doesn't feel right.
>
>
Yes, it is one of the requirement that the batch mode application should
know the order of queries it sent and the expected results. (Specified in
"Processing results" section of batch mode documentation)

Attached the latest code and test patches.

Thanks & Regards,
Vaishnavi
Fujitusu Australia.

  62. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-03-30T01:08:15Z

    On Wed, Mar 29, 2017 at 12:40 PM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
> Michael Paquier wrote:
>>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
>>modulescheck so as this new test is skipped. I am sure that nobody
>>will scream if this test is not run on Windows, but the buildfarm will
>>complain if the patch is let in its current state.
>
> Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
> "subdircheck" will fail for this module(because it does not have
> "sql"/"expected" folders in it) and hence it will be skipped.
> But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
> anyways will not be run in Windows and also because it uses linux specific
> APIs(gettimeofday , timersub).

src/port/gettimeofday.c extends things on Windows, and we could
consider to just drop the timing portion for simplicity (except
Windows I am not seeing any platform missing timersub). But that's
just a point of detail. Or we could just drop those tests from the
final patch, and re-implement them after having some psql-level
subcommands. That would far better for portability.

> There are 2 cases tested here:
>
> 1. Example for the case - Using COPY command in batch mode will trigger
> failure. (Specified in documentation)
> Failure can be seen only when processing the copy command's results. That
> is, after dispatching COPY command, server goes into COPY state , but the
> client dispatched another query in batch mode.
>
> Below error messages belongs to this case :
> Error status and message got from server due to COPY command failure are :
> PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during COPY from
> stdin
> CONTEXT:  COPY batch_demo, line 1
>
> message type 0x5a arrived from server while idle

Such messages are really confusing to the user as they refer to the
protocol going out of sync. I would think that something like "cannot
process COPY results during a batch processing" would be cleaner.
Isn't some more error handling necessary in GetCopyStart()?

> Attached the latest code and test patches.

For me the test is still broken:
ok 1 - testlibpqbatch disallowed_in_batch
ok 2 - testlibpqbatch simple_batch
ok 3 - testlibpqbatch multi_batch
ok 4 - testlibpqbatch batch_abort
ok 5 - testlibpqbatch timings
not ok 6 - testlibpqbatch copyfailure

Still broken here. I can see that this patch is having enough
safeguards in PQbatchBegin() and PQsendQueryStart(), but this issue is
really pointing out at something...
-- 
Michael

  63. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-03-30T05:02:02Z

    On Thu, Mar 30, 2017 at 12:08 PM, Michael Paquier <michael.paquier@gmail.com
> wrote:

> On Wed, Mar 29, 2017 at 12:40 PM, Vaishnavi Prabakaran
> <vaishnaviprabakaran@gmail.com> wrote:
> > Michael Paquier wrote:
> >>Could you as well update src/tools/msvc/vcregress.pl, aka the routine
> >>modulescheck so as this new test is skipped. I am sure that nobody
> >>will scream if this test is not run on Windows, but the buildfarm will
> >>complain if the patch is let in its current state.
> >
> > Thanks for the finding. Hmm, modulescheck will not pick up test_libpq as
> > "subdircheck" will fail for this module(because it does not have
> > "sql"/"expected" folders in it) and hence it will be skipped.
> > But, Modified "Mkvcbuild.pm" to exclude "test_libpq" module, as this test
> > anyways will not be run in Windows and also because it uses linux
> specific
> > APIs(gettimeofday , timersub).
>
> src/port/gettimeofday.c extends things on Windows, and we could
> consider to just drop the timing portion for simplicity (except
> Windows I am not seeing any platform missing timersub). But that's
> just a point of detail. Or we could just drop those tests from the
> final patch, and re-implement them after having some psql-level
> subcommands. That would far better for portability.
>

Yes agree, having tests with psql meta commands will be more easy to
understand also.  And, that is one of the reason I separated the patch into
two - code and test . So, yes, we can drop the test patch for now.


>
> > There are 2 cases tested here:
> >
> > 1. Example for the case - Using COPY command in batch mode will trigger
> > failure. (Specified in documentation)
> > Failure can be seen only when processing the copy command's results. That
> > is, after dispatching COPY command, server goes into COPY state , but the
> > client dispatched another query in batch mode.
> >
> > Below error messages belongs to this case :
> > Error status and message got from server due to COPY command failure are
> :
> > PGRES_FATAL_ERROR ERROR:  unexpected message type 0x50 during COPY from
> > stdin
> > CONTEXT:  COPY batch_demo, line 1
> >
> > message type 0x5a arrived from server while idle
>
> Such messages are really confusing to the user as they refer to the
> protocol going out of sync. I would think that something like "cannot
> process COPY results during a batch processing" would be cleaner.
> Isn't some more error handling necessary in GetCopyStart()?


Hmm, With batch mode, after sending COPY command to server(and server
started processing the query and goes into COPY state) , client does not
immediately read the result , but it keeps sending other queries to the
server. By this time, server already encountered the error
scenario(Receiving different message during COPY state) and sent error
messages. So, when client starts reading the result, already error messages
sent by server are present in socket.  I think trying to handle during
result processing is more like masking the server's error message with new
error message and I think it is not appropriate.



>
>
> Attached the latest code and test patches.
>
> For me the test is still broken:
> ok 1 - testlibpqbatch disallowed_in_batch
> ok 2 - testlibpqbatch simple_batch
> ok 3 - testlibpqbatch multi_batch
> ok 4 - testlibpqbatch batch_abort
> ok 5 - testlibpqbatch timings
> not ok 6 - testlibpqbatch copyfailure
>
> Still broken here. I can see that this patch is having enough
> safeguards in PQbatchBegin() and PQsendQueryStart(), but this issue is
> really pointing out at something...
>
>
I will check this one with fresh setup again and I guess that this should
not block the code patch.

Thanks & Regards,
Vaishnavi
Fujitsu Australia.

  64. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-03-30T18:12:14Z

    	Vaishnavi Prabakaran wrote:

> Hmm, With batch mode, after sending COPY command to server(and server
> started processing the query and goes into COPY state) , client does not
> immediately read the result , but it keeps sending other queries to the
> server. By this time, server already encountered the error
> scenario(Receiving different message during COPY state) and sent error
> messages

IOW, the test intentionally violates the protocol and then all goes wonky
because of that.
That's why I was wondering upthread what's it's supposed to test.
I mean, regression tests are meant to warn against a desirable behavior
being unknowingly changed by new code into an undesirable behavior.
Here we have the undesirable behavior to start with.
What kind of regression could we fear from that?

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  65. Re: PATCH: Batch/pipelining support for libpq

    David Steele <david@pgmasters.net> — 2017-03-31T15:03:26Z

    Hi,

On 3/30/17 2:12 PM, Daniel Verite wrote:
> 	Vaishnavi Prabakaran wrote:
>
>> Hmm, With batch mode, after sending COPY command to server(and server
>> started processing the query and goes into COPY state) , client does not
>> immediately read the result , but it keeps sending other queries to the
>> server. By this time, server already encountered the error
>> scenario(Receiving different message during COPY state) and sent error
>> messages
>
> IOW, the test intentionally violates the protocol and then all goes wonky
> because of that.
> That's why I was wondering upthread what's it's supposed to test.
> I mean, regression tests are meant to warn against a desirable behavior
> being unknowingly changed by new code into an undesirable behavior.
> Here we have the undesirable behavior to start with.
> What kind of regression could we fear from that?

The CF has been extended until April 7 but time is still growing short. 
Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this 
submission will be marked "Returned with Feedback".

Thanks,
-- 
-David
david@pgmasters.net

  66. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-04-03T04:10:47Z

    On Sat, Apr 1, 2017 at 2:03 AM, David Steele <david@pgmasters.net> wrote:

> Hi,
>
> On 3/30/17 2:12 PM, Daniel Verite wrote:
>
>>         Vaishnavi Prabakaran wrote:
>>
>> Hmm, With batch mode, after sending COPY command to server(and server
>>> started processing the query and goes into COPY state) , client does not
>>> immediately read the result , but it keeps sending other queries to the
>>> server. By this time, server already encountered the error
>>> scenario(Receiving different message during COPY state) and sent error
>>> messages
>>>
>>
>> IOW, the test intentionally violates the protocol and then all goes wonky
>> because of that.
>> That's why I was wondering upthread what's it's supposed to test.
>> I mean, regression tests are meant to warn against a desirable behavior
>> being unknowingly changed by new code into an undesirable behavior.
>> Here we have the undesirable behavior to start with.
>> What kind of regression could we fear from that?
>>
>
Yes, completely agree, demonstrating the undesirable behavior is not needed
as documentation gives enough warning to user.
The test patch is decided not to go in for now, but will be re-implemented
with PSQL commands later. So, during the re-implementation of test patch, I
will remove this test. Thanks .


>
> The CF has been extended until April 7 but time is still growing short.
> Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this
> submission will be marked "Returned with Feedback".
>
>
Thanks for the information, attached the latest patch resolving one
compilation warning. And, please discard the test patch as it will be
re-implemented later separately.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  67. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-04-03T21:05:01Z

    On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > The CF has been extended until April 7 but time is still growing short.
> > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or this
> > submission will be marked "Returned with Feedback".
> >
> >
> Thanks for the information, attached the latest patch resolving one
> compilation warning. And, please discard the test patch as it will be
> re-implemented later separately.

Hm.  If the tests aren't ready yet, it seems we'll have to move this to
the next CF.


> + <sect1 id="libpq-batch-mode">
> +  <title>Batch mode and query pipelining</title>
> +
> +  <indexterm zone="libpq-batch-mode">
> +   <primary>libpq</primary>
> +   <secondary>batch mode</secondary>
> +  </indexterm>
> +
> +  <indexterm zone="libpq-batch-mode">
> +   <primary>libpq</primary>
> +   <secondary>pipelining</secondary>
> +  </indexterm>
> +
> +  <para>
> +   <application>libpq</application> supports queueing up multiple queries into
> +   a pipeline to be executed as a batch on the server. Batching queries allows
> +   applications to avoid a client/server round-trip after each query to get
> +   the results before issuing the next query.
> +  </para>

"queueing .. into a pipeline" sounds weird to me - but I'm not a native
speaker.  Also batching != pipelining.


> +  <sect2>
> +   <title>When to use batching</title>
> +
> +   <para>
> +    Much like asynchronous query mode, there is no performance disadvantage to
> +    using batching and pipelining. It increases client application complexity
> +    and extra caution is required to prevent client/server deadlocks but
> +    offers considerable performance improvements.
> +   </para>

s/offers/can sometimes offer/


> +  <sect2 id="libpq-batch-using">
> +   <title>Using batch mode</title>
> +
> +   <para>
> +    To issue batches the application must switch
> +    <application>libpq</application> into batch mode.

s/libpq/a connection/?


> Enter batch mode with <link
> +    linkend="libpq-PQbatchBegin"><function>PQbatchBegin(conn)</function></link> or test
> +    whether batch mode is active with <link
> +    linkend="libpq-PQbatchStatus"><function>PQbatchStatus(conn)</function></link>. In batch mode only <link
> +    linkend="libpq-async">asynchronous operations</link> are permitted, and
> +    <literal>COPY</literal> is not recommended as it most likely will trigger failure in batch processing. 
> +    (The restriction on <literal>COPY</literal> is an implementation
> +    limit; the PostgreSQL protocol and server can support batched
> <literal>COPY</literal>).

Hm, I'm unconvinced that that's a useful parenthetical in the libpq
docs.


> +   <para>
> +    The client uses libpq's asynchronous query functions to dispatch work,
> +    marking the end of each batch with <function>PQbatchQueueSync</function>.
> +    Concurrently, it uses <function>PQgetResult</function> and
> +    <function>PQbatchQueueProcess</function> to get results.

"Concurrently" imo is a dangerous word, somewhat implying multi-threading.


> +   <note>
> +    <para>
> +     It is best to use batch mode with <application>libpq</application> in
> +     <link linkend="libpq-pqsetnonblocking">non-blocking mode</link>. If used in
> +     blocking mode it is possible for a client/server deadlock to occur. The
> +     client will block trying to send queries to the server, but the server will
> +     block trying to send results from queries it has already processed to the
> +     client. This only occurs when the client sends enough queries to fill its
> +     output buffer and the server's receive buffer before switching to
> +     processing input from the server, but it's hard to predict exactly when
> +     that'll happen so it's best to always use non-blocking mode.
> +    </para>
> +   </note>

Such deadlocks are possible just as well with non-blocking mode, unless
one can avoid sending queries and switching to receiving results anytime
in the application code.


> +    <para>
> +     Batched operations will be executed by the server in the order the client
> +     sends them. The server will send the results in the order the statements
> +     executed. The server may begin executing the batch before all commands
> +     in the batch are queued and the end of batch command is sent. If any
> +     statement encounters an error the server aborts the current transaction and
> +     skips processing the rest of the batch. Query processing resumes after the
> +     end of the failed batch.
> +    </para>

What if a batch contains transaction boundaries?


> +   <sect3 id="libpq-batch-results">
> +    <title>Processing results</title>
> +
> +    <para>
> +     The client <link linkend="libpq-batch-interleave">interleaves result
> +     processing with sending batch queries</link>, or for small batches may
> +     process all results after sending the whole batch.
> +    </para>

That's a very long <link> text, is it not?


> +    <para>
> +     To get the result of the first batch entry the client must call <link
> +     linkend="libpq-PQbatchQueueProcess"><function>PQbatchQueueProcess</function></link>. It must then call

What does 'QueueProcess' mean? Shouldn't it be 'ProcessQueue'? You're
not enquing a process or processing, right?


> +     <function>PQgetResult</function> and handle the results until
> +     <function>PQgetResult</function> returns null (or would return null if
> +     called).

What is that parenthetical referring to?  IIRC we don't provide any
external way to determine PQgetResult would return NULL.


Have you checked how this API works with PQsetSingleRowMode()?

> +    <para>
> +     To enter single-row mode, call <function>PQsetSingleRowMode</function> immediately after a
> +     successful call of <function>PQbatchQueueProcess</function>. This mode selection is effective 
> +     only for the query currently being processed. For more information on the use of <function>PQsetSingleRowMode
> +     </function>, refer to <xref linkend="libpq-single-row-mode">.

Hah ;).


> +    <para>
> +     The client must not assume that work is committed when it
> +     <emphasis>sends</emphasis> a <literal>COMMIT</literal>, only when the
> +     corresponding result is received to confirm the commit is complete.
> +     Because errors arrive asynchronously the application needs to be able to
> +     restart from the last <emphasis>received</emphasis> committed change and
> +     resend work done after that point if something goes wrong.
> +    </para>

That seems like a batch independent thing, right?  If so, maybe make it
a <note>?


> +     <listitem>
> +      <para>
> +      Causes a connection to enter batch mode if it is currently idle or
> +      already in batch mode.
> +
> +<synopsis>
> +int PQbatchBegin(PGconn *conn);
> +</synopsis>
> +
> +        </para>
> +        <para>
> +          Returns 1 for success. Returns 0 and has no 
> +          effect if the connection is not currently idle, i.e. it has a result 
> +          ready, is waiting for more input from the server, etc. This function 
> +          does not actually send anything to the server, it just changes the 
> +          <application>libpq</application> connection state.
> +
> +        </para>
> +     </listitem>
> +    </varlistentry>

That function name sounds a bit too much like it'd be relevant for a
single batch, not something that can send many batches. enterBatchMode?

> +    <varlistentry id="libpq-PQbatchEnd">
> +     <term>
> +      <function>PQbatchEnd</function>
> +      <indexterm>
> +       <primary>PQbatchEnd</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Causes a connection to exit batch mode if it is currently in batch mode
> +      with an empty queue and no pending results.
> +<synopsis>
> +int PQbatchEnd(PGconn *conn);
> +</synopsis>
> +        </para>
> +        <para>Returns 1 for success.
> +      Returns 1 and takes no action if not in batch mode. If the connection has
> +      pending batch items in the queue for reading with
> +      <function>PQbatchQueueProcess</function>, the current statement isn't finished
> +      processing or there are results pending for collection with
> +      <function>PQgetResult</function>, returns 0 and does nothing.
> +
> +      </para>
> +     </listitem>
> +    </varlistentry>

""


> +    <varlistentry id="libpq-PQbatchQueueSync">
> +     <term>
> +      <function>PQbatchQueueSync</function>
> +      <function>PQbatchQueueProcess</function>

As said above, I'm not a fan of these, because it sounds like you're
queueing a sync/process.


>  /* ----------------
> @@ -1108,7 +1113,7 @@ pqRowProcessor(PGconn *conn, const char **errmsgp)
>  		conn->next_result = conn->result;
>  		conn->result = res;
>  		/* And mark the result ready to return */
> -		conn->asyncStatus = PGASYNC_READY;
> +		conn->asyncStatus = PGASYNC_READY_MORE;
>  	}

Uhm, isn't that an API/ABI breakage issue?



>  /*
> - * Common startup code for PQsendQuery and sibling routines
> + * PQmakePipelinedCommand
> + *	Get a new command queue entry, allocating it if required. Doesn't add it to
> + *	the tail of the queue yet, use PQappendPipelinedCommand once the command has
> + *	been written for that. If a command fails once it's called this, it should
> + *	use PQrecyclePipelinedCommand to put it on the freelist or release it.

"command fails once it's called this"?


> +/*
> + * PQrecyclePipelinedCommand
> + *	Push a command queue entry onto the freelist. It must be a dangling entry
> + *	with null next pointer and not referenced by any other entry's next pointer.
> + */
> +static void
> +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> +{
> +	if (entry == NULL)
> +		return;
> +	if (entry->next != NULL)
> +	{
> +		fprintf(stderr, "tried to recycle non-dangling command queue entry");
> +		abort();

Needs a libpq_gettext()?



> +/*
> + * PQbatchEnd
> + * 	End batch mode and return to normal command mode.
> + *
> + * 	Has no effect unless the client has processed all results
> + * 	from all outstanding batches and the connection is idle.
> + *
> + * 	Returns true if batch mode ended.
> + */
> +int
> +PQbatchEnd(PGconn *conn)
> +{
> +	if (!conn)
> +		return false;
> +
> +	if (conn->batch_status == PQBATCH_MODE_OFF)
> +		return true;
> +
> +	switch (conn->asyncStatus)
> +	{
> +		case PGASYNC_IDLE:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, IDLE in batch mode"));
> +			break;
> +		case PGASYNC_COPY_IN:
> +		case PGASYNC_COPY_OUT:
> +		case PGASYNC_COPY_BOTH:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, COPY in batch mode"));
> +			break;

Why aren't you returning false here,

> +		case PGASYNC_READY:
> +		case PGASYNC_READY_MORE:
> +		case PGASYNC_BUSY:
> +			/* can't end batch while busy */
> +			return false;

but are here?

> +		case PGASYNC_QUEUED:
> +			break;
> +	}
> +


> +int
> +PQbatchQueueSync(PGconn *conn)
> +{
> +	PGcommandQueueEntry *entry;
> +
> +	if (!conn)
> +		return false;
> +
> +	if (conn->batch_status == PQBATCH_MODE_OFF)
> +		return false;
> +
> +	switch (conn->asyncStatus)
> +	{
> +		case PGASYNC_IDLE:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, IDLE in batch mode"));
> +			break;
> +		case PGASYNC_COPY_IN:
> +		case PGASYNC_COPY_OUT:
> +		case PGASYNC_COPY_BOTH:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, COPY in batch mode"));
> +			break;
> +		case PGASYNC_READY:
> +		case PGASYNC_READY_MORE:
> +		case PGASYNC_BUSY:
> +		case PGASYNC_QUEUED:
> +			/* can send sync to end this batch of cmds */
> +			break;
> +	}

Uhm, what is that switch actually achieving? We're not returning an
error code, so ...?

> +	/* Should try to flush immediately if there's room */
> +	PQflush(conn);

"room"?

Also, don't we need to process PQflush's return value?


> +/*
> + * PQbatchQueueProcess
> + *	 In batch mode, start processing the next query in the queue.
> + *
> + * Returns true if the next query was popped from the queue and can
> + * be processed by PQconsumeInput, PQgetResult, etc.
> + *
> + * Returns false if the current query isn't done yet, the connection
> + * is not in a batch, or there are no more queries to process.
> + */
> +int
> +PQbatchQueueProcess(PGconn *conn)
> +{
> +	PGcommandQueueEntry *next_query;
> +
> +	if (!conn)
> +		return false;
> +
> +	if (conn->batch_status == PQBATCH_MODE_OFF)
> +		return false;
> +
> +	switch (conn->asyncStatus)
> +	{
> +		case PGASYNC_COPY_IN:
> +		case PGASYNC_COPY_OUT:
> +		case PGASYNC_COPY_BOTH:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, COPY in batch mode"));
> +			break;
> +		case PGASYNC_READY:
> +		case PGASYNC_READY_MORE:
> +		case PGASYNC_BUSY:
> +			/* client still has to process current query or results */
> +			return false;
> +			break;
> +		case PGASYNC_IDLE:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, IDLE in batch mode"));
> +			break;
> +		case PGASYNC_QUEUED:
> +			/* next query please */
> +			break;
> +	}

Once more, I'm very unconvinced by the switch. Unless you do anything
with the errors, this seems pointless.


> +	if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass != PGQUERY_SYNC)
> +	{
> +		/*
> +		 * In an aborted batch we don't get anything from the server for each
> +		 * result; we're just discarding input until we get to the next sync
> +		 * from the server. The client needs to know its queries got aborted
> +		 * so we create a fake PGresult to return immediately from
> +		 * PQgetResult.
> +		 */
> +		conn->result = PQmakeEmptyPGresult(conn,
> +										   PGRES_BATCH_ABORTED);
> +		if (!conn->result)
> +		{
> +			printfPQExpBuffer(&conn->errorMessage,
> +							  libpq_gettext("out of memory"));
> +			pqSaveErrorResult(conn);
> +		}
> +		conn->asyncStatus = PGASYNC_READY;

So we still return true in the OOM case?



Greetings,

Andres Freund

  68. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-04-03T23:24:23Z

    Hi,
On Tue, Apr 4, 2017 at 7:05 AM, Andres Freund <andres@anarazel.de> wrote:

> On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > > The CF has been extended until April 7 but time is still growing short.
> > > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or
> this
> > > submission will be marked "Returned with Feedback".
> > >
> > >
> > Thanks for the information, attached the latest patch resolving one
> > compilation warning. And, please discard the test patch as it will be
> > re-implemented later separately.
>
> Hm.  If the tests aren't ready yet, it seems we'll have to move this to
> the next CF.
>
>
Thanks for your review and I will address your review comments and send the
newer version of patch shortly.
Just quickly, Is it not ok to consider only the code patch for this CF
without test patch?

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  69. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-04-03T23:26:13Z

    On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
> Hi,
> On Tue, Apr 4, 2017 at 7:05 AM, Andres Freund <andres@anarazel.de> wrote:
> 
> > On 2017-04-03 14:10:47 +1000, Vaishnavi Prabakaran wrote:
> > > > The CF has been extended until April 7 but time is still growing short.
> > > > Please respond with a new patch by 2017-04-04 00:00 AoE (UTC-12) or
> > this
> > > > submission will be marked "Returned with Feedback".
> > > >
> > > >
> > > Thanks for the information, attached the latest patch resolving one
> > > compilation warning. And, please discard the test patch as it will be
> > > re-implemented later separately.
> >
> > Hm.  If the tests aren't ready yet, it seems we'll have to move this to
> > the next CF.
> >
> >
> Thanks for your review and I will address your review comments and send the
> newer version of patch shortly.

Cool.

> Just quickly, Is it not ok to consider only the code patch for this CF
> without test patch?

I'd say no, it's not acceptable.  This is too much new code for it not
to be tested.

Andres

  70. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-04-03T23:57:33Z

    On Tue, Apr 4, 2017 at 8:26 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
>> Just quickly, Is it not ok to consider only the code patch for this CF
>> without test patch?
>
> I'd say no, it's not acceptable.  This is too much new code for it not
> to be tested.

Doesn't it depend actually? I would think that the final patch may not
include all the tests implemented if:
- The thread on which a patch has been developed had a set of tests
done and posted with it.
- Including them does not make sense if we have a way to run those
tests more efficiently. Sometimes a bunch of benchmarks or tests are
run on a patch bu for the final result keeping them around does not
make much sense.

In the case of this patch, it seems to me that we would have a far
better portable set of tests if we had a dedicated set of subcommands
available at psql level, particularly for Windows/MSVC. If that's a
requirement for this patch so let it be. I am not saying that tests
are not necessary. They are of course, but in this case having a bit
more infrastructure would be more be more helpful for users and the
tests themselves.

Note that I won't complain either if this set of C tests are included
at the end.
-- 
Michael

  71. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-04-04T04:52:01Z

    On 2017-04-04 08:57:33 +0900, Michael Paquier wrote:
> On Tue, Apr 4, 2017 at 8:26 AM, Andres Freund <andres@anarazel.de> wrote:
> > On 2017-04-04 09:24:23 +1000, Vaishnavi Prabakaran wrote:
> >> Just quickly, Is it not ok to consider only the code patch for this CF
> >> without test patch?
> >
> > I'd say no, it's not acceptable.  This is too much new code for it not
> > to be tested.
> 
> Doesn't it depend actually?

Well, I didn't make a general statement, I made one about this patch.
And this would add a significant bunch of untested code, and it'll likely
take years till it gets decent coverage outside.


> In the case of this patch, it seems to me that we would have a far
> better portable set of tests if we had a dedicated set of subcommands
> available at psql level, particularly for Windows/MSVC.

That's a really large scope creep imo.  Adding a bunch of user-facing
psql stuff doesn't compare in complexity to running a test across
platforms.  We can just do that from regess.c or such, if that ends up
being a problem..

> If that's a  requirement for this patch so let it be. I am not saying that tests
> are not necessary. They are of course, but in this case having a bit
> more infrastructure would be more be more helpful for users and the
> tests themselves.

I'm not following.


Greetings,

Andres Freund

  72. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-04-05T07:00:42Z

     Andres Freund <andres@anarazel.de> wrote:

> > +
> > +  <para>
> > +   <application>libpq</application> supports queueing up multiple
> queries into
> > +   a pipeline to be executed as a batch on the server. Batching queries
> allows
> > +   applications to avoid a client/server round-trip after each query to
> get
> > +   the results before issuing the next query.
> > +  </para>
>
> "queueing .. into a pipeline" sounds weird to me - but I'm not a native
> speaker.  Also batching != pipelining.


Re-phrased the sentence as "libpq supports queries to be queued up in
batches and pipeline them to the server, where it will be executed as a
batch" . Pipelining queries allows applications to avoid a client/server
round-trip after each query to get the results before issuing the next
query.


>
> > +  <sect2>
> > +   <title>When to use batching</title>
> > +
> > +   <para>
> > +    Much like asynchronous query mode, there is no performance
> disadvantage to
> > +    using batching and pipelining. It increases client application
> complexity
> > +    and extra caution is required to prevent client/server deadlocks but
> > +    offers considerable performance improvements.
> > +   </para>
>
> s/offers/can sometimes offer/
>
>
Corrected.


>
> > +  <sect2 id="libpq-batch-using">
> > +   <title>Using batch mode</title>
> > +
> > +   <para>
> > +    To issue batches the application must switch
> > +    <application>libpq</application> into batch mode.
>
> s/libpq/a connection/?
>
>
Corrected.


>
> > Enter batch mode with <link
> > +    linkend="libpq-PQbatchBegin"><function>PQbatchBegin(conn)</function></link>
> or test
> > +    whether batch mode is active with <link
> > +    linkend="libpq-PQbatchStatus"><function>PQbatchStatus(conn)</function></link>.
> In batch mode only <link
> > +    linkend="libpq-async">asynchronous operations</link> are
> permitted, and
> > +    <literal>COPY</literal> is not recommended as it most likely will
> trigger failure in batch processing.
> > +    (The restriction on <literal>COPY</literal> is an implementation
> > +    limit; the PostgreSQL protocol and server can support batched
> > <literal>COPY</literal>).
>
> Hm, I'm unconvinced that that's a useful parenthetical in the libpq
> docs.
>
>
Removed the parenthesis as the line before gives enough warning.



>
> > +   <para>
> > +    The client uses libpq's asynchronous query functions to dispatch
> work,
> > +    marking the end of each batch with <function>PQbatchQueueSync</
> function>.
> > +    Concurrently, it uses <function>PQgetResult</function> and
> > +    <function>PQbatchQueueProcess</function> to get results.
>
> "Concurrently" imo is a dangerous word, somewhat implying multi-threading.
>
>
Corrected by replacing "concurrently" with "And to get results"


>
> > +   <note>
> > +    <para>
> > +     It is best to use batch mode with <application>libpq</application>
> in
> > +     <link linkend="libpq-pqsetnonblocking">non-blocking mode</link>.
> If used in
> > +     blocking mode it is possible for a client/server deadlock to
> occur. The
> > +     client will block trying to send queries to the server, but the
> server will
> > +     block trying to send results from queries it has already processed
> to the
> > +     client. This only occurs when the client sends enough queries to
> fill its
> > +     output buffer and the server's receive buffer before switching to
> > +     processing input from the server, but it's hard to predict exactly
> when
> > +     that'll happen so it's best to always use non-blocking mode.
> > +    </para>
> > +   </note>
>
> Such deadlocks are possible just as well with non-blocking mode, unless
> one can avoid sending queries and switching to receiving results anytime
> in the application code.
>
>
True, and in the documentation , how to interleave result processing and
query dispatch is given.


>
> > +    <para>
> > +     Batched operations will be executed by the server in the order the
> client
> > +     sends them. The server will send the results in the order the
> statements
> > +     executed. The server may begin executing the batch before all
> commands
> > +     in the batch are queued and the end of batch command is sent. If
> any
> > +     statement encounters an error the server aborts the current
> transaction and
> > +     skips processing the rest of the batch. Query processing resumes
> after the
> > +     end of the failed batch.
> > +    </para>
>
> What if a batch contains transaction boundaries?
>
>
Regardless of transaction boundaries, failed command will make the entire
batch aborted and no commands will be processed in server until Batch sync
is sent. And, if the command inside the transaction fails, then the batch
will be aborted, but transaction will still be open in the server. Client
explicitly needs to close the transaction. This is explained further down
in the documentation section "Error handling" , paragraph starting with "If
the batch used an implicit transaction.." and "Note" in that section gives
warning too.


>
> > +   <sect3 id="libpq-batch-results">
> > +    <title>Processing results</title>
> > +
> > +    <para>
> > +     The client <link linkend="libpq-batch-interleave">interleaves
> result
> > +     processing with sending batch queries</link>, or for small batches
> may
> > +     process all results after sending the whole batch.
> > +    </para>
>
> That's a very long <link> text, is it not?
>
>
Corrected.


>
> > +    <para>
> > +     To get the result of the first batch entry the client must call
> <link
> > +     linkend="libpq-PQbatchQueueProcess"><
> function>PQbatchQueueProcess</function></link>. It must then call
>
> What does 'QueueProcess' mean? Shouldn't it be 'ProcessQueue'? You're
> not enquing a process or processing, right?
>
>
Changed to PQbatchProcessQueue.


>
> > +     <function>PQgetResult</function> and handle the results until
> > +     <function>PQgetResult</function> returns null (or would return
> null if
> > +     called).
>
> What is that parenthetical referring to?  IIRC we don't provide any
> external way to determine PQgetResult would return NULL.
>
>
True, it is just application's extra knowledge. Removed the parenthesis.


> > +    <para>
> > +     The client must not assume that work is committed when it
> > +     <emphasis>sends</emphasis> a <literal>COMMIT</literal>, only when
> the
> > +     corresponding result is received to confirm the commit is complete.
> > +     Because errors arrive asynchronously the application needs to be
> able to
> > +     restart from the last <emphasis>received</emphasis> committed
> change and
> > +     resend work done after that point if something goes wrong.
> > +    </para>
>
> That seems like a batch independent thing, right?  If so, maybe make it
> a <note>?
>
>
Yes, made it as Note.


> > +
> > +<synopsis>
> > +int PQbatchBegin(PGconn *conn);
> ...
>
> That function name sounds a bit too much like it'd be relevant for a
> single batch, not something that can send many batches. enterBatchMode?
>
> > +int PQbatchEnd(PGconn *conn);
> ...
> ""
>
>
Changed the function names to PQenterBatchMode and PQexitBatchMode.



>
> > +    <varlistentry id="libpq-PQbatchQueueSync">
> > +     <term>
> > +      <function>PQbatchQueueSync</function>
> > +      <function>PQbatchQueueProcess</function>
>
> As said above, I'm not a fan of these, because it sounds like you're
> queueing a sync/process.
>
>
Changed it to PQbatchSyncQueue.


>
> >  /* ----------------
> > @@ -1108,7 +1113,7 @@ pqRowProcessor(PGconn *conn, const char **errmsgp)
> >               conn->next_result = conn->result;
> >               conn->result = res;
> >               /* And mark the result ready to return */
> > -             conn->asyncStatus = PGASYNC_READY;
> > +             conn->asyncStatus = PGASYNC_READY_MORE;
> >       }
>
> Uhm, isn't that an API/ABI breakage issue?
>
>
I think no, asynstatus is in libpq-int.h which is meant to be used only by
frontend libpq, applications should use libpq-fe.h . Also, we don't have
any API to return the async status of connection to application. So, I
think it will not break any existing applications.



>
> >  /*
> > - * Common startup code for PQsendQuery and sibling routines
> > + * PQmakePipelinedCommand
> > + *   Get a new command queue entry, allocating it if required. Doesn't
> add it to
> > + *   the tail of the queue yet, use PQappendPipelinedCommand once the
> command has
> > + *   been written for that. If a command fails once it's called this,
> it should
> > + *   use PQrecyclePipelinedCommand to put it on the freelist or release
> it.
>
> "command fails once it's called this"?
>
>
Corrected.- "Command sending fails"


>
> > +/*
> > + * PQrecyclePipelinedCommand
> > + *   Push a command queue entry onto the freelist. It must be a
> dangling entry
> > + *   with null next pointer and not referenced by any other entry's
> next pointer.
> > + */
> > +static void
> > +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> > +{
> > +     if (entry == NULL)
> > +             return;
> > +     if (entry->next != NULL)
> > +     {
> > +             fprintf(stderr, "tried to recycle non-dangling command
> queue entry");
> > +             abort();
>
> Needs a libpq_gettext()?
>
>
>
Corrected.



>
> > +/*
> > + * PQbatchEnd
> > + *   End batch mode and return to normal command mode.
> > + *
> > + *   Has no effect unless the client has processed all results
> > + *   from all outstanding batches and the connection is idle.
> > + *
> > + *   Returns true if batch mode ended.
> > + */
> > +int
> > +PQbatchEnd(PGconn *conn)
> > +{
> > +     if (!conn)
> > +             return false;
> > +
> > +     if (conn->batch_status == PQBATCH_MODE_OFF)
> > +             return true;
> > +
> > +     switch (conn->asyncStatus)
> > +     {
> > +             case PGASYNC_IDLE:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> IDLE in batch mode"));
> > +                     break;
> > +             case PGASYNC_COPY_IN:
> > +             case PGASYNC_COPY_OUT:
> > +             case PGASYNC_COPY_BOTH:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > +                     break;
>
> Why aren't you returning false here,



Using copy command in batch mode is the error case, so instead of giving
opportunity to the application to finish the error scenario, we set the
error message and allows it to quit the batch mode.


>
> > +             case PGASYNC_READY:
> > +             case PGASYNC_READY_MORE:
> > +             case PGASYNC_BUSY:
> > +                     /* can't end batch while busy */
> > +                     return false;
>
> but are here?


Application gets a chance to finish the result processing and exit the
batch mode.



> > +int
> > +PQbatchQueueSync(PGconn *conn)
> > +{
> > +     PGcommandQueueEntry *entry;
> > +
> > +     if (!conn)
> > +             return false;
> > +
> > +     if (conn->batch_status == PQBATCH_MODE_OFF)
> > +             return false;
> > +
> > +     switch (conn->asyncStatus)
> > +     {
> > +             case PGASYNC_IDLE:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> IDLE in batch mode"));
> > +                     break;
> > +             case PGASYNC_COPY_IN:
> > +             case PGASYNC_COPY_OUT:
> > +             case PGASYNC_COPY_BOTH:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > +                     break;
> > +             case PGASYNC_READY:
> > +             case PGASYNC_READY_MORE:
> > +             case PGASYNC_BUSY:
> > +             case PGASYNC_QUEUED:
> > +                     /* can send sync to end this batch of cmds */
> > +                     break;
> > +     }
>
> Uhm, what is that switch actually achieving? We're not returning an
> error code, so ...?


These errors are marked here, so that the application later when reading
the results for batch queries, will have more information about what went
wrong. This is just an additional information to user only, but doesn't
stop the error scenario.



>
>
> > +     /* Should try to flush immediately if there's room */
> > +     PQflush(conn);
>
> "room"?
>
> Also, don't we need to process PQflush's return value?
>
>
Corrected both .



>
> > +/*
> > + * PQbatchQueueProcess
> > + *    In batch mode, start processing the next query in the queue.
> > + *
> > + * Returns true if the next query was popped from the queue and can
> > + * be processed by PQconsumeInput, PQgetResult, etc.
> > + *
> > + * Returns false if the current query isn't done yet, the connection
> > + * is not in a batch, or there are no more queries to process.
> > + */
> > +int
> > +PQbatchQueueProcess(PGconn *conn)
> > +{
> > +     PGcommandQueueEntry *next_query;
> > +
> > +     if (!conn)
> > +             return false;
> > +
> > +     if (conn->batch_status == PQBATCH_MODE_OFF)
> > +             return false;
> > +
> > +     switch (conn->asyncStatus)
> > +     {
> > +             case PGASYNC_COPY_IN:
> > +             case PGASYNC_COPY_OUT:
> > +             case PGASYNC_COPY_BOTH:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > +                     break;
> > +             case PGASYNC_READY:
> > +             case PGASYNC_READY_MORE:
> > +             case PGASYNC_BUSY:
> > +                     /* client still has to process current query or
> results */
> > +                     return false;
> > +                     break;
> > +             case PGASYNC_IDLE:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> IDLE in batch mode"));
> > +                     break;
> > +             case PGASYNC_QUEUED:
> > +                     /* next query please */
> > +                     break;
> > +     }
>
> Once more, I'm very unconvinced by the switch. Unless you do anything
> with the errors, this seems pointless.


Same answer as above switch question.



>
>
> > +     if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass
> != PGQUERY_SYNC)
> > +     {
> > +             /*
> > +              * In an aborted batch we don't get anything from the
> server for each
> > +              * result; we're just discarding input until we get to the
> next sync
> > +              * from the server. The client needs to know its queries
> got aborted
> > +              * so we create a fake PGresult to return immediately from
> > +              * PQgetResult.
> > +              */
> > +             conn->result = PQmakeEmptyPGresult(conn,
> > +
>         PGRES_BATCH_ABORTED);
> > +             if (!conn->result)
> > +             {
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +
>  libpq_gettext("out of memory"));
> > +                     pqSaveErrorResult(conn);
> > +             }
> > +             conn->asyncStatus = PGASYNC_READY;
>
> So we still return true in the OOM case?
>
>
Corrected to return false, as further calls to this function will also
fail. And , application as well can check for this failure reason from
PQgetResult.

Regarding test patch, I have corrected the test suite after David Steele's
comments.
Also, I would like to mention that a companion patch was submitted by David
Steele up-thread.

Attached the latest code and test patch.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  73. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-04-05T22:45:26Z

    Hi,

On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
> Regarding test patch, I have corrected the test suite after David Steele's
> comments.
> Also, I would like to mention that a companion patch was submitted by David
> Steele up-thread.
> 
> Attached the latest code and test patch.

My impression is that this'll need a couple more rounds of review. Given
that this'll establish API we'll pretty much ever going to be able to
change/remove, I think it'd be a bad idea to rush this into v10.
Therefore I propose moving this to the next CF.

- Andres

  74. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-19T22:49:38Z

    On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
> Hi,
> 
> On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
> > Regarding test patch, I have corrected the test suite after David Steele's
> > comments.
> > Also, I would like to mention that a companion patch was submitted by David
> > Steele up-thread.
> > 
> > Attached the latest code and test patch.
> 
> My impression is that this'll need a couple more rounds of review. Given
> that this'll establish API we'll pretty much ever going to be able to
> change/remove, I think it'd be a bad idea to rush this into v10.
> Therefore I propose moving this to the next CF.

Craig, Vaishnavi, everyone else: Are you planning to continue to work on
this for v11?  I'm willing to do another round, but only if it's
worthwhile.

FWIW, I still think this needs a pgbench or similar example integration,
so we can actually properly measure the benefits.

- Andres

  75. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-06-20T00:09:37Z

    On Tue, Jun 20, 2017 at 8:49 AM, Andres Freund <andres@anarazel.de> wrote:

> On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
> > Hi,
> >
> > On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
> > > Regarding test patch, I have corrected the test suite after David
> Steele's
> > > comments.
> > > Also, I would like to mention that a companion patch was submitted by
> David
> > > Steele up-thread.
> > >
> > > Attached the latest code and test patch.
> >
> > My impression is that this'll need a couple more rounds of review. Given
> > that this'll establish API we'll pretty much ever going to be able to
> > change/remove, I think it'd be a bad idea to rush this into v10.
> > Therefore I propose moving this to the next CF.
>
> Craig, Vaishnavi, everyone else: Are you planning to continue to work on
> this for v11?  I'm willing to do another round, but only if it's
> worthwhile.
>

Yes, am willing to continue working on this patch for v11.


>
> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.
>

I will investigate on this further.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  76. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-06-20T01:43:24Z

    On 20 June 2017 at 06:49, Andres Freund <andres@anarazel.de> wrote:
> On 2017-04-05 15:45:26 -0700, Andres Freund wrote:
>> Hi,
>>
>> On 2017-04-05 17:00:42 +1000, Vaishnavi Prabakaran wrote:
>> > Regarding test patch, I have corrected the test suite after David Steele's
>> > comments.
>> > Also, I would like to mention that a companion patch was submitted by David
>> > Steele up-thread.
>> >
>> > Attached the latest code and test patch.
>>
>> My impression is that this'll need a couple more rounds of review. Given
>> that this'll establish API we'll pretty much ever going to be able to
>> change/remove, I think it'd be a bad idea to rush this into v10.
>> Therefore I propose moving this to the next CF.
>
> Craig, Vaishnavi, everyone else: Are you planning to continue to work on
> this for v11?  I'm willing to do another round, but only if it's
> worthwhile.

I'm happy to work on review, and will try to make some time, but have
to focus primarily on logical rep infrastructure. This patch was a
proof of concept and fun hack for me and while I'm glad folks are
interested, it's not something I can dedicate much time to. Especially
with a 6-week-old baby now....

> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.

I agree. I originally wanted to patch psql, but it's pretty intrusive.
pgbench is likely a better target. Also pg_restore.

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

  77. Re: PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-20T02:11:29Z

    On Tue, Jun 20, 2017 at 10:43 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> Especially with a 6-week-old baby now....

Congratulations!
-- 
Michael

  78. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-06-20T15:51:23Z

    	Andres Freund wrote:

> FWIW, I still think this needs a pgbench or similar example integration,
> so we can actually properly measure the benefits.

Here's an updated version of the patch I made during review,
adding \beginbatch and \endbatch to pgbench.
The performance improvement appears clearly
with a custom script of this kind:
  \beginbatch
     UPDATE pgbench_branches SET bbalance = bbalance + 1 WHERE bid = 0;
     ..above repeated 1000 times...
  \endbatch

versus the same with a BEGIN; END; pair instead of \beginbatch \endbatch

On localhost on my desktop I tend to see a 30% difference in favor
of the batch mode with that kind of test.
On slower networks there are much bigger differences.

The latest main patch (v10) must also be slightly updated for HEAD,
because of this:
error: patch failed: src/interfaces/libpq/exports.txt:171
v11 attached without any other change.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  79. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-21T23:40:48Z

    On 2017-06-20 17:51:23 +0200, Daniel Verite wrote:
> 	Andres Freund wrote:
> 
> > FWIW, I still think this needs a pgbench or similar example integration,
> > so we can actually properly measure the benefits.
> 
> Here's an updated version of the patch I made during review,
> adding \beginbatch and \endbatch to pgbench.
> The performance improvement appears clearly
> with a custom script of this kind:
>   \beginbatch
>      UPDATE pgbench_branches SET bbalance = bbalance + 1 WHERE bid = 0;
>      ..above repeated 1000 times...
>   \endbatch
> 
> versus the same with a BEGIN; END; pair instead of \beginbatch \endbatch
> 
> On localhost on my desktop I tend to see a 30% difference in favor
> of the batch mode with that kind of test.
> On slower networks there are much bigger differences.

This is seriously impressive.  Just using the normal pgbench mixed
workload, wrapping a whole transaction into a batch *doubles* the
throughput.  And that's locally over a unix socket - the gain over
actual network will be larger.

\set nbranches 1 * :scale
\set ntellers 10 * :scale
\set naccounts 100000 * :scale
\set aid random(1, :naccounts)
\set bid random(1, :nbranches)
\set tid random(1, :ntellers)
\set delta random(-5000, 5000)
\beginbatch
BEGIN;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
SELECT abalance FROM pgbench_accounts WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_branches SET bbalance = bbalance + :delta WHERE bid = :bid;
INSERT INTO pgbench_history (tid, bid, aid, delta, mtime) VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);
END;
\endbatch

- Andres

  80. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-06-21T23:44:03Z

    On 22 Jun. 2017 07:40, "Andres Freund" <andres@anarazel.de> wrote:

On 2017-06-20 17:51:23 +0200, Daniel Verite wrote:
>       Andres Freund wrote:
>
> > FWIW, I still think this needs a pgbench or similar example integration,
> > so we can actually properly measure the benefits.
>
> Here's an updated version of the patch I made during review,
> adding \beginbatch and \endbatch to pgbench.
> The performance improvement appears clearly
> with a custom script of this kind:
>   \beginbatch
>      UPDATE pgbench_branches SET bbalance = bbalance + 1 WHERE bid = 0;
>      ..above repeated 1000 times...
>   \endbatch
>
> versus the same with a BEGIN; END; pair instead of \beginbatch \endbatch
>
> On localhost on my desktop I tend to see a 30% difference in favor
> of the batch mode with that kind of test.
> On slower networks there are much bigger differences.

This is seriously impressive.  Just using the normal pgbench mixed
workload, wrapping a whole transaction into a batch *doubles* the
throughput.  And that's locally over a unix socket - the gain over
actual network will be larger.


In my original tests I got over a 300x improvement on WAN :) . I should
check if the same applies with pgbench.

  81. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-22T00:29:08Z

    On 2017-06-21 16:40:48 -0700, Andres Freund wrote:
> On 2017-06-20 17:51:23 +0200, Daniel Verite wrote:
> > 	Andres Freund wrote:
> > 
> > > FWIW, I still think this needs a pgbench or similar example integration,
> > > so we can actually properly measure the benefits.
> > 
> > Here's an updated version of the patch I made during review,
> > adding \beginbatch and \endbatch to pgbench.
> > The performance improvement appears clearly
> > with a custom script of this kind:
> >   \beginbatch
> >      UPDATE pgbench_branches SET bbalance = bbalance + 1 WHERE bid = 0;
> >      ..above repeated 1000 times...
> >   \endbatch
> > 
> > versus the same with a BEGIN; END; pair instead of \beginbatch \endbatch
> > 
> > On localhost on my desktop I tend to see a 30% difference in favor
> > of the batch mode with that kind of test.
> > On slower networks there are much bigger differences.
> 
> This is seriously impressive.  Just using the normal pgbench mixed
> workload, wrapping a whole transaction into a batch *doubles* the
> throughput.  And that's locally over a unix socket - the gain over
> actual network will be larger.

I've not analyzed this further, but something with the way network is
done isn't yet quite right either in the pgbench patch or in the libpq
patch.  You'll currently get IO like:

sendto(3, "B\0\0\0\22\0P1_2\0\0\0\0\0\0\1\0\0D\0\0\0\6P\0E\0\0\0\t\0"..., 36, MSG_NOSIGNAL, NULL, 0) = 36
sendto(3, "B\0\0\0\35\0P1_4\0\0\0\0\1\0\0\0\0073705952\0\1\0\0D\0"..., 47, MSG_NOSIGNAL, NULL, 0) = 47
sendto(3, "B\0\0\0\35\0P1_6\0\0\0\0\1\0\0\0\0077740854\0\1\0\0D\0"..., 47, MSG_NOSIGNAL, NULL, 0) = 47
sendto(3, "B\0\0\0\35\0P1_8\0\0\0\0\1\0\0\0\0071570280\0\1\0\0D\0"..., 47, MSG_NOSIGNAL, NULL, 0) = 47
sendto(3, "B\0\0\0\36\0P1_10\0\0\0\0\1\0\0\0\0072634305\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\36\0P1_12\0\0\0\0\1\0\0\0\0078960656\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\36\0P1_14\0\0\0\0\1\0\0\0\0073030370\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\35\0P1_16\0\0\0\0\1\0\0\0\006376125\0\1\0\0D\0"..., 47, MSG_NOSIGNAL, NULL, 0) = 47
sendto(3, "B\0\0\0\36\0P1_18\0\0\0\0\1\0\0\0\0072982423\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\36\0P1_20\0\0\0\0\1\0\0\0\0073860195\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\36\0P1_22\0\0\0\0\1\0\0\0\0072794433\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\36\0P1_24\0\0\0\0\1\0\0\0\0075475271\0\1\0\0D"..., 48, MSG_NOSIGNAL, NULL, 0) = 48
sendto(3, "B\0\0\0\23\0P1_25\0\0\0\0\0\0\1\0\0D\0\0\0\6P\0E\0\0\0\t"..., 37, MSG_NOSIGNAL, NULL, 0) = 37
sendto(3, "S\0\0\0\4", 5, MSG_NOSIGNAL, NULL, 0) = 5
recvfrom(3, "2\0\0\0\4n\0\0\0\4C\0\0\0\nBEGIN\0002\0\0\0\4T\0\0\0!\0"..., 16384, 0, NULL, NULL) = 775
recvfrom(3, 0x559a02667ff2, 15630, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667fb1, 15695, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667f6c, 15764, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667f2b, 15829, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667eea, 15894, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667ea9, 15959, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667e68, 16024, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667e24, 16092, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667de3, 16157, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667da2, 16222, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667d5d, 16291, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667d1c, 16356, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
recvfrom(3, 0x559a02667d06, 16378, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)

I.e. we're doing tiny write send() syscalls (they should be coalesced)
and then completely unnecessarily call recv() over and over again
without polling.  To me it looks very much like we're just doing either
exactly once per command...

- Andres

  82. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-06-22T01:03:05Z

    On 22 June 2017 at 08:29, Andres Freund <andres@anarazel.de> wrote:

> I.e. we're doing tiny write send() syscalls (they should be coalesced)

That's likely worth doing, but can probably wait for a separate patch.
The kernel will usually do some packet aggregation unless we use
TCP_NODELAY (which we don't and shouldn't), and the syscall overhead
is IMO not worth worrying about just yet.

> and then completely unnecessarily call recv() over and over again
> without polling.  To me it looks very much like we're just doing either
> exactly once per command...

Yeah, that looks suspect.

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

  83. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-22T01:07:21Z

    On 2017-06-22 09:03:05 +0800, Craig Ringer wrote:
> On 22 June 2017 at 08:29, Andres Freund <andres@anarazel.de> wrote:
> 
> > I.e. we're doing tiny write send() syscalls (they should be coalesced)
> 
> That's likely worth doing, but can probably wait for a separate patch.

I don't think so, we should get this right, it could have API influence.


> The kernel will usually do some packet aggregation unless we use
> TCP_NODELAY (which we don't and shouldn't), and the syscall overhead
> is IMO not worth worrying about just yet.

1)
					/*
					 * Select socket options: no delay of outgoing data for
					 * TCP sockets, nonblock mode, close-on-exec. Fail if any
					 * of this fails.
					 */
					if (!IS_AF_UNIX(addr_cur->ai_family))
					{
						if (!connectNoDelay(conn))
						{
							pqDropConnection(conn, true);
							conn->addr_cur = addr_cur->ai_next;
							continue;
						}
					}

2) Even if nodelay weren't set, this can still lead to smaller packets
   being sent, because you start sending normal sized tcp packets,
   rather than jumbo ones, even if configured (pretty common these
   days).

3) Syscall overhead is actually quite significant.

- Andres

  84. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-06-22T01:18:41Z

    On 22 June 2017 at 09:07, Andres Freund <andres@anarazel.de> wrote:
> On 2017-06-22 09:03:05 +0800, Craig Ringer wrote:
>> On 22 June 2017 at 08:29, Andres Freund <andres@anarazel.de> wrote:
>>
>> > I.e. we're doing tiny write send() syscalls (they should be coalesced)
>>
>> That's likely worth doing, but can probably wait for a separate patch.
>
> I don't think so, we should get this right, it could have API influence.
>
>
>> The kernel will usually do some packet aggregation unless we use
>> TCP_NODELAY (which we don't and shouldn't), and the syscall overhead
>> is IMO not worth worrying about just yet.
>
> 1)
>                                         /*
>                                          * Select socket options: no delay of outgoing data for
>                                          * TCP sockets, nonblock mode, close-on-exec. Fail if any
>                                          * of this fails.
>                                          */
>                                         if (!IS_AF_UNIX(addr_cur->ai_family))
>                                         {
>                                                 if (!connectNoDelay(conn))
>                                                 {
>                                                         pqDropConnection(conn, true);
>                                                         conn->addr_cur = addr_cur->ai_next;
>                                                         continue;
>                                                 }
>                                         }
>
> 2) Even if nodelay weren't set, this can still lead to smaller packets
>    being sent, because you start sending normal sized tcp packets,
>    rather than jumbo ones, even if configured (pretty common these
>    days).
>
> 3) Syscall overhead is actually quite significant.

Fair enough, and *headdesk* re not checking NODELAY. I thought I'd
checked for our use of that before, but I must've remembered wrong.

We could use TCP_CORK but it's not portable and it'd be better to just
collect up a buffer to dispatch.

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

  85. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-22T01:21:55Z

    On 2017-06-21 18:07:21 -0700, Andres Freund wrote:
> On 2017-06-22 09:03:05 +0800, Craig Ringer wrote:
> > On 22 June 2017 at 08:29, Andres Freund <andres@anarazel.de> wrote:
> > 
> > > I.e. we're doing tiny write send() syscalls (they should be coalesced)
> > 
> > That's likely worth doing, but can probably wait for a separate patch.
> 
> I don't think so, we should get this right, it could have API influence.
> 
> 
> > The kernel will usually do some packet aggregation unless we use
> > TCP_NODELAY (which we don't and shouldn't), and the syscall overhead
> > is IMO not worth worrying about just yet.
> 
> 1)
> 					/*
> 					 * Select socket options: no delay of outgoing data for
> 					 * TCP sockets, nonblock mode, close-on-exec. Fail if any
> 					 * of this fails.
> 					 */
> 					if (!IS_AF_UNIX(addr_cur->ai_family))
> 					{
> 						if (!connectNoDelay(conn))
> 						{
> 							pqDropConnection(conn, true);
> 							conn->addr_cur = addr_cur->ai_next;
> 							continue;
> 						}
> 					}
> 
> 2) Even if nodelay weren't set, this can still lead to smaller packets
>    being sent, because you start sending normal sized tcp packets,
>    rather than jumbo ones, even if configured (pretty common these
>    days).
> 
> 3) Syscall overhead is actually quite significant.

Proof of the pudding:

pgbench of 10 pgbench select statements in a batch:

as submitted by Daniel:

pgbench -h localhost -M prepared -S -n -c 16 -j 16 -T 10000 -P 1 -f ~/tmp/pgbench-select-only-batch.sq
progress: 1.0 s, 24175.5 tps, lat 0.647 ms stddev 0.782
progress: 2.0 s, 27737.6 tps, lat 0.577 ms stddev 0.625
progress: 3.0 s, 28853.3 tps, lat 0.554 ms stddev 0.619
progress: 4.0 s, 26660.8 tps, lat 0.600 ms stddev 0.776
progress: 5.0 s, 30023.8 tps, lat 0.533 ms stddev 0.484
progress: 6.0 s, 29959.3 tps, lat 0.534 ms stddev 0.450
progress: 7.0 s, 29944.9 tps, lat 0.534 ms stddev 0.536
progress: 8.0 s, 30137.7 tps, lat 0.531 ms stddev 0.533
progress: 9.0 s, 30285.2 tps, lat 0.528 ms stddev 0.479
progress: 10.0 s, 30228.7 tps, lat 0.529 ms stddev 0.460
progress: 11.0 s, 29921.4 tps, lat 0.534 ms stddev 0.613
progress: 12.0 s, 29982.4 tps, lat 0.533 ms stddev 0.510
progress: 13.0 s, 29247.4 tps, lat 0.547 ms stddev 0.526
progress: 14.0 s, 28757.3 tps, lat 0.556 ms stddev 0.635
progress: 15.0 s, 29035.3 tps, lat 0.551 ms stddev 0.523
^C

sample vmstat:
 r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa st
19  0      0 488992 787332 23558676    0    0     0     0 9720 455099 65 35  0  0  0

(i.e. ~450k context switches)

hackily patched:
pgbench -h localhost -M prepared -S -n -c 16 -j 16 -T 10000 -P 1 -f ~/tmp/pgbench-select-only-batch.sq
progress: 1.0 s, 40545.2 tps, lat 0.386 ms stddev 0.625
progress: 2.0 s, 48158.0 tps, lat 0.332 ms stddev 0.277
progress: 3.0 s, 50125.7 tps, lat 0.319 ms stddev 0.204
progress: 4.0 s, 50740.6 tps, lat 0.315 ms stddev 0.250
progress: 5.0 s, 50795.6 tps, lat 0.315 ms stddev 0.246
progress: 6.0 s, 51195.6 tps, lat 0.312 ms stddev 0.207
progress: 7.0 s, 50746.7 tps, lat 0.315 ms stddev 0.264
progress: 8.0 s, 50619.1 tps, lat 0.316 ms stddev 0.250
progress: 9.0 s, 50619.4 tps, lat 0.316 ms stddev 0.228
progress: 10.0 s, 46967.8 tps, lat 0.340 ms stddev 0.499
progress: 11.0 s, 50480.1 tps, lat 0.317 ms stddev 0.239
progress: 12.0 s, 50242.5 tps, lat 0.318 ms stddev 0.286
progress: 13.0 s, 49912.7 tps, lat 0.320 ms stddev 0.266
progress: 14.0 s, 49841.7 tps, lat 0.321 ms stddev 0.271
progress: 15.0 s, 49807.1 tps, lat 0.321 ms stddev 0.248
^C

sample vmstat:
 r  b   swpd   free   buff  cache   si   so    bi    bo   in   cs us sy id wa st
23  0      0 482008 787312 23558996    0    0     0     0 8219 105097 87 14  0  0  0

(i.e. ~100k context switches)

That's *localhost*.


It's completely possible that I've screwed something up here, I didn't
test it besides running pgbench, but the send/recv'd data looks like
it's similar amounts of data, just fewer syscalls.

Greetings,

Andres Freund

  86. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-06-22T11:23:11Z

    	Craig Ringer wrote:

> The kernel will usually do some packet aggregation unless we use
> TCP_NODELAY (which we don't and shouldn't)

Not sure. As a point of comparison, Oracle has it as a tunable
parameter (TCP.NODELAY), and they changed its default from
No to Yes starting from their 10g R2.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  87. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-06-22T11:43:35Z

    	Andres Freund wrote:

-	if (pqFlush(conn) < 0)
-		goto sendFailed;
+	if (conn->batch_status == PQBATCH_MODE_OFF)
+	{
+		/*
+		 * Give the data a push.  In nonblock mode, don't complain if
we're unable
+		 * to send it all; PQgetResult() will do any additional
flushing needed.
+		 */
+		if (pqFlush(conn) < 0)
+			goto sendFailed;
+	}

Seems to be responsible for roughly an 1.7x speedup in tps and equivalent
decrease in latency, based on the "progress" info.
I wonder how much of that corresponds to a decrease in the number of
packets versus the number of syscalls. Both matter, I guess.

But OTOH there are certainly batch workloads where it will be preferrable
for the first query to reach the server ASAP, rather than waiting to be
coalesced with the next ones.
libpq is not going to know what's best.
One option may be to leave that decision to the user by providing a
PQBatchAutoFlush(true|false) property, along with a PQBatchFlush()
function. Maybe we could even let the user set the size of the sending
buffer, so those who really want to squeeze performance may tune it
for their network and workload.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  88. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-06-22T15:52:04Z

    Hi,

On 2017-06-22 13:43:35 +0200, Daniel Verite wrote:
> But OTOH there are certainly batch workloads where it will be preferrable
> for the first query to reach the server ASAP, rather than waiting to be
> coalesced with the next ones.

Is that really something people expect from a batch API?  I suspect it's
not really, and nothing would stop one from adding PQflush() or similar
calls if desirable anyway.

FWIW, the way I did that in the hack clearly isn't ok: If you were to
send a gigabyte of queries, it'd buffer them all up in memory... So some
more intelligence is going to be needed.


> libpq is not going to know what's best.
> One option may be to leave that decision to the user by providing a
> PQBatchAutoFlush(true|false) property, along with a PQBatchFlush()
> function.

What'd be the difference between PQflush() and PQbatchFlush()?

Greetings,

Andres Freund

  89. Re: PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2017-06-22T17:42:11Z

    	Andres Freund wrote:

> > One option may be to leave that decision to the user by providing a
> > PQBatchAutoFlush(true|false) property, along with a PQBatchFlush()
> > function.
> 
> What'd be the difference between PQflush() and PQbatchFlush()?

I guess no difference, I was just not seeing that libpq already provides
this functionality...

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  90. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-08-10T05:23:06Z

    Andres Freund wrote :
> If you were to send a gigabyte of queries, it'd buffer them all up in
memory... So some
>more intelligence is going to be needed.

Firstly, sorry for the delayed response as I got busy with other
activities.

To buffer up the queries before flushing them to the socket, I think
"conn->outCount>=65536" is ok to use, as 64k is considered to be safe in
Windows as per comments in pqSendSome() API.

Attached the code patch to replace pqFlush calls with pqBatchFlush in the
asynchronous libpq function calls flow.
Still pqFlush is used in "PQbatchSyncQueue" and
"PQexitBatchMode" functions.

Am failing to see the benefit in allowing user to set
PQBatchAutoFlush(true|false) property? Is it really needed?

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  91. Re: PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2017-08-23T09:40:53Z

    Hi,

On 2017-08-10 15:23:06 +1000, Vaishnavi Prabakaran wrote:
> Andres Freund wrote :
> > If you were to send a gigabyte of queries, it'd buffer them all up in
> memory... So some
> >more intelligence is going to be needed.
> 
> Firstly, sorry for the delayed response as I got busy with other
> activities.

No worries - development of new features was slowed down anyway, due to
the v10 feature freeze.


> To buffer up the queries before flushing them to the socket, I think
> "conn->outCount>=65536" is ok to use, as 64k is considered to be safe in
> Windows as per comments in pqSendSome() API.
> 
> Attached the code patch to replace pqFlush calls with pqBatchFlush in the
> asynchronous libpq function calls flow.
> Still pqFlush is used in "PQbatchSyncQueue" and
> "PQexitBatchMode" functions.


> Am failing to see the benefit in allowing user to set
> PQBatchAutoFlush(true|false) property? Is it really needed?

I'm inclined not to introduce that for now. If somebody comes up with a
convincing usecase and numbers, we can add it later. Libpq API is set in
stone, so I'd rather not introduce unnecessary stuff...



> +   <para>
> +    Much like asynchronous query mode, there is no performance disadvantage to
> +    using batching and pipelining. It increases client application complexity
> +    and extra caution is required to prevent client/server deadlocks but
> +    can sometimes offer considerable performance improvements.
> +   </para>

That's not necessarily true, is it? Unless you count always doing
batches of exactly size 1.


> +   <para>
> +    Batching is most useful when the server is distant, i.e. network latency
> +    (<quote>ping time</quote>) is high, and when many small operations are being performed in
> +    rapid sequence. There is usually less benefit in using batches when each
> +    query takes many multiples of the client/server round-trip time to execute.
> +    A 100-statement operation run on a server 300ms round-trip-time away would take
> +    30 seconds in network latency alone without batching; with batching it may spend
> +    as little as 0.3s waiting for results from the server.
> +   </para>

I'd add a remark that this is frequently beneficial even in cases of
minimal latency - as e.g. shown by the numbers I presented upthread.


> +   <para>
> +    Use batches when your application does lots of small
> +    <literal>INSERT</literal>, <literal>UPDATE</literal> and
> +    <literal>DELETE</literal> operations that can't easily be transformed into
> +    operations on sets or into a
> +    <link linkend="libpq-copy"><literal>COPY</literal></link> operation.
> +   </para>

Aren't SELECTs also a major beneficiarry of this?


> +   <para>
> +    Batching is less useful when information from one operation is required by the
> +    client before it knows enough to send the next operation.

s/less/not/


> +   <note>
> +    <para>
> +     The batch API was introduced in PostgreSQL 10.0, but clients using PostgresSQL 10.0 version of libpq can
> +     use batches on server versions 8.4 and newer. Batching works on any server
> +     that supports the v3 extended query protocol.
> +    </para>
> +   </note>

Where's the 8.4 coming from?


> +   <para>
> +    The client uses libpq's asynchronous query functions to dispatch work,
> +    marking the end of each batch with <function>PQbatchSyncQueue</function>.
> +    And to get results, it uses <function>PQgetResult</function> and
> +    <function>PQbatchProcessQueue</function>. It may eventually exit
> +    batch mode with <function>PQexitBatchMode</function> once all results are
> +    processed.
> +   </para>
> +
> +   <note>
> +    <para>
> +     It is best to use batch mode with <application>libpq</application> in
> +     <link linkend="libpq-pqsetnonblocking">non-blocking mode</link>. If used in
> +     blocking mode it is possible for a client/server deadlock to occur. The
> +     client will block trying to send queries to the server, but the server will
> +     block trying to send results from queries it has already processed to the
> +     client. This only occurs when the client sends enough queries to fill its
> +     output buffer and the server's receive buffer before switching to
> +     processing input from the server, but it's hard to predict exactly when
> +     that'll happen so it's best to always use non-blocking mode.
> +    </para>
> +   </note>

Mention that nonblocking only actually helps if send/recv is done as
required, and can essentially require unbound memory?  We probably
should either document or implement some smarts about when to signal
read/write readyness. Otherwise we e.g. might be receiving tons of
result data without having sent the next query - or the other way round.


> +   <sect3 id="libpq-batch-sending">
> +    <title>Issuing queries</title>
> +
> +    <para>
> +     After entering batch mode the application dispatches requests
> +     using normal asynchronous <application>libpq</application> functions such as 
> +     <function>PQsendQueryParams</function>, <function>PQsendPrepare</function>,
> +     <function>PQsendQueryPrepared</function>, <function>PQsendDescribePortal</function>,
> +     <function>PQsendDescribePrepared</function>.
> +     The asynchronous requests are followed by a <link
> +     linkend="libpq-PQbatchSyncQueue"><function>PQbatchSyncQueue(conn)</function></link> call to mark
> +     the end of the batch. The client <emphasis>does not</emphasis> need to call
> +     <function>PQgetResult</function> immediately after dispatching each
> +     operation. <link linkend="libpq-batch-results">Result processing</link>
> +     is handled separately.
> +    </para>
> +    
> +    <para>
> +     Batched operations will be executed by the server in the order the client
> +     sends them. The server will send the results in the order the statements
> +     executed. The server may begin executing the batch before all commands
> +     in the batch are queued and the end of batch command is sent. If any
> +     statement encounters an error the server aborts the current transaction and
> +     skips processing the rest of the batch. Query processing resumes after the
> +     end of the failed batch.
> +    </para>

Maybe note that multiple batches can be "in flight"?
I.e. PQbatchSyncQueue() is about error handling, nothing else? Don't
have a great idea, but we might want to rename...


> +    <note>
> +     <para>
> +      The client must not assume that work is committed when it
> +      <emphasis>sends</emphasis> a <literal>COMMIT</literal>, only when the
> +      corresponding result is received to confirm the commit is complete.
> +      Because errors arrive asynchronously the application needs to be able to
> +      restart from the last <emphasis>received</emphasis> committed change and
> +      resend work done after that point if something goes wrong.
> +     </para>
> +    </note>

This seems fairly independent of batching.


> +   </sect3>
> +
> +   <sect3 id="libpq-batch-interleave">
> +    <title>Interleaving result processing and query dispatch</title>
> +
> +    <para>
> +     To avoid deadlocks on large batches the client should be structured around
> +     a nonblocking I/O loop using a function like <function>select</function>,
> +     <function>poll</function>, <function>epoll</function>,
> +     <function>WaitForMultipleObjectEx</function>, etc.
> +    </para>
> +
> +    <para>
> +     The client application should generally maintain a queue of work still to
> +     be dispatched and a queue of work that has been dispatched but not yet had
> +     its results processed.

Hm. Why? If queries are just issued, no such queue is required?


> When the socket is writable it should dispatch more
> +     work. When the socket is readable it should read results and process them,
> +     matching them up to the next entry in its expected results queue. Batches
> +     should be scoped to logical units of work, usually (but not always) one
> +     transaction per batch. There's no need to exit batch mode and re-enter it
> +     between batches or to wait for one batch to finish before sending the next.
> +    </para>

This really needs to take memory usage into account.

> +       </variablelist>
> +
> +     </listitem>
> +    </varlistentry>
> +
> +    <varlistentry id="libpq-PQenterBatchMode">
> +     <term>
> +      <function>PQenterBatchMode</function>
> +      <indexterm>
> +       <primary>PQenterBatchMode</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Causes a connection to enter batch mode if it is currently idle or
> +      already in batch mode.
> +
> +<synopsis>
> +int PQenterBatchMode(PGconn *conn);
> +</synopsis>
> +
> +        </para>
> +        <para>
> +          Returns 1 for success. Returns 0 and has no 
> +          effect if the connection is not currently idle, i.e. it has a result 
> +          ready, is waiting for more input from the server, etc. This function 
> +          does not actually send anything to the server, it just changes the 
> +          <application>libpq</application> connection state.
> +
> +        </para>
> +     </listitem>
> +    </varlistentry>
> +
> +    <varlistentry id="libpq-PQexitBatchMode">
> +     <term>
> +      <function>PQexitBatchMode</function>
> +      <indexterm>
> +       <primary>PQexitBatchMode</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Causes a connection to exit batch mode if it is currently in batch mode
> +      with an empty queue and no pending results.
> +<synopsis>
> +int PQexitBatchMode(PGconn *conn);
> +</synopsis>
> +        </para>
> +        <para>Returns 1 for success.
> +      Returns 1 and takes no action if not in batch mode. If the connection has

"returns 1"?


> +    <varlistentry id="libpq-PQbatchSyncQueue">
> +     <term>
> +      <function>PQbatchSyncQueue</function>
> +      <indexterm>
> +       <primary>PQbatchSyncQueue</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Delimits the end of a set of a batched commands by sending a <link
> +      linkend="protocol-flow-ext-query">sync message</link> and flushing
> +      the send buffer. The end of a batch serves as 
> +      the delimiter of an implicit transaction and
> +      an error recovery point; see <link linkend="libpq-batch-errors">
> +      error handling</link>.

I wonder why this isn't framed as PQbatchIssue/Send/...()? Syncing seems
to mostly make sense from a protocol POV.


> +    <varlistentry id="libpq-PQbatchQueueCount">
> +     <term>
> +      <function>PQbatchQueueCount</function>
> +      <indexterm>
> +       <primary>PQbatchQueueCount</primary>
> +      </indexterm>
> +     </term>
> +
> +     <listitem>
> +      <para>
> +      Returns the number of queries still in the queue for this batch, not
> +      including any query that's currently having results being processed.
> +      This is the number of times <function>PQbatchProcessQueue</function> has to be
> +      called before the query queue is empty again.
> +
> +<synopsis>
> +int PQbatchQueueCount(PGconn *conn);
> +</synopsis>
> +
> +      </para>
> +     </listitem>
> +    </varlistentry>

Given that apps are supposed to track this, I'm not sure why we have
this?


> +/*
> + * PQrecyclePipelinedCommand
> + *	Push a command queue entry onto the freelist. It must be a dangling entry
> + *	with null next pointer and not referenced by any other entry's next pointer.
> + */
> +static void
> +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> +{
> +	if (entry == NULL)
> +		return;
> +	if (entry->next != NULL)
> +	{
> +		fprintf(stderr, libpq_gettext("tried to recycle non-dangling command queue entry"));
> +		abort();

Don't think we use abort() in libpq like that. There's some Assert()s
tho.


>  static bool
>  PQsendQueryStart(PGconn *conn)
> @@ -1377,20 +1486,59 @@ PQsendQueryStart(PGconn *conn)
>  						  libpq_gettext("no connection to the server\n"));
>  		return false;
>  	}
> -	/* Can't send while already busy, either. */
> -	if (conn->asyncStatus != PGASYNC_IDLE)
> +	/* Can't send while already busy, either, unless enqueuing for later */
> +	if (conn->asyncStatus != PGASYNC_IDLE && conn->batch_status == PQBATCH_MODE_OFF)
>  	{
>  		printfPQExpBuffer(&conn->errorMessage,
>  						  libpq_gettext("another command is already in progress\n"));
>  		return false;
>  	}
>  
> -	/* initialize async result-accumulation state */
> -	pqClearAsyncResult(conn);
> +	if (conn->batch_status != PQBATCH_MODE_OFF)
> +	{
> +	/*

Weirdly indented.


> +	if (conn->batch_status != PQBATCH_MODE_OFF)
> +	{
> +		pipeCmd = PQmakePipelinedCommand(conn);
> +
> +		if (pipeCmd == NULL)
> +			return 0;			/* error msg already set */
> +
> +		last_query = &pipeCmd->query;
> +		queryclass = &pipeCmd->queryclass;
> +	}
> +	else
> +	{
> +		last_query = &conn->last_query;
> +		queryclass = &conn->queryclass;
> +	}

The amount of complexity / branches we're adding to all of these is more
than a bit unsightly.


> +/*
> + * PQbatchQueueCount
> + * 	Return number of queries currently pending in batch mode
> + */
> +int
> +PQbatchQueueCount(PGconn *conn)
> +{
> +	int			count = 0;
> +	PGcommandQueueEntry *entry;
> +
> +	if (PQbatchStatus(conn) == PQBATCH_MODE_OFF)
> +		return 0;
> +
> +	entry = conn->cmd_queue_head;
> +	while (entry != NULL)
> +	{
> +		++count;
> +		entry = entry->next;
> +	}
> +	return count;
> +}

Ugh, O(N)? In that case I'd rather just remove this.


> +/*
> + * PQbatchBegin

Mismatched w/ actual function name.


> + * 	Put an idle connection in batch mode. Commands submitted after this
> + * 	can be pipelined on the connection, there's no requirement to wait for
> + * 	one to finish before the next is dispatched.
> + *
> + * 	Queuing of new query or syncing during COPY is not allowed.

+"a"?

> + * 	A set of commands is terminated by a PQbatchQueueSync. Multiple sets of batched
> + * 	commands may be sent while in batch mode. Batch mode can be exited by
> + * 	calling PQbatchEnd() once all results are processed.
> + *
> + * 	This doesn't actually send anything on the wire, it just puts libpq
> + * 	into a state where it can pipeline work.
> + */
> +int
> +PQenterBatchMode(PGconn *conn)
> +{
> +	if (!conn)
> +		return false;

true/false isn't quite in line with int return code.


> +/*
> + * PQbatchEnd

wrong name.

> + * 	End batch mode and return to normal command mode.
> + *
> + * 	Has no effect unless the client has processed all results
> + * 	from all outstanding batches and the connection is idle.

That seems wrong - will lead to hard to diagnose errors.


> + * 	Returns true if batch mode ended.
> + */
> +int
> +PQexitBatchMode(PGconn *conn)
> +{
> +	if (!conn)
> +		return false;
> +
> +	if (conn->batch_status == PQBATCH_MODE_OFF)
> +		return true;
> +
> +	switch (conn->asyncStatus)
> +	{
> +		case PGASYNC_IDLE:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, IDLE in batch mode"));
> +			break;
> +		case PGASYNC_COPY_IN:
> +		case PGASYNC_COPY_OUT:
> +		case PGASYNC_COPY_BOTH:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, COPY in batch mode"));
> +			break;

So we'll still check the queue in this case, that's a bit weird?



> +/*
> + * PQbatchQueueSync

Out of sync.

> + * 	End a batch submission by sending a protocol sync. The connection will
> + * 	remain in batch mode and unavailable for new non-batch commands until all
> + * 	results from the batch are processed by the client.

"unavailable for new non-batch commands" - that's hard to follow, and
seems pretty redundant with PQendBatchMode (or however it's called).

> + * 	It's legal to start submitting another batch immediately, without waiting
> + * 	for the results of the current batch. There's no need to end batch mode
> + * 	and start it again.
> + *
> + * 	If a command in a batch fails, every subsequent command up to and including
> + * 	the PQbatchQueueSync command result gets set to PGRES_BATCH_ABORTED state. If the
> + * 	whole batch is processed without error, a PGresult with PGRES_BATCH_END is
> + * 	produced.

Hm, should probably mention that that's only true for commands since the
last PQbatchQueueSync?



> +/*
> + * PQbatchQueueProcess

Out of sync.


> + * Returns false if the current query isn't done yet, the connection
> + * is not in a batch, or there are no more queries to process.

Last complaint about this - think this forgiving mode is a mistake.


> + */
> +int
> +PQbatchProcessQueue(PGconn *conn)
> +{

> +	/* This command's results will come in immediately.
> +	 * Initialize async result-accumulation state */
> +	pqClearAsyncResult(conn);

I'm not following?


>  /*
>   * PQgetResult
> @@ -1749,10 +2228,32 @@ PQgetResult(PGconn *conn)

> +			if (conn->batch_status != PQBATCH_MODE_OFF)
> +			{
> +				/*
> +				 * batched queries aren't followed by a Sync to put us back in
> +				 * PGASYNC_IDLE state, and when we do get a sync we could
> +				 * still have another batch coming after this one.

This needs rephrasing.


> +				 * The connection isn't idle since we can't submit new
> +				 * nonbatched commands. It isn't also busy since the current
> +				 * command is done and we need to process a new one.
> +				 */
> +				conn->asyncStatus = PGASYNC_QUEUED;

Not sure I like the name.


> +	if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status != PQBATCH_MODE_OFF)
> +	{
> +		printfPQExpBuffer(&conn->errorMessage,
> +						  libpq_gettext("Synchronous command execution functions are not allowed in batch mode\n"));
> +		return false;
> +	}

Why do we need the PGASYNC_QUEUED test here?

> +/* pqBatchFlush
> + * In batch mode, data will be flushed only when the out buffer reaches the threshold value.
> + * In non-batch mode, data will be flushed all the time.
> + */
> +static int
> +pqBatchFlush(PGconn *conn)
> +{
> +	if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount>=65536))
> +		return(pqFlush(conn));
> +	return 0; /* Just to keep compiler quiet */
> +}

This should be defined in a macro or such, rather than hardcoded.


Falling over now. This seems like enough feedback for a bit of work
anyway.

Regards,

Andres

  92. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-09-13T05:06:50Z

    On Wed, Aug 23, 2017 at 7:40 PM, Andres Freund <andres@anarazel.de> wrote:

>
>
>
> > Am failing to see the benefit in allowing user to set
> > PQBatchAutoFlush(true|false) property? Is it really needed?
>
> I'm inclined not to introduce that for now. If somebody comes up with a
> convincing usecase and numbers, we can add it later. Libpq API is set in
> stone, so I'd rather not introduce unnecessary stuff...
>
>
Thanks for reviewing the patch and yes ok.


>
>
> > +   <para>
> > +    Much like asynchronous query mode, there is no performance
> disadvantage to
> > +    using batching and pipelining. It increases client application
> complexity
> > +    and extra caution is required to prevent client/server deadlocks but
> > +    can sometimes offer considerable performance improvements.
> > +   </para>
>
> That's not necessarily true, is it? Unless you count always doing
> batches of exactly size 1.
>

Client application complexity is increased in batch mode,because
application needs to remember the query queue status. Results processing
can be done at anytime, so the application needs to know till what query,
the results are consumed.


> +   <para>
> > +    Use batches when your application does lots of small
> > +    <literal>INSERT</literal>, <literal>UPDATE</literal> and
> > +    <literal>DELETE</literal> operations that can't easily be
> transformed into
> > +    operations on sets or into a
> > +    <link linkend="libpq-copy"><literal>COPY</literal></link>
> operation.
> > +   </para>
>
> Aren't SELECTs also a major beneficiarry of this?
>


Hmm, though SELECTs also benefit from batch mode, doing multiple selects in
batch mode will fill up the memory rapidly and might not be as beneficial
as other operations listed.



> > +   <para>
> > +    Batching is less useful when information from one operation is
> required by the
> > +    client before it knows enough to send the next operation.
>
> s/less/not/
>
>
Corrected.


>
> > +   <note>
> > +    <para>
> > +     The batch API was introduced in PostgreSQL 10.0, but clients using
> PostgresSQL 10.0 version of libpq can
> > +     use batches on server versions 8.4 and newer. Batching works on
> any server
> > +     that supports the v3 extended query protocol.
> > +    </para>
> > +   </note>
>
> Where's the 8.4 coming from?
>
>

I guess it is 7.4 where "PQsendQueryParams" is introduced, and not 8.4.
Corrected.


> +   <note>
> > +    <para>
> > +     It is best to use batch mode with <application>libpq</application>
> in
> > +     <link linkend="libpq-pqsetnonblocking">non-blocking mode</link>.
> If used in
> > +     blocking mode it is possible for a client/server deadlock to
> occur. The
> > +     client will block trying to send queries to the server, but the
> server will
> > +     block trying to send results from queries it has already processed
> to the
> > +     client. This only occurs when the client sends enough queries to
> fill its
> > +     output buffer and the server's receive buffer before switching to
> > +     processing input from the server, but it's hard to predict exactly
> when
> > +     that'll happen so it's best to always use non-blocking mode.
> > +    </para>
> > +   </note>
>
> Mention that nonblocking only actually helps if send/recv is done as
> required, and can essentially require unbound memory?  We probably
> should either document or implement some smarts about when to signal
> read/write readyness. Otherwise we e.g. might be receiving tons of
> result data without having sent the next query - or the other way round.
>
>

Added a statement for caution in documentation and again this is one of the
reason why SELECT query is not so beneficial in batch mode.



> Maybe note that multiple batches can be "in flight"?
> I.e. PQbatchSyncQueue() is about error handling, nothing else? Don't
> have a great idea, but we might want to rename...
>
>
This function not only does error handling, but also sends the "Sync"
message to backend. In batch mode, "Sync" message is not sent with every
query but will
be sent only via this function to mark the end of implicit transaction.
Renamed it to PQbatchCommitQueue. Kindly let me know if you think of any
other better name.



> > +    <note>
> > +     <para>
> > +      The client must not assume that work is committed when it
> > +      <emphasis>sends</emphasis> a <literal>COMMIT</literal>, only when
> the
> > +      corresponding result is received to confirm the commit is
> complete.
> > +      Because errors arrive asynchronously the application needs to be
> able to
> > +      restart from the last <emphasis>received</emphasis> committed
> change and
> > +      resend work done after that point if something goes wrong.
> > +     </para>
> > +    </note>
>
> This seems fairly independent of batching.
>
>
Yes and the reason why is it explicitly specified for batch mode is that if
more than one explicit transactions are used in Single batch, then failure
of one transaction will lead to skipping the consequent transactions until
the end of current batch is reached. This behavior is specific to batch
mode, so adding a precautionary note here is needed I think.



> > +   </sect3>
> > +
> > +   <sect3 id="libpq-batch-interleave">
> > +    <title>Interleaving result processing and query dispatch</title>
> > +
> > +    <para>
> > +     To avoid deadlocks on large batches the client should be
> structured around
> > +     a nonblocking I/O loop using a function like
> <function>select</function>,
> > +     <function>poll</function>, <function>epoll</function>,
> > +     <function>WaitForMultipleObjectEx</function>, etc.
> > +    </para>
> > +
> > +    <para>
> > +     The client application should generally maintain a queue of work
> still to
> > +     be dispatched and a queue of work that has been dispatched but not
> yet had
> > +     its results processed.
>
> Hm. Why? If queries are just issued, no such queue is required?
>


This is essentially telling that client application should remember the
order of queries sent , so that result processing will be based on what
results are expected. For e.g, if the order of queries are 1)SELECT,
2)INSERT, then in result processing, "PGRES_TUPLES_OK" and later
"PGRES_COMMAND_OK" checks are required. Above mentioned queue of work means
the intelligent to remember the queue of queries.



> > When the socket is writable it should dispatch more
> > +     work. When the socket is readable it should read results and
> process them,
> > +     matching them up to the next entry in its expected results queue.
> Batches
> > +     should be scoped to logical units of work, usually (but not
> always) one
> > +     transaction per batch. There's no need to exit batch mode and
> re-enter it
> > +     between batches or to wait for one batch to finish before sending
> the next.
> > +    </para>
>
> This really needs to take memory usage into account.
>
>
Modified the para to include that frequency of result processing should be
based on available memory.



> > +<synopsis>
> > +int PQenterBatchMode(PGconn *conn);
> > +</synopsis>
> > +<synopsis>
> > +int PQexitBatchMode(PGconn *conn);
> > +</synopsis>
> > +        </para>
> > +        <para>Returns 1 for success.
> > +      Returns 1 and takes no action if not in batch mode. If the
> connection has
>
> "returns 1"?
>


Modified the code to return 1/0.



>
>
> > +    <varlistentry id="libpq-PQbatchSyncQueue">
> > +     <term>
> > +      <function>PQbatchSyncQueue</function>
> > +      <indexterm>
> > +       <primary>PQbatchSyncQueue</primary>
> > +      </indexterm>
> > +     </term>
>
> I wonder why this isn't framed as PQbatchIssue/Send/...()? Syncing seems
> to mostly make sense from a protocol POV.
>
>
Renamed to PQbatchCommitQueue.



> > +<synopsis>
> > +int PQbatchQueueCount(PGconn *conn);
> > +</synopsis>
> > +
>
> Given that apps are supposed to track this, I'm not sure why we have
> this?
>
>
Removed this function considering your another comment about O(N) as well.



> > +static void
> > +PQrecyclePipelinedCommand(PGconn *conn, PGcommandQueueEntry * entry)
> > +{
> > +     if (entry == NULL)
> > +             return;
> > +     if (entry->next != NULL)
> > +     {
> > +             fprintf(stderr, libpq_gettext("tried to recycle
> non-dangling command queue entry"));
> > +             abort();
>
> Don't think we use abort() in libpq like that. There's some Assert()s
> tho.
>
>

For out-of-memory cases, we do abort(), and here abort will happen only
during some memory corruption. So, I think it is ok to abort here. Please
let me know if you think otherwise.



>
> >  static bool
> >  PQsendQueryStart(PGconn *conn)
> .....


>
Weirdly indented.
>
>
Corrected.



> > +/*
> > + * PQbatchBegin
>
> Mismatched w/ actual function name.
>
>
Corrected.



>
> > + *   Put an idle connection in batch mode. Commands submitted after this
> > + *   can be pipelined on the connection, there's no requirement to wait
> for
> > + *   one to finish before the next is dispatched.
> > + *
> > + *   Queuing of new query or syncing during COPY is not allowed.
>
> +"a"?
>

Hmm, Can you explain the question please. I don't understand.


>
> > +/*
> > + * PQbatchEnd
>
> wrong name.
>

Corrected.


>
> > + *   End batch mode and return to normal command mode.
> > + *
> > + *   Has no effect unless the client has processed all results
> > + *   from all outstanding batches and the connection is idle.
>
> That seems wrong - will lead to hard to diagnose errors.


I have now added a error message when the batch end is failed to ease the
error diagnose. And, this behavior is documented. So the application should
not assume that the batch mode ended without checking the return value.



> > + *   Returns true if batch mode ended.
> > + */
> > +int
> > +PQexitBatchMode(PGconn *conn)
> > +{
> > +     if (!conn)
> > +             return false;
> > +
> > +     if (conn->batch_status == PQBATCH_MODE_OFF)
> > +             return true;
> > +
> > +     switch (conn->asyncStatus)
> > +     {
> > +             case PGASYNC_IDLE:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> IDLE in batch mode"));
> > +                     break;
> > +             case PGASYNC_COPY_IN:
> > +             case PGASYNC_COPY_OUT:
> > +             case PGASYNC_COPY_BOTH:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > +                     break;
>
> So we'll still check the queue in this case, that's a bit weird?
>

Removed above cases as adding error here is not of much help as well.



>
> > +/*
> > + * PQbatchQueueSync
>
> Out of sync.
>


Corrected.



>
> > + *   End a batch submission by sending a protocol sync. The connection
> will
> > + *   remain in batch mode and unavailable for new non-batch commands
> until all
> > + *   results from the batch are processed by the client.
>
> "unavailable for new non-batch commands" - that's hard to follow, and
> seems pretty redundant with PQendBatchMode (or however it's called).
>
>
Modified documentation to be more precise and difference between
PQexitBatchMode and this one is that to exit batch, application should have
consumed all the results pending in buffer and post exit batch, an
application can call synchronous command execution functions. Whereas with
batch sync/commit, application cannot issue synchronous commands but can
issue asynchronous commands without reading all the results pending in
buffer. This function basically marks the end of implicit transaction.



> > + *   It's legal to start submitting another batch immediately, without
> waiting
> > + *   for the results of the current batch. There's no need to end batch
> mode
> > + *   and start it again.
> > + *
> > + *   If a command in a batch fails, every subsequent command up to and
> including
> > + *   the PQbatchQueueSync command result gets set to
> PGRES_BATCH_ABORTED state. If the
> > + *   whole batch is processed without error, a PGresult with
> PGRES_BATCH_END is
> > + *   produced.
>
> Hm, should probably mention that that's only true for commands since the
> last PQbatchQueueSync?
>
>

"Batch" means the set of commands between PQbatchCommitQueue(newly renamed
name) calls except first batch which count from beginning to
PQbatchCommitQueue. And in documentation it is mentioned that "The result
for <function>PQbatchSyncQueue</function> is reported as
<literal>PGRES_BATCH_END</literal> to signal the end of the aborted batch
and resumption of normal result processing."




>
> > +/*
> > + * PQbatchQueueProcess
>
> Out of sync.
>

Corrected .



> > + * Returns false if the current query isn't done yet, the connection
> > + * is not in a batch, or there are no more queries to process.
>
> Last complaint about this - think this forgiving mode is a mistake.
>
>
Now added an error message using printfPQExpBuffer(). I think aborting here
is not really needed. Please let me know if you prefer to abort() rather
than this solution.



>
> > + */
> > +int
> > +PQbatchProcessQueue(PGconn *conn)
> > +{
>
> > +     /* This command's results will come in immediately.
> > +      * Initialize async result-accumulation state */
> > +     pqClearAsyncResult(conn);
>
> I'm not following?
>
>
First line of comment is removed. It is copy/paste error.



>
> >  /*
> >   * PQgetResult
> > @@ -1749,10 +2228,32 @@ PQgetResult(PGconn *conn)
>
> > +                     if (conn->batch_status != PQBATCH_MODE_OFF)
> > +                     {
> > +                             /*
> > +                              * batched queries aren't followed by a
> Sync to put us back in
> > +                              * PGASYNC_IDLE state, and when we do get
> a sync we could
> > +                              * still have another batch coming after
> this one.
>
> This needs rephrasing.
>
>

Rephrased as "In batch mode, query execution state cannot be IDLE as there
can be other queries or results waiting in the queue ..." . Hope this is
simple and enough.



> > +                              * The connection isn't idle since we
> can't submit new
> > +                              * nonbatched commands. It isn't also busy
> since the current
> > +                              * command is done and we need to process
> a new one.
> > +                              */
> > +                             conn->asyncStatus = PGASYNC_QUEUED;
>
> Not sure I like the name.
>
>
Changed the name to PGASYNC_BATCH to make it more align to batch mode as
this status will not be set anywhere in non-batch mode.


>
> > +     if (conn->asyncStatus == PGASYNC_QUEUED || conn->batch_status !=
> PQBATCH_MODE_OFF)
> > +     {
> > +             printfPQExpBuffer(&conn->errorMessage,
> > +
>  libpq_gettext("Synchronous command execution functions are not allowed in
> batch mode\n"));
> > +             return false;
> > +     }
>
> Why do we need the PGASYNC_QUEUED test here?
>


Yes, we don't need this check here. It was removed in PQfn and similarly it
is not needed here and in pqParseInput2 too.



> > +static int
> > +pqBatchFlush(PGconn *conn)
> > +{
> > +     if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->
> outCount>=65536))
> > +             return(pqFlush(conn));
> > +     return 0; /* Just to keep compiler quiet */
> > +}
>
> This should be defined in a macro or such, rather than hardcoded.
>
>
Added macro "OUTBUFFER_THRESHOLD"



>
> Falling over now. This seems like enough feedback for a bit of work
> anyway.
>

Thanks once again for reviewing the patch. Attached the patch with your
review comments incorporation.


Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  93. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-09-13T05:33:02Z

    On 13 September 2017 at 13:06, Vaishnavi Prabakaran <
vaishnaviprabakaran@gmail.com> wrote:

>
>
> On Wed, Aug 23, 2017 at 7:40 PM, Andres Freund <andres@anarazel.de> wrote:
>
>>
>>
>>
>> > Am failing to see the benefit in allowing user to set
>> > PQBatchAutoFlush(true|false) property? Is it really needed?
>>
>> I'm inclined not to introduce that for now. If somebody comes up with a
>> convincing usecase and numbers, we can add it later. Libpq API is set in
>> stone, so I'd rather not introduce unnecessary stuff...
>>
>>
> Thanks for reviewing the patch and yes ok.
>
>
>>
>>
>> > +   <para>
>> > +    Much like asynchronous query mode, there is no performance
>> disadvantage to
>> > +    using batching and pipelining. It increases client application
>> complexity
>> > +    and extra caution is required to prevent client/server deadlocks
>> but
>> > +    can sometimes offer considerable performance improvements.
>> > +   </para>
>>
>> That's not necessarily true, is it? Unless you count always doing
>> batches of exactly size 1.
>>
>
> Client application complexity is increased in batch mode,because
> application needs to remember the query queue status. Results processing
> can be done at anytime, so the application needs to know till what query,
> the results are consumed.
>
>

Yep. Also, the client/server deadlocks at issue here are a buffer
management issue, and deadlock is probably not exactly the right word. Your
app has to process replies from the server while it's sending queries,
otherwise it can get into a state where it has no room left in its send
buffer, but the server isn't consuming its receive buffer because the
server's send buffer is full. To allow the system to make progress, the
client must read from the client receive buffer.

This isn't an issue when using libpq normally.

PgJDBC has similar issues with its batch mode, but in PgJDBC it's much
worse because there's no non-blocking send available. In libpq you can at
least set your sending socket to non-blocking.



>
> > +   <para>
>> > +    Use batches when your application does lots of small
>> > +    <literal>INSERT</literal>, <literal>UPDATE</literal> and
>> > +    <literal>DELETE</literal> operations that can't easily be
>> transformed into
>> > +    operations on sets or into a
>> > +    <link linkend="libpq-copy"><literal>COPY</literal></link>
>> operation.
>> > +   </para>
>>
>> Aren't SELECTs also a major beneficiarry of this?
>>
>
Yes, many individual SELECTs that cannot be assembled into a single more
efficient query would definitely also benefit.


> Hmm, though SELECTs also benefit from batch mode, doing multiple selects
> in batch mode will fill up the memory rapidly and might not be as
> beneficial as other operations listed.
>

Depends on the SELECT. With wide results you'll get less benefit, but even
then you can gain if you're on a high latency network. With "n+1" patterns
and similar, you'll see huge gains.


> Maybe note that multiple batches can be "in flight"?
>> I.e. PQbatchSyncQueue() is about error handling, nothing else? Don't
>> have a great idea, but we might want to rename...
>>
>>
> This function not only does error handling, but also sends the "Sync"
> message to backend. In batch mode, "Sync" message is not sent with every
> query but will
> be sent only via this function to mark the end of implicit transaction.
> Renamed it to PQbatchCommitQueue. Kindly let me know if you think of any
> other better name.
>

I really do not like calling it "commit" as that conflates with a database
commit.

A batch can embed multiple BEGINs and COMMITs. It's entirely possible for
an earlier part of the batch to succeed and commit, then a later part to
fail, if that's the case. So that name is IMO wrong.


>>
>> > +    <varlistentry id="libpq-PQbatchSyncQueue">
>> > +     <term>
>> > +      <function>PQbatchSyncQueue</function>
>> > +      <indexterm>
>> > +       <primary>PQbatchSyncQueue</primary>
>> > +      </indexterm>
>> > +     </term>
>>
>> I wonder why this isn't framed as PQbatchIssue/Send/...()? Syncing seems
>> to mostly make sense from a protocol POV.
>>
>>
> Renamed to PQbatchCommitQueue.
>
>
Per above, strong -1 on that. But SendQueue seems OK, or FlushQueue?


>
>> > + *   Put an idle connection in batch mode. Commands submitted after
>> this
>> > + *   can be pipelined on the connection, there's no requirement to
>> wait for
>> > + *   one to finish before the next is dispatched.
>> > + *
>> > + *   Queuing of new query or syncing during COPY is not allowed.
>>
>> +"a"?
>>
>
> Hmm, Can you explain the question please. I don't understand.
>

s/of new query/of a new query/


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

  94. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-09-13T05:44:00Z

    On Wed, Sep 13, 2017 at 3:33 PM, Craig Ringer <craig@2ndquadrant.com> wrote:

>
> I really do not like calling it "commit" as that conflates with a database
> commit.
>
> A batch can embed multiple BEGINs and COMMITs. It's entirely possible for
> an earlier part of the batch to succeed and commit, then a later part to
> fail, if that's the case. So that name is IMO wrong.
>

Ok, SendQueue seems ok to me as well. Will change it in next version.



>>> +"a"?
>>>
>>
>> Hmm, Can you explain the question please. I don't understand.
>>
>
> s/of new query/of a new query/
>
>
Thanks for explaining. Will change this too in next version.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  95. Re: PATCH: Batch/pipelining support for libpq

    Craig Ringer <craig@2ndquadrant.com> — 2017-09-13T06:01:56Z

    On 13 September 2017 at 13:44, Vaishnavi Prabakaran <
vaishnaviprabakaran@gmail.com> wrote:


> Thanks for explaining. Will change this too in next version.
>
>
Thankyou, a lot, for picking up this patch.

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

  96. Re: PATCH: Batch/pipelining support for libpq

    Daniel Gustafsson <daniel@yesql.se> — 2017-10-02T09:31:07Z

    > On 13 Sep 2017, at 07:44, Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> wrote:
> 
> On Wed, Sep 13, 2017 at 3:33 PM, Craig Ringer <craig@2ndquadrant.com <mailto:craig@2ndquadrant.com>> wrote:
> 
> I really do not like calling it "commit" as that conflates with a database commit.
> 
> A batch can embed multiple BEGINs and COMMITs. It's entirely possible for an earlier part of the batch to succeed and commit, then a later part to fail, if that's the case. So that name is IMO wrong.
> 
> Ok, SendQueue seems ok to me as well. Will change it in next version. 
>  
> +"a"?
> 
> Hmm, Can you explain the question please. I don't understand.
> 
> s/of new query/of a new query/
> 
> Thanks for explaining. Will change this too in next version. 

Based on the discussions in this thread, and that a new version hasn’t been
submitted, I’m marking this Returned with Feedback.  Please re-submit the new
version in an upcoming commitfest when ready.

cheers ./daniel

  97. Re: PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2017-10-05T00:58:46Z

    On Mon, Oct 2, 2017 at 8:31 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

> > On 13 Sep 2017, at 07:44, Vaishnavi Prabakaran <
> vaishnaviprabakaran@gmail.com> wrote:
> >
> > On Wed, Sep 13, 2017 at 3:33 PM, Craig Ringer <craig@2ndquadrant.com
> <mailto:craig@2ndquadrant.com>> wrote:
> >
> > I really do not like calling it "commit" as that conflates with a
> database commit.
> >
> > A batch can embed multiple BEGINs and COMMITs. It's entirely possible
> for an earlier part of the batch to succeed and commit, then a later part
> to fail, if that's the case. So that name is IMO wrong.
> >
> > Ok, SendQueue seems ok to me as well. Will change it in next version.
> >
> > +"a"?
> >
> > Hmm, Can you explain the question please. I don't understand.
> >
> > s/of new query/of a new query/
> >
> > Thanks for explaining. Will change this too in next version.
>
> Based on the discussions in this thread, and that a new version hasn’t been
> submitted, I’m marking this Returned with Feedback.  Please re-submit the
> new
> version in an upcoming commitfest when ready.



Thanks for the suggestion and, OK I will create a new patch in upcoming
commitfest with attached patch addressing above review comments.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  98. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-28T01:57:13Z

    On Thu, Oct 5, 2017 at 9:58 AM, Vaishnavi Prabakaran
<vaishnaviprabakaran@gmail.com> wrote:
> Thanks for the suggestion and, OK I will create a new patch in upcoming
> commitfest with attached patch addressing above review comments.

The patch still applies and there has been no updates for the last
month, as well as no reviews. I am bumping it to next CF.
-- 
Michael

  99. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2018-01-05T05:55:50Z

    On Tue, Nov 28, 2017 at 12:57 PM, Michael Paquier <michael.paquier@gmail.com
> wrote:

> On Thu, Oct 5, 2017 at 9:58 AM, Vaishnavi Prabakaran
> <vaishnaviprabakaran@gmail.com> wrote:
> > Thanks for the suggestion and, OK I will create a new patch in upcoming
> > commitfest with attached patch addressing above review comments.
>
> The patch still applies and there has been no updates for the last
> month, as well as no reviews. I am bumping it to next CF.


Thank you, I see the patch generates a compilation error due to usage of
"FALSE" with latest postgres code, Hence attaching the patch with
correction.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  100. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> — 2018-01-11T23:12:35Z

    On Fri, Jan 5, 2018 at 4:55 PM, Vaishnavi Prabakaran <
vaishnaviprabakaran@gmail.com> wrote:

>
>
> On Tue, Nov 28, 2017 at 12:57 PM, Michael Paquier <
> michael.paquier@gmail.com> wrote:
>
>> On Thu, Oct 5, 2017 at 9:58 AM, Vaishnavi Prabakaran
>> <vaishnaviprabakaran@gmail.com> wrote:
>> > Thanks for the suggestion and, OK I will create a new patch in upcoming
>> > commitfest with attached patch addressing above review comments.
>>
>> The patch still applies and there has been no updates for the last
>> month, as well as no reviews. I am bumping it to next CF.
>
>
> Thank you, I see the patch generates a compilation error due to usage of
> "FALSE" with latest postgres code, Hence attaching the patch with
> correction.
>

Corrected compilation error in documentation portion of patch with latest
postgres code. Attached the corrected patch.

Thanks & Regards,
Vaishnavi,
Fujitsu Australia.

  101. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> — 2018-03-22T12:11:48Z

    Hello.

At Fri, 12 Jan 2018 10:12:35 +1100, Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com> wrote in <CAOoUkxRRTBm8ztzVXL45EVk=BC8AjfaFM5=ZMtrOMMfm+dnbwA@mail.gmail.com>
> Corrected compilation error in documentation portion of patch with latest
> postgres code. Attached the corrected patch.

My understanding of this patch is that it intends to do the
following things without changing the protocol.

A. Refrain from sending sync message and socket flush during batching.
B. Set required information to PGconn before receiving a result.

Multi statement query does the a similar thing but a bit
different.  Queries are not needed to be built and mreged as a
string in advance with this feature.

Since I'm not sure what is the current stage of this patch and I
haven't fully recognize the disucssion so far, I might make
stupid or duplicate comments but would like to make some
comments.


- Previous comments

A disucssion on psql batch mode was held in another branch of
this thread. How do we treat that?

- Interface complteness 

I think PQenter/exitBatchMode() is undoubtedly needed.

PQbatchSendQueue() seems to be required to make sure to send
queries before calling PQgetResult.

PQbatchProcessQueue() doesn't seem essential. If there's no case
where it is called without following PQgetResult, it can be
included in PQgetResult. Client code may be simpler if it is not
required.

The latest patch has extern definition of PQbatchQueueCount, the
body and doc of which seems to have been removed, or haven't
exist since the beginning.


- Some comments on the code

-- PQbatchProcessQueue()

PQbatchProcessQueue() returns 0 for several undistinguishable
reasons. If asyncStatus is PGASYNC_BUSY at the time, the
connection is out of sync and doesn't accept further
operations. So it should be distinguished from the end-of-queue
case. (It can reutrn an error result if combining with
PQgetResult.)

The function handles COPY_* in inconsistent way. It sets an error
message to conn in hardly noticeable way but allows to go
further. The document is telling as the follows.

> COPY is not recommended as it most likely will trigger failure
> in batch processing

I don't think the desciription is informative, but the reason for
the description would be the fact that we cannot detect a command
leads to protocol failure before the failure actually
happens. The test code suggests that that is the case where any
command (other than Sync) following "COPY FROM" breaks the
protocol. We get an error messages like below in the case. (The
header part is of my test program.)

> ABORTED: (PGRES_BATCH_ABORTED) ERROR:  unexpected message type 0x42 during COPY from stdin
> CONTEXT:  COPY t, line 1

I haven't investigated further, but it seems unable to restore
the sync of connection until disconnection. It would need a
side-channel for COPY_IN to avoid the failure but we don't have
it now. The possible choices here are:

A. Make the description more precisely, by adding the condition
   to cause the error and how it looks.

B. The situation seems detectable in PGgetResult. Return an error
   result containing any meaningful error. But this doesn't stop
   the following protocol error messages.

> ABORTED: (PGRES_FATAL_ERROR) COPY_IN cannot be followed by any command in a batch
> ABORTED: (PGRES_FATAL_ERROR) ERROR:  08P01: unexpected message type 0x42 during COPY from stdin
> ABORTED: (PGRES_BATCH_ABORTED) ERROR:  unexpected message type 0x42 during COPY from stdin
> ABORTED: (PGRES_BATCH_ABORTED) ERROR:  unexpected message type 0x42 during COPY from stdin

C. Cancel COPY on the server and return error to the client for
   the situation. This might let us be able to avoid
   unrecoverable desync of the protocol in the case. (I'm not
   sure this can be done cleanly.)

D. Wait for a protocol change that can solve this problem.


-- PQbatchFlush()

> static int pqBatchFlush(Pgconn *conn) 
...
>  if (...)
>     return(pqFlush(conn));
>  return 0; /* Just to keep compiler quiet */

The comment in the last line is wrong.

+	if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount >= OUTBUFFER_T

Isn't this a bit too long?


-- PGcommandQueueEntry

The resason for the fact that it is a linked list seems to me to
allow queueing and result processing happen alternately in one
batch period. But I coundn't find a description about the
behavior in the documentation. (State transition chart is
required?)

Aside from the above, the interface to handle the command queue
seems to be divided into too-small pieces.

 PQsendQueryGuts()
 >  el = PQmakePipelinedCommand(conn)
 >  el->members = hoge;
 >  ..
 >  PQappendPipelinedCommand(conn, el)

Isn't the following enough instead of the above?

 >  PQappendPipelinedCommand(conn, conn->queryclass, conn->last_query);


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  102. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Daniel Verite <daniel@manitou-mail.org> — 2018-03-23T13:18:09Z

    	Kyotaro HORIGUCHI wrote:

> A disucssion on psql batch mode was held in another branch of
> this thread. How do we treat that?

There's a batch mode for pgbench in a patch posted in [1],
with \beginbatch and \endbatch commands, but nothing
for psql AFAICS.
psql is more complicated because currently it uses a 
blocking PQexec() call at its core. Craig mentioned psql
integration in [2] and [3].
Also a script can have inter-query dependencies such as in
   insert into table(...) returning id \gset
   update othertable set col= :id where ...;
which is a problem in batch mode, as we don't want
to send the update before the right value for :id is
known. Whether we want to support these dependencies
and how needs discussion.
For instance we might not support them at all, or
create a synchronization command that collects
all results of queries sent so far, or do it implicitly
when a variable is injected into a query...
This looks like substantial work that might be best
done separately from the libpq patch.

[1]
https://www.postgresql.org/message-id/b4e34135-2bd9-4b8a-94ca-27d760da26d7@manitou-mail.org
[2]
https://www.postgresql.org/message-id/CAMsr+YGLhaDkjymLuNVQy4MrSKQoA=F1vO=aN8XQf30N=aQuVA@mail.gmail.com
[3]
https://www.postgresql.org/message-id/CAMsr+YE6BK4iAaQz=nY3xDnbLhnNZ_4tp-PTJqbNNpsZMgoo8Q@mail.gmail.com


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

  103. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Michael Paquier <michael@paquier.xyz> — 2018-10-01T06:34:17Z

    On Fri, Mar 23, 2018 at 02:18:09PM +0100, Daniel Verite wrote:
> There's a batch mode for pgbench in a patch posted in [1],
> with \beginbatch and \endbatch commands, but nothing
> for psql AFAICS.
> psql is more complicated because currently it uses a 
> blocking PQexec() call at its core. Craig mentioned psql
> integration in [2] and [3].

This patch has been around for some time now, the last version fails to
apply cleanly and in-depth reviews have happened.  I am moving that to
the next CF, waiting on its author. 
--
Michael

  104. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-11-29T16:14:57Z

    > On Mon, Oct 1, 2018 at 8:34 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Mar 23, 2018 at 02:18:09PM +0100, Daniel Verite wrote:
> > There's a batch mode for pgbench in a patch posted in [1],
> > with \beginbatch and \endbatch commands, but nothing
> > for psql AFAICS.
> > psql is more complicated because currently it uses a
> > blocking PQexec() call at its core. Craig mentioned psql
> > integration in [2] and [3].
>
> This patch has been around for some time now, the last version fails to
> apply cleanly and in-depth reviews have happened.  I am moving that to
> the next CF, waiting on its author.

Unfortunately, nothing was changed since then, so there is already some amount
of unaddressed review feedback. I'll move this patch to "Returned with
feedback".

  105. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Nikhil Sontakke <nikhils@2ndquadrant.com> — 2019-08-30T13:36:34Z

    Hi,

> > This patch has been around for some time now, the last version fails to
> > apply cleanly and in-depth reviews have happened.  I am moving that to
> > the next CF, waiting on its author.
>
> Unfortunately, nothing was changed since then, so there is already some amount
> of unaddressed review feedback. I'll move this patch to "Returned with
> feedback".
>

Craig Ringer mentioned about this thread to me recently.

This effort has seen decent reviews from Craig, Andres and Michael
already. So, I thought of refreshing it to work against latest master
HEAD.

PFA, main patch as well as the test patch (I named the test patch v17
to be consistent with the main patch). The major grouse with the test
patch AFAICS was the use of non-Windows compliant timersub() function.
I have now used INSTR_TIME_SET_CURRENT/INSTR_TIME_SUBTRACT family of
portable macros for the same.

Please let me know on what we think of the above.

Regards,
Nikhil
-- 
Nikhil Sontakke
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/

  106. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Amit Kapila <amit.kapila16@gmail.com> — 2019-09-09T07:20:26Z

    On Fri, Aug 30, 2019 at 7:06 PM Nikhil Sontakke <nikhils@2ndquadrant.com> wrote:
>
> Hi,
>
> > > This patch has been around for some time now, the last version fails to
> > > apply cleanly and in-depth reviews have happened.  I am moving that to
> > > the next CF, waiting on its author.
> >
> > Unfortunately, nothing was changed since then, so there is already some amount
> > of unaddressed review feedback. I'll move this patch to "Returned with
> > feedback".
> >
>
> Craig Ringer mentioned about this thread to me recently.
>
> This effort has seen decent reviews from Craig, Andres and Michael
> already. So, I thought of refreshing it to work against latest master
> HEAD.
>

Thanks for picking up this.  However, I noticed that previously
Horiguchi-San has given some comments on this patch [1] which doesn't
seem to be addressed or at least not all of them are addressed.  It is
possible that you would have already addressed those, but in that
case, it would be good if you respond to his email as well.  If those
are not addressed, then it will be good to address those.


[1] - https://www.postgresql.org/message-id/20180322.211148.187821341.horiguchi.kyotaro%40lab.ntt.co.jp

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  107. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-07-10T23:01:49Z

    On 2019-Sep-09, Amit Kapila wrote:

> Thanks for picking up this.  However, I noticed that previously
> Horiguchi-San has given some comments on this patch [1] which doesn't
> seem to be addressed or at least not all of them are addressed.  It is
> possible that you would have already addressed those, but in that
> case, it would be good if you respond to his email as well.  If those
> are not addressed, then it will be good to address those.

Totally unasked for, here's a rebase of this patch series.  I didn't do
anything other than rebasing to current master, solving a couple of very
trivial conflicts, fixing some whitespace complaints by git apply, and
running tests to verify everthing works.

I don't foresee working on this at all, so if anyone is interested in
seeing this feature in, I encourage them to read and address
Horiguchi-san's feedback.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  108. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2020-07-14T19:18:01Z

    Hi,

On 2020-07-10 19:01:49 -0400, Alvaro Herrera wrote:
> Totally unasked for, here's a rebase of this patch series.  I didn't do
> anything other than rebasing to current master, solving a couple of very
> trivial conflicts, fixing some whitespace complaints by git apply, and
> running tests to verify everthing works.
> 
> I don't foresee working on this at all, so if anyone is interested in
> seeing this feature in, I encourage them to read and address
> Horiguchi-san's feedback.

Nor am I planning to do so, but I do think its a pretty important
improvement.



> +/*
> + * PQrecyclePipelinedCommand
> + *	Push a command queue entry onto the freelist. It must be a dangling entry
> + *	with null next pointer and not referenced by any other entry's next pointer.
> + */

Dangling sounds a bit like it's already freed.



> +/*
> + * PQbatchSendQueue
> + * 	End a batch submission by sending a protocol sync. The connection will
> + * 	remain in batch mode and unavailable for new synchronous command execution
> + * 	functions until all results from the batch are processed by the client.

I feel like the reference to the protocol sync is a bit too low level
for an external API. It should first document what the function does
from a user's POV.

I think it'd also be good to document whether / whether not queries can
already have been sent before PQbatchSendQueue is called or not.


> +/*
> + * PQbatchProcessQueue
> + *	 In batch mode, start processing the next query in the queue.
> + *
> + * Returns 1 if the next query was popped from the queue and can
> + * be processed by PQconsumeInput, PQgetResult, etc.
> + *
> + * Returns 0 if the current query isn't done yet, the connection
> + * is not in a batch, or there are no more queries to process.
> + */
> +int
> +PQbatchProcessQueue(PGconn *conn)
> +{
> +	PGcommandQueueEntry *next_query;
> +
> +	if (!conn)
> +		return 0;
> +
> +	if (conn->batch_status == PQBATCH_MODE_OFF)
> +		return 0;
> +
> +	switch (conn->asyncStatus)
> +	{
> +		case PGASYNC_COPY_IN:
> +		case PGASYNC_COPY_OUT:
> +		case PGASYNC_COPY_BOTH:
> +			printfPQExpBuffer(&conn->errorMessage,
> +				   libpq_gettext_noop("internal error, COPY in batch mode"));
> +			break;

Shouldn't there be a return 0 here?



> +	if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass != PGQUERY_SYNC)
> +	{
> +		/*
> +		 * In an aborted batch we don't get anything from the server for each
> +		 * result; we're just discarding input until we get to the next sync
> +		 * from the server. The client needs to know its queries got aborted
> +		 * so we create a fake PGresult to return immediately from
> +		 * PQgetResult.
> +		 */
> +		conn->result = PQmakeEmptyPGresult(conn,
> +										   PGRES_BATCH_ABORTED);
> +		if (!conn->result)
> +		{
> +			printfPQExpBuffer(&conn->errorMessage,
> +							  libpq_gettext("out of memory"));
> +			pqSaveErrorResult(conn);
> +			return 0;

Is there any way an application can recover at this point? ISTM we'd be
stuck in the previous asyncStatus, no?


> +/* pqBatchFlush
> + * In batch mode, data will be flushed only when the out buffer reaches the threshold value.
> + * In non-batch mode, data will be flushed all the time.
> + */
> +static int
> +pqBatchFlush(PGconn *conn)
> +{
> +	if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount >= OUTBUFFER_THRESHOLD))
> +		return(pqFlush(conn));
> +	return 0; /* Just to keep compiler quiet */
> +}

unnecessarily long line.


> +/*
> + * Connection's outbuffer threshold is set to 64k as it is safe
> + * in Windows as per comments in pqSendSome() API.
> + */
> +#define OUTBUFFER_THRESHOLD	65536

I don't think the comment explains much. It's fine to send more than 64k
with pqSendSome(), they'll just be send with separate pgsecure_write()
invocations. And only on windows.

It clearly makes sense to start sending out data at a certain
granularity to avoid needing unnecessary amounts of memory, and to make
more efficient use of latency / serer side compute.

It's not implausible that 64k is the right amount for that, I just don't
think the explanation above is good.

> diff --git a/src/test/modules/test_libpq/testlibpqbatch.c b/src/test/modules/test_libpq/testlibpqbatch.c
> new file mode 100644
> index 0000000000..4d6ba266e5
> --- /dev/null
> +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> @@ -0,0 +1,1456 @@
> +/*
> + * src/test/modules/test_libpq/testlibpqbatch.c
> + *
> + *
> + * testlibpqbatch.c
> + *		Test of batch execution functionality
> + */
> +
> +#ifdef WIN32
> +#include <windows.h>
> +#endif

ISTM that this shouldn't be needed in a test program like this?
Shouldn't libpq abstract all of this away?


> +static void
> +simple_batch(PGconn *conn)
> +{

ISTM that all or at least several of these should include tests of
transactional behaviour with pipelining (e.g. using both implicit and
explicit transactions inside a single batch, using transactions across
batches, multiple explicit transactions inside a batch).



> +	PGresult   *res = NULL;
> +	const char *dummy_params[1] = {"1"};
> +	Oid			dummy_param_oids[1] = {INT4OID};
> +
> +	fprintf(stderr, "simple batch... ");
> +	fflush(stderr);

Why do we need fflush()? IMO that shouldn't be needed in a use like
this? Especially not on stderr, which ought to be unbuffered?


> +	/*
> +	 * Enter batch mode and dispatch a set of operations, which we'll then
> +	 * process the results of as they come in.
> +	 *
> +	 * For a simple case we should be able to do this without interim
> +	 * processing of results since our out buffer will give us enough slush to
> +	 * work with and we won't block on sending. So blocking mode is fine.
> +	 */
> +	if (PQisnonblocking(conn))
> +	{
> +		fprintf(stderr, "Expected blocking connection mode\n");
> +		goto fail;
> +	}

Perhaps worth adding a helper for this?

#define EXPECT(condition, explanation) \
...
or such?

Greetings,

Andres Freund

  109. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2021-03-04T11:22:16Z

    On Wed, Jul 15, 2020 at 12:18 AM Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> On 2020-07-10 19:01:49 -0400, Alvaro Herrera wrote:
> > Totally unasked for, here's a rebase of this patch series.  I didn't do
> > anything other than rebasing to current master, solving a couple of very
> > trivial conflicts, fixing some whitespace complaints by git apply, and
> > running tests to verify everthing works.
> >
> > I don't foresee working on this at all, so if anyone is interested in
> > seeing this feature in, I encourage them to read and address
> > Horiguchi-san's feedback.
>
> Nor am I planning to do so, but I do think its a pretty important
> improvement.
>
>
>
> > +/*
> > + * PQrecyclePipelinedCommand
> > + *   Push a command queue entry onto the freelist. It must be a
> dangling entry
> > + *   with null next pointer and not referenced by any other entry's
> next pointer.
> > + */
>
> Dangling sounds a bit like it's already freed.
>
>
>
> > +/*
> > + * PQbatchSendQueue
> > + *   End a batch submission by sending a protocol sync. The connection
> will
> > + *   remain in batch mode and unavailable for new synchronous command
> execution
> > + *   functions until all results from the batch are processed by the
> client.
>
> I feel like the reference to the protocol sync is a bit too low level
> for an external API. It should first document what the function does
> from a user's POV.
>
> I think it'd also be good to document whether / whether not queries can
> already have been sent before PQbatchSendQueue is called or not.
>
>
> > +/*
> > + * PQbatchProcessQueue
> > + *    In batch mode, start processing the next query in the queue.
> > + *
> > + * Returns 1 if the next query was popped from the queue and can
> > + * be processed by PQconsumeInput, PQgetResult, etc.
> > + *
> > + * Returns 0 if the current query isn't done yet, the connection
> > + * is not in a batch, or there are no more queries to process.
> > + */
> > +int
> > +PQbatchProcessQueue(PGconn *conn)
> > +{
> > +     PGcommandQueueEntry *next_query;
> > +
> > +     if (!conn)
> > +             return 0;
> > +
> > +     if (conn->batch_status == PQBATCH_MODE_OFF)
> > +             return 0;
> > +
> > +     switch (conn->asyncStatus)
> > +     {
> > +             case PGASYNC_COPY_IN:
> > +             case PGASYNC_COPY_OUT:
> > +             case PGASYNC_COPY_BOTH:
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +                                libpq_gettext_noop("internal error,
> COPY in batch mode"));
> > +                     break;
>
> Shouldn't there be a return 0 here?
>
>
>
> > +     if (conn->batch_status == PQBATCH_MODE_ABORTED && conn->queryclass
> != PGQUERY_SYNC)
> > +     {
> > +             /*
> > +              * In an aborted batch we don't get anything from the
> server for each
> > +              * result; we're just discarding input until we get to the
> next sync
> > +              * from the server. The client needs to know its queries
> got aborted
> > +              * so we create a fake PGresult to return immediately from
> > +              * PQgetResult.
> > +              */
> > +             conn->result = PQmakeEmptyPGresult(conn,
> > +
>         PGRES_BATCH_ABORTED);
> > +             if (!conn->result)
> > +             {
> > +                     printfPQExpBuffer(&conn->errorMessage,
> > +
>  libpq_gettext("out of memory"));
> > +                     pqSaveErrorResult(conn);
> > +                     return 0;
>
> Is there any way an application can recover at this point? ISTM we'd be
> stuck in the previous asyncStatus, no?
>
>
> > +/* pqBatchFlush
> > + * In batch mode, data will be flushed only when the out buffer reaches
> the threshold value.
> > + * In non-batch mode, data will be flushed all the time.
> > + */
> > +static int
> > +pqBatchFlush(PGconn *conn)
> > +{
> > +     if ((conn->batch_status == PQBATCH_MODE_OFF)||(conn->outCount >=
> OUTBUFFER_THRESHOLD))
> > +             return(pqFlush(conn));
> > +     return 0; /* Just to keep compiler quiet */
> > +}
>
> unnecessarily long line.
>
>
> > +/*
> > + * Connection's outbuffer threshold is set to 64k as it is safe
> > + * in Windows as per comments in pqSendSome() API.
> > + */
> > +#define OUTBUFFER_THRESHOLD  65536
>
> I don't think the comment explains much. It's fine to send more than 64k
> with pqSendSome(), they'll just be send with separate pgsecure_write()
> invocations. And only on windows.
>
> It clearly makes sense to start sending out data at a certain
> granularity to avoid needing unnecessary amounts of memory, and to make
> more efficient use of latency / serer side compute.
>
> It's not implausible that 64k is the right amount for that, I just don't
> think the explanation above is good.
>
> > diff --git a/src/test/modules/test_libpq/testlibpqbatch.c
> b/src/test/modules/test_libpq/testlibpqbatch.c
> > new file mode 100644
> > index 0000000000..4d6ba266e5
> > --- /dev/null
> > +++ b/src/test/modules/test_libpq/testlibpqbatch.c
> > @@ -0,0 +1,1456 @@
> > +/*
> > + * src/test/modules/test_libpq/testlibpqbatch.c
> > + *
> > + *
> > + * testlibpqbatch.c
> > + *           Test of batch execution functionality
> > + */
> > +
> > +#ifdef WIN32
> > +#include <windows.h>
> > +#endif
>
> ISTM that this shouldn't be needed in a test program like this?
> Shouldn't libpq abstract all of this away?
>
>
> > +static void
> > +simple_batch(PGconn *conn)
> > +{
>
> ISTM that all or at least several of these should include tests of
> transactional behaviour with pipelining (e.g. using both implicit and
> explicit transactions inside a single batch, using transactions across
> batches, multiple explicit transactions inside a batch).
>
>
>
> > +     PGresult   *res = NULL;
> > +     const char *dummy_params[1] = {"1"};
> > +     Oid                     dummy_param_oids[1] = {INT4OID};
> > +
> > +     fprintf(stderr, "simple batch... ");
> > +     fflush(stderr);
>
> Why do we need fflush()? IMO that shouldn't be needed in a use like
> this? Especially not on stderr, which ought to be unbuffered?
>
>
> > +     /*
> > +      * Enter batch mode and dispatch a set of operations, which we'll
> then
> > +      * process the results of as they come in.
> > +      *
> > +      * For a simple case we should be able to do this without interim
> > +      * processing of results since our out buffer will give us enough
> slush to
> > +      * work with and we won't block on sending. So blocking mode is
> fine.
> > +      */
> > +     if (PQisnonblocking(conn))
> > +     {
> > +             fprintf(stderr, "Expected blocking connection mode\n");
> > +             goto fail;
> > +     }
>
> Perhaps worth adding a helper for this?
>
> #define EXPECT(condition, explanation) \
> ...
> or such?
>
> Greetings,
>
> Andres Freund
>
>
>
The build is failing for this patch, can you please take a look at this?

https://cirrus-ci.com/task/4568547922804736
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.129221


I am marking the patch "Waiting on Author"

-- 
Ibrar Ahmed

  110. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T14:26:27Z

    On 2021-Mar-04, Ibrar Ahmed wrote:

> The build is failing for this patch, can you please take a look at this?
> 
> https://cirrus-ci.com/task/4568547922804736
> https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.129221
> 
> 
> I am marking the patch "Waiting on Author"

I don't know why you chose to respond to such an old message, but here's
a rebase which also includes the workaround I suggested last night for
the problem of the outdated query printed by error messages, as well as
Justin's suggested fixes.  (I also made a few tweaks to the TAP test).

-- 
Álvaro Herrera       Valdivia, Chile
"No renuncies a nada. No te aferres a nada."

  111. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T15:01:37Z

    Apparently, the archives system or the commitfest system is not picking
up new messages to the thread, so the CF app is trying to apply a
very old patch version.  I'm not sure what's up with that.  Thomas, any
clues on where to look?

-- 
Álvaro Herrera       Valdivia, Chile
"Oh, great altar of passive entertainment, bestow upon me thy discordant images
at such speed as to render linear thought impossible" (Calvin a la TV)

  112. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Ibrar Ahmed <ibrar.ahmad@gmail.com> — 2021-03-04T16:41:52Z

    On Thu, Mar 4, 2021 at 8:01 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

> Apparently, the archives system or the commitfest system is not picking
> up new messages to the thread, so the CF app is trying to apply a
> very old patch version.  I'm not sure what's up with that.  Thomas, any
> clues on where to look?
>
> --
> Álvaro Herrera       Valdivia, Chile
> "Oh, great altar of passive entertainment, bestow upon me thy discordant
> images
> at such speed as to render linear thought impossible" (Calvin a la TV)
>

Hi Alvaro,

The thread splits and CF app still has the previous thread. The old thread
has the v18 as the latest patch which is failing. The new thread which  (
https://www.postgresql.org/message-id/CAJkzx4T5E-2cQe3dtv2R78dYFvz%2Bin8PY7A8MArvLhs_pg75gg%40mail.gmail.com
)
has the new patch.


-- 
Ibrar Ahmed

  113. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Justin Pryzby <pryzby@telsasoft.com> — 2021-03-04T16:47:58Z

    On Thu, Mar 04, 2021 at 12:01:37PM -0300, Alvaro Herrera wrote:
> Apparently, the archives system or the commitfest system is not picking
> up new messages to the thread, so the CF app is trying to apply a
> very old patch version.  I'm not sure what's up with that.  Thomas, any
> clues on where to look?

I think it's just because you forgot the patch.
https://www.postgresql.org/message-id/20210304142627.GA5978%40alvherre.pgsql

-- 
Justin

  114. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T17:05:36Z

    > I think it's just because you forgot the patch.
> https://www.postgresql.org/message-id/20210304142627.GA5978%40alvherre.pgsql


-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  115. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T19:24:09Z

    v30 contains changes to hopefully make it build on MSVC.

-- 
Álvaro Herrera       Valdivia, Chile

  116. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T19:48:37Z

    On 2021-Mar-04, Alvaro Herrera wrote:

> v30 contains changes to hopefully make it build on MSVC.

Hm, that didn't work -- appveyor still says:

Project "C:\projects\postgresql\pgsql.sln" (1) is building "C:\projects\postgresql\pipeline.vcxproj" (75) on node 1 (default targets).
PrepareForBuild:
  Creating directory ".\Release\pipeline\".
  Creating directory ".\Release\pipeline\pipeline.tlog\".
InitializeBuildStatus:
  Creating ".\Release\pipeline\pipeline.tlog\unsuccessfulbuild" because "AlwaysCreate" was specified.
ClCompile:
  C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\bin\x86_amd64\CL.exe /c /Isrc/include /Isrc/include/port/win32 /Isrc/include/port/win32_msvc /Zi /nologo /W3 /WX- /Ox /D WIN32 /D _WINDOWS /D __WINDOWS__ /D __WIN32__ /D WIN32_STACK_RLIMIT=4194304 /D _CRT_SECURE_NO_DEPRECATE /D _CRT_NONSTDC_NO_DEPRECATE /D _MBCS /GF /Gm- /EHsc /MD /GS /fp:precise /Zc:wchar_t /Zc:forScope /Fo".\Release\pipeline\\" /Fd".\Release\pipeline\vc120.pdb" /Gd /TC /wd4018 /wd4244 /wd4273 /wd4102 /wd4090 /wd4267 /errorReport:queue /MP src/test/modules/test_libpq/pipeline.c
  pipeline.c
src/test/modules/test_libpq/pipeline.c(11): fatal error C1083: Cannot open include file: 'libpq-fe.h': No such file or directory [C:\projects\postgresql\pipeline.vcxproj]
Done Building Project "C:\projects\postgresql\pipeline.vcxproj" (default targets) -- FAILED.
Project "C:\projects\postgresql\pgsql.sln" (1) is building "C:\projects\postgresql\test_parser.vcxproj" (76) on node 1 (default targets).

I think the problem is that the project is called pipeline and not test_libpq,
so there's no match in the name.  I'm going to rename the whole thing to
src/test/modules/libpq_pipeline/ and see if the msvc tooling likes that
better.


-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  117. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T19:59:55Z

    On 2021-Mar-04, Alvaro Herrera wrote:

> I think the problem is that the project is called pipeline and not
> test_libpq, so there's no match in the name.  I'm going to rename the
> whole thing to src/test/modules/libpq_pipeline/ and see if the msvc
> tooling likes that better.

v31.

-- 
Álvaro Herrera       Valdivia, Chile
"I'm impressed how quickly you are fixing this obscure issue. I came from 
MS SQL and it would be hard for me to put into words how much of a better job
you all are doing on [PostgreSQL]."
 Steve Midgley, http://archives.postgresql.org/pgsql-sql/2008-08/msg00000.php

  118. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T20:51:50Z

    On 2021-Mar-04, Alvaro Herrera wrote:

> v31.

Got this:

libpq_pipeline.obj : error LNK2019: unresolved external symbol __WSAFDIsSet referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
5019libpq_pipeline.obj : error LNK2019: unresolved external symbol __imp_select referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
5020libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_snprintf referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
5021libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_vfprintf referenced in function pg_fatal_impl [C:\projects\postgresql\libpq_pipeline.vcxproj]
5022libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_fprintf referenced in function pg_fatal_impl [C:\projects\postgresql\libpq_pipeline.vcxproj]
5023libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_printf referenced in function pg_fatal_impl [C:\projects\postgresql\libpq_pipeline.vcxproj]
5024libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_strerror referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
5025libpq_pipeline.obj : error LNK2019: unresolved external symbol pg_strdup referenced in function main [C:\projects\postgresql\libpq_pipeline.vcxproj]
5026libpq_pipeline.obj : error LNK2019: unresolved external symbol pfree referenced in function test_singlerowmode [C:\projects\postgresql\libpq_pipeline.vcxproj]
5027libpq_pipeline.obj : error LNK2019: unresolved external symbol psprintf referenced in function test_singlerowmode [C:\projects\postgresql\libpq_pipeline.vcxproj]

pg_snprintf, pg_vfprintf, pg_fprintf, pg_printf, pg_strerror are in pgport.
pg_strdup and pfree, psprintf are in pgcommon.

I don't know where do __WSAFDIsSet and __imp_select come from or what to
do about them.  Let's see if adding pgport and pgcommon fixes things.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  119. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-04T21:40:17Z

    On 2021-Mar-04, Alvaro Herrera wrote:

> I don't know where do __WSAFDIsSet and __imp_select come from or what to
> do about them.  Let's see if adding pgport and pgcommon fixes things.

Indeed all those other problems were fixed and these remain.  New
failure is:

"C:\projects\postgresql\pgsql.sln" (default target) (1) ->
6007"C:\projects\postgresql\libpq_pipeline.vcxproj" (default target) (55) ->
6008(Link target) -> 
6009  libpq_pipeline.obj : error LNK2019: unresolved external symbol __WSAFDIsSet referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
6010  libpq_pipeline.obj : error LNK2019: unresolved external symbol __imp_select referenced in function test_pipelined_insert [C:\projects\postgresql\libpq_pipeline.vcxproj]
6011  .\Release\libpq_pipeline\libpq_pipeline.exe : fatal error LNK1120: 2 unresolved externals [C:\projects\postgresql\libpq_pipeline.vcxproj]

I did notice that isolationtester.c is using select(), and one
difference is that it includes <sys/select.h> which libpq_pipeline.c
does not -- and it also pulls in ws2_32.lib.  Let's see if those two
changes fix things.

-- 
Álvaro Herrera       Valdivia, Chile

  120. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Zhihong Yu <zyu@yugabyte.com> — 2021-03-06T00:31:47Z

    Hi,

+      <literal>\gset</literal> and <literal>\aset</literal> cannot be used
+      pipeline mode, since query results are not immediately

'used pipeline mode' -> 'used in pipeline mode'

--- /dev/null
+++ b/src/test/modules/libpq_pipeline/libpq_pipeline.c
@@ -0,0 +1,1144 @@
+/*
+ * src/test/modules/libpq_pipeline/libpq_pipeline.c

Looks like license information is missing from the header.

Cheers

On Thu, Mar 4, 2021 at 1:40 PM Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

> On 2021-Mar-04, Alvaro Herrera wrote:
>
> > I don't know where do __WSAFDIsSet and __imp_select come from or what to
> > do about them.  Let's see if adding pgport and pgcommon fixes things.
>
> Indeed all those other problems were fixed and these remain.  New
> failure is:
>
> "C:\projects\postgresql\pgsql.sln" (default target) (1) ->
> 6007"C:\projects\postgresql\libpq_pipeline.vcxproj" (default target) (55)
> ->
> 6008(Link target) ->
> 6009  libpq_pipeline.obj : error LNK2019: unresolved external symbol
> __WSAFDIsSet referenced in function test_pipelined_insert
> [C:\projects\postgresql\libpq_pipeline.vcxproj]
> 6010  libpq_pipeline.obj : error LNK2019: unresolved external symbol
> __imp_select referenced in function test_pipelined_insert
> [C:\projects\postgresql\libpq_pipeline.vcxproj]
> 6011  .\Release\libpq_pipeline\libpq_pipeline.exe : fatal error LNK1120: 2
> unresolved externals [C:\projects\postgresql\libpq_pipeline.vcxproj]
>
> I did notice that isolationtester.c is using select(), and one
> difference is that it includes <sys/select.h> which libpq_pipeline.c
> does not -- and it also pulls in ws2_32.lib.  Let's see if those two
> changes fix things.
>
> --
> Álvaro Herrera       Valdivia, Chile
>

  121. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-06T00:35:59Z

    v33 was indeed marked a pass by cfbot.  However, it only *builds* the
test program, it does not *run* it.  I guess we'll have to wait for the
buildfarm to tell us more.

In the meantime, I implemented PQsendQuery() as callable in pipeline
mode; it does that by using the extended-query protocol directly rather
than sending 'Q' as in non-pipeline mode.  I also adjusted the docs a
little bit more.  That's what you see here as v34.

I'll take the weekend to think about the issue with conn->last_query and
conn->queryclass that I mentioned yesterday; other than that detail my
feeling is that this is committable, so I'll be looking at getting this
pushed early next weeks, barring opinions from others.

-- 
Álvaro Herrera       Valdivia, Chile
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

  122. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-11T16:00:13Z

    On 2021-Mar-05, Alvaro Herrera wrote:

> I'll take the weekend to think about the issue with conn->last_query and
> conn->queryclass that I mentioned yesterday; other than that detail my
> feeling is that this is committable, so I'll be looking at getting this
> pushed early next weeks, barring opinions from others.

It took longer than I expected, but it works well now.  conn->last_query
is gone; all commands, both in pipeline mode and in no-pipeline mode, go
via the command queue.  This is cleaner all around; we don't have to
have the pipeline code "cheat" so that it looks like each command is
"last" at each point.

I have not absorbed David Johnston's latest doc suggestions yet.

I'm going to give the code a last renaming pass, on the idea that the
command queue is no longer exclusively for the pipeline mode, so some
things need less exclusionary names.  But functionality wise AFAICS this
patch has the shape it ought to have.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  123. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-03-11T18:49:02Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> [ v35-libpq-pipeline.patch ]

I think the changes in pqParseInput3() are broken.  You should have
kept the else-structure as-is and inserted the check for "not really
idle" inside the else-clause that reports an error.  As it stands,
after successfully processing an asynchronously-received error or
ParameterStatus message, the added code will cause us to return without
advancing inStart, creating an infinite loop of reprocessing that message.

It's possible that we should redefine the way things happen so that if
we're waiting for another pipeline event, we should hold off processing
of async error & ParameterStatus; but in that case you should have added
the pre-emptive return ahead of that if/else structure, where the existing
"If not IDLE state, just wait ..." test is.  My guess though is that we
do need to process error messages in that state, so that the correct
patch looks more like

            else
            {
+                /*
+                 * We're notionally not-IDLE when in pipeline mode we have
+                 * completed processing the results of one query and are waiting
+                 * for the next one in the pipeline.  In this case, as above, just
+                 * wait.
+                 */
+                if (conn->asyncStatus == PGASYNC_IDLE &&
+                    conn->pipelineStatus != PQ_PIPELINE_OFF &&
+                    conn->cmd_queue_head != NULL)
+                    return;
+
                pqInternalNotice(&conn->noticeHooks,
                                 "message type 0x%02x arrived from server while idle",
                                 id);
                /* Discard the unexpected message */
                conn->inCursor += msgLength;
            }

It'd be appropriate to do more than nothing to the comment block above
this if/else chain, too, because really that one ought to explain why we
should consume ERROR when in pipeline state.

(I've not looked at the rest of this patch, just scanned what you did
in fe-protocol3.c, because I wondered if there would be any interaction
with the where-to-advance-inStart changes I'm about to commit.  Looks
okay modulo this issue.)

			regards, tom lane

  124. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-13T04:28:15Z

    On 2021-Mar-11, Tom Lane wrote:

> I think the changes in pqParseInput3() are broken.  You should have
> kept the else-structure as-is and inserted the check for "not really
> idle" inside the else-clause that reports an error.  As it stands,
> after successfully processing an asynchronously-received error or
> ParameterStatus message, the added code will cause us to return without
> advancing inStart, creating an infinite loop of reprocessing that message.
> 
> It's possible that we should redefine the way things happen so that if
> we're waiting for another pipeline event, we should hold off processing
> of async error & ParameterStatus; but in that case you should have added
> the pre-emptive return ahead of that if/else structure, where the existing
> "If not IDLE state, just wait ..." test is.

I think I agree that holding off 'E' and 'S' messages when in between
processing results for different queries in a pipeline, so keeping the
original if/else structure is correct.  An error would be correctly
dealt with in the BUSY state immediately afterwards; and the fact that
we pass 'inError' false at that point causes the wrong reaction (namely
that the pipeline is not put in aborted state).

I made a number of other changes: documentation adjustments per comments
from David Johnston, some function renaming as previously noted, and
added test code for PQsendDescribePrepared, PQsendDescribePortal.  Also
rebased on latest changes.  I also absorbed one change that I already
had when I submitted v35, but hadn't done "git add" on (which caused a
compile failure for CF bot).

-- 
Álvaro Herrera       Valdivia, Chile

  125. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-15T18:54:54Z

    Here's what seems a final version of the patch.  I renamed one more
function: PQsendPipeline is now PQpipelineSync.  I also reworded the
docs in a couple of places, added a few tests to the pgbench patch, and
made it work.

Note the pgbench results in pipeline mode:

./pgbench -r -Mextended -n -f /home/alvherre/Code/pgsql-build/pipeline/src/bin/pgbench/tmp_check/t_001_pgbench_with_server_main_data/001_pgbench_pipeline -c 100 -t10000
pgbench (PostgreSQL) 14.0
transaction type: /home/alvherre/Code/pgsql-build/pipeline/src/bin/pgbench/tmp_check/t_001_pgbench_with_server_main_data/001_pgbench_pipeline
scaling factor: 1
query mode: extended
number of clients: 100
number of threads: 1
number of transactions per client: 10000
number of transactions actually processed: 1000000/1000000
latency average = 2.316 ms
initial connection time = 113.859 ms
tps = 43182.438635 (without initial connection time)
statement latencies in milliseconds:
         0.000  \startpipeline
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         0.000  select 1;
         1.624  \endpipeline

If I just replace the \startpipeline and \endpipeline lines with BEGIN
and COMMIT respectively, I get this:

tps = 10220.259051 (without initial connection time)

         0.830  begin;
         0.765  select 1;
         0.752  select 1;
         0.753  select 1;
         0.755  select 1;
         0.754  select 1;
         0.755  select 1;
         0.757  select 1;
         0.756  select 1;
         0.756  select 1;
         0.756  select 1;
         0.750  commit;

Yes, you could say that this is a liiiitle bit unfair -- but it seems
quite impressive nonetheless.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W

  126. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Justin Pryzby <pryzby@telsasoft.com> — 2021-03-15T19:00:24Z

    Are you going to update the assertion ?

+#if 0                                                                                                                                                                                                                             
        Assert((meta == META_NONE && varprefix == NULL) ||                                                                                                                                                                         
                   ((meta == META_GSET || meta == META_ASET) && varprefix != NULL));                                                                                                                                               
+#endif

  127. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-03-15T19:47:25Z

    On 2021-Mar-15, Justin Pryzby wrote:

> Are you going to update the assertion ?
> 
> +#if 0                                                                                                                                                                                                                             
>         Assert((meta == META_NONE && varprefix == NULL) ||                                                                                                                                                                         
>                    ((meta == META_GSET || meta == META_ASET) && varprefix != NULL));                                                                                                                                               
> +#endif                                                                                                                                                                                                                            

Yeah, caught that just after sending.  Here's a notpatch.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"La virtud es el justo medio entre dos defectos" (Aristóteles)

  128. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Andres Freund <andres@anarazel.de> — 2021-03-16T20:00:40Z

    Hi,

On 2021-03-05 21:35:59 -0300, Alvaro Herrera wrote:
> I'll take the weekend to think about the issue with conn->last_query and
> conn->queryclass that I mentioned yesterday; other than that detail my
> feeling is that this is committable, so I'll be looking at getting this
> pushed early next weeks, barring opinions from others.

It is *very* exciting to see this being merged. Thanks for all the work
to all that contributed!

Greetings,

Andres Freund

  129. Re: [HACKERS] PATCH: Batch/pipelining support for libpq

    Matthieu Garrigues <matthieu.garrigues@gmail.com> — 2021-03-18T11:28:02Z

    Thanks a lot for the merge. I did some tests and the master branch
runs up to 15% faster than the last
patch I tried (v22). Amazing!

Cheers,
Matthieu Garrigues

On Tue, Mar 16, 2021 at 9:00 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2021-03-05 21:35:59 -0300, Alvaro Herrera wrote:
> > I'll take the weekend to think about the issue with conn->last_query and
> > conn->queryclass that I mentioned yesterday; other than that detail my
> > feeling is that this is committable, so I'll be looking at getting this
> > pushed early next weeks, barring opinions from others.
>
> It is *very* exciting to see this being merged. Thanks for all the work
> to all that contributed!
>
> Greetings,
>
> Andres Freund