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

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