Re: PATCH: Batch/pipelining support for libpq - Mailing list pgsql-hackers

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