Hello. At Fri, 12 Jan 2018 10:12:35 +1100, Vaishnavi Prabakaran <vaishnaviprabaka...@gmail.com> wrote in <CAOoUkxRRTBm8ztzVXL45EVk=BC8AjfaFM5=zmtrommfm+dn...@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
